Re: [PATCH v12 1/2] scsi: ufs: Enable power management for wlun
From: Asutosh Das (asd)
Date: Fri Mar 19 2021 - 11:10:19 EST
On 3/18/2021 8:12 PM, Bart Van Assche wrote:
On 3/18/21 5:35 PM, Asutosh Das wrote:
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 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().
Hi Bart,
Thanks for the review comments.
What is the role of the WLUN during runtime suspend and why does a
command need to be sent to the WLUN during runtime suspend? Although it
is possible to derive this from the source code, please explain this in
the patch description.
Ok. Will explain it in the next version.
What does the acronym SSU stand for? This doesn't seem like a commonly
used kernel acronym to me so please expand that acronym.
START STOP UNIT.
Anyway, I'll expand it in the next version.
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.
That's an interesting solution.
-void __exit ufs_debugfs_exit(void)
+void ufs_debugfs_exit(void)
Is the above change related to the rest of this patch?
Yes, it's used to handle an error in ufshcd_core_init() function.
static struct platform_driver ufs_qcom_pltform = {
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 5b2bc1a..cbb5a90 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -97,7 +97,7 @@ static int ufs_bsg_request(struct bsg_job *job)
bsg_reply->reply_payload_rcv_len = 0;
- pm_runtime_get_sync(hba->dev);
+ scsi_autopm_get_device(hba->sdev_ufs_device);
Can the pm_runtime_get_sync() to scsi_autopm_get_device() changes be
moved into a separate patch?
I guess so. But then this patch would have issues when used independently.
+static inline bool is_rpmb_wlun(struct scsi_device *sdev)
+{
+ return (sdev->lun == ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN));
+}
Has this patch been verified with checkpatch? Checkpatch should have
reported the following for the above code:
return is not a function, parentheses are not required
Yes, it has been verified. But I didn't see any error reports.
Below is the o/p of checkpatch:
$ ./scripts/checkpatch.pl /tmp/up/ufs-pm-v12/*
------------------------------------------
/tmp/up/ufs-pm-v12/0000-cover-letter.patch
------------------------------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75
chars per line)
#107:
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project.
total: 0 errors, 1 warnings, 0 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or
--fix-inplace.
/tmp/up/ufs-pm-v12/0000-cover-letter.patch has style problems, please
review.
-----------------------------------------------------------------------
/tmp/up/ufs-pm-v12/0001-scsi-ufs-Enable-power-management-for-wlun.patch
-----------------------------------------------------------------------
total: 0 errors, 0 warnings, 1180 lines checked
/tmp/up/ufs-pm-v12/0001-scsi-ufs-Enable-power-management-for-wlun.patch
has no obvious style problems and is ready for submission.
---------------------------------------------------------------------
/tmp/up/ufs-pm-v12/0002-ufs-sysfs-Resume-the-proper-scsi-device.patch
---------------------------------------------------------------------
total: 0 errors, 0 warnings, 91 lines checked
/tmp/up/ufs-pm-v12/0002-ufs-sysfs-Resume-the-proper-scsi-device.patch
has no obvious style problems and is ready for submission.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
+static inline bool is_device_wlun(struct scsi_device *sdev)
+{
+ return (sdev->lun ==
+ ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN));
+}
Same comment here.
/*
- * Don't assume anything of pm_runtime_get_sync(), if
+ * Don't assume anything of resume, if
* resume fails, irq and clocks can be OFF, and powers
* can be OFF or in LPM.
*/
Please make better use of the horizontal space in the above comment by
making comment lines longer.
Ok Sure.
Thanks,
Bart.
-asd
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project