[PATCH v2 0/3] Three minor changes related with error handling

[PATCH v2 0/3] Three minor changes related with error handling

[Can Guo]

The 1st change is a complementary change for error handling.
The 2nd change disables IRQ in host reset path to avoid possible NoC issues.
The 3rd change is a minor cleanup to !hba checks in suspend/resume paths

Can Guo (2):
  scsi: ufs: Minor adjustments to error handling
  scsi: ufs: Remove redundant checks of !hba in suspend/resume callbacks

Nitin Rawat (1):
  scsi: ufs-qcom: Disable interrupt in reset path

 drivers/scsi/ufs/ufs-qcom.c | 10 ++++++++++
 drivers/scsi/ufs/ufshcd.c   | 39 ++++++++++++---------------------------
 2 files changed, 22 insertions(+), 27 deletions(-)

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

[PATCH v2 1/3] scsi: ufs: Minor adjustments to error handling

[Can Guo]

In error handling prepare stage, after SCSI requests are blocked, do a
down/up_write(clk_scaling_lock) to clean up the queuecommand() path.
Meanwhile, stop eeh_work in case it disturbs error recovery. Moreover,
reset ufshcd_state at the entrance of ufshcd_probe_hba(), since it may be
called multiple times during error recovery.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 80620c8..013eb73 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4987,6 +4987,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 			 * UFS device needs urgent BKOPs.
 			 */
 			if (!hba->pm_op_in_progress &&
+			    !ufshcd_eh_in_progress(hba) &&
 			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr) &&
 			    schedule_work(&hba->eeh_work)) {
 				/*
@@ -5784,13 +5785,20 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 			ufshcd_suspend_clkscaling(hba);
 		ufshcd_clk_scaling_allow(hba, false);
 	}
+	ufshcd_scsi_block_requests(hba);
+	/* Drain ufshcd_queuecommand() */
+	down_write(&hba->clk_scaling_lock);
+	up_write(&hba->clk_scaling_lock);
+	cancel_work_sync(&hba->eeh_work);
 }
 
 static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 {
+	ufshcd_scsi_unblock_requests(hba);
 	ufshcd_release(hba);
 	if (ufshcd_is_clkscaling_supported(hba))
 		ufshcd_clk_scaling_suspend(hba, false);
+	ufshcd_clear_ua_wluns(hba);
 	pm_runtime_put(hba->dev);
 }
 
@@ -5882,8 +5890,8 @@ static void ufshcd_err_handler(struct work_struct *work)
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	ufshcd_err_handling_prepare(hba);
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	ufshcd_scsi_block_requests(hba);
-	hba->ufshcd_state = UFSHCD_STATE_RESET;
+	if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
+		hba->ufshcd_state = UFSHCD_STATE_RESET;
 
 	/* Complete requests that have door-bell cleared by h/w */
 	ufshcd_complete_requests(hba);
@@ -6042,12 +6050,8 @@ static void ufshcd_err_handler(struct work_struct *work)
 	}
 	ufshcd_clear_eh_in_progress(hba);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	ufshcd_scsi_unblock_requests(hba);
 	ufshcd_err_handling_unprepare(hba);
 	up(&hba->host_sem);
-
-	if (!err && needs_reset)
-		ufshcd_clear_ua_wluns(hba);
 }
 
 /**
@@ -7858,6 +7862,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool async)
 	unsigned long flags;
 	ktime_t start = ktime_get();
 
+	hba->ufshcd_state = UFSHCD_STATE_RESET;
+
 	ret = ufshcd_link_startup(hba);
 	if (ret)
 		goto out;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 2/3] scsi: ufs-qcom: Disable interrupt in reset path

[Can Guo]

From: Nitin Rawat <nitirawa@codeaurora.org>

Disable interrupt in reset path to flush pending IRQ handler in order to
avoid possible NoC issues.

Signed-off-by: Nitin Rawat <nitirawa@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index f97d7b0..a9dc8d7 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -253,12 +253,17 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
 {
 	int ret = 0;
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	bool reenable_intr = false;
 
 	if (!host->core_reset) {
 		dev_warn(hba->dev, "%s: reset control not set\n", __func__);
 		goto out;
 	}
 
+	reenable_intr = hba->is_irq_enabled;
+	disable_irq(hba->irq);
+	hba->is_irq_enabled = false;
+
 	ret = reset_control_assert(host->core_reset);
 	if (ret) {
 		dev_err(hba->dev, "%s: core_reset assert failed, err = %d\n",
@@ -280,6 +285,11 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
 
 	usleep_range(1000, 1100);
 
+	if (reenable_intr) {
+		enable_irq(hba->irq);
+		hba->is_irq_enabled = true;
+	}
+
 out:
 	return ret;
 }
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 3/3] scsi: ufs: Remove redundant checks of !hba in suspend/resume callbacks

[Can Guo]

Runtime and system suspend/resume can only come after hba probe invokes
platform_set_drvdata(pdev, hba), meaning hba cannot be NULL in these PM
callbacks, so remove the checks of !hba.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 013eb73..2517ef1 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -95,8 +95,6 @@
 		       16, 4, buf, __len, false);                        \
 } while (0)
 
-static bool early_suspend;
-
 int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 		     const char *prefix)
 {
@@ -8978,11 +8976,6 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
-	if (!hba) {
-		early_suspend = true;
-		return 0;
-	}
-
 	down(&hba->host_sem);
 
 	if (!hba->is_powered)
@@ -9034,14 +9027,6 @@ int ufshcd_system_resume(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
-	if (!hba)
-		return -EINVAL;
-
-	if (unlikely(early_suspend)) {
-		early_suspend = false;
-		down(&hba->host_sem);
-	}
-
 	if (!hba->is_powered || pm_runtime_suspended(hba->dev))
 		/*
 		 * Let the runtime resume take care of resuming
@@ -9074,9 +9059,6 @@ int ufshcd_runtime_suspend(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
-	if (!hba)
-		return -EINVAL;
-
 	if (!hba->is_powered)
 		goto out;
 	else
@@ -9115,9 +9097,6 @@ int ufshcd_runtime_resume(struct ufs_hba *hba)
 	int ret = 0;
 	ktime_t start = ktime_get();
 
-	if (!hba)
-		return -EINVAL;
-
 	if (!hba->is_powered)
 		goto out;
 	else
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

RE: [PATCH v2 2/3] scsi: ufs-qcom: Disable interrupt in reset path

[Avri Altman]

> 
> From: Nitin Rawat <nitirawa@codeaurora.org>
> 
> Disable interrupt in reset path to flush pending IRQ handler in order to
> avoid possible NoC issues.
> 
> Signed-off-by: Nitin Rawat <nitirawa@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs-qcom.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index f97d7b0..a9dc8d7 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -253,12 +253,17 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
>  {
>         int ret = 0;
>         struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +       bool reenable_intr = false;
> 
>         if (!host->core_reset) {
>                 dev_warn(hba->dev, "%s: reset control not set\n", __func__);
>                 goto out;
>         }
> 
> +       reenable_intr = hba->is_irq_enabled;
> +       disable_irq(hba->irq);
> +       hba->is_irq_enabled = false;
> +
>         ret = reset_control_assert(host->core_reset);
>         if (ret) {
>                 dev_err(hba->dev, "%s: core_reset assert failed, err = %d\n",
> @@ -280,6 +285,11 @@ static int ufs_qcom_host_reset(struct ufs_hba *hba)
> 
>         usleep_range(1000, 1100);
> 
> +       if (reenable_intr) {
> +               enable_irq(hba->irq);
> +               hba->is_irq_enabled = true;
> +       }
> +
If in the future, you will enable UFSHCI_QUIRK_BROKEN_HCE on your platform (currently only for Exynos),
Will this code still work?

Re: [PATCH v2 2/3] scsi: ufs-qcom: Disable interrupt in reset path

[Can Guo]

On 2021-02-28 22:23, Avri Altman wrote:
>> 
>> From: Nitin Rawat <nitirawa@codeaurora.org>
>> 
>> Disable interrupt in reset path to flush pending IRQ handler in order 
>> to
>> avoid possible NoC issues.
>> 
>> Signed-off-by: Nitin Rawat <nitirawa@codeaurora.org>
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufs-qcom.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> 
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index f97d7b0..a9dc8d7 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -253,12 +253,17 @@ static int ufs_qcom_host_reset(struct ufs_hba 
>> *hba)
>>  {
>>         int ret = 0;
>>         struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> +       bool reenable_intr = false;
>> 
>>         if (!host->core_reset) {
>>                 dev_warn(hba->dev, "%s: reset control not set\n", 
>> __func__);
>>                 goto out;
>>         }
>> 
>> +       reenable_intr = hba->is_irq_enabled;
>> +       disable_irq(hba->irq);
>> +       hba->is_irq_enabled = false;
>> +
>>         ret = reset_control_assert(host->core_reset);
>>         if (ret) {
>>                 dev_err(hba->dev, "%s: core_reset assert failed, err = 
>> %d\n",
>> @@ -280,6 +285,11 @@ static int ufs_qcom_host_reset(struct ufs_hba 
>> *hba)
>> 
>>         usleep_range(1000, 1100);
>> 
>> +       if (reenable_intr) {
>> +               enable_irq(hba->irq);
>> +               hba->is_irq_enabled = true;
>> +       }
>> +
> If in the future, you will enable UFSHCI_QUIRK_BROKEN_HCE on your
> platform (currently only for Exynos),
> Will this code still work?

Yes, it still works.

Thanks,
Can Guo.

RE: [PATCH v2 2/3] scsi: ufs-qcom: Disable interrupt in reset path

[Avri Altman]

> 
> On 2021-02-28 22:23, Avri Altman wrote:
> >>
> >> From: Nitin Rawat <nitirawa@codeaurora.org>
> >>
> >> Disable interrupt in reset path to flush pending IRQ handler in order
> >> to
> >> avoid possible NoC issues.
> >>
> >> Signed-off-by: Nitin Rawat <nitirawa@codeaurora.org>
> >> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>


> >> ---
> >>  drivers/scsi/ufs/ufs-qcom.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> >> index f97d7b0..a9dc8d7 100644
> >> --- a/drivers/scsi/ufs/ufs-qcom.c
> >> +++ b/drivers/scsi/ufs/ufs-qcom.c
> >> @@ -253,12 +253,17 @@ static int ufs_qcom_host_reset(struct ufs_hba
> >> *hba)
> >>  {
> >>         int ret = 0;
> >>         struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> >> +       bool reenable_intr = false;
> >>
> >>         if (!host->core_reset) {
> >>                 dev_warn(hba->dev, "%s: reset control not set\n",
> >> __func__);
> >>                 goto out;
> >>         }
> >>
> >> +       reenable_intr = hba->is_irq_enabled;
> >> +       disable_irq(hba->irq);
> >> +       hba->is_irq_enabled = false;
> >> +
> >>         ret = reset_control_assert(host->core_reset);
> >>         if (ret) {
> >>                 dev_err(hba->dev, "%s: core_reset assert failed, err =
> >> %d\n",
> >> @@ -280,6 +285,11 @@ static int ufs_qcom_host_reset(struct ufs_hba
> >> *hba)
> >>
> >>         usleep_range(1000, 1100);
> >>
> >> +       if (reenable_intr) {
> >> +               enable_irq(hba->irq);
> >> +               hba->is_irq_enabled = true;
> >> +       }
> >> +
> > If in the future, you will enable UFSHCI_QUIRK_BROKEN_HCE on your
> > platform (currently only for Exynos),
> > Will this code still work?
> 
> Yes, it still works.
> 
> Thanks,
> Can Guo.

RE: [PATCH v2 3/3] scsi: ufs: Remove redundant checks of !hba in suspend/resume callbacks

[Avri Altman]

> 
> Runtime and system suspend/resume can only come after hba probe invokes
> platform_set_drvdata(pdev, hba), meaning hba cannot be NULL in these PM
> callbacks, so remove the checks of !hba.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>


> ---
>  drivers/scsi/ufs/ufshcd.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 013eb73..2517ef1 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -95,8 +95,6 @@
>                        16, 4, buf, __len, false);                        \
>  } while (0)
> 
> -static bool early_suspend;
> -
>  int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
>                      const char *prefix)
>  {
> @@ -8978,11 +8976,6 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
>         int ret = 0;
>         ktime_t start = ktime_get();
> 
> -       if (!hba) {
> -               early_suspend = true;
> -               return 0;
> -       }
> -
>         down(&hba->host_sem);
> 
>         if (!hba->is_powered)
> @@ -9034,14 +9027,6 @@ int ufshcd_system_resume(struct ufs_hba *hba)
>         int ret = 0;
>         ktime_t start = ktime_get();
> 
> -       if (!hba)
> -               return -EINVAL;
> -
> -       if (unlikely(early_suspend)) {
> -               early_suspend = false;
> -               down(&hba->host_sem);
> -       }
> -
>         if (!hba->is_powered || pm_runtime_suspended(hba->dev))
>                 /*
>                  * Let the runtime resume take care of resuming
> @@ -9074,9 +9059,6 @@ int ufshcd_runtime_suspend(struct ufs_hba *hba)
>         int ret = 0;
>         ktime_t start = ktime_get();
> 
> -       if (!hba)
> -               return -EINVAL;
> -
>         if (!hba->is_powered)
>                 goto out;
>         else
> @@ -9115,9 +9097,6 @@ int ufshcd_runtime_resume(struct ufs_hba *hba)
>         int ret = 0;
>         ktime_t start = ktime_get();
> 
> -       if (!hba)
> -               return -EINVAL;
> -
>         if (!hba->is_powered)
>                 goto out;
>         else
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project.

RE: [PATCH v2 1/3] scsi: ufs: Minor adjustments to error handling

[Avri Altman]

> 
> 
> In error handling prepare stage, after SCSI requests are blocked, do a
> down/up_write(clk_scaling_lock) to clean up the queuecommand() path.
> Meanwhile, stop eeh_work in case it disturbs error recovery. Moreover,
> reset ufshcd_state at the entrance of ufshcd_probe_hba(), since it may be
> called multiple times during error recovery.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
I noticed that you tagged Adrian's patch -
https://lore.kernel.org/lkml/20210301191940.15247-1-adrian.hunter@intel.com/
So this patch needs to be adjusted accordingly?

Thanks,
Avri

> ---
>  drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 80620c8..013eb73 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4987,6 +4987,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>                          * UFS device needs urgent BKOPs.
>                          */
>                         if (!hba->pm_op_in_progress &&
> +                           !ufshcd_eh_in_progress(hba) &&
>                             ufshcd_is_exception_event(lrbp->ucd_rsp_ptr) &&
>                             schedule_work(&hba->eeh_work)) {
>                                 /*
> @@ -5784,13 +5785,20 @@ static void ufshcd_err_handling_prepare(struct
> ufs_hba *hba)
>                         ufshcd_suspend_clkscaling(hba);
>                 ufshcd_clk_scaling_allow(hba, false);
>         }
> +       ufshcd_scsi_block_requests(hba);
> +       /* Drain ufshcd_queuecommand() */
> +       down_write(&hba->clk_scaling_lock);
> +       up_write(&hba->clk_scaling_lock);
> +       cancel_work_sync(&hba->eeh_work);
>  }
> 
>  static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>  {
> +       ufshcd_scsi_unblock_requests(hba);
>         ufshcd_release(hba);
>         if (ufshcd_is_clkscaling_supported(hba))
>                 ufshcd_clk_scaling_suspend(hba, false);
> +       ufshcd_clear_ua_wluns(hba);
>         pm_runtime_put(hba->dev);
>  }
> 
> @@ -5882,8 +5890,8 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>         ufshcd_err_handling_prepare(hba);
>         spin_lock_irqsave(hba->host->host_lock, flags);
> -       ufshcd_scsi_block_requests(hba);
> -       hba->ufshcd_state = UFSHCD_STATE_RESET;
> +       if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> +               hba->ufshcd_state = UFSHCD_STATE_RESET;
> 
>         /* Complete requests that have door-bell cleared by h/w */
>         ufshcd_complete_requests(hba);
> @@ -6042,12 +6050,8 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>         }
>         ufshcd_clear_eh_in_progress(hba);
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> -       ufshcd_scsi_unblock_requests(hba);
>         ufshcd_err_handling_unprepare(hba);
>         up(&hba->host_sem);
> -
> -       if (!err && needs_reset)
> -               ufshcd_clear_ua_wluns(hba);
>  }
> 
>  /**
> @@ -7858,6 +7862,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool async)
>         unsigned long flags;
>         ktime_t start = ktime_get();
> 
> +       hba->ufshcd_state = UFSHCD_STATE_RESET;
> +
>         ret = ufshcd_link_startup(hba);
>         if (ret)
>                 goto out;
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project.

Re: [PATCH v2 1/3] scsi: ufs: Minor adjustments to error handling

[Can Guo]

Hi Avri,

On 2021-03-03 15:22, Avri Altman wrote:
>> 
>> 
>> In error handling prepare stage, after SCSI requests are blocked, do a
>> down/up_write(clk_scaling_lock) to clean up the queuecommand() path.
>> Meanwhile, stop eeh_work in case it disturbs error recovery. Moreover,
>> reset ufshcd_state at the entrance of ufshcd_probe_hba(), since it may 
>> be
>> called multiple times during error recovery.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
> I noticed that you tagged Adrian's patch -
> https://lore.kernel.org/lkml/20210301191940.15247-1-adrian.hunter@intel.com/
> So this patch needs to be adjusted accordingly?

Thanks for pointing me to that one, I will rebase mine.

Regards,
Can Guo.

> 
> Thanks,
> Avri
> 
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 80620c8..013eb73 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -4987,6 +4987,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
>> struct ufshcd_lrb *lrbp)
>>                          * UFS device needs urgent BKOPs.
>>                          */
>>                         if (!hba->pm_op_in_progress &&
>> +                           !ufshcd_eh_in_progress(hba) &&
>>                             
>> ufshcd_is_exception_event(lrbp->ucd_rsp_ptr) &&
>>                             schedule_work(&hba->eeh_work)) {
>>                                 /*
>> @@ -5784,13 +5785,20 @@ static void ufshcd_err_handling_prepare(struct
>> ufs_hba *hba)
>>                         ufshcd_suspend_clkscaling(hba);
>>                 ufshcd_clk_scaling_allow(hba, false);
>>         }
>> +       ufshcd_scsi_block_requests(hba);
>> +       /* Drain ufshcd_queuecommand() */
>> +       down_write(&hba->clk_scaling_lock);
>> +       up_write(&hba->clk_scaling_lock);
>> +       cancel_work_sync(&hba->eeh_work);
>>  }
>> 
>>  static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>>  {
>> +       ufshcd_scsi_unblock_requests(hba);
>>         ufshcd_release(hba);
>>         if (ufshcd_is_clkscaling_supported(hba))
>>                 ufshcd_clk_scaling_suspend(hba, false);
>> +       ufshcd_clear_ua_wluns(hba);
>>         pm_runtime_put(hba->dev);
>>  }
>> 
>> @@ -5882,8 +5890,8 @@ static void ufshcd_err_handler(struct 
>> work_struct
>> *work)
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>>         ufshcd_err_handling_prepare(hba);
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>> -       ufshcd_scsi_block_requests(hba);
>> -       hba->ufshcd_state = UFSHCD_STATE_RESET;
>> +       if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
>> +               hba->ufshcd_state = UFSHCD_STATE_RESET;
>> 
>>         /* Complete requests that have door-bell cleared by h/w */
>>         ufshcd_complete_requests(hba);
>> @@ -6042,12 +6050,8 @@ static void ufshcd_err_handler(struct 
>> work_struct
>> *work)
>>         }
>>         ufshcd_clear_eh_in_progress(hba);
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>> -       ufshcd_scsi_unblock_requests(hba);
>>         ufshcd_err_handling_unprepare(hba);
>>         up(&hba->host_sem);
>> -
>> -       if (!err && needs_reset)
>> -               ufshcd_clear_ua_wluns(hba);
>>  }
>> 
>>  /**
>> @@ -7858,6 +7862,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
>> bool async)
>>         unsigned long flags;
>>         ktime_t start = ktime_get();
>> 
>> +       hba->ufshcd_state = UFSHCD_STATE_RESET;
>> +
>>         ret = ufshcd_link_startup(hba);
>>         if (ret)
>>                 goto out;
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
>> Linux
>> Foundation Collaborative Project.

Re: [PATCH v2 1/3] scsi: ufs: Minor adjustments to error handling

[Can Guo]

On 2021-03-03 18:03, Can Guo wrote:
> Hi Avri,
> 
> On 2021-03-03 15:22, Avri Altman wrote:
>>> 
>>> 
>>> In error handling prepare stage, after SCSI requests are blocked, do 
>>> a
>>> down/up_write(clk_scaling_lock) to clean up the queuecommand() path.
>>> Meanwhile, stop eeh_work in case it disturbs error recovery. 
>>> Moreover,
>>> reset ufshcd_state at the entrance of ufshcd_probe_hba(), since it 
>>> may be
>>> called multiple times during error recovery.
>>> 
>>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> I noticed that you tagged Adrian's patch -
>> https://lore.kernel.org/lkml/20210301191940.15247-1-adrian.hunter@intel.com/
>> So this patch needs to be adjusted accordingly?
> 
> Thanks for pointing me to that one, I will rebase mine.
> 
> Regards,
> Can Guo.
> 

Just noticed that Adrian's change comes later than mine, so I may not 
need to
adjust mine.

Thanks,
Can Guo.

>> 
>> Thanks,
>> Avri
>> 
>>> ---
>>>  drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++------
>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>> index 80620c8..013eb73 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -4987,6 +4987,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
>>> struct ufshcd_lrb *lrbp)
>>>                          * UFS device needs urgent BKOPs.
>>>                          */
>>>                         if (!hba->pm_op_in_progress &&
>>> +                           !ufshcd_eh_in_progress(hba) &&
>>>                             
>>> ufshcd_is_exception_event(lrbp->ucd_rsp_ptr) &&
>>>                             schedule_work(&hba->eeh_work)) {
>>>                                 /*
>>> @@ -5784,13 +5785,20 @@ static void 
>>> ufshcd_err_handling_prepare(struct
>>> ufs_hba *hba)
>>>                         ufshcd_suspend_clkscaling(hba);
>>>                 ufshcd_clk_scaling_allow(hba, false);
>>>         }
>>> +       ufshcd_scsi_block_requests(hba);
>>> +       /* Drain ufshcd_queuecommand() */
>>> +       down_write(&hba->clk_scaling_lock);
>>> +       up_write(&hba->clk_scaling_lock);
>>> +       cancel_work_sync(&hba->eeh_work);
>>>  }
>>> 
>>>  static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>>>  {
>>> +       ufshcd_scsi_unblock_requests(hba);
>>>         ufshcd_release(hba);
>>>         if (ufshcd_is_clkscaling_supported(hba))
>>>                 ufshcd_clk_scaling_suspend(hba, false);
>>> +       ufshcd_clear_ua_wluns(hba);
>>>         pm_runtime_put(hba->dev);
>>>  }
>>> 
>>> @@ -5882,8 +5890,8 @@ static void ufshcd_err_handler(struct 
>>> work_struct
>>> *work)
>>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>         ufshcd_err_handling_prepare(hba);
>>>         spin_lock_irqsave(hba->host->host_lock, flags);
>>> -       ufshcd_scsi_block_requests(hba);
>>> -       hba->ufshcd_state = UFSHCD_STATE_RESET;
>>> +       if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
>>> +               hba->ufshcd_state = UFSHCD_STATE_RESET;
>>> 
>>>         /* Complete requests that have door-bell cleared by h/w */
>>>         ufshcd_complete_requests(hba);
>>> @@ -6042,12 +6050,8 @@ static void ufshcd_err_handler(struct 
>>> work_struct
>>> *work)
>>>         }
>>>         ufshcd_clear_eh_in_progress(hba);
>>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>>> -       ufshcd_scsi_unblock_requests(hba);
>>>         ufshcd_err_handling_unprepare(hba);
>>>         up(&hba->host_sem);
>>> -
>>> -       if (!err && needs_reset)
>>> -               ufshcd_clear_ua_wluns(hba);
>>>  }
>>> 
>>>  /**
>>> @@ -7858,6 +7862,8 @@ static int ufshcd_probe_hba(struct ufs_hba 
>>> *hba,
>>> bool async)
>>>         unsigned long flags;
>>>         ktime_t start = ktime_get();
>>> 
>>> +       hba->ufshcd_state = UFSHCD_STATE_RESET;
>>> +
>>>         ret = ufshcd_link_startup(hba);
>>>         if (ret)
>>>                 goto out;
>>> --
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
>>> Linux
>>> Foundation Collaborative Project.

RE: [PATCH v2 1/3] scsi: ufs: Minor adjustments to error handling

[Avri Altman]

> 
> In error handling prepare stage, after SCSI requests are blocked, do a
> down/up_write(clk_scaling_lock) to clean up the queuecommand() path.
> Meanwhile, stop eeh_work in case it disturbs error recovery. Moreover,
> reset ufshcd_state at the entrance of ufshcd_probe_hba(), since it may be
> called multiple times during error recovery.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>


> ---
>  drivers/scsi/ufs/ufshcd.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 80620c8..013eb73 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4987,6 +4987,7 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>                          * UFS device needs urgent BKOPs.
>                          */
>                         if (!hba->pm_op_in_progress &&
> +                           !ufshcd_eh_in_progress(hba) &&
>                             ufshcd_is_exception_event(lrbp->ucd_rsp_ptr) &&
>                             schedule_work(&hba->eeh_work)) {
>                                 /*
> @@ -5784,13 +5785,20 @@ static void ufshcd_err_handling_prepare(struct
> ufs_hba *hba)
>                         ufshcd_suspend_clkscaling(hba);
>                 ufshcd_clk_scaling_allow(hba, false);
>         }
> +       ufshcd_scsi_block_requests(hba);
> +       /* Drain ufshcd_queuecommand() */
> +       down_write(&hba->clk_scaling_lock);
> +       up_write(&hba->clk_scaling_lock);
> +       cancel_work_sync(&hba->eeh_work);
>  }
> 
>  static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>  {
> +       ufshcd_scsi_unblock_requests(hba);
>         ufshcd_release(hba);
>         if (ufshcd_is_clkscaling_supported(hba))
>                 ufshcd_clk_scaling_suspend(hba, false);
> +       ufshcd_clear_ua_wluns(hba);
>         pm_runtime_put(hba->dev);
>  }
> 
> @@ -5882,8 +5890,8 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>         ufshcd_err_handling_prepare(hba);
>         spin_lock_irqsave(hba->host->host_lock, flags);
> -       ufshcd_scsi_block_requests(hba);
> -       hba->ufshcd_state = UFSHCD_STATE_RESET;
> +       if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> +               hba->ufshcd_state = UFSHCD_STATE_RESET;
> 
>         /* Complete requests that have door-bell cleared by h/w */
>         ufshcd_complete_requests(hba);
> @@ -6042,12 +6050,8 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>         }
>         ufshcd_clear_eh_in_progress(hba);
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> -       ufshcd_scsi_unblock_requests(hba);
>         ufshcd_err_handling_unprepare(hba);
>         up(&hba->host_sem);
> -
> -       if (!err && needs_reset)
> -               ufshcd_clear_ua_wluns(hba);
>  }
> 
>  /**
> @@ -7858,6 +7862,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool async)
>         unsigned long flags;
>         ktime_t start = ktime_get();
> 
> +       hba->ufshcd_state = UFSHCD_STATE_RESET;
> +
>         ret = ufshcd_link_startup(hba);
>         if (ret)
>                 goto out;
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project.

Re: [PATCH v2 0/3] Three minor changes related with error handling

[Martin K. Petersen]

On Tue, 23 Feb 2021 21:36:46 -0800, Can Guo wrote:

> The 1st change is a complementary change for error handling.
> The 2nd change disables IRQ in host reset path to avoid possible NoC issues.
> The 3rd change is a minor cleanup to !hba checks in suspend/resume paths
> 
> Can Guo (2):
>   scsi: ufs: Minor adjustments to error handling
>   scsi: ufs: Remove redundant checks of !hba in suspend/resume callbacks
> 
> [...]

Applied to 5.12/scsi-fixes, thanks!

[1/3] scsi: ufs: Minor adjustments to error handling
      https://git.kernel.org/mkp/scsi/c/5e7363b98e21
[2/3] scsi: ufs-qcom: Disable interrupt in reset path
      https://git.kernel.org/mkp/scsi/c/51d31ee8de31
[3/3] scsi: ufs: Remove redundant checks of !hba in suspend/resume callbacks
      https://git.kernel.org/mkp/scsi/c/8514907e4f64

-- 
Martin K. Petersen	Oracle Linux Engineering