[v3] scsi: ufs: Remove overzealous memory barriers

[PATCH RFC v3 11/11] scsi: ufs: core: Perform read back before writing run/stop regs

Posted by Andrew Halaney 1 year, 12 months ago

Currently a wmb() is used to ensure that writes to the
UTP_TASK_REQ_LIST_BASE* regs are completed prior to following writes to
the run/stop registers.

wmb() ensure that the write completes, but completion doesn't mean that
it isn't stored in a buffer somewhere. The recommendation for
ensuring the bits have 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

Let's do that to ensure the bits hit the device. Because the wmb()'s
purpose wasn't to add extra ordering (on top of the ordering guaranteed
by writel()/readl()), it can safely be removed.

Fixes: 897efe628d7e ("scsi: ufs: add missing memory barriers")
Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 drivers/ufs/core/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index caebd589e08c..7c1975a1181f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4726,7 +4726,7 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba)
 	 * Make sure base address and interrupt setup are updated before
 	 * enabling the run/stop registers below.
 	 */
-	wmb();
+	ufshcd_readl(hba, REG_UTP_TASK_REQ_LIST_BASE_H);
 
 	/*
 	 * UCRDY, UTMRLDY and UTRLRDY bits must be 1

-- 
2.43.0

Re: [PATCH RFC v3 11/11] scsi: ufs: core: Perform read back before writing run/stop regs

Posted by Manivannan Sadhasivam 1 year, 11 months ago

On Thu, Dec 21, 2023 at 01:09:57PM -0600, Andrew Halaney wrote:
> Currently a wmb() is used to ensure that writes to the
> UTP_TASK_REQ_LIST_BASE* regs are completed prior to following writes to
> the run/stop registers.
> 
> wmb() ensure that the write completes, but completion doesn't mean that
> it isn't stored in a buffer somewhere. The recommendation for
> ensuring the bits have 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
> 
> Let's do that to ensure the bits hit the device. Because the wmb()'s
> purpose wasn't to add extra ordering (on top of the ordering guaranteed
> by writel()/readl()), it can safely be removed.
> 
> Fixes: 897efe628d7e ("scsi: ufs: add missing memory barriers")
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
>  drivers/ufs/core/ufshcd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index caebd589e08c..7c1975a1181f 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4726,7 +4726,7 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba)
>  	 * Make sure base address and interrupt setup are updated before
>  	 * enabling the run/stop registers below.
>  	 */
> -	wmb();
> +	ufshcd_readl(hba, REG_UTP_TASK_REQ_LIST_BASE_H);

I don't think the readback is really needed here. Because, the dependency is
with UTP registers and both should be in the same domain. So there is no way the
following write can happen before prior UTP write completion.

- Mani

>  
>  	/*
>  	 * UCRDY, UTMRLDY and UTRLRDY bits must be 1
> 
> -- 
> 2.43.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH RFC v3 11/11] scsi: ufs: core: Perform read back before writing run/stop regs

Posted by Can Guo 1 year, 12 months ago


On 12/22/2023 3:09 AM, Andrew Halaney wrote:
> Currently a wmb() is used to ensure that writes to the
> UTP_TASK_REQ_LIST_BASE* regs are completed prior to following writes to
> the run/stop registers.
> 
> wmb() ensure that the write completes, but completion doesn't mean that
> it isn't stored in a buffer somewhere. The recommendation for
> ensuring the bits have 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
> 
> Let's do that to ensure the bits hit the device. Because the wmb()'s
> purpose wasn't to add extra ordering (on top of the ordering guaranteed
> by writel()/readl()), it can safely be removed.
> 
> Fixes: 897efe628d7e ("scsi: ufs: add missing memory barriers")
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
>   drivers/ufs/core/ufshcd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index caebd589e08c..7c1975a1181f 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4726,7 +4726,7 @@ int ufshcd_make_hba_operational(struct ufs_hba *hba)
>   	 * Make sure base address and interrupt setup are updated before
>   	 * enabling the run/stop registers below.
>   	 */
> -	wmb();
> +	ufshcd_readl(hba, REG_UTP_TASK_REQ_LIST_BASE_H);
>   
>   	/*
>   	 * UCRDY, UTMRLDY and UTRLRDY bits must be 1
> 
Reviewed-by: Can Guo <quic_cang@quicinc.com>

Re: [PATCH RFC v3 11/11] scsi: ufs: core: Perform read back before writing run/stop regs

Posted by Bart Van Assche 1 year, 12 months ago

On 12/21/23 11:09, Andrew Halaney wrote:
> Currently a wmb() is used to ensure that writes to the
> UTP_TASK_REQ_LIST_BASE* regs are completed prior to following writes to
> the run/stop registers.
> 
> wmb() ensure that the write completes, but completion doesn't mean that
> it isn't stored in a buffer somewhere. The recommendation for
> ensuring the bits have 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
> 
> Let's do that to ensure the bits hit the device. Because the wmb()'s
> purpose wasn't to add extra ordering (on top of the ordering guaranteed
> by writel()/readl()), it can safely be removed.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>