| Message ID | 20231120073522.34180-3-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix runtime suspended device resume | expand |
On 11/20/23 10:35 AM, Damien Le Moal wrote: > Ti is not always possible to keep a device in the runtime suspended s/Ti/It? :-) > state when a system level suspend/resume cycle is executed. E.g. for ATA > devices connected to AHCI adapters, system resume resets the ATA ports, > which causes connected devices to spin up. In such case, a runtime > suspended disk will incorrectly be seen with a suspended runtime state > because the device is not resumed by sd_resume_system(). The power state > seen by the user is different than the actual device physical power > state. > > Fix this issue by introducing the struct scsi_device flag > force_runtime_start_on_system_start. When set, this flag causes > sd_resume_system() to request a runtime resume operation for runtime > suspended devices. This results in the user seeing the device > runtime_state as active after a system resume, thus correctly reflecting > the device physical power state. > > Fixes: 9131bff6a9f1 ("scsi: core: pm: Only runtime resume if necessary") > Cc: stable@vger.kernel.org > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> [...] MBR, Sergey
On 11/20/23 17:50, Sergey Shtylyov wrote: > On 11/20/23 10:35 AM, Damien Le Moal wrote: > >> Ti is not always possible to keep a device in the runtime suspended > > s/Ti/It? :-) Arg. Yes. > >> state when a system level suspend/resume cycle is executed. E.g. for ATA >> devices connected to AHCI adapters, system resume resets the ATA ports, >> which causes connected devices to spin up. In such case, a runtime >> suspended disk will incorrectly be seen with a suspended runtime state >> because the device is not resumed by sd_resume_system(). The power state >> seen by the user is different than the actual device physical power >> state. >> >> Fix this issue by introducing the struct scsi_device flag >> force_runtime_start_on_system_start. When set, this flag causes >> sd_resume_system() to request a runtime resume operation for runtime >> suspended devices. This results in the user seeing the device >> runtime_state as active after a system resume, thus correctly reflecting >> the device physical power state. >> >> Fixes: 9131bff6a9f1 ("scsi: core: pm: Only runtime resume if necessary") >> Cc: stable@vger.kernel.org >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > [...] > > MBR, Sergey
On Mon, Nov 20, 2023 at 06:00:08PM +0900, Damien Le Moal wrote: > On 11/20/23 17:50, Sergey Shtylyov wrote: > > On 11/20/23 10:35 AM, Damien Le Moal wrote: > > > >> Ti is not always possible to keep a device in the runtime suspended > > > > s/Ti/It? :-) > > Arg. Yes. While we are nitpicking :) > > > > >> state when a system level suspend/resume cycle is executed. E.g. for ATA > >> devices connected to AHCI adapters, system resume resets the ATA ports, Double space between AHCI and adapters. Kind regards, Niklas
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 63317449f6ea..0a0f483124c3 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1055,9 +1055,14 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) * Ask the sd driver to issue START STOP UNIT on runtime suspend * and resume and shutdown only. For system level suspend/resume, * devices power state is handled directly by libata EH. + * Given that disks are always spun up on system resume, also + * make sure that the sd driver forces runtime suspended disks + * to be resumed to correctly reflect the power state of the + * device. */ sdev->manage_runtime_start_stop = 1; sdev->manage_shutdown = 1; + sdev->force_runtime_start_on_system_start = 1; } /* diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index fa00dd503cbf..542a4bbb21bc 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3949,8 +3949,15 @@ static int sd_resume(struct device *dev, bool runtime) static int sd_resume_system(struct device *dev) { - if (pm_runtime_suspended(dev)) + if (pm_runtime_suspended(dev)) { + struct scsi_disk *sdkp = dev_get_drvdata(dev); + struct scsi_device *sdp = sdkp ? sdkp->device : NULL; + + if (sdp && sdp->force_runtime_start_on_system_start) + pm_request_resume(dev); + return 0; + } return sd_resume(dev, false); } diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 1fb460dfca0c..5ec1e71a09de 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -181,6 +181,12 @@ struct scsi_device { */ unsigned manage_shutdown:1; + /* + * If set and if the device is runtime suspended, ask the high-level + * device driver (sd) to force a runtime resume of the device. + */ + unsigned force_runtime_start_on_system_start:1; + unsigned removable:1; unsigned changed:1; /* Data invalid due to media change */ unsigned busy:1; /* Used to prevent races */
Ti is not always possible to keep a device in the runtime suspended state when a system level suspend/resume cycle is executed. E.g. for ATA devices connected to AHCI adapters, system resume resets the ATA ports, which causes connected devices to spin up. In such case, a runtime suspended disk will incorrectly be seen with a suspended runtime state because the device is not resumed by sd_resume_system(). The power state seen by the user is different than the actual device physical power state. Fix this issue by introducing the struct scsi_device flag force_runtime_start_on_system_start. When set, this flag causes sd_resume_system() to request a runtime resume operation for runtime suspended devices. This results in the user seeing the device runtime_state as active after a system resume, thus correctly reflecting the device physical power state. Fixes: 9131bff6a9f1 ("scsi: core: pm: Only runtime resume if necessary") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-scsi.c | 5 +++++ drivers/scsi/sd.c | 9 ++++++++- include/scsi/scsi_device.h | 6 ++++++ 3 files changed, 19 insertions(+), 1 deletion(-)