diff mbox series

[4/5] scsi: ufs: core: Remove redundant wmb() in ufshcd_send_command()

Message ID 20220422132140.313390-5-manivannan.sadhasivam@linaro.org (mailing list archive)
State Superseded
Headers show
Series Qcom UFS driver updates | expand

Commit Message

Manivannan Sadhasivam April 22, 2022, 1:21 p.m. UTC
The wmb() inside ufshcd_send_command() is added to make sure that the
doorbell is committed immediately. This leads to couple of expectations:

1. The doorbell write should complete before the function return.
2. The doorbell write should not cross the function boundary.

2nd expectation is fullfilled by the Linux memory model as there is a
guarantee that the critical section won't cross the unlock (release)
operation.

1st expectation is not really needed here as there is no following read/
write that depends on the doorbell to be complete implicitly. Even if the
doorbell write is in a CPUs Write Buffer (WB), wmb() won't flush it. And
there is no real need of a WB flush here as well.

So let's get rid of the wmb() that seems redundant.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/scsi/ufs/ufshcd.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Bart Van Assche April 23, 2022, 5:19 a.m. UTC | #1
On 4/22/22 06:21, Manivannan Sadhasivam wrote:
> The wmb() inside ufshcd_send_command() is added to make sure that the
> doorbell is committed immediately.

That's not the purpose of the wmb() call so I think the comment is wrong.

> This leads to couple of expectations:
> 
> 1. The doorbell write should complete before the function return.
> 2. The doorbell write should not cross the function boundary.
> 
> 2nd expectation is fullfilled by the Linux memory model as there is a
> guarantee that the critical section won't cross the unlock (release)
> operation.

I think you meant that the writel() won't cross the unlock operation?

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 9349557b8a01..ec514a6c5393 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2116,9 +2116,6 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
>   	__set_bit(task_tag, &hba->outstanding_reqs);
>   	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>   	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> -
> -	/* Make sure that doorbell is committed immediately */
> -	wmb();
>   }

Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Manivannan Sadhasivam April 23, 2022, 11:46 a.m. UTC | #2
On Fri, Apr 22, 2022 at 10:19:14PM -0700, Bart Van Assche wrote:
> On 4/22/22 06:21, Manivannan Sadhasivam wrote:
> > The wmb() inside ufshcd_send_command() is added to make sure that the
> > doorbell is committed immediately.
> 
> That's not the purpose of the wmb() call so I think the comment is wrong.
> 
> > This leads to couple of expectations:
> > 
> > 1. The doorbell write should complete before the function return.
> > 2. The doorbell write should not cross the function boundary.
> > 
> > 2nd expectation is fullfilled by the Linux memory model as there is a
> > guarantee that the critical section won't cross the unlock (release)
> > operation.
> 
> I think you meant that the writel() won't cross the unlock operation?
> 

yes!

Thanks,
Mani

> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 9349557b8a01..ec514a6c5393 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -2116,9 +2116,6 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
> >   	__set_bit(task_tag, &hba->outstanding_reqs);
> >   	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> >   	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
> > -
> > -	/* Make sure that doorbell is committed immediately */
> > -	wmb();
> >   }
> 
> Anyway:
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9349557b8a01..ec514a6c5393 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2116,9 +2116,6 @@  void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
 	__set_bit(task_tag, &hba->outstanding_reqs);
 	ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	spin_unlock_irqrestore(&hba->outstanding_lock, flags);
-
-	/* Make sure that doorbell is committed immediately */
-	wmb();
 }
 
 /**