diff mbox series

[v1,5/6] scsi: ufs: Let host_sem cover the entire system suspend/resume

Message ID 1620885319-15151-7-git-send-email-cang@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Complementary changes for error handling | expand

Commit Message

Can Guo May 13, 2021, 5:55 a.m. UTC
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(-)

Comments

Bart Van Assche May 14, 2021, 3:55 a.m. UTC | #1
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.
Can Guo May 17, 2021, 3:22 a.m. UTC | #2
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.
Bart Van Assche May 17, 2021, 3:05 p.m. UTC | #3
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 mbox series

Patch

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);