[PATCH v4 1/7] scsi: ufs: Add device reset in link recovery path

[PATCH v4 1/7] scsi: ufs: Add device reset in link recovery path

[Can Guo]

In order to recover from hibern8 exit failure, perform a reset in
link recovery path before issuing link start-up.

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 c28c144..525f8e6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3859,6 +3859,9 @@ static int ufshcd_link_recovery(struct ufs_hba *hba)
 	ufshcd_set_eh_in_progress(hba);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
+	/* Reset the attached device */
+	ufshcd_vops_device_reset(hba);
+
 	ret = ufshcd_host_reset_and_restore(hba);
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 2/7] scsi: ufs-qcom: Add reset control support for host controller

[Can Guo]

Add reset control for host controller so that host controller can be reset
as required in its power up sequence.

Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-qcom.h |  3 +++
 2 files changed, 56 insertions(+)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index a5b7148..c69c29a1c 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -246,6 +246,44 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
 	mb();
 }
 
+/**
+ * ufs_qcom_host_reset - reset host controller and PHY
+ */
+static int ufs_qcom_host_reset(struct ufs_hba *hba)
+{
+	int ret = 0;
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+
+	if (!host->core_reset) {
+		dev_warn(hba->dev, "%s: reset control not set\n", __func__);
+		goto out;
+	}
+
+	ret = reset_control_assert(host->core_reset);
+	if (ret) {
+		dev_err(hba->dev, "%s: core_reset assert failed, err = %d\n",
+				 __func__, ret);
+		goto out;
+	}
+
+	/*
+	 * The hardware requirement for delay between assert/deassert
+	 * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
+	 * ~125us (4/32768). To be on the safe side add 200us delay.
+	 */
+	usleep_range(200, 210);
+
+	ret = reset_control_deassert(host->core_reset);
+	if (ret)
+		dev_err(hba->dev, "%s: core_reset deassert failed, err = %d\n",
+				 __func__, ret);
+
+	usleep_range(1000, 1100);
+
+out:
+	return ret;
+}
+
 static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
@@ -254,6 +292,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 	bool is_rate_B = (UFS_QCOM_LIMIT_HS_RATE == PA_HS_MODE_B)
 							? true : false;
 
+	/* Reset UFS Host Controller and PHY */
+	ret = ufs_qcom_host_reset(hba);
+	if (ret)
+		dev_warn(hba->dev, "%s: host reset returned %d\n",
+				  __func__, ret);
+
 	if (is_rate_B)
 		phy_set_mode(phy, PHY_MODE_UFS_HS_B);
 
@@ -1101,6 +1145,15 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	host->hba = hba;
 	ufshcd_set_variant(hba, host);
 
+	/* Setup the reset control of HCI */
+	host->core_reset = devm_reset_control_get(hba->dev, "rst");
+	if (IS_ERR(host->core_reset)) {
+		err = PTR_ERR(host->core_reset);
+		dev_warn(dev, "Failed to get reset control %d\n", err);
+		host->core_reset = NULL;
+		err = 0;
+	}
+
 	/* Fire up the reset controller. Failure here is non-fatal. */
 	host->rcdev.of_node = dev->of_node;
 	host->rcdev.ops = &ufs_qcom_reset_ops;
diff --git a/drivers/scsi/ufs/ufs-qcom.h b/drivers/scsi/ufs/ufs-qcom.h
index d401f17..2d95e7c 100644
--- a/drivers/scsi/ufs/ufs-qcom.h
+++ b/drivers/scsi/ufs/ufs-qcom.h
@@ -6,6 +6,7 @@
 #define UFS_QCOM_H_
 
 #include <linux/reset-controller.h>
+#include <linux/reset.h>
 
 #define MAX_UFS_QCOM_HOSTS	1
 #define MAX_U32                 (~(u32)0)
@@ -233,6 +234,8 @@ struct ufs_qcom_host {
 	u32 dbg_print_en;
 	struct ufs_qcom_testbus testbus;
 
+	/* Reset control of HCI */
+	struct reset_control *core_reset;
 	struct reset_controller_dev rcdev;
 
 	struct gpio_desc *device_reset;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v4 2/7] scsi: ufs-qcom: Add reset control support for host controller

[Avri Altman]

Hi,

> 
> 
> Add reset control for host controller so that host controller can be reset as
> required in its power up sequence.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> +       ret = reset_control_assert(host->core_reset);
> +       if (ret) {
> +               dev_err(hba->dev, "%s: core_reset assert failed, err = %d\n",
> +                                __func__, ret);
> +               goto out;
> +       }
> +
> +       /*
> +        * The hardware requirement for delay between assert/deassert
> +        * is at least 3-4 sleep clock (32.7KHz) cycles, which comes to
> +        * ~125us (4/32768). To be on the safe side add 200us delay.
> +        */
> +       usleep_range(200, 210);
Aren't you sleeping anyway in your reset_control_ops?

> +
> +       ret = reset_control_deassert(host->core_reset);
> +       if (ret)
> +               dev_err(hba->dev, "%s: core_reset deassert failed, err = %d\n",
> +                                __func__, ret);
> +
> +       usleep_range(1000, 1100);
ditto

Re: [PATCH v4 2/7] scsi: ufs-qcom: Add reset control support for host controller

[Can Guo]

On 2019-11-14 17:03, Avri Altman wrote:
> Hi,
> 
>> 
>> 
>> Add reset control for host controller so that host controller can be 
>> reset as
>> required in its power up sequence.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> +       ret = reset_control_assert(host->core_reset);
>> +       if (ret) {
>> +               dev_err(hba->dev, "%s: core_reset assert failed, err = 
>> %d\n",
>> +                                __func__, ret);
>> +               goto out;
>> +       }
>> +
>> +       /*
>> +        * The hardware requirement for delay between assert/deassert
>> +        * is at least 3-4 sleep clock (32.7KHz) cycles, which comes 
>> to
>> +        * ~125us (4/32768). To be on the safe side add 200us delay.
>> +        */
>> +       usleep_range(200, 210);
> Aren't you sleeping anyway in your reset_control_ops?
> 

For our cases, reset_control_assert uses the reset_control_ops->assert() 
we registered for
node &clock_gcc. There is no sleep or delay in Q's 
reset_control_ops->assert() func.

>> +
>> +       ret = reset_control_deassert(host->core_reset);
>> +       if (ret)
>> +               dev_err(hba->dev, "%s: core_reset deassert failed, err 
>> = %d\n",
>> +                                __func__, ret);
>> +
>> +       usleep_range(1000, 1100);
> ditto

Same as above.

Best Regards,
Can Guo.

[PATCH v4 3/7] scsi: ufs: Fix up auto hibern8 enablement

[Can Guo]

Fix up possible unclocked register access to auto hibern8 register in
resume path and through sysfs entry. Meanwhile, enable auto hibern8
only after device is fully initialized in probe path.

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

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 969a36b..6c2f19e 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -126,13 +126,18 @@ static void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
 		return;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	if (hba->ahit == ahit)
-		goto out_unlock;
-	hba->ahit = ahit;
-	if (!pm_runtime_suspended(hba->dev))
-		ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER);
-out_unlock:
+	if (hba->ahit != ahit)
+		hba->ahit = ahit;
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
+	if (!pm_runtime_suspended(hba->dev)) {
+		pm_runtime_get_sync(hba->dev);
+		ufshcd_hold(hba, false);
+		spin_lock_irqsave(hba->host->host_lock, flags);
+		ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER);
+		spin_unlock_irqrestore(hba->host->host_lock, flags);
+		ufshcd_release(hba);
+		pm_runtime_put(hba->dev);
+	}
 }
 
 /* Convert Auto-Hibernate Idle Timer register value to microseconds */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 525f8e6..f12f5a7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6892,9 +6892,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 	/* UniPro link is active now */
 	ufshcd_set_link_active(hba);
 
-	/* Enable Auto-Hibernate if configured */
-	ufshcd_auto_hibern8_enable(hba);
-
 	ret = ufshcd_verify_dev_init(hba);
 	if (ret)
 		goto out;
@@ -6945,6 +6942,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 	/* set the state as operational after switching to desired gear */
 	hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
 
+	/* Enable Auto-Hibernate if configured */
+	ufshcd_auto_hibern8_enable(hba);
+
 	/*
 	 * If we are in error handling context or in power management callbacks
 	 * context, no need to scan the host
@@ -7962,12 +7962,12 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	if (hba->clk_scaling.is_allowed)
 		ufshcd_resume_clkscaling(hba);
 
-	/* Schedule clock gating in case of no access to UFS device yet */
-	ufshcd_release(hba);
-
 	/* Enable Auto-Hibernate if configured */
 	ufshcd_auto_hibern8_enable(hba);
 
+	/* Schedule clock gating in case of no access to UFS device yet */
+	ufshcd_release(hba);
+
 	goto out;
 
 set_old_link_state:
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v4 3/7] scsi: ufs: Fix up auto hibern8 enablement

[Avri Altman]

> 
> 
> Fix up possible unclocked register access to auto hibern8 register in resume path
> and through sysfs entry. Meanwhile, enable auto hibern8 only after device is
> fully initialized in probe path.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Acked-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/scsi/ufs/ufs-sysfs.c | 17 +++++++++++------
>  drivers/scsi/ufs/ufshcd.c    | 12 ++++++------
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c index
> 969a36b..6c2f19e 100644
> --- a/drivers/scsi/ufs/ufs-sysfs.c
> +++ b/drivers/scsi/ufs/ufs-sysfs.c
> @@ -126,13 +126,18 @@ static void ufshcd_auto_hibern8_update(struct
> ufs_hba *hba, u32 ahit)
>                 return;
> 
>         spin_lock_irqsave(hba->host->host_lock, flags);
> -       if (hba->ahit == ahit)
> -               goto out_unlock;
> -       hba->ahit = ahit;
> -       if (!pm_runtime_suspended(hba->dev))
> -               ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER);
> -out_unlock:
> +       if (hba->ahit != ahit)
> +               hba->ahit = ahit;
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> +       if (!pm_runtime_suspended(hba->dev)) {
> +               pm_runtime_get_sync(hba->dev);
> +               ufshcd_hold(hba, false);
> +               spin_lock_irqsave(hba->host->host_lock, flags);
> +               ufshcd_writel(hba, hba->ahit, REG_AUTO_HIBERNATE_IDLE_TIMER);
> +               spin_unlock_irqrestore(hba->host->host_lock, flags);
Call ufshcd_auto_hibern8_enable instead of open coding? 

> +               ufshcd_release(hba);
> +               pm_runtime_put(hba->dev);
> +       }
>  }
> 
>  /* Convert Auto-Hibernate Idle Timer register value to microseconds */ diff --git
> a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 525f8e6..f12f5a7
> 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6892,9 +6892,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>         /* UniPro link is active now */
>         ufshcd_set_link_active(hba);
> 
> -       /* Enable Auto-Hibernate if configured */
> -       ufshcd_auto_hibern8_enable(hba);
> -
>         ret = ufshcd_verify_dev_init(hba);
>         if (ret)
>                 goto out;
> @@ -6945,6 +6942,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>         /* set the state as operational after switching to desired gear */
>         hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
> 
> +       /* Enable Auto-Hibernate if configured */
> +       ufshcd_auto_hibern8_enable(hba);
> +
>         /*
>          * If we are in error handling context or in power management callbacks
>          * context, no need to scan the host @@ -7962,12 +7962,12 @@ static int
> ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>         if (hba->clk_scaling.is_allowed)
>                 ufshcd_resume_clkscaling(hba);
> 
> -       /* Schedule clock gating in case of no access to UFS device yet */
> -       ufshcd_release(hba);
> -
>         /* Enable Auto-Hibernate if configured */
>         ufshcd_auto_hibern8_enable(hba);
> 
> +       /* Schedule clock gating in case of no access to UFS device yet */
> +       ufshcd_release(hba);
> +
>         goto out;
> 
>  set_old_link_state:
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v4 3/7] scsi: ufs: Fix up auto hibern8 enablement

[Can Guo]

On 2019-11-14 17:23, Avri Altman wrote:
>> 
>> 
>> Fix up possible unclocked register access to auto hibern8 register in 
>> resume path
>> and through sysfs entry. Meanwhile, enable auto hibern8 only after 
>> device is
>> fully initialized in probe path.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
> Acked-by: Avri Altman <avri.altman@wdc.com>
> 
>> ---
>>  drivers/scsi/ufs/ufs-sysfs.c | 17 +++++++++++------
>>  drivers/scsi/ufs/ufshcd.c    | 12 ++++++------
>>  2 files changed, 17 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufs-sysfs.c 
>> b/drivers/scsi/ufs/ufs-sysfs.c index
>> 969a36b..6c2f19e 100644
>> --- a/drivers/scsi/ufs/ufs-sysfs.c
>> +++ b/drivers/scsi/ufs/ufs-sysfs.c
>> @@ -126,13 +126,18 @@ static void ufshcd_auto_hibern8_update(struct
>> ufs_hba *hba, u32 ahit)
>>                 return;
>> 
>>         spin_lock_irqsave(hba->host->host_lock, flags);
>> -       if (hba->ahit == ahit)
>> -               goto out_unlock;
>> -       hba->ahit = ahit;
>> -       if (!pm_runtime_suspended(hba->dev))
>> -               ufshcd_writel(hba, hba->ahit, 
>> REG_AUTO_HIBERNATE_IDLE_TIMER);
>> -out_unlock:
>> +       if (hba->ahit != ahit)
>> +               hba->ahit = ahit;
>>         spin_unlock_irqrestore(hba->host->host_lock, flags);
>> +       if (!pm_runtime_suspended(hba->dev)) {
>> +               pm_runtime_get_sync(hba->dev);
>> +               ufshcd_hold(hba, false);
>> +               spin_lock_irqsave(hba->host->host_lock, flags);
>> +               ufshcd_writel(hba, hba->ahit, 
>> REG_AUTO_HIBERNATE_IDLE_TIMER);
>> +               spin_unlock_irqrestore(hba->host->host_lock, flags);
> Call ufshcd_auto_hibern8_enable instead of open coding?
> 

In that case, we would have to export ufshcd_auto_hibern8_enable() as it 
is static for now.
Shall do that.

Thanks,

Can Guo.

>> +               ufshcd_release(hba);
>> +               pm_runtime_put(hba->dev);
>> +       }
>>  }
>> 
>>  /* Convert Auto-Hibernate Idle Timer register value to microseconds 
>> */ diff --git
>> a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 
>> 525f8e6..f12f5a7
>> 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -6892,9 +6892,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>>         /* UniPro link is active now */
>>         ufshcd_set_link_active(hba);
>> 
>> -       /* Enable Auto-Hibernate if configured */
>> -       ufshcd_auto_hibern8_enable(hba);
>> -
>>         ret = ufshcd_verify_dev_init(hba);
>>         if (ret)
>>                 goto out;
>> @@ -6945,6 +6942,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
>>         /* set the state as operational after switching to desired 
>> gear */
>>         hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
>> 
>> +       /* Enable Auto-Hibernate if configured */
>> +       ufshcd_auto_hibern8_enable(hba);
>> +
>>         /*
>>          * If we are in error handling context or in power management 
>> callbacks
>>          * context, no need to scan the host @@ -7962,12 +7962,12 @@ 
>> static int
>> ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>         if (hba->clk_scaling.is_allowed)
>>                 ufshcd_resume_clkscaling(hba);
>> 
>> -       /* Schedule clock gating in case of no access to UFS device 
>> yet */
>> -       ufshcd_release(hba);
>> -
>>         /* Enable Auto-Hibernate if configured */
>>         ufshcd_auto_hibern8_enable(hba);
>> 
>> +       /* Schedule clock gating in case of no access to UFS device 
>> yet */
>> +       ufshcd_release(hba);
>> +
>>         goto out;
>> 
>>  set_old_link_state:
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project

[PATCH v4 4/7] scsi: ufs: Fix register dump caused sleep in atomic context

[Can Guo]

ufshcd_print_host_regs() can be called by interrupt handler, but it may
sleep due to ufshcd_dump_regs() allocates the dump buffer memory with flag
GFP_KERNEL. Fix it by changing GFP_KERNEL to GFP_ATMOIC.

Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f12f5a7..671ea2a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -117,7 +117,7 @@ int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 	if (offset % 4 != 0 || len % 4 != 0) /* keep readl happy */
 		return -EINVAL;
 
-	regs = kzalloc(len, GFP_KERNEL);
+	regs = kzalloc(len, GFP_ATOMIC);
 	if (!regs)
 		return -ENOMEM;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 5/7] scsi: ufs: Fix irq return code

[Can Guo]

From: Venkat Gopalakrishnan <venkatg@codeaurora.org>

Return IRQ_HANDLED only if the irq is really handled, this will
help in catching spurious interrupts that go unhandled.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 671ea2a..8d1b04f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -240,7 +240,7 @@ struct ufs_pm_lvl_states ufs_pm_lvl_states[] = {
 	END_FIX
 };
 
-static void ufshcd_tmc_handler(struct ufs_hba *hba);
+static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
 static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 static int ufshcd_reset_and_restore(struct ufs_hba *hba);
 static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
@@ -4799,19 +4799,29 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
  * ufshcd_uic_cmd_compl - handle completion of uic command
  * @hba: per adapter instance
  * @intr_status: interrupt status generated by the controller
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 {
+	irqreturn_t retval = IRQ_NONE;
+
 	if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
 		hba->active_uic_cmd->argument2 |=
 			ufshcd_get_uic_cmd_result(hba);
 		hba->active_uic_cmd->argument3 =
 			ufshcd_get_dme_attr_val(hba);
 		complete(&hba->active_uic_cmd->done);
+		retval = IRQ_HANDLED;
 	}
 
-	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done)
+	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
 		complete(hba->uic_async_done);
+		retval = IRQ_HANDLED;
+	}
+	return retval;
 }
 
 /**
@@ -4867,8 +4877,12 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 /**
  * ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
+static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 {
 	unsigned long completed_reqs;
 	u32 tr_doorbell;
@@ -4887,7 +4901,12 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
 	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
 
-	__ufshcd_transfer_req_compl(hba, completed_reqs);
+	if (completed_reqs) {
+		__ufshcd_transfer_req_compl(hba, completed_reqs);
+		return IRQ_HANDLED;
+	} else {
+		return IRQ_NONE;
+	}
 }
 
 /**
@@ -5406,61 +5425,77 @@ static void ufshcd_err_handler(struct work_struct *work)
 /**
  * ufshcd_update_uic_error - check and set fatal UIC error flags.
  * @hba: per-adapter instance
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_update_uic_error(struct ufs_hba *hba)
+static irqreturn_t ufshcd_update_uic_error(struct ufs_hba *hba)
 {
 	u32 reg;
+	irqreturn_t retval = IRQ_NONE;
 
 	/* PHY layer lane error */
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_PHY_ADAPTER_LAYER);
 	/* Ignore LINERESET indication, as this is not an error */
 	if ((reg & UIC_PHY_ADAPTER_LAYER_ERROR) &&
-			(reg & UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK)) {
+	    (reg & UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK)) {
 		/*
 		 * To know whether this error is fatal or not, DB timeout
 		 * must be checked but this error is handled separately.
 		 */
 		dev_dbg(hba->dev, "%s: UIC Lane error reported\n", __func__);
 		ufshcd_update_reg_hist(&hba->ufs_stats.pa_err, reg);
+		retval |= IRQ_HANDLED;
 	}
 
 	/* PA_INIT_ERROR is fatal and needs UIC reset */
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
-	if (reg)
+	if ((reg & UIC_DATA_LINK_LAYER_ERROR) &&
+	    (reg & UIC_DATA_LINK_LAYER_ERROR_CODE_MASK)) {
 		ufshcd_update_reg_hist(&hba->ufs_stats.dl_err, reg);
 
-	if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
-		hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
-	else if (hba->dev_quirks &
-		   UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
-		if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
-			hba->uic_error |=
-				UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
-		else if (reg & UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
-			hba->uic_error |= UFSHCD_UIC_DL_TCx_REPLAY_ERROR;
+		if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
+			hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
+		else if (hba->dev_quirks &
+				UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
+			if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
+				hba->uic_error |=
+					UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
+			else if (reg & UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
+				hba->uic_error |= UFSHCD_UIC_DL_TCx_REPLAY_ERROR;
+		}
+		retval |= IRQ_HANDLED;
 	}
 
 	/* UIC NL/TL/DME errors needs software retry */
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_NETWORK_LAYER);
-	if (reg) {
+	if ((reg & UIC_NETWORK_LAYER_ERROR) &&
+	    (reg & UIC_NETWORK_LAYER_ERROR_CODE_MASK)) {
 		ufshcd_update_reg_hist(&hba->ufs_stats.nl_err, reg);
 		hba->uic_error |= UFSHCD_UIC_NL_ERROR;
+		retval |= IRQ_HANDLED;
 	}
 
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_TRANSPORT_LAYER);
-	if (reg) {
+	if ((reg & UIC_TRANSPORT_LAYER_ERROR) &&
+	    (reg & UIC_TRANSPORT_LAYER_ERROR_CODE_MASK)) {
 		ufshcd_update_reg_hist(&hba->ufs_stats.tl_err, reg);
 		hba->uic_error |= UFSHCD_UIC_TL_ERROR;
+		retval |= IRQ_HANDLED;
 	}
 
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DME);
-	if (reg) {
+	if ((reg & UIC_DME_ERROR) &&
+	    (reg & UIC_DME_ERROR_CODE_MASK)) {
 		ufshcd_update_reg_hist(&hba->ufs_stats.dme_err, reg);
 		hba->uic_error |= UFSHCD_UIC_DME_ERROR;
+		retval |= IRQ_HANDLED;
 	}
 
 	dev_dbg(hba->dev, "%s: UIC error flags = 0x%08x\n",
 			__func__, hba->uic_error);
+	return retval;
 }
 
 static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
@@ -5483,10 +5518,15 @@ static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
 /**
  * ufshcd_check_errors - Check for errors that need s/w attention
  * @hba: per-adapter instance
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_check_errors(struct ufs_hba *hba)
+static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba)
 {
 	bool queue_eh_work = false;
+	irqreturn_t retval = IRQ_NONE;
 
 	if (hba->errors & INT_FATAL_ERRORS) {
 		ufshcd_update_reg_hist(&hba->ufs_stats.fatal_err, hba->errors);
@@ -5495,7 +5535,7 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 
 	if (hba->errors & UIC_ERROR) {
 		hba->uic_error = 0;
-		ufshcd_update_uic_error(hba);
+		retval = ufshcd_update_uic_error(hba);
 		if (hba->uic_error)
 			queue_eh_work = true;
 	}
@@ -5543,6 +5583,7 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 			}
 			schedule_work(&hba->eh_work);
 		}
+		retval |= IRQ_HANDLED;
 	}
 	/*
 	 * if (!queue_eh_work) -
@@ -5550,44 +5591,62 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 	 * itself without s/w intervention or errors that will be
 	 * handled by the SCSI core layer.
 	 */
+	return retval;
 }
 
 /**
  * ufshcd_tmc_handler - handle task management function completion
  * @hba: per adapter instance
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_tmc_handler(struct ufs_hba *hba)
+static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
 {
 	u32 tm_doorbell;
 
 	tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
 	hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
-	wake_up(&hba->tm_wq);
+	if (hba->tm_condition) {
+		wake_up(&hba->tm_wq);
+		return IRQ_HANDLED;
+	} else {
+		return IRQ_NONE;
+	}
 }
 
 /**
  * ufshcd_sl_intr - Interrupt service routine
  * @hba: per adapter instance
  * @intr_status: contains interrupts generated by the controller
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
+static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 {
+	irqreturn_t retval = IRQ_NONE;
+
 	hba->errors = UFSHCD_ERROR_MASK & intr_status;
 
 	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
 		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
 
 	if (hba->errors)
-		ufshcd_check_errors(hba);
+		retval |= ufshcd_check_errors(hba);
 
 	if (intr_status & UFSHCD_UIC_MASK)
-		ufshcd_uic_cmd_compl(hba, intr_status);
+		retval |= ufshcd_uic_cmd_compl(hba, intr_status);
 
 	if (intr_status & UTP_TASK_REQ_COMPL)
-		ufshcd_tmc_handler(hba);
+		retval |= ufshcd_tmc_handler(hba);
 
 	if (intr_status & UTP_TRANSFER_REQ_COMPL)
-		ufshcd_transfer_req_compl(hba);
+		retval |= ufshcd_transfer_req_compl(hba);
+
+	return retval;
 }
 
 /**
@@ -5595,8 +5654,9 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  * @irq: irq number
  * @__hba: pointer to adapter instance
  *
- * Returns IRQ_HANDLED - If interrupt is valid
- *		IRQ_NONE - If invalid interrupt
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
 static irqreturn_t ufshcd_intr(int irq, void *__hba)
 {
@@ -5619,14 +5679,18 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
 			intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 		if (intr_status)
 			ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
-		if (enabled_intr_status) {
-			ufshcd_sl_intr(hba, enabled_intr_status);
-			retval = IRQ_HANDLED;
-		}
+		if (enabled_intr_status)
+			retval |= ufshcd_sl_intr(hba, enabled_intr_status);
 
 		intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
 	} while (intr_status && --retries);
 
+	if (retval == IRQ_NONE) {
+		dev_err(hba->dev, "%s: Unhandled interrupt 0x%08x\n",
+					__func__, intr_status);
+		ufshcd_dump_regs(hba, 0, UFSHCI_REG_SPACE_SIZE, "host_regs: ");
+	}
+
 	spin_unlock(hba->host->host_lock);
 	return retval;
 }
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 v4 5/7] scsi: ufs: Fix irq return code

[Avri Altman]

> 
> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> 
> Return IRQ_HANDLED only if the irq is really handled, this will help in catching
> spurious interrupts that go unhandled.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Interesting enough - did your patch exposed unhandled interrupts?
If it does - maybe you can share some info in your commit log.

Thanks,
Avri

> ---
>  drivers/scsi/ufs/ufshcd.c | 134 ++++++++++++++++++++++++++++++++++-----
> -------
>  drivers/scsi/ufs/ufshci.h |   2 +-
>  2 files changed, 100 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 671ea2a..8d1b04f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -240,7 +240,7 @@ struct ufs_pm_lvl_states ufs_pm_lvl_states[] = {
>         END_FIX
>  };
> 
> -static void ufshcd_tmc_handler(struct ufs_hba *hba);
> +static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
>  static void ufshcd_async_scan(void *data, async_cookie_t cookie);  static int
> ufshcd_reset_and_restore(struct ufs_hba *hba);  static int
> ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd); @@ -4799,19
> +4799,29 @@ static void ufshcd_slave_destroy(struct scsi_device *sdev)
>   * ufshcd_uic_cmd_compl - handle completion of uic command
>   * @hba: per adapter instance
>   * @intr_status: interrupt status generated by the controller
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
> +static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32
> +intr_status)
>  {
> +       irqreturn_t retval = IRQ_NONE;
> +
>         if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
>                 hba->active_uic_cmd->argument2 |=
>                         ufshcd_get_uic_cmd_result(hba);
>                 hba->active_uic_cmd->argument3 =
>                         ufshcd_get_dme_attr_val(hba);
>                 complete(&hba->active_uic_cmd->done);
> +               retval = IRQ_HANDLED;
>         }
> 
> -       if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done)
> +       if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done)
> + {
>                 complete(hba->uic_async_done);
> +               retval = IRQ_HANDLED;
> +       }
> +       return retval;
>  }
> 
>  /**
> @@ -4867,8 +4877,12 @@ static void __ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>  /**
>   * ufshcd_transfer_req_compl - handle SCSI and query command completion
>   * @hba: per adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
>  {
>         unsigned long completed_reqs;
>         u32 tr_doorbell;
> @@ -4887,7 +4901,12 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
>         tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>         completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
> 
> -       __ufshcd_transfer_req_compl(hba, completed_reqs);
> +       if (completed_reqs) {
> +               __ufshcd_transfer_req_compl(hba, completed_reqs);
> +               return IRQ_HANDLED;
> +       } else {
> +               return IRQ_NONE;
> +       }
>  }
> 
>  /**
> @@ -5406,61 +5425,77 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>  /**
>   * ufshcd_update_uic_error - check and set fatal UIC error flags.
>   * @hba: per-adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_update_uic_error(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_update_uic_error(struct ufs_hba *hba)
>  {
>         u32 reg;
> +       irqreturn_t retval = IRQ_NONE;
> 
>         /* PHY layer lane error */
>         reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_PHY_ADAPTER_LAYER);
>         /* Ignore LINERESET indication, as this is not an error */
>         if ((reg & UIC_PHY_ADAPTER_LAYER_ERROR) &&
> -                       (reg & UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK)) {
> +           (reg & UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK)) {
>                 /*
>                  * To know whether this error is fatal or not, DB timeout
>                  * must be checked but this error is handled separately.
>                  */
>                 dev_dbg(hba->dev, "%s: UIC Lane error reported\n", __func__);
>                 ufshcd_update_reg_hist(&hba->ufs_stats.pa_err, reg);
> +               retval |= IRQ_HANDLED;
>         }
> 
>         /* PA_INIT_ERROR is fatal and needs UIC reset */
>         reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
> -       if (reg)
> +       if ((reg & UIC_DATA_LINK_LAYER_ERROR) &&
> +           (reg & UIC_DATA_LINK_LAYER_ERROR_CODE_MASK)) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.dl_err, reg);
> 
> -       if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> -               hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
> -       else if (hba->dev_quirks &
> -                  UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
> -               if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
> -                       hba->uic_error |=
> -                               UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
> -               else if (reg & UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
> -                       hba->uic_error |= UFSHCD_UIC_DL_TCx_REPLAY_ERROR;
> +               if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> +                       hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
> +               else if (hba->dev_quirks &
> +                               UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
> +                       if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
> +                               hba->uic_error |=
> +                                       UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
> +                       else if (reg &
> UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
> +                               hba->uic_error |= UFSHCD_UIC_DL_TCx_REPLAY_ERROR;
> +               }
> +               retval |= IRQ_HANDLED;
>         }
> 
>         /* UIC NL/TL/DME errors needs software retry */
>         reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_NETWORK_LAYER);
> -       if (reg) {
> +       if ((reg & UIC_NETWORK_LAYER_ERROR) &&
> +           (reg & UIC_NETWORK_LAYER_ERROR_CODE_MASK)) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.nl_err, reg);
>                 hba->uic_error |= UFSHCD_UIC_NL_ERROR;
> +               retval |= IRQ_HANDLED;
>         }
> 
>         reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_TRANSPORT_LAYER);
> -       if (reg) {
> +       if ((reg & UIC_TRANSPORT_LAYER_ERROR) &&
> +           (reg & UIC_TRANSPORT_LAYER_ERROR_CODE_MASK)) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.tl_err, reg);
>                 hba->uic_error |= UFSHCD_UIC_TL_ERROR;
> +               retval |= IRQ_HANDLED;
>         }
> 
>         reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DME);
> -       if (reg) {
> +       if ((reg & UIC_DME_ERROR) &&
> +           (reg & UIC_DME_ERROR_CODE_MASK)) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.dme_err, reg);
>                 hba->uic_error |= UFSHCD_UIC_DME_ERROR;
> +               retval |= IRQ_HANDLED;
>         }
> 
>         dev_dbg(hba->dev, "%s: UIC error flags = 0x%08x\n",
>                         __func__, hba->uic_error);
> +       return retval;
>  }
> 
>  static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba, @@ -5483,10
> +5518,15 @@ static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
>  /**
>   * ufshcd_check_errors - Check for errors that need s/w attention
>   * @hba: per-adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_check_errors(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba)
>  {
>         bool queue_eh_work = false;
> +       irqreturn_t retval = IRQ_NONE;
> 
>         if (hba->errors & INT_FATAL_ERRORS) {
>                 ufshcd_update_reg_hist(&hba->ufs_stats.fatal_err, hba->errors); @@
> -5495,7 +5535,7 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
> 
>         if (hba->errors & UIC_ERROR) {
>                 hba->uic_error = 0;
> -               ufshcd_update_uic_error(hba);
> +               retval = ufshcd_update_uic_error(hba);
>                 if (hba->uic_error)
>                         queue_eh_work = true;
>         }
> @@ -5543,6 +5583,7 @@ static void ufshcd_check_errors(struct ufs_hba
> *hba)
>                         }
>                         schedule_work(&hba->eh_work);
>                 }
> +               retval |= IRQ_HANDLED;
>         }
>         /*
>          * if (!queue_eh_work) -
> @@ -5550,44 +5591,62 @@ static void ufshcd_check_errors(struct ufs_hba
> *hba)
>          * itself without s/w intervention or errors that will be
>          * handled by the SCSI core layer.
>          */
> +       return retval;
>  }
> 
>  /**
>   * ufshcd_tmc_handler - handle task management function completion
>   * @hba: per adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_tmc_handler(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
>  {
>         u32 tm_doorbell;
> 
>         tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
>         hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
> -       wake_up(&hba->tm_wq);
> +       if (hba->tm_condition) {
> +               wake_up(&hba->tm_wq);
> +               return IRQ_HANDLED;
> +       } else {
> +               return IRQ_NONE;
> +       }
>  }
> 
>  /**
>   * ufshcd_sl_intr - Interrupt service routine
>   * @hba: per adapter instance
>   * @intr_status: contains interrupts generated by the controller
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> +static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
>  {
> +       irqreturn_t retval = IRQ_NONE;
> +
>         hba->errors = UFSHCD_ERROR_MASK & intr_status;
> 
>         if (ufshcd_is_auto_hibern8_error(hba, intr_status))
>                 hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
> 
>         if (hba->errors)
> -               ufshcd_check_errors(hba);
> +               retval |= ufshcd_check_errors(hba);
> 
>         if (intr_status & UFSHCD_UIC_MASK)
> -               ufshcd_uic_cmd_compl(hba, intr_status);
> +               retval |= ufshcd_uic_cmd_compl(hba, intr_status);
> 
>         if (intr_status & UTP_TASK_REQ_COMPL)
> -               ufshcd_tmc_handler(hba);
> +               retval |= ufshcd_tmc_handler(hba);
> 
>         if (intr_status & UTP_TRANSFER_REQ_COMPL)
> -               ufshcd_transfer_req_compl(hba);
> +               retval |= ufshcd_transfer_req_compl(hba);
> +
> +       return retval;
>  }
> 
>  /**
> @@ -5595,8 +5654,9 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32
> intr_status)
>   * @irq: irq number
>   * @__hba: pointer to adapter instance
>   *
> - * Returns IRQ_HANDLED - If interrupt is valid
> - *             IRQ_NONE - If invalid interrupt
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
>  static irqreturn_t ufshcd_intr(int irq, void *__hba)  { @@ -5619,14 +5679,18
> @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
>                         intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
>                 if (intr_status)
>                         ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> -               if (enabled_intr_status) {
> -                       ufshcd_sl_intr(hba, enabled_intr_status);
> -                       retval = IRQ_HANDLED;
> -               }
> +               if (enabled_intr_status)
> +                       retval |= ufshcd_sl_intr(hba,
> + enabled_intr_status);
> 
>                 intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
>         } while (intr_status && --retries);
> 
> +       if (retval == IRQ_NONE) {
> +               dev_err(hba->dev, "%s: Unhandled interrupt 0x%08x\n",
> +                                       __func__, intr_status);
> +               ufshcd_dump_regs(hba, 0, UFSHCI_REG_SPACE_SIZE, "host_regs: ");
> +       }
> +
>         spin_unlock(hba->host->host_lock);
>         return retval;
>  }
> 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

[PATCH v4 6/7] scsi: ufs: Abort gating if clock on request is pending

[Can Guo]

From: Asutosh Das <asutoshd@codeaurora.org>

This change attempts to abort gating of clocks if a
request to turn-on clocks is pending.
This would in turn avoid turning OFF and back ON the
clocks.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8d1b04f..7a5a904 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1610,7 +1610,7 @@ static void ufshcd_gate_work(struct work_struct *work)
 	 * state to CLKS_ON.
 	 */
 	if (hba->clk_gating.is_suspended ||
-		(hba->clk_gating.state == REQ_CLKS_ON)) {
+		(hba->clk_gating.state != REQ_CLKS_OFF)) {
 		hba->clk_gating.state = CLKS_ON;
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v4 6/7] scsi: ufs: Abort gating if clock on request is pending

[Avri Altman]

> 
> 
> From: Asutosh Das <asutoshd@codeaurora.org>
> 
> This change attempts to abort gating of clocks if a request to turn-on clocks is
> pending.
> This would in turn avoid turning OFF and back ON the clocks.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
You forgot to add the Reviewed-by tag from Bean.

[PATCH v4 7/7] scsi: ufs: Fix error handing during hibern8 enter

[Can Guo]

From: Subhash Jadavani <subhashj@codeaurora.org>

During clock gating (ufshcd_gate_work()), we first put the link hibern8 by
calling ufshcd_uic_hibern8_enter() and if ufshcd_uic_hibern8_enter()
returns success (0) then we gate all the clocks.
Now let’s zoom in to what ufshcd_uic_hibern8_enter() does internally:
It calls __ufshcd_uic_hibern8_enter() which on detecting the LINERESET,
initiates the full recovery and puts the link back to highest HS gear and
returns success (0) to ufshcd_uic_hibern8_enter() which is the issue as
link is still in active state due to recovery!
Now ufshcd_uic_hibern8_enter() returns success to ufshcd_gate_work() and
hence it goes ahead with gating the UFS clock while link is still in active
state hence I believe controller would raise UIC error interrupts. But when
we service the interrupt, clocks might have already been disabled!

This change fixes for this by returning failure from
__ufshcd_uic_hibern8_enter() if recovery succeeds as link is still not in
hibern8, upon receiving the error ufshcd_hibern8_enter() would initiate
retry to put the link state back into hibern8.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 7a5a904..934c27a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3891,15 +3891,24 @@ static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);
 
 	if (ret) {
+		int err;
+
 		dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d\n",
 			__func__, ret);
 
 		/*
-		 * If link recovery fails then return error so that caller
-		 * don't retry the hibern8 enter again.
+		 * If link recovery fails then return error code (-ENOLINK)
+		 * returned ufshcd_link_recovery().
+		 * If link recovery succeeds then return -EAGAIN to attempt
+		 * hibern8 enter retry again.
 		 */
-		if (ufshcd_link_recovery(hba))
-			ret = -ENOLINK;
+		err = ufshcd_link_recovery(hba);
+		if (err) {
+			dev_err(hba->dev, "%s: link recovery failed", __func__);
+			ret = err;
+		} else {
+			ret = -EAGAIN;
+		}
 	} else
 		ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER,
 								POST_CHANGE);
@@ -3913,7 +3922,7 @@ static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 
 	for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; retries--) {
 		ret = __ufshcd_uic_hibern8_enter(hba);
-		if (!ret || ret == -ENOLINK)
+		if (!ret)
 			goto out;
 	}
 out:
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v4 7/7] scsi: ufs: Fix error handing during hibern8 enter

[Avri Altman]

> 
> 
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> During clock gating (ufshcd_gate_work()), we first put the link hibern8 by
> calling ufshcd_uic_hibern8_enter() and if ufshcd_uic_hibern8_enter() returns
> success (0) then we gate all the clocks.
> Now let’s zoom in to what ufshcd_uic_hibern8_enter() does internally:
> It calls __ufshcd_uic_hibern8_enter() which on detecting the LINERESET,
> initiates the full recovery and puts the link back to highest HS gear and returns
> success (0) to ufshcd_uic_hibern8_enter() which is the issue as link is still in
> active state due to recovery!
> Now ufshcd_uic_hibern8_enter() returns success to ufshcd_gate_work() and
> hence it goes ahead with gating the UFS clock while link is still in active state
> hence I believe controller would raise UIC error interrupts. But when we service
> the interrupt, clocks might have already been disabled!
> 
> This change fixes for this by returning failure from
> __ufshcd_uic_hibern8_enter() if recovery succeeds as link is still not in hibern8,
> upon receiving the error ufshcd_hibern8_enter() would initiate retry to put the
> link state back into hibern8.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 7a5a904..934c27a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3891,15 +3891,24 @@ static int __ufshcd_uic_hibern8_enter(struct
> ufs_hba *hba)
>                              ktime_to_us(ktime_sub(ktime_get(), start)), ret);
> 
>         if (ret) {
> +               int err;
> +
>                 dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d\n",
>                         __func__, ret);
> 
>                 /*
> -                * If link recovery fails then return error so that caller
> -                * don't retry the hibern8 enter again.
> +                * If link recovery fails then return error code (-ENOLINK)
> +                * returned ufshcd_link_recovery().
> +                * If link recovery succeeds then return -EAGAIN to attempt
> +                * hibern8 enter retry again.
You no longer returning -ENOLINK, and either way retrying, regardless of the error code.
Better check that the commit log is still telling the correct story, taking into consideration all those recent fixes and all.

>                  */
> -               if (ufshcd_link_recovery(hba))
> -                       ret = -ENOLINK;
> +               err = ufshcd_link_recovery(hba);
> +               if (err) {
> +                       dev_err(hba->dev, "%s: link recovery failed", __func__);
> +                       ret = err;
> +               } else {
> +                       ret = -EAGAIN;
> +               }
>         } else
>                 ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER,
>                                                                 POST_CHANGE); @@ -3913,7 +3922,7 @@
> static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
> 
>         for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; retries--) {
>                 ret = __ufshcd_uic_hibern8_enter(hba);
> -               if (!ret || ret == -ENOLINK)
> +               if (!ret)
>                         goto out;
>         }
>  out:
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v4 7/7] scsi: ufs: Fix error handing during hibern8 enter

[Can Guo]

On 2019-11-14 19:06, Avri Altman wrote:
>> 
>> 
>> From: Subhash Jadavani <subhashj@codeaurora.org>
>> 
>> During clock gating (ufshcd_gate_work()), we first put the link 
>> hibern8 by
>> calling ufshcd_uic_hibern8_enter() and if ufshcd_uic_hibern8_enter() 
>> returns
>> success (0) then we gate all the clocks.
>> Now let’s zoom in to what ufshcd_uic_hibern8_enter() does internally:
>> It calls __ufshcd_uic_hibern8_enter() which on detecting the 
>> LINERESET,
>> initiates the full recovery and puts the link back to highest HS gear 
>> and returns
>> success (0) to ufshcd_uic_hibern8_enter() which is the issue as link 
>> is still in
>> active state due to recovery!
>> Now ufshcd_uic_hibern8_enter() returns success to ufshcd_gate_work() 
>> and
>> hence it goes ahead with gating the UFS clock while link is still in 
>> active state
>> hence I believe controller would raise UIC error interrupts. But when 
>> we service
>> the interrupt, clocks might have already been disabled!
>> 
>> This change fixes for this by returning failure from
>> __ufshcd_uic_hibern8_enter() if recovery succeeds as link is still not 
>> in hibern8,
>> upon receiving the error ufshcd_hibern8_enter() would initiate retry 
>> to put the
>> link state back into hibern8.
>> 
>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c 
>> index
>> 7a5a904..934c27a 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -3891,15 +3891,24 @@ static int __ufshcd_uic_hibern8_enter(struct
>> ufs_hba *hba)
>>                              ktime_to_us(ktime_sub(ktime_get(), 
>> start)), ret);
>> 
>>         if (ret) {
>> +               int err;
>> +
>>                 dev_err(hba->dev, "%s: hibern8 enter failed. ret = 
>> %d\n",
>>                         __func__, ret);
>> 
>>                 /*
>> -                * If link recovery fails then return error so that 
>> caller
>> -                * don't retry the hibern8 enter again.
>> +                * If link recovery fails then return error code 
>> (-ENOLINK)
>> +                * returned ufshcd_link_recovery().
>> +                * If link recovery succeeds then return -EAGAIN to 
>> attempt
>> +                * hibern8 enter retry again.
> You no longer returning -ENOLINK, and either way retrying, regardless
> of the error code.
> Better check that the commit log is still telling the correct story,
> taking into consideration all those recent fixes and all.
> 

Thanks for pointing this.

Best Regards,
Can Guo.

>>                  */
>> -               if (ufshcd_link_recovery(hba))
>> -                       ret = -ENOLINK;
>> +               err = ufshcd_link_recovery(hba);
>> +               if (err) {
>> +                       dev_err(hba->dev, "%s: link recovery failed", 
>> __func__);
>> +                       ret = err;
>> +               } else {
>> +                       ret = -EAGAIN;
>> +               }
>>         } else
>>                 ufshcd_vops_hibern8_notify(hba, 
>> UIC_CMD_DME_HIBER_ENTER,
>>                                                                 
>> POST_CHANGE); @@ -3913,7 +3922,7 @@
>> static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>> 
>>         for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; 
>> retries--) {
>>                 ret = __ufshcd_uic_hibern8_enter(hba);
>> -               if (!ret || ret == -ENOLINK)
>> +               if (!ret)
>>                         goto out;
>>         }
>>  out:
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project