Fix runtime suspended device resume
Damien Le Moal
maintainercover Fix runtime suspended device resume
The first patch changes the use of the bool type back to the regular unsigned:1 for the manage_xxx scsi device flags. This is marked as a fix and CC-stable to avoid issues with later eventual fixes in this area.
The second patch addresses an issue with system resume with devices that were runtime suspended. For ATA devices, this leads to a disk still being reported as suspended while it is in fact spun up due to how ATA resume is done (port reset).
Damien Le Moal (2):
scsi: Change scsi device boolean fields to single bit flags
scsi: sd: fix system start for ATA devices
drivers/ata/libata-scsi.c | 9 +++++++-- drivers/firewire/sbp2.c | 6 +++--- drivers/scsi/sd.c | 9 ++++++++- include/scsi/scsi_device.h | 12 +++++++++--- 4 files changed, 27 insertions(+), 9 deletions(-)
Signature · 1 line
2.42.0
Damien Le Moal
maintainerPATCH 1/2 scsi: Change scsi device boolean fields to single bit flags
Commit 3cc2ffe5c16d ("scsi: sd: Differentiate system and runtime start/stop management") changed the single bit manage_start_stop flag into 2 boolean fields of the SCSI device structure. Commit 24eca2dce0f8 ("scsi: sd: Introduce manage_shutdown device flag") introduced the manage_shutdown boolean field for the same structure. Together, these 2 commits increase the size of struct scsi_device by 8 bytes by using booleans instead of defining the manage_xxx fields as single bit flags, similarly to other flags of this structure.
Avoid this unnecessary structure size increase and be consistent with the definition of other flags by reverting the definitions of the manage_xxx fields as single bit flags.
--- drivers/ata/libata-scsi.c | 4 ++-- drivers/firewire/sbp2.c | 6 +++--- include/scsi/scsi_device.h | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-)
drivers/ata/libata-scsi.c +2 -2
@@ -1056,8 +1056,8 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)* and resume and shutdown only. For system level suspend/resume, * devices power state is handled directly by libata EH. */ - sdev->manage_runtime_start_stop = true; - sdev->manage_shutdown = true; + sdev->manage_runtime_start_stop =1 ; + sdev->manage_shutdown =1 ; }/*
drivers/firewire/sbp2.c +3 -3
@@ -1519,9 +1519,9 @@ static int sbp2_scsi_slave_configure(struct scsi_device *sdev) sdev->use_10_for_rw =1 ;if (sbp2_param_exclusive_login) { - sdev->manage_system_start_stop = true; - sdev->manage_runtime_start_stop = true; - sdev->manage_shutdown = true; + sdev->manage_system_start_stop =1 ; + sdev->manage_runtime_start_stop =1 ; + sdev->manage_shutdown =1 ; }if (sdev->type ==TYPE_ROM )
include/scsi/scsi_device.h +3 -3
@@ -167,19 +167,19 @@ struct scsi_device {* power state for system suspend/resume (suspend to RAM and * hibernation) operations. */ -bool manage_system_start_stop; + unsigned manage_system_start_stop:1;/* * If true, let the high-level device driver (sd) manage the device * power state for runtime device suspand and resume operations. */ -bool manage_runtime_start_stop; + unsigned manage_runtime_start_stop:1;/* * If true, let the high-level device driver (sd) manage the device * power state for system shutdown (power off) operations. */ -bool manage_shutdown; + unsigned manage_shutdown:1; unsigned removable:1; unsigned changed:1; /* Data invalid due to media change */
Signature · 1 line
2.42.0
Re: [PATCH 1/2] scsi: Change scsi device boolean fields to single bit flags
Quoted 17 lines · Damien Le Moal
include/scsi/scsi_device.h +2
-2@@ -167,19 +167,19 @@ struct scsi_device {* power state for system suspend/resume (suspend to RAM and * hibernation) operations. */ -bool manage_system_start_stop; + unsigned manage_system_start_stop:1;/* * If true, let the high-level device driver (sd) manage the device * power state for runtime device suspand and resume operations. */ -bool manage_runtime_start_stop; + unsigned manage_runtime_start_stop:1;
include/scsi/scsi_device.h struct scsi_device { · line 185
Quoted 10 lines
include/scsi/scsi_device.h +1
-1/* * If true, let the high-level device driver (sd) manage the device * power state for system shutdown (power off) operations. */ -bool manage_shutdown; + unsigned manage_shutdown:1; unsigned removable:1; unsigned changed:1; /* Data invalid due to media change */
Is there any code that modifies the above flags from different threads simultaneously? I'm wondering whether this patch introduces one or more race conditions related to changing these flags.
Thanks,
Bart.
Damien Le Moal
maintainerRe: [PATCH 1/2] scsi: Change scsi device boolean fields to single bit flags
Quoted 26 lines · Bart Van Assche
Quoted 25 lines · Damien Le Moal
include/scsi/scsi_device.h +3
-3@@ -167,19 +167,19 @@ struct scsi_device {* power state for system suspend/resume (suspend to RAM and * hibernation) operations. */ -bool manage_system_start_stop; + unsigned manage_system_start_stop:1;/* * If true, let the high-level device driver (sd) manage the device * power state for runtime device suspand and resume operations. */ -bool manage_runtime_start_stop; + unsigned manage_runtime_start_stop:1;/* * If true, let the high-level device driver (sd) manage the device * power state for system shutdown (power off) operations. */ -bool manage_shutdown; + unsigned manage_shutdown:1;
Quoted 6 lines
include/scsi/scsi_device.h struct scsi_device { · line 185
Quoted 2 lines
include/scsi/scsi_device.h +0
-0unsigned removable:1; unsigned changed:1; /* Data invalid due to media change */Is there any code that modifies the above flags from different threads simultaneously? I'm wondering whether this patch introduces one or more race conditions related to changing these flags.
These are set once when the LLD probes and initialize the scsi device and never changed again by the LLD. The manage_xxx_start_stop flags can be changed through sysfs though, but doing so would be a mistake as things would stop working as expected... I do think that we should change these flags to be RO in sysfs.
Note that only libata and the firewire/sbp2 driver use these flags.
Quoted 4 lines
Thanks,
Bart.
Signature · 2 lines
Damien Le Moal Western Digital Research
Damien Le Moal
maintainerPATCH 2/2 scsi: sd: fix system start for ATA devices
In reply to Damien Le MoalTi 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.
--- drivers/ata/libata-scsi.c | 5 +++++ drivers/scsi/sd.c | 9 ++++++++- include/scsi/scsi_device.h | 6 ++++++ 3 files changed, 19 insertions(+), 1 deletion(-)
drivers/ata/libata-scsi.c +5 -0
@@ -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 ; }/*
drivers/scsi/sd.c +8 -1
@@ -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); }
include/scsi/scsi_device.h +6 -0
@@ -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 */
Signature · 1 line
2.42.0
Re: [PATCH 2/2] scsi: sd: fix system start for ATA devices
Quoted 1 line · Damien Le Moal
Ti is not always possible to keep a device in the runtime suspended
s/Ti/It? :-)
Quoted 15 lines
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_deviceflagforce_runtime_start_on_system_start. When set, this flag causessd_resume_system()to request a runtime resume operation for runtime suspended devices. This results in the user seeing the deviceruntime_stateas active after a system resume, thus correctly reflecting the device physical power state.
Quoted 3 lines
Fixes 9131bff6a9f1 ("scsi: core: pm: Only runtime resume if necessary")Signed-off-by Damien Le Moal <[email protected]>
[...]
MBR, Sergey
Damien Le Moal
maintainerRe: [PATCH 2/2] scsi: sd: fix system start for ATA devices
Quoted 5 lines · Sergey Shtylyov
Quoted 1 line · Damien Le Moal
Ti is not always possible to keep a device in the runtime suspended
s/Ti/It? :-)
Arg. Yes.
Quoted 22 lines
Quoted 16 lines
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_deviceflagforce_runtime_start_on_system_start. When set, this flag causessd_resume_system()to request a runtime resume operation for runtime suspended devices. This results in the user seeing the deviceruntime_stateas active after a system resume, thus correctly reflecting the device physical power state.Quoted 3 lines
Fixes 9131bff6a9f1 ("scsi: core: pm: Only runtime resume if necessary")Signed-off-by Damien Le Moal <[email protected]>[...]
MBR, Sergey
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH 2/2] scsi: sd: fix system start for ATA devices
Quoted 8 lines · Damien Le Moal
Quoted 5 lines · Sergey Shtylyov
Quoted 1 line · Damien Le Moal
Ti is not always possible to keep a device in the runtime suspended
s/Ti/It? :-)Arg. Yes.
While we are nitpicking :)
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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