On 05/10/2021 21:52, Bart Van Assche wrote:
> On 10/5/21 6:44 AM, Adrian Hunter wrote:
>> UFS SCSI devices make use of device links to establish PM dependencies.
>> However, SCSI PM will force devices' runtime PM state to be active during
>> system resume. That can break runtime PM dependencies for UFS devices.
>> Fix by adding a flag 'preserve_rpm' to let UFS SCSI devices opt-out of
>> the unwanted behaviour.
>>
>> Fixes: b294ff3e34490f ("scsi: ufs: core: Enable power management for wlun")
>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> ---
>> drivers/scsi/scsi_pm.c | 16 +++++++++++-----
>> drivers/scsi/ufs/ufshcd.c | 1 +
>> include/scsi/scsi_device.h | 1 +
>> 3 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>> index 3717eea37ecb..0557c1ad304d 100644
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -73,13 +73,22 @@ static int scsi_dev_type_resume(struct device *dev,
>> int (*cb)(struct device *, const struct dev_pm_ops *))
>> {
>> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> + struct scsi_device *sdev = NULL;
>> + bool preserve_rpm = false;
>> int err = 0;
>> + if (scsi_is_sdev_device(dev)) {
>> + sdev = to_scsi_device(dev);
>> + preserve_rpm = sdev->preserve_rpm;
>> + if (preserve_rpm && pm_runtime_suspended(dev))
>> + return 0;
>> + }
>> +
>> err = cb(dev, pm);
>> scsi_device_resume(to_scsi_device(dev));
>> dev_dbg(dev, "scsi resume: %d\n", err);
>> - if (err == 0) {
>> + if (err == 0 && !preserve_rpm) {
>> pm_runtime_disable(dev);
>> err = pm_runtime_set_active(dev);
>> pm_runtime_enable(dev);
>> @@ -91,11 +100,8 @@ static int scsi_dev_type_resume(struct device *dev,
>> *
>> * The resume hook will correct runtime PM status of the disk.
>> */
>> - if (!err && scsi_is_sdev_device(dev)) {
>> - struct scsi_device *sdev = to_scsi_device(dev);
>> -
>> + if (!err && sdev)
>> blk_set_runtime_active(sdev->request_queue);
>> - }
>> }
>> return err;
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index d91a405fd181..b70f566f7f8a 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -5016,6 +5016,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>> pm_runtime_get_noresume(&sdev->sdev_gendev);
>> else if (ufshcd_is_rpm_autosuspend_allowed(hba))
>> sdev->rpm_autosuspend = 1;
>> + sdev->preserve_rpm = 1;
>> ufshcd_crypto_setup_rq_keyslot_manager(hba, q);
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 09a17f6e93a7..47eb30a6b7b2 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -197,6 +197,7 @@ struct scsi_device {
>> unsigned no_read_disc_info:1; /* Avoid READ_DISC_INFO cmds */
>> unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
>> unsigned try_rc_10_first:1; /* Try READ_CAPACACITY_10 first */
>> + unsigned preserve_rpm:1; /* Preserve runtime PM */
>> unsigned security_supported:1; /* Supports Security Protocols */
>> unsigned is_visible:1; /* is the device visible in sysfs */
>> unsigned wce_default_on:1; /* Cache is ON by default */
>
> So a new flag is added in struct scsi_device and that flag is only used by
> the UFS driver? I'm less than enthusiast about this patch. I think that the
> SCSI core needs to be modified such that system suspend and resume is
> separated from runtime suspend and resume.
That is a tidy-up, whereas this patch is a fix needed also in stable
kernels.
> The following code:
>
> if (err == 0) {
> pm_runtime_disable(dev);
> err = pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> [ ... ]
> }
>
> has been introduced in scsi_dev_type_resume() by commit 3c31b52f96f7
> ("scsi: async sd resume"). I'm in favor of removing that code.
Presumably that code is necessary. Trying to remove it really seems a
separate issue.
