diff mbox series

[v1,2/2] scsi: ufs: Try to save power mode change and UIC cmd completion timeout

Message ID 1604384682-15837-3-git-send-email-cang@codeaurora.org (mailing list archive)
State Accepted
Headers show
Series Two minor fixes for UFS driver | expand

Commit Message

Can Guo Nov. 3, 2020, 6:24 a.m. UTC
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@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 26 insertions(+), 2 deletions(-)

Comments

Stanley Chu Nov. 3, 2020, 7:20 a.m. UTC | #1
Hi Can,

Except for below nit, otherwise looks good to me.

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

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@codeaurora.org>
> ---
>  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
Can Guo Nov. 3, 2020, 8:01 a.m. UTC | #2
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@mediatek.com>
> 
> 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@codeaurora.org>
>> ---
>>  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.

	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?

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
Stanley Chu Nov. 3, 2020, 2:12 p.m. UTC | #3
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@mediatek.com>
> > 
> > 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@codeaurora.org>
> >> ---
> >>  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
diff mbox series

Patch

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)
+			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;
 };