Re: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun

[Asutosh Das (asd)]

On 4/9/2021 3:07 AM, Adrian Hunter wrote:
> On 9/04/21 5:27 am, Daejun Park wrote:
> > Hi Asutosh Das,
> >
> > > During runtime-suspend of ufs host, the scsi devices are
> > > already suspended and so are the queues associated with them.
> > > But the ufs host sends SSU (START_STOP_UNIT) to wlun
> > > during its runtime-suspend.
> > > During the process blk_queue_enter checks if the queue is not in
> > > suspended state. If so, it waits for the queue to resume, and never
> > > comes out of it.
> > > The commit
> > > (d55d15a33: scsi: block: Do not accept any requests while suspended)
> > > adds the check if the queue is in suspended state in blk_queue_enter().
> > >
> > > Call trace:
> > > __switch_to+0x174/0x2c4
> > > __schedule+0x478/0x764
> > > schedule+0x9c/0xe0
> > > blk_queue_enter+0x158/0x228
> > > blk_mq_alloc_request+0x40/0xa4
> > > blk_get_request+0x2c/0x70
> > > __scsi_execute+0x60/0x1c4
> > > ufshcd_set_dev_pwr_mode+0x124/0x1e4
> > > ufshcd_suspend+0x208/0x83c
> > > ufshcd_runtime_suspend+0x40/0x154
> > > ufshcd_pltfrm_runtime_suspend+0x14/0x20
> > > pm_generic_runtime_suspend+0x28/0x3c
> > > __rpm_callback+0x80/0x2a4
> > > rpm_suspend+0x308/0x614
> > > rpm_idle+0x158/0x228
> > > pm_runtime_work+0x84/0xac
> > > process_one_work+0x1f0/0x470
> > > worker_thread+0x26c/0x4c8
> > > kthread+0x13c/0x320
> > > ret_from_fork+0x10/0x18
> > >
> > > Fix this by registering ufs device wlun as a scsi driver and
> > > registering it for block runtime-pm. Also make this as a
> > > supplier for all other luns. That way, this device wlun
> > > suspends after all the consumers and resumes after
> > > hba resumes.
> > >
> > > Co-developed-by: Can Guo <cang@xxxxxxxxxxxxxx>
> > > Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> > > Signed-off-by: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
> > > ---
> > > drivers/scsi/ufs/cdns-pltfrm.c     |   2 +
> > > drivers/scsi/ufs/tc-dwc-g210-pci.c |   2 +
> > > drivers/scsi/ufs/ufs-debugfs.c     |   6 +-
> > > drivers/scsi/ufs/ufs-debugfs.h     |   2 +-
> > > drivers/scsi/ufs/ufs-exynos.c      |   2 +
> > > drivers/scsi/ufs/ufs-hisi.c        |   2 +
> > > drivers/scsi/ufs/ufs-mediatek.c    |  12 +-
> > > drivers/scsi/ufs/ufs-qcom.c        |   2 +
> > > drivers/scsi/ufs/ufs_bsg.c         |   6 +-
> > > drivers/scsi/ufs/ufshcd-pci.c      |  36 +--
> > > drivers/scsi/ufs/ufshcd.c          | 642 ++++++++++++++++++++++++++-----------
> > > drivers/scsi/ufs/ufshcd.h          |   6 +
> > > include/trace/events/ufs.h         |  20 ++
> > > 13 files changed, 509 insertions(+), 231 deletions(-)
> >
> > In this patch, you changed pm_runtime_{get, put}_sync to scsi_autopm_{get, put}_device.
> > But, scsi_autopm_get_device() calls pm_runtime_put_sync() in case of error
> > of pm_runtime_get_sync(). So, pm_runtime_put_sync() can be called twice if
> > scsi_autopm_get_device has error.
>
> Also it might be tidy to make wrappers e.g.
>
> static inline int ufshcd_rpm_get_sync(struct ufs_hba *hba)
> {
>      return pm_runtime_get_sync(&hba->sdev_ufs_device->sdev_gendev);
> }
>     
> static inline int ufshcd_rpm_put(struct ufs_hba *hba)
> {
>      return pm_runtime_put(&hba->sdev_ufs_device->sdev_gendev);
> }
>
> static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba)
> {
>      return pm_runtime_put_sync(&hba->sdev_ufs_device->sdev_gendev);
> }
>
> And also consider matching: e.g.
>
> 	pm_runtime_put(hba->dev)	to	ufshcd_rpm_put(hba)
> 	pm_runtime_put_sync(hba->dev)	to	ufshcd_rpm_put_sync(hba)

Ok, I'll push the changes shortly.

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project