Hi Can,
On Tue, 2020-11-03 at 16:01 +0800, Can Guo wrote:
> Hi Stanley,
>
> On 2020-11-03 15:20, Stanley Chu wrote:
> > 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?
> >
>
> Thanks for your quick response.
>
> In the case of PMC, UIC cmd completion IRQ comes first, then power
> status change IRQ comes next. In this case, an UIC cmd's lifecyle
> ends only after the power status change IRQ comes [1].
>
> I guess you may want to say that in current code since we have
> masked UIC cmd completion IRQ in the case of a PMC operation, so
> no need to check it here since we won't be here anyways before
> power status change IRQ comes. So, removing the check here
> definitely works, and then we won't even need below line as well.
>
You read my mind : )
> 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;
>
> If my guess is right, my opinion is that the current change may
> be more readable and comprehensive as it strictly follows my
> description in [1]. What do you think?
Both looks fine to me.
Thanks for the detailed description.
Stanley Chu
>
> Thanks,
>
> Can Guo.
>
> >> + 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
