| Message ID | 20240329-ufs-reset-ensure-effect-before-delay-v5-10-181252004586@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: Remove overzealous memory barriers | expand |
On Fri, Mar 29, 2024 at 03:46:52PM -0500, Andrew Halaney wrote: > Currently, the doorbell is written to and a wmb() is used to commit it > immediately. > > wmb() ensures that the write completes before following writes occur, > but completion doesn't mean that it isn't stored in a buffer somewhere. > The recommendation for ensuring this bit has taken effect on the device > is to perform a read back to force it to make it all the way to the > device. This is documented in device-io.rst and a talk by Will Deacon on > this can be seen over here: > > https://youtu.be/i6DayghhA8Q?si=MiyxB5cKJXSaoc01&t=1678 > > But, completion and taking effect aren't necessary to guarantee here. > > There's already other examples of the doorbell being rung that don't do > this. The writel() of the doorbell guarantees prior writes by this > thread (to the request being setup for example) complete prior to the > ringing of the doorbell, and the following > wait_for_completion_io_timeout() doesn't require any special memory > barriers either. > > With that in mind, just remove the wmb() altogether here. > > Fixes: ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the doorbell") > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > drivers/ufs/core/ufshcd.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index dfa4f827766a..a2f2941450fd 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -7090,10 +7090,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, > > /* send command to the controller */ > __set_bit(task_tag, &hba->outstanding_tasks); > - > ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL); > - /* Make sure that doorbell is committed immediately */ > - wmb(); > > spin_unlock_irqrestore(host->host_lock, flags); > > > -- > 2.44.0 > >
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index dfa4f827766a..a2f2941450fd 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -7090,10 +7090,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, /* send command to the controller */ __set_bit(task_tag, &hba->outstanding_tasks); - ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL); - /* Make sure that doorbell is committed immediately */ - wmb(); spin_unlock_irqrestore(host->host_lock, flags);
Currently, the doorbell is written to and a wmb() is used to commit it immediately. wmb() ensures that the write completes before following writes occur, but completion doesn't mean that it isn't stored in a buffer somewhere. The recommendation for ensuring this bit has taken effect on the device is to perform a read back to force it to make it all the way to the device. This is documented in device-io.rst and a talk by Will Deacon on this can be seen over here: https://youtu.be/i6DayghhA8Q?si=MiyxB5cKJXSaoc01&t=1678 But, completion and taking effect aren't necessary to guarantee here. There's already other examples of the doorbell being rung that don't do this. The writel() of the doorbell guarantees prior writes by this thread (to the request being setup for example) complete prior to the ringing of the doorbell, and the following wait_for_completion_io_timeout() doesn't require any special memory barriers either. With that in mind, just remove the wmb() altogether here. Fixes: ad1a1b9cd67a ("scsi: ufs: commit descriptors before setting the doorbell") Signed-off-by: Andrew Halaney <ahalaney@redhat.com> --- drivers/ufs/core/ufshcd.c | 3 --- 1 file changed, 3 deletions(-)