diff mbox series

[v5,10/11] scsi: ufs: core: Remove unnecessary wmb() after ringing doorbell

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

Commit Message

Andrew Halaney March 29, 2024, 8:46 p.m. UTC
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(-)

Comments

Manivannan Sadhasivam April 2, 2024, 4:49 a.m. UTC | #1
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 mbox series

Patch

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