| Message ID | 1620885319-15151-7-git-send-email-cang@codeaurora.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Complementary changes for error handling | expand |
On 5/12/21 10:55 PM, Can Guo wrote: > UFS error handling now is doing more than just re-probing, but also sending > scsi cmds, e.g., for clearing UACs, and recovering runtime PM error, which > may change runtime status of scsi devices. To protect system suspend/resume > from being disturbed by error handling, move the host_sem from wl pm ops > to ufshcd_suspend_prepare() and ufshcd_resume_complete(). In ufshcd.h I found the following: * @host_sem: semaphore used to serialize concurrent contexts That's the wrong way to use a synchronization object. A synchronization object must protect data instead of code. Does host_sem perhaps need to be split into multiple synchronization objects? Thanks, Bart.
Hi Bart, On 2021-05-14 11:55, Bart Van Assche wrote: > On 5/12/21 10:55 PM, Can Guo wrote: >> UFS error handling now is doing more than just re-probing, but also >> sending >> scsi cmds, e.g., for clearing UACs, and recovering runtime PM error, >> which >> may change runtime status of scsi devices. To protect system >> suspend/resume >> from being disturbed by error handling, move the host_sem from wl pm >> ops >> to ufshcd_suspend_prepare() and ufshcd_resume_complete(). > > In ufshcd.h I found the following: > > * @host_sem: semaphore used to serialize concurrent contexts > > That's the wrong way to use a synchronization object. A synchronization > object must protect data instead of code. Does host_sem perhaps need to > be split into multiple synchronization objects? Thanks for the comments. These contexts are changing critical data and registers, so the sem is used to protect data actually, just like the scaling_lock protecting scaling and cmd transations. Thanks, Can Guo. > > Thanks, > > Bart.
On 5/16/21 8:22 PM, Can Guo wrote: > Hi Bart, > > On 2021-05-14 11:55, Bart Van Assche wrote: >> On 5/12/21 10:55 PM, Can Guo wrote: >>> UFS error handling now is doing more than just re-probing, but also >>> sending >>> scsi cmds, e.g., for clearing UACs, and recovering runtime PM error, >>> which >>> may change runtime status of scsi devices. To protect system >>> suspend/resume >>> from being disturbed by error handling, move the host_sem from wl pm ops >>> to ufshcd_suspend_prepare() and ufshcd_resume_complete(). >> >> In ufshcd.h I found the following: >> >> * @host_sem: semaphore used to serialize concurrent contexts >> >> That's the wrong way to use a synchronization object. A synchronization >> object must protect data instead of code. Does host_sem perhaps need to >> be split into multiple synchronization objects? > > Thanks for the comments. These contexts are changing critical data and > registers, so the sem is used to protect data actually, just like the > scaling_lock protecting scaling and cmd transations. But where is the documentation that explains which data members are protected by hba->host_sem and which data members are protected by hba->host->host_lock? Was the host_lock protection perhaps introduced before scsi-mq was introduced? Before scsi-mq acquiring the host_lock was sufficient to serialize against ufshcd_queuecommand() but that is not sufficient when using scsi-mq. I want to verify whether locking is used correctly in the UFS driver but without documentation of which synchronization object protects which data members that is not possible. Thanks, Bart.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 631c5f8..a6313cf40 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -8991,16 +8991,13 @@ static int ufshcd_wl_suspend(struct device *dev) ktime_t start = ktime_get(); hba = shost_priv(sdev->host); - down(&hba->host_sem); if (pm_runtime_suspended(dev)) goto out; ret = __ufshcd_wl_suspend(hba, UFS_SYSTEM_PM); - if (ret) { + if (ret) dev_err(&sdev->sdev_gendev, "%s failed: %d\n", __func__, ret); - up(&hba->host_sem); - } out: if (!ret) @@ -9033,7 +9030,6 @@ static int ufshcd_wl_resume(struct device *dev) hba->curr_dev_pwr_mode, hba->uic_link_state); if (!ret) hba->is_wl_sys_suspended = false; - up(&hba->host_sem); return ret; } #endif @@ -9600,6 +9596,7 @@ void ufshcd_resume_complete(struct device *dev) ufshcd_rpmb_rpm_put(hba); hba->rpmb_complete_put = false; } + up(&hba->host_sem); } EXPORT_SYMBOL_GPL(ufshcd_resume_complete); @@ -9626,6 +9623,7 @@ int ufshcd_suspend_prepare(struct device *dev) ufshcd_rpmb_rpm_get_sync(hba); hba->rpmb_complete_put = true; } + down(&hba->host_sem); return 0; } EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
UFS error handling now is doing more than just re-probing, but also sending scsi cmds, e.g., for clearing UACs, and recovering runtime PM error, which may change runtime status of scsi devices. To protect system suspend/resume from being disturbed by error handling, move the host_sem from wl pm ops to ufshcd_suspend_prepare() and ufshcd_resume_complete(). Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufshcd.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)