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
