Re: [PATCH] scsi: ufs: Try to save power mode change and UIC cmd completion timeout

From: Can Guo <cang@codeaurora.org>
To: asutoshd@codeaurora.org, nguyenb@codeaurora.org,
	hongwus@codeaurora.org, rnayak@codeaurora.org,
	linux-scsi@vger.kernel.org, kernel-team@android.com,
	saravanak@google.com, salyzyn@google.com, cang@codeaurora.org
Cc: Alim Akhtar <alim.akhtar@samsung.com>,
	Avri Altman <avri.altman@wdc.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Stanley Chu <stanley.chu@mediatek.com>,
	Bean Huo <beanhuo@micron.com>,
	Bart Van Assche <bvanassche@acm.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] scsi: ufs: Try to save power mode change and UIC cmd completion timeout
Date: Tue, 03 Nov 2020 14:27:26 +0800	[thread overview]
Message-ID: <ba1a544b827cd290d1a48e8307a8e2d0@codeaurora.org> (raw)
In-Reply-To: <1604384682-15837-4-git-send-email-cang@codeaurora.org>

Sorry, please ignore this specific change, which is wrongly sent out.

On 2020-11-03 14:24, Can Guo wrote:
> Use the uic_cmd->cmd_active as a flag to track the lifecycle of an UIC 
> cmd.
> The flag is set before send the UIC cmd and cleared after the 
> completion is
> raised in IRQ handler. For a power mode change operation, including 
> hibern8
> enter/exit, the flag is cleared only after hba->uic_async_done 
> completion
> is raised. When completion timeout happens, if the flag is cleared, 
> instead
> of returning timeout error, simply ignore it.
> 
> Change-Id: Ie3cd6ae6221a44619925fb2cf78136a5617fdd5d
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 252e022..8b291c3 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2131,10 +2131,20 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
>  	unsigned long flags;
> 
>  	if (wait_for_completion_timeout(&uic_cmd->done,
> -					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
> +					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
>  		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
> -	else
> +	} else {
>  		ret = -ETIMEDOUT;
> +		dev_err(hba->dev,
> +			"uic cmd 0x%x with arg3 0x%x completion timeout\n",
> +			uic_cmd->command, uic_cmd->argument3);
> +
> +		if (!uic_cmd->cmd_active) {
> +			dev_err(hba->dev, "%s: UIC cmd has been completed, return the 
> result\n",
> +				__func__);
> +			ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
> +		}
> +	}
> 
>  	spin_lock_irqsave(hba->host->host_lock, flags);
>  	hba->active_uic_cmd = NULL;
> @@ -2166,6 +2176,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd,
>  	if (completion)
>  		init_completion(&uic_cmd->done);
> 
> +	uic_cmd->cmd_active = 1;
>  	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
> 
>  	return 0;
> @@ -3944,10 +3955,18 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
> *hba, struct uic_command *cmd)
>  		dev_err(hba->dev,
>  			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
>  			cmd->command, cmd->argument3);
> +
> +		if (!cmd->cmd_active) {
> +			dev_err(hba->dev, "%s: Power Mode Change operation has been
> completed, go check UPMCRS\n",
> +				__func__);
> +			goto check_upmcrs;
> +		}
> +
>  		ret = -ETIMEDOUT;
>  		goto out;
>  	}
> 
> +check_upmcrs:
>  	status = ufshcd_get_upmcrs(hba);
>  	if (status != PWR_LOCAL) {
>  		dev_err(hba->dev,
> @@ -5060,11 +5079,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct
> ufs_hba *hba, u32 intr_status)
>  		hba->active_uic_cmd->argument3 =
>  			ufshcd_get_dme_attr_val(hba);
>  		complete(&hba->active_uic_cmd->done);
> +		if (!hba->uic_async_done)
> +			hba->active_uic_cmd->cmd_active = 0;
>  		retval = IRQ_HANDLED;
>  	}
> 
>  	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
>  		complete(hba->uic_async_done);
> +		hba->active_uic_cmd->cmd_active = 0;
>  		retval = IRQ_HANDLED;
>  	}
>  	return retval;