Re: [PATCH v4 04/23] scsi: sd: Differentiate system and runtime start/stop management

[Damien Le Moal <dlemoal@kernel.org>]

On 2023/09/21 14:45, Bart Van Assche wrote:
> On 9/20/23 06:54, Damien Le Moal wrote:
>> The underlying device and driver of a scsi disk may have different
>> system and runtime power mode control requirements. This is because
>> runtime power management affects only the scsi disk, while sustem level
>> power management affects all devices, including the controller for the
>> scsi disk.
>>
>> For instance, issuing a START STOP UNIT command when a scsi disk is
>> runtime suspended and resumed is fine: the command is translated to a
>> STANDBY IMMEDIATE command to spin down the ATA disk and to a VERIFY
>> command to wake it up. The scsi disk runtime operations have no effect
>> on the ata port device used to connect the ATA disk. However, for
>> system suspend/resume operations, the ATA port used to connect the
>> device will also be suspended and resumed, with the resum operation
> 
> resum -> resume
> 
>> requiring re-validating the device link and the device itseld. In this
> 
> itseld -> itself
> 
>> -static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
>> +static inline bool sd_do_start_stop(struct scsi_device *sdev, bool runtime)
>> +{
>> +	return (sdev->manage_system_start_stop && !runtime) ||
>> +		(sdev->manage_runtime_start_stop && runtime);
>> +}
> 
> This function wouldn't be necessary if the sd_suspend_common() callers 
> would pass sdev->manage_system_start_stop / 
> sdev->manage_runtime_start_stop as an additional argument to 
> sd_suspend_common().

Sure, but then we need to get the sdkp for the struct device and use
sdkp->device in both sd_suspend_runtime() and sd_suspend_system(). This is in my
opinion not as clean as the little inline helper. But if you insist, I can make
that change.

> 
>> -		if (ignore_stop_errors)
>> +		if (!runtime)
>>   			ret = 0;
>>   	}
> 
> The old code was self-documenting. If the name of the "runtime" argument 
> is retained, a comment above this if-statement that explains why stop 
> errors are ignored during a system suspend would be welcome.

There is a comment already (unchanged) above the call to sd_start_stop_device(),
one line above the "if".

> 
> Otherwise this patch looks good to me.
> 
> Thanks,
> 
> Bart.
> 
> 

-- 
Damien Le Moal
Western Digital Research