[PATCH v2 1/4] scsi: ufs: Recheck bkops level if bkops is disabled

[PATCH v2 1/4] scsi: ufs: Recheck bkops level if bkops is disabled

[Can Guo]

From: Asutosh Das <asutoshd@codeaurora.org>

Bkops level should be rechecked upon receiving an exception.
Currently the bkops level is being cached and never updated.

Update the same each time the level is checked.
Also do not use the cached bkops level value if it is disabled
and then enabled.

Fixes: afdfff59a0e0 (scsi: ufs: handle non spec compliant bkops behaviour by device)
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3910c58..8e7c362 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5099,6 +5099,7 @@ static int ufshcd_disable_auto_bkops(struct ufs_hba *hba)
 
 	hba->auto_bkops_enabled = false;
 	trace_ufshcd_auto_bkops_state(dev_name(hba->dev), "Disabled");
+	hba->is_urgent_bkops_lvl_checked = false;
 out:
 	return err;
 }
@@ -5123,6 +5124,7 @@ static void ufshcd_force_reset_auto_bkops(struct ufs_hba *hba)
 		hba->ee_ctrl_mask &= ~MASK_EE_URGENT_BKOPS;
 		ufshcd_disable_auto_bkops(hba);
 	}
+	hba->is_urgent_bkops_lvl_checked = false;
 }
 
 static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32 *status)
@@ -5169,6 +5171,7 @@ static int ufshcd_bkops_ctrl(struct ufs_hba *hba,
 		err = ufshcd_enable_auto_bkops(hba);
 	else
 		err = ufshcd_disable_auto_bkops(hba);
+	hba->urgent_bkops_lvl = curr_status;
 out:
 	return err;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 1/4] scsi: ufs: Recheck bkops level if bkops is disabled

[Alim Akhtar]

Hi Can

On Mon, Nov 18, 2019 at 9:21 AM Can Guo <cang@qti.qualcomm.com> wrote:
>
> From: Asutosh Das <asutoshd@codeaurora.org>
>
> Bkops level should be rechecked upon receiving an exception.
> Currently the bkops level is being cached and never updated.
>
> Update the same each time the level is checked.
> Also do not use the cached bkops level value if it is disabled
> and then enabled.
>
> Fixes: afdfff59a0e0 (scsi: ufs: handle non spec compliant bkops behaviour by device)
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> ---
Feel free to add
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Ran these patches on exynos7 platfrom, and no regression observed,
basic read/write works, so
Tested-by: Alim Akhtar <alim.akhtar@samsung.com
>
>  drivers/scsi/ufs/ufshcd.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3910c58..8e7c362 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5099,6 +5099,7 @@ static int ufshcd_disable_auto_bkops(struct ufs_hba *hba)
>
>         hba->auto_bkops_enabled = false;
>         trace_ufshcd_auto_bkops_state(dev_name(hba->dev), "Disabled");
> +       hba->is_urgent_bkops_lvl_checked = false;
>  out:
>         return err;
>  }
> @@ -5123,6 +5124,7 @@ static void ufshcd_force_reset_auto_bkops(struct ufs_hba *hba)
>                 hba->ee_ctrl_mask &= ~MASK_EE_URGENT_BKOPS;
>                 ufshcd_disable_auto_bkops(hba);
>         }
> +       hba->is_urgent_bkops_lvl_checked = false;
>  }
>
>  static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32 *status)
> @@ -5169,6 +5171,7 @@ static int ufshcd_bkops_ctrl(struct ufs_hba *hba,
>                 err = ufshcd_enable_auto_bkops(hba);
>         else
>                 err = ufshcd_disable_auto_bkops(hba);
> +       hba->urgent_bkops_lvl = curr_status;
>  out:
>         return err;
>  }
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>


-- 
Regards,
Alim

[PATCH v2 2/4] scsi: ufs: Update VCCQ2 and VCCQ min/max voltage hard codes

[Can Guo]

From: Can Guo <cang@codeaurora.org>

Per UFS 3.0 JEDEC standard, the VCCQ2 min voltage is 1.7v and the VCCQ
voltage range is 1.14v ~ 1.26v. Update their hard codes accordingly to
make sure they work in a safe range compliant for ver 1.0/2.0/2.1/3.0
UFS devices.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 385bac8..9df4f4d 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -500,9 +500,9 @@ struct ufs_query_res {
 #define UFS_VREG_VCC_MAX_UV	   3600000 /* uV */
 #define UFS_VREG_VCC_1P8_MIN_UV    1700000 /* uV */
 #define UFS_VREG_VCC_1P8_MAX_UV    1950000 /* uV */
-#define UFS_VREG_VCCQ_MIN_UV	   1100000 /* uV */
+#define UFS_VREG_VCCQ_MIN_UV	   1140000 /* uV */
 #define UFS_VREG_VCCQ_MAX_UV	   1300000 /* uV */
-#define UFS_VREG_VCCQ2_MIN_UV	   1650000 /* uV */
+#define UFS_VREG_VCCQ2_MIN_UV	   1700000 /* uV */
 #define UFS_VREG_VCCQ2_MAX_UV	   1950000 /* uV */
 
 /*
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v2 2/4] scsi: ufs: Update VCCQ2 and VCCQ min/max voltage hard codes

[Avri Altman]

> 
> From: Can Guo <cang@codeaurora.org>
> 
> Per UFS 3.0 JEDEC standard, the VCCQ2 min voltage is 1.7v and the VCCQ
> voltage range is 1.14v ~ 1.26v. Update their hard codes accordingly to make
> sure they work in a safe range compliant for ver 1.0/2.0/2.1/3.0 UFS devices.
So to keep it safe, we need to use largest range: 
min_uV = min over all spec ranges, and max_uV = max over all spec ranges.
Meaning leave it as it is if we want to be backward compatible with UFS1.0.

Thanks,
Avri

> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index 385bac8..9df4f4d
> 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -500,9 +500,9 @@ struct ufs_query_res {
>  #define UFS_VREG_VCC_MAX_UV       3600000 /* uV */
>  #define UFS_VREG_VCC_1P8_MIN_UV    1700000 /* uV */
>  #define UFS_VREG_VCC_1P8_MAX_UV    1950000 /* uV */
> -#define UFS_VREG_VCCQ_MIN_UV      1100000 /* uV */
> +#define UFS_VREG_VCCQ_MIN_UV      1140000 /* uV */
>  #define UFS_VREG_VCCQ_MAX_UV      1300000 /* uV */
> -#define UFS_VREG_VCCQ2_MIN_UV     1650000 /* uV */
> +#define UFS_VREG_VCCQ2_MIN_UV     1700000 /* uV */
>  #define UFS_VREG_VCCQ2_MAX_UV     1950000 /* uV */
> 
>  /*
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v2 2/4] scsi: ufs: Update VCCQ2 and VCCQ min/max voltage hard codes

[cang]

On 2019-11-18 15:15, Avri Altman wrote:
>> 
>> From: Can Guo <cang@codeaurora.org>
>> 
>> Per UFS 3.0 JEDEC standard, the VCCQ2 min voltage is 1.7v and the VCCQ
>> voltage range is 1.14v ~ 1.26v. Update their hard codes accordingly to 
>> make
>> sure they work in a safe range compliant for ver 1.0/2.0/2.1/3.0 UFS 
>> devices.
> So to keep it safe, we need to use largest range:
> min_uV = min over all spec ranges, and max_uV = max over all spec 
> ranges.
> Meaning leave it as it is if we want to be backward compatible with 
> UFS1.0.
> 
> Thanks,
> Avri
> 

Hi Avri,

Sorry I don't quite follow you here.
Leaving it as it is means for UFS2.1 devices, when boot up, if we call
regulator_set_voltage(1.65, 1.95) to setup its VCCQ2, 
regulator_set_voltage() will
give you 1.65v on VCCQ2 if the voltage level of this regulator is wider, 
say (1.60, 1.95).
Meaning you will finally set 1.65v to VCCQ2. But 1.65v is out of spec 
for UFS v2.1 as it
requires min voltage to be 1.7v on VCCQ2. So, the smallest range is 
safe.
Of course, in real board design, the regulator's voltage level is 
limited/designed by power team
to be in a safe range, say (1.8, 1.92), so that calling 
regulator_set_voltage(1.65, 1.95) still gives
you 1.8v. But it does not mean the current hard codes are compliant for 
all UFS devices.

Best Regards,
Can Guo.

>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufs.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index 
>> 385bac8..9df4f4d
>> 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -500,9 +500,9 @@ struct ufs_query_res {
>>  #define UFS_VREG_VCC_MAX_UV       3600000 /* uV */
>>  #define UFS_VREG_VCC_1P8_MIN_UV    1700000 /* uV */
>>  #define UFS_VREG_VCC_1P8_MAX_UV    1950000 /* uV */
>> -#define UFS_VREG_VCCQ_MIN_UV      1100000 /* uV */
>> +#define UFS_VREG_VCCQ_MIN_UV      1140000 /* uV */
>>  #define UFS_VREG_VCCQ_MAX_UV      1300000 /* uV */
>> -#define UFS_VREG_VCCQ2_MIN_UV     1650000 /* uV */
>> +#define UFS_VREG_VCCQ2_MIN_UV     1700000 /* uV */
>>  #define UFS_VREG_VCCQ2_MAX_UV     1950000 /* uV */
>> 
>>  /*
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project

RE: [PATCH v2 2/4] scsi: ufs: Update VCCQ2 and VCCQ min/max voltage hard codes

[Avri Altman]

> 
> On 2019-11-18 15:15, Avri Altman wrote:
> >>
> >> From: Can Guo <cang@codeaurora.org>
> >>
> >> Per UFS 3.0 JEDEC standard, the VCCQ2 min voltage is 1.7v and the
> >> VCCQ voltage range is 1.14v ~ 1.26v. Update their hard codes
> >> accordingly to make sure they work in a safe range compliant for ver
> >> 1.0/2.0/2.1/3.0 UFS devices.
> > So to keep it safe, we need to use largest range:
> > min_uV = min over all spec ranges, and max_uV = max over all spec
> > ranges.
> > Meaning leave it as it is if we want to be backward compatible with
> > UFS1.0.
> >
> > Thanks,
> > Avri
> >
> 
> Hi Avri,
> 
> Sorry I don't quite follow you here.
> Leaving it as it is means for UFS2.1 devices, when boot up, if we call
> regulator_set_voltage(1.65, 1.95) to setup its VCCQ2,
> regulator_set_voltage() will
> give you 1.65v on VCCQ2 if the voltage level of this regulator is wider, say (1.60,
> 1.95).
> Meaning you will finally set 1.65v to VCCQ2. But 1.65v is out of spec for UFS
> v2.1 as it requires min voltage to be 1.7v on VCCQ2. So, the smallest range is
> safe.
> Of course, in real board design, the regulator's voltage level is limited/designed
> by power team to be in a safe range, say (1.8, 1.92), so that calling
> regulator_set_voltage(1.65, 1.95) still gives you 1.8v. But it does not mean the
> current hard codes are compliant for all UFS devices.
You are correct - the narrowest the range the better - as long as you don't cross the limits of previous spec.
So changing 1.1 -> 1.14  and 1.65 -> 1.7 is fine.
While at it, Vccq max in UFS3.0 is 1.26, why not change 1.3 -> 1.26, like you indicated in your commit log?

Thanks,
Avri

> 
> Best Regards,
> Can Guo.
> 
> >>
> >> Signed-off-by: Can Guo <cang@codeaurora.org>
> >> ---
> >>  drivers/scsi/ufs/ufs.h | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index
> >> 385bac8..9df4f4d
> >> 100644
> >> --- a/drivers/scsi/ufs/ufs.h
> >> +++ b/drivers/scsi/ufs/ufs.h
> >> @@ -500,9 +500,9 @@ struct ufs_query_res {
> >>  #define UFS_VREG_VCC_MAX_UV       3600000 /* uV */
> >>  #define UFS_VREG_VCC_1P8_MIN_UV    1700000 /* uV */
> >>  #define UFS_VREG_VCC_1P8_MAX_UV    1950000 /* uV */
> >> -#define UFS_VREG_VCCQ_MIN_UV      1100000 /* uV */
> >> +#define UFS_VREG_VCCQ_MIN_UV      1140000 /* uV */
> >>  #define UFS_VREG_VCCQ_MAX_UV      1300000 /* uV */
> >> -#define UFS_VREG_VCCQ2_MIN_UV     1650000 /* uV */
> >> +#define UFS_VREG_VCCQ2_MIN_UV     1700000 /* uV */
> >>  #define UFS_VREG_VCCQ2_MAX_UV     1950000 /* uV */
> >>
> >>  /*
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> >> Forum, a Linux Foundation Collaborative Project

Re: [PATCH v2 2/4] scsi: ufs: Update VCCQ2 and VCCQ min/max voltage hard codes

[cang]

On 2019-11-19 20:41, Avri Altman wrote:
>> 
>> On 2019-11-18 15:15, Avri Altman wrote:
>> >>
>> >> From: Can Guo <cang@codeaurora.org>
>> >>
>> >> Per UFS 3.0 JEDEC standard, the VCCQ2 min voltage is 1.7v and the
>> >> VCCQ voltage range is 1.14v ~ 1.26v. Update their hard codes
>> >> accordingly to make sure they work in a safe range compliant for ver
>> >> 1.0/2.0/2.1/3.0 UFS devices.
>> > So to keep it safe, we need to use largest range:
>> > min_uV = min over all spec ranges, and max_uV = max over all spec
>> > ranges.
>> > Meaning leave it as it is if we want to be backward compatible with
>> > UFS1.0.
>> >
>> > Thanks,
>> > Avri
>> >
>> 
>> Hi Avri,
>> 
>> Sorry I don't quite follow you here.
>> Leaving it as it is means for UFS2.1 devices, when boot up, if we call
>> regulator_set_voltage(1.65, 1.95) to setup its VCCQ2,
>> regulator_set_voltage() will
>> give you 1.65v on VCCQ2 if the voltage level of this regulator is 
>> wider, say (1.60,
>> 1.95).
>> Meaning you will finally set 1.65v to VCCQ2. But 1.65v is out of spec 
>> for UFS
>> v2.1 as it requires min voltage to be 1.7v on VCCQ2. So, the smallest 
>> range is
>> safe.
>> Of course, in real board design, the regulator's voltage level is 
>> limited/designed
>> by power team to be in a safe range, say (1.8, 1.92), so that calling
>> regulator_set_voltage(1.65, 1.95) still gives you 1.8v. But it does 
>> not mean the
>> current hard codes are compliant for all UFS devices.
> You are correct - the narrowest the range the better - as long as you
> don't cross the limits of previous spec.
> So changing 1.1 -> 1.14  and 1.65 -> 1.7 is fine.
> While at it, Vccq max in UFS3.0 is 1.26, why not change 1.3 -> 1.26,
> like you indicated in your commit log?
> 
> Thanks,
> Avri
> 

Thank you Avri, sorry I missed the change to max voltage of VCCQ, I
will update it in the next version.

Best Regards,
Can Guo.

>> 
>> Best Regards,
>> Can Guo.
>> 
>> >>
>> >> Signed-off-by: Can Guo <cang@codeaurora.org>
>> >> ---
>> >>  drivers/scsi/ufs/ufs.h | 4 ++--
>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index
>> >> 385bac8..9df4f4d
>> >> 100644
>> >> --- a/drivers/scsi/ufs/ufs.h
>> >> +++ b/drivers/scsi/ufs/ufs.h
>> >> @@ -500,9 +500,9 @@ struct ufs_query_res {
>> >>  #define UFS_VREG_VCC_MAX_UV       3600000 /* uV */
>> >>  #define UFS_VREG_VCC_1P8_MIN_UV    1700000 /* uV */
>> >>  #define UFS_VREG_VCC_1P8_MAX_UV    1950000 /* uV */
>> >> -#define UFS_VREG_VCCQ_MIN_UV      1100000 /* uV */
>> >> +#define UFS_VREG_VCCQ_MIN_UV      1140000 /* uV */
>> >>  #define UFS_VREG_VCCQ_MAX_UV      1300000 /* uV */
>> >> -#define UFS_VREG_VCCQ2_MIN_UV     1650000 /* uV */
>> >> +#define UFS_VREG_VCCQ2_MIN_UV     1700000 /* uV */
>> >>  #define UFS_VREG_VCCQ2_MAX_UV     1950000 /* uV */
>> >>
>> >>  /*
>> >> --
>> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> >> Forum, a Linux Foundation Collaborative Project

[PATCH v2 3/4] scsi: ufs: Avoid messing up the compl_time_stamp of lrbs

[Can Guo]

From: Can Guo <cang@codeaurora.org>

To be on the safe side, do not touch one lrb after clear its slot in the
lrb_in_use bitmap to avoid messing up the next task which would possibly
occupy this lrb.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8e7c362..5950a7c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4902,12 +4902,14 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 			cmd->result = result;
 			/* Mark completed command as NULL in LRB */
 			lrbp->cmd = NULL;
+			lrbp->compl_time_stamp = ktime_get();
 			clear_bit_unlock(index, &hba->lrb_in_use);
 			/* Do not touch lrbp after scsi done */
 			cmd->scsi_done(cmd);
 			__ufshcd_release(hba);
 		} else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE ||
 			lrbp->command_type == UTP_CMD_TYPE_UFS_STORAGE) {
+			lrbp->compl_time_stamp = ktime_get();
 			if (hba->dev_cmd.complete) {
 				ufshcd_add_command_trace(hba, index,
 						"dev_complete");
@@ -4916,8 +4918,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 		}
 		if (ufshcd_is_clkscaling_supported(hba))
 			hba->clk_scaling.active_reqs--;
-
-		lrbp->compl_time_stamp = ktime_get();
 	}
 
 	/* clear corresponding bits of completed commands */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v2 3/4] scsi: ufs: Avoid messing up the compl_time_stamp of lrbs

[Avri Altman]

 
> 
> From: Can Guo <cang@codeaurora.org>
> 
> To be on the safe side, do not touch one lrb after clear its slot in the lrb_in_use
> bitmap to avoid messing up the next task which would possibly occupy this lrb.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed by: Avri Altman <avri.altman@wdc.com>

RE: [EXT] [PATCH v2 3/4] scsi: ufs: Avoid messing up the compl_time_stamp of lrbs

[Bean Huo (beanhuo)]

> 
> From: Can Guo <cang@codeaurora.org>
> 
> To be on the safe side, do not touch one lrb after clear its slot in the lrb_in_use
> bitmap to avoid messing up the next task which would possibly occupy this lrb.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v2 4/4] scsi: ufs: Complete pending requests in host reset and restore path

[Can Guo]

From: Can Guo <cang@codeaurora.org>

In UFS host reset and restore path, before probe, we stop and start the
host controller once. After host controller is stopped, the pending
requests, if any, are cleared from the doorbell, but no completion IRQ
would be raised due to the hba is stopped.
These pending requests shall be completed along with the first NOP_OUT
command(as it is the first command which can raise a transfer completion
IRQ) sent during probe.
Since the OCSs of these pending requests are not SUCCESS(because they are
not yet literally finished), their UPIUs shall be dumped. When there are
multiple pending requests, the UPIU dump can be overwhelming and may lead
to stability issues because it is in atomic context.
Therefore, before probe, complete these pending requests right after host
controller is stopped and silence the UPIU dump from them.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 24 ++++++++++--------------
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5950a7c..b92a3f4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4845,7 +4845,7 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
 		break;
 	} /* end of switch */
 
-	if (host_byte(result) != DID_OK)
+	if ((host_byte(result) != DID_OK) && !hba->silence_err_logs)
 		ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
 	return result;
 }
@@ -5404,8 +5404,8 @@ static void ufshcd_err_handler(struct work_struct *work)
 
 	/*
 	 * if host reset is required then skip clearing the pending
-	 * transfers forcefully because they will automatically get
-	 * cleared after link startup.
+	 * transfers forcefully because they will get cleared during
+	 * host reset and restore
 	 */
 	if (needs_reset)
 		goto skip_pending_xfer_clear;
@@ -6333,9 +6333,15 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	int err;
 	unsigned long flags;
 
-	/* Reset the host controller */
+	/*
+	 * Stop the host controller and complete the requests
+	 * cleared by h/w
+	 */
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	ufshcd_hba_stop(hba, false);
+	hba->silence_err_logs = true;
+	ufshcd_complete_requests(hba);
+	hba->silence_err_logs = false;
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/* scale up clocks to max frequency before full reinitialization */
@@ -6369,7 +6375,6 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 static int ufshcd_reset_and_restore(struct ufs_hba *hba)
 {
 	int err = 0;
-	unsigned long flags;
 	int retries = MAX_HOST_RESET_RETRIES;
 
 	do {
@@ -6379,15 +6384,6 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
 		err = ufshcd_host_reset_and_restore(hba);
 	} while (err && --retries);
 
-	/*
-	 * After reset the door-bell might be cleared, complete
-	 * outstanding requests in s/w here.
-	 */
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	ufshcd_transfer_req_compl(hba);
-	ufshcd_tmc_handler(hba);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-
 	return err;
 }
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e0fe247..1e51034 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -513,6 +513,7 @@ struct ufs_stats {
  * @uic_error: UFS interconnect layer error status
  * @saved_err: sticky error mask
  * @saved_uic_err: sticky UIC error mask
+ * @silence_err_logs: flag to silence error logs
  * @dev_cmd: ufs device management command information
  * @last_dme_cmd_tstamp: time stamp of the last completed DME command
  * @auto_bkops_enabled: to track whether bkops is enabled in device
@@ -670,6 +671,7 @@ struct ufs_hba {
 	u32 saved_err;
 	u32 saved_uic_err;
 	struct ufs_stats ufs_stats;
+	bool silence_err_logs;
 
 	/* Device management request data */
 	struct ufs_dev_cmd dev_cmd;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v2 4/4] scsi: ufs: Complete pending requests in host reset and restore path

[Avri Altman]

> 
> 
> From: Can Guo <cang@codeaurora.org>
> 
> In UFS host reset and restore path, before probe, we stop and start the host
> controller once. After host controller is stopped, the pending requests, if any,
> are cleared from the doorbell, but no completion IRQ would be raised due to the
> hba is stopped.
> These pending requests shall be completed along with the first NOP_OUT
> command(as it is the first command which can raise a transfer completion
> IRQ) sent during probe.
> Since the OCSs of these pending requests are not SUCCESS(because they are not
> yet literally finished), their UPIUs shall be dumped. When there are multiple
> pending requests, the UPIU dump can be overwhelming and may lead to
> stability issues because it is in atomic context.
> Therefore, before probe, complete these pending requests right after host
> controller is stopped and silence the UPIU dump from them.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Already reviewed by Alim & Bean, and tested by Bean.

Please keep those tag updated - will make it clear what is still required to merge this series.

Thanks,
Avri

Re: [PATCH v2 4/4] scsi: ufs: Complete pending requests in host reset and restore path

[Alim Akhtar]

On Mon, Nov 18, 2019 at 9:22 AM Can Guo <cang@qti.qualcomm.com> wrote:
>
> From: Can Guo <cang@codeaurora.org>
>
> In UFS host reset and restore path, before probe, we stop and start the
> host controller once. After host controller is stopped, the pending
> requests, if any, are cleared from the doorbell, but no completion IRQ
> would be raised due to the hba is stopped.
> These pending requests shall be completed along with the first NOP_OUT
> command(as it is the first command which can raise a transfer completion
> IRQ) sent during probe.
> Since the OCSs of these pending requests are not SUCCESS(because they are
> not yet literally finished), their UPIUs shall be dumped. When there are
> multiple pending requests, the UPIU dump can be overwhelming and may lead
> to stability issues because it is in atomic context.
> Therefore, before probe, complete these pending requests right after host
> controller is stopped and silence the UPIU dump from them.
>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---

Tested-by: Alim Akhtar <alim.akhtar@samsung.com>

Please add all previous Ack/reviewed and tested-by tags so that we are
aware what need to be done for this patch.
Thanks

>  drivers/scsi/ufs/ufshcd.c | 24 ++++++++++--------------
>  drivers/scsi/ufs/ufshcd.h |  2 ++
>  2 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 5950a7c..b92a3f4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4845,7 +4845,7 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>                 break;
>         } /* end of switch */
>
> -       if (host_byte(result) != DID_OK)
> +       if ((host_byte(result) != DID_OK) && !hba->silence_err_logs)
>                 ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
>         return result;
>  }
> @@ -5404,8 +5404,8 @@ static void ufshcd_err_handler(struct work_struct *work)
>
>         /*
>          * if host reset is required then skip clearing the pending
> -        * transfers forcefully because they will automatically get
> -        * cleared after link startup.
> +        * transfers forcefully because they will get cleared during
> +        * host reset and restore
>          */
>         if (needs_reset)
>                 goto skip_pending_xfer_clear;
> @@ -6333,9 +6333,15 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>         int err;
>         unsigned long flags;
>
> -       /* Reset the host controller */
> +       /*
> +        * Stop the host controller and complete the requests
> +        * cleared by h/w
> +        */
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         ufshcd_hba_stop(hba, false);
> +       hba->silence_err_logs = true;
> +       ufshcd_complete_requests(hba);
> +       hba->silence_err_logs = false;
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>
>         /* scale up clocks to max frequency before full reinitialization */
> @@ -6369,7 +6375,6 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>  static int ufshcd_reset_and_restore(struct ufs_hba *hba)
>  {
>         int err = 0;
> -       unsigned long flags;
>         int retries = MAX_HOST_RESET_RETRIES;
>
>         do {
> @@ -6379,15 +6384,6 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
>                 err = ufshcd_host_reset_and_restore(hba);
>         } while (err && --retries);
>
> -       /*
> -        * After reset the door-bell might be cleared, complete
> -        * outstanding requests in s/w here.
> -        */
> -       spin_lock_irqsave(hba->host->host_lock, flags);
> -       ufshcd_transfer_req_compl(hba);
> -       ufshcd_tmc_handler(hba);
> -       spin_unlock_irqrestore(hba->host->host_lock, flags);
> -
>         return err;
>  }
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index e0fe247..1e51034 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -513,6 +513,7 @@ struct ufs_stats {
>   * @uic_error: UFS interconnect layer error status
>   * @saved_err: sticky error mask
>   * @saved_uic_err: sticky UIC error mask
> + * @silence_err_logs: flag to silence error logs
>   * @dev_cmd: ufs device management command information
>   * @last_dme_cmd_tstamp: time stamp of the last completed DME command
>   * @auto_bkops_enabled: to track whether bkops is enabled in device
> @@ -670,6 +671,7 @@ struct ufs_hba {
>         u32 saved_err;
>         u32 saved_uic_err;
>         struct ufs_stats ufs_stats;
> +       bool silence_err_logs;
>
>         /* Device management request data */
>         struct ufs_dev_cmd dev_cmd;
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>


-- 
Regards,
Alim

Re: [PATCH v2 4/4] scsi: ufs: Complete pending requests in host reset and restore path

[cang]

On 2019-11-18 21:23, Alim Akhtar wrote:
> On Mon, Nov 18, 2019 at 9:22 AM Can Guo <cang@qti.qualcomm.com> wrote:
>> 
>> From: Can Guo <cang@codeaurora.org>
>> 
>> In UFS host reset and restore path, before probe, we stop and start 
>> the
>> host controller once. After host controller is stopped, the pending
>> requests, if any, are cleared from the doorbell, but no completion IRQ
>> would be raised due to the hba is stopped.
>> These pending requests shall be completed along with the first NOP_OUT
>> command(as it is the first command which can raise a transfer 
>> completion
>> IRQ) sent during probe.
>> Since the OCSs of these pending requests are not SUCCESS(because they 
>> are
>> not yet literally finished), their UPIUs shall be dumped. When there 
>> are
>> multiple pending requests, the UPIU dump can be overwhelming and may 
>> lead
>> to stability issues because it is in atomic context.
>> Therefore, before probe, complete these pending requests right after 
>> host
>> controller is stopped and silence the UPIU dump from them.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
> 
> Tested-by: Alim Akhtar <alim.akhtar@samsung.com>
> 
> Please add all previous Ack/reviewed and tested-by tags so that we are
> aware what need to be done for this patch.
> Thanks
> 

Hi Alim,

Thanks for pointing out it. I updated the patch a little bit so I think 
the
prevoius tags are not valid any more.

Best Regards,
Can Guo.

>>  drivers/scsi/ufs/ufshcd.c | 24 ++++++++++--------------
>>  drivers/scsi/ufs/ufshcd.h |  2 ++
>>  2 files changed, 12 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 5950a7c..b92a3f4 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -4845,7 +4845,7 @@ static void ufshcd_slave_destroy(struct 
>> scsi_device *sdev)
>>                 break;
>>         } /* end of switch */
>> 
>> -       if (host_byte(result) != DID_OK)
>> +       if ((host_byte(result) != DID_OK) && !hba->silence_err_logs)
>>                 ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
>>         return result;
>>  }
>> @@ -5404,8 +5404,8 @@ static void ufshcd_err_handler(struct 
>> work_struct *work)
>> 
>>         /*
>>          * if host reset is required then skip clearing the pending
>> -        * transfers forcefully because they will automatically get
>> -        * cleared after link startup.
>> +        * transfers forcefully because they will get cleared during
>> +        * host reset and restore
>>          */
>>         if (needs_reset)
>>                 goto skip_pending_xfer_clear;
>> @@ -6333,9 +6333,15 @@ static int ufshcd_host_reset_and_restore(struct 
>> ufs_hba *hba)
>>         int err;
>>         unsigned long flags;
>> 
>> -       /* Reset the host controller */
>> +       /*
>> +        * Stop the host controller and complete the requests
>> +        * cleared by h/w
>> +        */
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>>         ufshcd_hba_stop(hba, false);
>> +       hba->silence_err_logs = true;
>> +       ufshcd_complete_requests(hba);
>> +       hba->silence_err_logs = false;
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>> 
>>         /* scale up clocks to max frequency before full 
>> reinitialization */
>> @@ -6369,7 +6375,6 @@ static int ufshcd_host_reset_and_restore(struct 
>> ufs_hba *hba)
>>  static int ufshcd_reset_and_restore(struct ufs_hba *hba)
>>  {
>>         int err = 0;
>> -       unsigned long flags;
>>         int retries = MAX_HOST_RESET_RETRIES;
>> 
>>         do {
>> @@ -6379,15 +6384,6 @@ static int ufshcd_reset_and_restore(struct 
>> ufs_hba *hba)
>>                 err = ufshcd_host_reset_and_restore(hba);
>>         } while (err && --retries);
>> 
>> -       /*
>> -        * After reset the door-bell might be cleared, complete
>> -        * outstanding requests in s/w here.
>> -        */
>> -       spin_lock_irqsave(hba->host->host_lock, flags);
>> -       ufshcd_transfer_req_compl(hba);
>> -       ufshcd_tmc_handler(hba);
>> -       spin_unlock_irqrestore(hba->host->host_lock, flags);
>> -
>>         return err;
>>  }
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index e0fe247..1e51034 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -513,6 +513,7 @@ struct ufs_stats {
>>   * @uic_error: UFS interconnect layer error status
>>   * @saved_err: sticky error mask
>>   * @saved_uic_err: sticky UIC error mask
>> + * @silence_err_logs: flag to silence error logs
>>   * @dev_cmd: ufs device management command information
>>   * @last_dme_cmd_tstamp: time stamp of the last completed DME command
>>   * @auto_bkops_enabled: to track whether bkops is enabled in device
>> @@ -670,6 +671,7 @@ struct ufs_hba {
>>         u32 saved_err;
>>         u32 saved_uic_err;
>>         struct ufs_stats ufs_stats;
>> +       bool silence_err_logs;
>> 
>>         /* Device management request data */
>>         struct ufs_dev_cmd dev_cmd;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

[PATCH v2 4/4] scsi: ufs: Complete pending requests in host reset and restore path

[Can Guo]

In UFS host reset and restore path, before probe, we stop and start the
host controller once. After host controller is stopped, the pending
requests, if any, are cleared from the doorbell, but no completion IRQ
would be raised due to the hba is stopped.
These pending requests shall be completed along with the first NOP_OUT
command(as it is the first command which can raise a transfer completion
IRQ) sent during probe.
Since the OCSs of these pending requests are not SUCCESS(because they are
not yet literally finished), their UPIUs shall be dumped. When there are
multiple pending requests, the UPIU dump can be overwhelming and may lead
to stability issues because it is in atomic context.
Therefore, before probe, complete these pending requests right after host
controller is stopped and silence the UPIU dump from them.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 24 ++++++++++--------------
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5950a7c..b92a3f4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4845,7 +4845,7 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
 		break;
 	} /* end of switch */
 
-	if (host_byte(result) != DID_OK)
+	if ((host_byte(result) != DID_OK) && !hba->silence_err_logs)
 		ufshcd_print_trs(hba, 1 << lrbp->task_tag, true);
 	return result;
 }
@@ -5404,8 +5404,8 @@ static void ufshcd_err_handler(struct work_struct *work)
 
 	/*
 	 * if host reset is required then skip clearing the pending
-	 * transfers forcefully because they will automatically get
-	 * cleared after link startup.
+	 * transfers forcefully because they will get cleared during
+	 * host reset and restore
 	 */
 	if (needs_reset)
 		goto skip_pending_xfer_clear;
@@ -6333,9 +6333,15 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	int err;
 	unsigned long flags;
 
-	/* Reset the host controller */
+	/*
+	 * Stop the host controller and complete the requests
+	 * cleared by h/w
+	 */
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	ufshcd_hba_stop(hba, false);
+	hba->silence_err_logs = true;
+	ufshcd_complete_requests(hba);
+	hba->silence_err_logs = false;
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/* scale up clocks to max frequency before full reinitialization */
@@ -6369,7 +6375,6 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 static int ufshcd_reset_and_restore(struct ufs_hba *hba)
 {
 	int err = 0;
-	unsigned long flags;
 	int retries = MAX_HOST_RESET_RETRIES;
 
 	do {
@@ -6379,15 +6384,6 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba)
 		err = ufshcd_host_reset_and_restore(hba);
 	} while (err && --retries);
 
-	/*
-	 * After reset the door-bell might be cleared, complete
-	 * outstanding requests in s/w here.
-	 */
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	ufshcd_transfer_req_compl(hba);
-	ufshcd_tmc_handler(hba);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-
 	return err;
 }
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e0fe247..1e51034 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -513,6 +513,7 @@ struct ufs_stats {
  * @uic_error: UFS interconnect layer error status
  * @saved_err: sticky error mask
  * @saved_uic_err: sticky UIC error mask
+ * @silence_err_logs: flag to silence error logs
  * @dev_cmd: ufs device management command information
  * @last_dme_cmd_tstamp: time stamp of the last completed DME command
  * @auto_bkops_enabled: to track whether bkops is enabled in device
@@ -670,6 +671,7 @@ struct ufs_hba {
 	u32 saved_err;
 	u32 saved_uic_err;
 	struct ufs_stats ufs_stats;
+	bool silence_err_logs;
 
 	/* Device management request data */
 	struct ufs_dev_cmd dev_cmd;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project