Re: [PATCH 13/21] ufs: Remove several wmb() calls

["Asutosh Das (asd)" <asutoshd@codeaurora.org>]

On 7/1/2021 2:12 PM, Bart Van Assche wrote:
>  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>
> ---
>   drivers/scsi/ufs/ufshcd.c | 11 -----------
>   1 file changed, 11 deletions(-)
> 

Hi Bart,
Did you verify this change? I think we've seen OCS errors which were 
fixed with the wmb() in queuecommand.

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 7091798e6245..25ab603acad1 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2768,8 +2768,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:
> @@ -2879,8 +2877,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)) {
> @@ -2955,8 +2951,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);
> @@ -6514,9 +6508,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();
> @@ -6687,8 +6678,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);
>   	/*
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project