[PATCH 0/2] Fix runtime suspended device resume

[PATCH 0/2] Fix runtime suspended device resume

[Damien Le Moal]

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(-)

-- 
2.42.0

[PATCH 1/2] scsi: Change scsi device boolean fields to single bit flags

[Damien Le Moal]

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.

Fixes: 3cc2ffe5c16d ("scsi: sd: Differentiate system and runtime start/stop management")
Fixes: 24eca2dce0f8 ("scsi: sd: Introduce manage_shutdown device flag")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-scsi.c  | 4 ++--
 drivers/firewire/sbp2.c    | 6 +++---
 include/scsi/scsi_device.h | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index c10ff8985203..63317449f6ea 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -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;
 	}
 
 	/*
diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 7edf2c95282f..e779d866022b 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -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)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 10480eb582b2..1fb460dfca0c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -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 */
-- 
2.42.0

Re: [PATCH 1/2] scsi: Change scsi device boolean fields to single bit flags

[Bart Van Assche]

On 11/19/23 23:35, Damien Le Moal wrote:
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 10480eb582b2..1fb460dfca0c 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -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 */

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.

Re: [PATCH 1/2] scsi: Change scsi device boolean fields to single bit flags

[Damien Le Moal]

On 11/21/23 01:06, Bart Van Assche wrote:
> On 11/19/23 23:35, Damien Le Moal wrote:
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 10480eb582b2..1fb460dfca0c 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -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 */
> 
> 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.

> 
> Thanks,
> 
> Bart.

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/2] scsi: sd: fix system start for ATA devices

[Damien Le Moal]

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(-)

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 */
-- 
2.42.0

Re: [PATCH 2/2] scsi: sd: fix system start for ATA devices

[Sergey Shtylyov]

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

Re: [PATCH 2/2] scsi: sd: fix system start for ATA devices

[Damien Le Moal]

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

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] scsi: sd: fix system start for ATA devices

[Niklas Cassel]

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