Hi Can,
Except for below nit, otherwise looks good to me.
Reviewed-by: Stanley Chu <stanley.chu@xxxxxxxxxxxx>
On Mon, 2020-11-02 at 22:24 -0800, 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 in IRQ handler. When a
> PMC or UIC cmd completion timeout happens, if the flag is not set, instead
> of returning timeout error, we still treat it as a successful operation.
> This is to deal with the scenario in which completion has been raised but
> the one waiting for the completion cannot be awaken in time due to kernel
> scheduling problem.
>
> Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> ---
> drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
> drivers/scsi/ufs/ufshcd.h | 2 ++
> 2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index efa7d86..7f33310 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2122,10 +2122,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;
> @@ -2157,6 +2167,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;
> @@ -3828,10 +3839,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,
> @@ -4923,11 +4942,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
> ufshcd_get_uic_cmd_result(hba);
> hba->active_uic_cmd->argument3 =
> ufshcd_get_dme_attr_val(hba);
> + if (!hba->uic_async_done)
Is this check necessary?
> + hba->active_uic_cmd->cmd_active = 0;
> complete(&hba->active_uic_cmd->done);
> retval = IRQ_HANDLED;
> }
>
> if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
> + hba->active_uic_cmd->cmd_active = 0;
> complete(hba->uic_async_done);
> retval = IRQ_HANDLED;
> }
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 66e5338..be982ed 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -64,6 +64,7 @@ enum dev_cmd_type {
> * @argument1: UIC command argument 1
> * @argument2: UIC command argument 2
> * @argument3: UIC command argument 3
> + * @cmd_active: Indicate if UIC command is outstanding
> * @done: UIC command completion
> */
> struct uic_command {
> @@ -71,6 +72,7 @@ struct uic_command {
> u32 argument1;
> u32 argument2;
> u32 argument3;
> + int cmd_active;
> struct completion done;
> };
>
Thanks,
Stanley Chu
