RE: [PATCH v3 09/18] scsi: ufs: Remove several wmb() calls

From: Avri Altman <Avri.Altman@wdc.com>
To: Bart Van Assche <bvanassche@acm.org>,
	"Martin K . Petersen" <martin.petersen@oracle.com>
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Stanley Chu <stanley.chu@mediatek.com>,
	Can Guo <cang@codeaurora.org>,
	Asutosh Das <asutoshd@codeaurora.org>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Bean Huo <beanhuo@micron.com>
Subject: RE: [PATCH v3 09/18] scsi: ufs: Remove several wmb() calls
Date: Sun, 25 Jul 2021 13:20:19 +0000	[thread overview]
Message-ID: <DM6PR04MB6575F9CC44AFA830C4442FB0FCE79@DM6PR04MB6575.namprd04.prod.outlook.com> (raw)
In-Reply-To: <20210722033439.26550-10-bvanassche@acm.org>

 
> From arch/arm/include/asm/io.h
> 
>   #define __iowmb() wmb()
>   [ ... ]
>   #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })
> 
> From Documentation/memory-barriers.txt: "Note that, when using writel(),
> a
> prior wmb() is not needed to guarantee that the cache coherent memory
> writes have completed before writing to the MMIO region."
> 
> In other words, calling wmb() before writel() is not necessary. Hence
> remove the wmb() calls that precede a writel() call. Remove the wmb()
> calls that precede a ufshcd_send_command() call since the latter function
> uses writel(). Remove the wmb() call from ufshcd_wait_for_dev_cmd()
> since the following chain of events guarantees that the CPU will see
> up-to-date LRB values:
> * UFS controller writes to host memory.
> * UFS controller posts completion interrupt after the memory writes from
>   the previous step are visible to the CPU.
> * complete(hba->dev_cmd.complete) is called from the UFS interrupt
> handler.
> * The wait_for_completion(hba->dev_cmd.complete) call in
>   ufshcd_wait_for_dev_cmd() returns.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Tested-by: Avri altman <avri.altman@wdc.com>


> ---
>  drivers/scsi/ufs/ufshcd.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f467630be7df..d1c3a984d803 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2769,8 +2769,6 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
>                 ufshcd_release(hba);
>                 goto out;
>         }
> -       /* Make sure descriptors are ready before ringing the doorbell */
> -       wmb();
> 
>         ufshcd_send_command(hba, tag);
>  out:
> @@ -2880,8 +2878,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba
> *hba,
>         time_left = wait_for_completion_timeout(hba->dev_cmd.complete,
>                         msecs_to_jiffies(max_timeout));
> 
> -       /* Make sure descriptors are ready before ringing the doorbell */
> -       wmb();
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         hba->dev_cmd.complete = NULL;
>         if (likely(time_left)) {
> @@ -2960,8 +2956,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>         hba->dev_cmd.complete = &wait;
> 
>         ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp-
> >ucd_req_ptr);
> -       /* Make sure descriptors are ready before ringing the doorbell */
> -       wmb();
> 
>         ufshcd_send_command(hba, tag);
>         err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
> @@ -6521,9 +6515,6 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba
> *hba,
>         /* send command to the controller */
>         __set_bit(task_tag, &hba->outstanding_tasks);
> 
> -       /* Make sure descriptors are ready before ringing the task doorbell */
> -       wmb();
> -
>         ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL);
>         /* Make sure that doorbell is committed immediately */
>         wmb();
> @@ -6695,8 +6686,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>         hba->dev_cmd.complete = &wait;
> 
>         ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp-
> >ucd_req_ptr);
> -       /* Make sure descriptors are ready before ringing the doorbell */
> -       wmb();
> 
>         ufshcd_send_command(hba, tag);
>         /*