[PATCH v1 0/5] UFS driver general fixes bundle 5

[PATCH v1 0/5] UFS driver general fixes bundle 5

[Can Guo]

This bundle includes 5 general fixes for UFS driver.

Asutosh Das (1):
  scsi: ufs: Recheck bkops level if bkops is disabled

Can Guo (4):
  scsi: ufs: Add new bit field PA_INIT to UECDL register
  scsi: ufs: Update VCCQ2 and VCCQ min voltage hard codes
  scsi: ufs: Avoid messing up the compl_time_stamp of lrbs
  scsi: ufs: Complete pending requests in host reset and restore path

 drivers/scsi/ufs/ufs.h    |  4 ++--
 drivers/scsi/ufs/ufshcd.c | 27 ++++++++++++---------------
 drivers/scsi/ufs/ufshci.h |  2 +-
 3 files changed, 15 insertions(+), 18 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 1/5] 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.

Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 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 v1 1/5] scsi: ufs: Recheck bkops level if bkops is disabled

[Avri Altman]

> 
> 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.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
Acked-by Avri Altman <avri.altman@wdc.com>

This is essentially a bug fix:
Fixes: afdfff59a0e0 (scsi: ufs: handle non spec compliant bkops behaviour by device)

RE: [EXT] [PATCH v1 1/5] scsi: ufs: Recheck bkops level if bkops is disabled

[Bean Huo (beanhuo)]

> Subject: [EXT] [PATCH v1 1/5] scsi: ufs: Recheck bkops level if bkops is disabled
> 
> 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.
> 

Yes, this makes sense, once receiving a bkops exception, we should recheck its level.
Because device side has changed its status.

> 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.
> 
should use current level.

> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v1 2/5] scsi: ufs: Add new bit field PA_INIT to UECDL register

[Can Guo]

Add new bit field (bit-15) PA_INIT to UECDL register, this will correctly
handle any PA_INIT error.

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

diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index dbb75cd..c2961d3 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -195,7 +195,7 @@ enum {
 
 /* UECDL - Host UIC Error Code Data Link Layer 3Ch */
 #define UIC_DATA_LINK_LAYER_ERROR		0x80000000
-#define UIC_DATA_LINK_LAYER_ERROR_CODE_MASK	0x7FFF
+#define UIC_DATA_LINK_LAYER_ERROR_CODE_MASK	0xFFFF
 #define UIC_DATA_LINK_LAYER_ERROR_TCX_REP_TIMER_EXP	0x2
 #define UIC_DATA_LINK_LAYER_ERROR_AFCX_REQ_TIMER_EXP	0x4
 #define UIC_DATA_LINK_LAYER_ERROR_FCX_PRO_TIMER_EXP	0x8
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v1 2/5] scsi: ufs: Add new bit field PA_INIT to UECDL register

[Avri Altman]

 
> 
> Add new bit field (bit-15) PA_INIT to UECDL register, this will correctly handle
> any PA_INIT error.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Acked-by Avri Altman <avri.altman@wdc.com>

This is a HCI3.0 change, so maybe make note of that?
But UIC_DATA_LINK_LAYER_ERROR_CODE_MASK isn't being used anywhere, better just remove it, don't you think?
Instead, while at it, fix ufshcd_update_uic_error to check UIC_DATA_LINK_LAYER_ERROR when checking for data link layer errors?

Re: [PATCH v1 2/5] scsi: ufs: Add new bit field PA_INIT to UECDL register

[cang]

On 2019-11-12 15:53, Avri Altman wrote:
>> 
>> Add new bit field (bit-15) PA_INIT to UECDL register, this will 
>> correctly handle
>> any PA_INIT error.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
> Acked-by Avri Altman <avri.altman@wdc.com>
> 
> This is a HCI3.0 change, so maybe make note of that?
> But UIC_DATA_LINK_LAYER_ERROR_CODE_MASK isn't being used anywhere,
> better just remove it, don't you think?
> Instead, while at it, fix ufshcd_update_uic_error to check
> UIC_DATA_LINK_LAYER_ERROR when checking for data link layer errors?

Hi Avri,

I will squash this change to my patch, it is used there.
[PATCH v3 5/7] scsi: ufs: Fix irq return code
url - https://lore.kernel.org/patchwork/patch/1148656/

Thanks,
Can Guo.

[PATCH v1 3/5] scsi: ufs: Update VCCQ2 and VCCQ min voltage hard codes

[Can Guo]

Per UFS 3.0 JEDEC standard, the VCCQ2 min voltage is 1.7v and the VCCQ min
voltage is 1.14v, update their hard codes accordingly.

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 v1 3/5] scsi: ufs: Update VCCQ2 and VCCQ min voltage hard codes

[Avri Altman]

> 
> 
> Per UFS 3.0 JEDEC standard, the VCCQ2 min voltage is 1.7v and the VCCQ min
> voltage is 1.14v, update their hard codes accordingly.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
AFAIK, Vccq2 is 1.7 - 1.95 in UFS2.1 as well.
Current constants applies to UFS1.1, as indicated in the original patch.
Vccq is <1.1 - 1.3> in UFS2.1,  and <1.14 - 1.26>, so need to update the max as well, and  
make the assignments in ufshcd_populate_vreg depends on hba->ufs_version?

> ---
>  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 v1 3/5] scsi: ufs: Update VCCQ2 and VCCQ min voltage hard codes

[cang]

On 2019-11-12 15:56, Avri Altman wrote:
>> 
>> 
>> Per UFS 3.0 JEDEC standard, the VCCQ2 min voltage is 1.7v and the VCCQ 
>> min
>> voltage is 1.14v, update their hard codes accordingly.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
> AFAIK, Vccq2 is 1.7 - 1.95 in UFS2.1 as well.
> Current constants applies to UFS1.1, as indicated in the original 
> patch.
> Vccq is <1.1 - 1.3> in UFS2.1,  and <1.14 - 1.26>, so need to update
> the max as well, and
> make the assignments in ufshcd_populate_vreg depends on 
> hba->ufs_version?
> 

Hi Avri,

Thank you for the comments. I will also update max voltage of VCCQ in 
next series.

BTW, making the assignments in ufshcd_populate_vregs depends on 
hba->ufs_version is
not practical. #1. hba->ufs_version is only get after vregs and clocks 
are ON,
which is way after ufshcd_populate_vregs. #2. hba->ufs_version is the 
version
of HCI, but we need to know the version of the connected UFS device.

The purpose of this change is to make sure the voltages of VCCQ and 
VCCQ2 work in
a safe range for all ver 1.1/2.0/2.1/3.0 UFS devices that can be 
connected to a host.

Best Regards,
Can Guo.

>> ---
>>  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 v1 4/5] scsi: ufs: Avoid messing up the compl_time_stamp of lrbs

[Can Guo]

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 v1 4/5] scsi: ufs: Avoid messing up the compl_time_stamp of lrbs

[Avri Altman]

> 
> 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>
Yeah I guess. But practically this can't really happen.
Acked-by Avri Altman <avri.altman@wdc.com>

[PATCH v1 5/5] 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.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5950a7c..4df4136 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -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,13 @@ 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);
+	ufshcd_complete_requests(hba);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	/* scale up clocks to max frequency before full reinitialization */
@@ -6369,7 +6373,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 +6382,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;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Alim Akhtar]

Hi Can,

On Fri, Nov 8, 2019 at 1:50 PM Can Guo <cang@codeaurora.org> wrote:
>
> 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.
>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
Looks good, I hope this is tested on your platform.
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

>  drivers/scsi/ufs/ufshcd.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 5950a7c..4df4136 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -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,13 @@ 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);
> +       ufshcd_complete_requests(hba);
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>
>         /* scale up clocks to max frequency before full reinitialization */
> @@ -6369,7 +6373,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 +6382,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;
>  }
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>


-- 
Regards,
Alim

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

[cang]

On 2019-11-13 10:24, Alim Akhtar wrote:
> Hi Can,
> 
> On Fri, Nov 8, 2019 at 1:50 PM Can Guo <cang@codeaurora.org> wrote:
>> 
>> 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.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
> Looks good, I hope this is tested on your platform.
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> 

Hi Alim,

Thanks for the review. We've tested it out on our platforms.

Best regards,
Can Guo.

>>  drivers/scsi/ufs/ufshcd.c | 20 +++++++-------------
>>  1 file changed, 7 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 5950a7c..4df4136 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -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,13 @@ 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);
>> +       ufshcd_complete_requests(hba);
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>> 
>>         /* scale up clocks to max frequency before full 
>> reinitialization */
>> @@ -6369,7 +6373,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 +6382,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;
>>  }
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

RE: [EXT] [PATCH v1 5/5] scsi: ufs: Complete pending requests in host reset and restore path

[Bean Huo (beanhuo)]

> 
> 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.

Hi, Can
I am not sure for this point, because there is HW/SW device reset before or after host reset/restore.
Device HW/SW reset also will clear the pended tasks in device side. That will be better.
I think Qcom platform already enabled HW reset.

//Bean

> 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.

Re: [EXT] [PATCH v1 5/5] scsi: ufs: Complete pending requests in host reset and restore path

[cang]

On 2019-11-14 06:04, Bean Huo (beanhuo) wrote:
>> 
>> 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.
> 
> Hi, Can
> I am not sure for this point, because there is HW/SW device reset
> before or after host reset/restore.
> Device HW/SW reset also will clear the pended tasks in device side.
> That will be better.
> I think Qcom platform already enabled HW reset.
> 
> //Bean
> 

Hi Bean,

By pending tasks here, it means the requests sent down from scsi/block 
layer,
but have not yet been handled by ufs driver(cmd->scsi_done() have not 
been called yet for these requests).
For these requests, although removed by host and UFS device in their HW 
queues(doorbell),
UFS driver still needs to complete them from SW side(call 
cmd->scsi_done() for each one of them) to
let upper layer know that they are finished(although not successfully) 
to avoid hitting
timeout of these pending tasks. I hope I make my explanation clearly.

Best Regards,
Can Guo.

>> 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.

Re: [EXT] [PATCH v1 5/5] scsi: ufs: Complete pending requests in host reset and restore path

[cang]

On 2019-11-14 09:03, cang@codeaurora.org wrote:
> On 2019-11-14 06:04, Bean Huo (beanhuo) wrote:
>>> 
>>> 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.
>> 
>> Hi, Can
>> I am not sure for this point, because there is HW/SW device reset
>> before or after host reset/restore.
>> Device HW/SW reset also will clear the pended tasks in device side.
>> That will be better.
>> I think Qcom platform already enabled HW reset.
>> 
>> //Bean
>> 
> 
> Hi Bean,
> 
> By pending tasks here, it means the requests sent down from scsi/block 
> layer,
> but have not yet been handled by ufs driver(cmd->scsi_done() have not
> been called yet for these requests).
> For these requests, although removed by host and UFS device in their
> HW queues(doorbell),
> UFS driver still needs to complete them from SW side(call
> cmd->scsi_done() for each one of them) to
> let upper layer know that they are finished(although not successfully)
> to avoid hitting
> timeout of these pending tasks. I hope I make my explanation clearly.
> 
> Best Regards,
> Can Guo.
> 

Hi Bean,

Just want to add up more phrases. We do have HW/SW reset.
Sorry about below lines which make you confused. Here I am just 
describing what
is like with previous code. Since these pending requests does not have
a chance to be handled in their IRQ handler after hba is stopped, and as
they have been cleared from doorbell already, then once there is an 
available
transfer completion IRQ, these requests will be handled in the IRQ 
handler,
no matter what is the transfer completion IRQ fired for. And NOP_OUT is 
just
the first command that can fire a transer completion IRQ.

Can Guo.

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.

RE: [EXT] [PATCH v1 5/5] scsi: ufs: Complete pending requests in host reset and restore path

[Bean Huo (beanhuo)]

> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Tested-by: Bean Huo <beanhuo@micron.com>