[PATCH v1 0/3] scsi: ufs: add error handlings of auto-hibern8

[PATCH v1 0/3] scsi: ufs: add error handlings of auto-hibern8

[Stanley Chu]

Currently auto-hibern8 is activated if host supports
auto-hibern8 capability. However no error handlings are existed thus
this feature is kind of risky.

If "Hibernate Enter" or "Hibernate Exit" fail happens
during auto-hibern8 flow, the corresponding interrupt
"UIC_HIBERNATE_ENTER" or "UIC_HIBERNATE_EXIT" shall be raised
according to UFS specification.

This patch adds auto-hibern8 error handlings:

- Monitor "Hibernate Enter" and "Hibernate Exit" interrupts after
  auto-hibern8 feature is activated.
- If fail happens, trigger error handlings just like "manual-hibernate"
  fail and use the same flow: Identify errors and schedule UFS error
  handler in ufshcd_check_errors(), and then do host reset and restore
  in UFS error handler.

Stanley Chu (3):
  scsi: ufs: do not overwrite auto-hibern8 timer
  scsi: ufs: add error handling of auto-hibern8
  scsi: ufs: use re-factored auto_hibern8 function

 drivers/scsi/ufs/ufshcd.c | 16 +++++++++++++++-
 drivers/scsi/ufs/ufshcd.h | 13 +++++++++++++
 drivers/scsi/ufs/ufshci.h |  3 +++
 3 files changed, 31 insertions(+), 1 deletion(-)

-- 
2.18.0

[PATCH v1 1/3] scsi: ufs: do not overwrite auto-hibern8 timer

[Stanley Chu]

Some vendor-specific initialization flow may set its own
auto-hibern8 timer. In this case, do not overwrite timer value
as "default value" in ufshcd_init().

Signed-off-by: Stanley Chu <stanley.chu@mediatek.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 e040f9dd9ff3..1665820c22fd 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8309,7 +8309,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 						UIC_LINK_HIBERN8_STATE);
 
 	/* Set the default auto-hiberate idle timer value to 150 ms */
-	if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT) {
+	if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT & !hba->ahit) {
 		hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 150) |
 			    FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3);
 	}
-- 
2.18.0

[PATCH v1 2/3] scsi: ufs: add error handling of auto-hibern8

[Stanley Chu]

Currently auto-hibern8 is activated if host supports
auto-hibern8 capability. However no error handlings are existed thus
this feature is kind of risky.

If "Hibernate Enter" or "Hibernate Exit" fail happens
during auto-hibern8 flow, the corresponding interrupt
"UIC_HIBERNATE_ENTER" or "UIC_HIBERNATE_EXIT" shall be raised
according to UFS specification.

This patch adds auto-hibern8 error handlings:

- Monitor "Hibernate Enter" and "Hibernate Exit" interrupts after
  auto-hibern8 feature is activated.
- If fail happens, trigger error handlings just like "manual-hibernate"
  fail and use the same flow: Identify errors and schedule UFS error
  handler in ufshcd_check_errors(), and then do host reset and restore
  in UFS error handler.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 14 ++++++++++++++
 drivers/scsi/ufs/ufshcd.h | 13 +++++++++++++
 drivers/scsi/ufs/ufshci.h |  3 +++
 3 files changed, 30 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1665820c22fd..e0e3930abc19 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5254,6 +5254,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 			goto skip_err_handling;
 	}
 	if ((hba->saved_err & INT_FATAL_ERRORS) ||
+	    ufshcd_is_auto_hibern8_error(hba, hba->saved_err) ||
 	    ((hba->saved_err & UIC_ERROR) &&
 	    (hba->saved_uic_err & (UFSHCD_UIC_DL_PA_INIT_ERROR |
 				   UFSHCD_UIC_DL_NAC_RECEIVED_ERROR |
@@ -5431,6 +5432,15 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 			queue_eh_work = true;
 	}
 
+	if (ufshcd_is_auto_hibern8_error(hba, hba->errors)) {
+		dev_err(hba->dev,
+			"%s: Auto Hibern8 %s failed - status: 0x%08x, upmcrs: 0x%08x\n",
+			__func__, (hba->errors & UIC_HIBERNATE_ENTER) ?
+			"Enter" : "Exit",
+			hba->errors, ufshcd_get_upmcrs(hba));
+		queue_eh_work = true;
+	}
+
 	if (queue_eh_work) {
 		/*
 		 * update the transfer error masks to sticky bits, let's do this
@@ -5493,6 +5503,10 @@ static void ufshcd_tmc_handler(struct ufs_hba *hba)
 static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 {
 	hba->errors = UFSHCD_ERROR_MASK & intr_status;
+
+	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
+		hba->errors |= (UFSHCD_UIC_AH8_ERROR_MASK & intr_status);
+
 	if (hba->errors)
 		ufshcd_check_errors(hba);
 
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index ecfa898b9ccc..1bd9c8b61ed2 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -740,6 +740,19 @@ return true;
 #endif
 }
 
+static inline bool ufshcd_is_auto_hibern8_supported(struct ufs_hba *hba)
+{
+	return (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT);
+}
+
+static inline bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
+						u32 intr_mask)
+{
+	return (ufshcd_is_auto_hibern8_supported(hba) &&
+		!hba->uic_async_done &&
+		(intr_mask & UFSHCD_UIC_AH8_ERROR_MASK));
+}
+
 #define ufshcd_writel(hba, val, reg)	\
 	writel((val), (hba)->mmio_base + (reg))
 #define ufshcd_readl(hba, reg)	\
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index 6fa889de5ee5..4bcb205f2077 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -148,6 +148,9 @@ enum {
 				UIC_HIBERNATE_EXIT |\
 				UIC_POWER_MODE)
 
+#define UFSHCD_UIC_AH8_ERROR_MASK	(UIC_HIBERNATE_ENTER |\
+					UIC_HIBERNATE_EXIT)
+
 #define UFSHCD_UIC_MASK		(UIC_COMMAND_COMPL | UFSHCD_UIC_PWR_MASK)
 
 #define UFSHCD_ERROR_MASK	(UIC_ERROR |\
-- 
2.18.0

RE: [EXT] [PATCH v1 2/3] scsi: ufs: add error handling of auto-hibern8

[Bean Huo (beanhuo)]

Hi, Stanley

>+
>+static inline bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
>+						u32 intr_mask)
>+{
>+	return (ufshcd_is_auto_hibern8_supported(hba) &&
>+		!hba->uic_async_done &&

Here check if uic_async_done is NULL, no big problem so far, but not safe enough.
How about setting a flag in ufshcd_auto_hibern8_enable(),

I concern about how to compatible with auto_hibern8 disabled condition.


//Bean

RE: [EXT] [PATCH v1 2/3] scsi: ufs: add error handling of auto-hibern8

[Stanley Chu]

Hi Bean,

Thanks so much for review.

On Mon, 2019-05-13 at 18:21 +0000, Bean Huo (beanhuo) wrote:
> Hi, Stanley
> 
> >+
> >+static inline bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
> >+						u32 intr_mask)
> >+{
> >+	return (ufshcd_is_auto_hibern8_supported(hba) &&
> >+		!hba->uic_async_done &&
> 
> Here check if uic_async_done is NULL, no big problem so far, but not safe enough.
> How about setting a flag in ufshcd_auto_hibern8_enable(),

> 
> I concern about how to compatible with auto_hibern8 disabled condition.

Currently auto-hibern8 disabling method is not implemented in
mainstream, so an "enabling" flag may looks redundant unless disabling
path is really existed.

I agree that checking hba->uic_async_done here does not look so
intuitive. However even if auto-hibern8 is disabled, these checks could
be safe enough because both "UIC_HIBERNATE_ENTER" and
"UIC_HIBERNATE_EXIT" are raised only if "manual-hibernate" is performed,
and in this case hba->uic_async_done shall be true.

Anything else or corner case I missed?

> 
> 
> //Bean

Thanks,
Stanley

> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

RE: [EXT] [PATCH v1 2/3] scsi: ufs: add error handling of auto-hibern8

[Bean Huo (beanhuo)]

Hi, Stanley
Thanks for reply.

>
>On Mon, 2019-05-13 at 18:21 +0000, Bean Huo (beanhuo) wrote:
>> Hi, Stanley
>>
>> >+
>> >+static inline bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
>> >+						u32 intr_mask)
>> >+{
>> >+	return (ufshcd_is_auto_hibern8_supported(hba) &&
>> >+		!hba->uic_async_done &&
>>
>> Here check if uic_async_done is NULL, no big problem so far, but not safe
>enough.
>> How about setting a flag in ufshcd_auto_hibern8_enable(),
>
>>
>> I concern about how to compatible with auto_hibern8 disabled condition.
>
>Currently auto-hibern8 disabling method is not implemented in mainstream,
>so an "enabling" flag may looks redundant unless disabling path is really
>existed.
>
Did you try to update Auto-Hibernate Idle Timer with 0 through '/sys'  (scsi: ufs: Add support for Auto-Hibernate Idle Timer)? 
I don't know if this will disable your UFS controller Auto-Hibernate.
If having a look at UFS host Spec, software writes “0” to disable Auto-Hibernate Idle Timer.
Sorry I cannot verify this on my platform since it doesn't support auto-hibernate.


>I agree that checking hba->uic_async_done here does not look so intuitive.
>However even if auto-hibern8 is disabled, these checks could be safe enough
>because both "UIC_HIBERNATE_ENTER" and "UIC_HIBERNATE_EXIT" are
>raised only if "manual-hibernate" is performed, and in this case hba-
>>uic_async_done shall be true.
>
Yes, most of cases ,this is no problem.

>Anything else or corner case I missed?
>
The others are fine. I only concern checking hba->uic_async_done.

//Bean
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [EXT] [PATCH v1 2/3] scsi: ufs: add error handling of auto-hibern8

[Stanley Chu]

Hi Bean,

On Tue, 2019-05-14 at 11:14 +0000, Bean Huo (beanhuo) wrote:
> Hi, Stanley
> Thanks for reply.
> 
> >
> >On Mon, 2019-05-13 at 18:21 +0000, Bean Huo (beanhuo) wrote:
> >> Hi, Stanley
> >>
> >> >+
> >> >+static inline bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
> >> >+						u32 intr_mask)
> >> >+{
> >> >+	return (ufshcd_is_auto_hibern8_supported(hba) &&
> >> >+		!hba->uic_async_done &&
> >>
> >> Here check if uic_async_done is NULL, no big problem so far, but not safe
> >enough.
> >> How about setting a flag in ufshcd_auto_hibern8_enable(),
> >
> >>
> >> I concern about how to compatible with auto_hibern8 disabled condition.
> >
> >Currently auto-hibern8 disabling method is not implemented in mainstream,
> >so an "enabling" flag may looks redundant unless disabling path is really
> >existed.
> >
> Did you try to update Auto-Hibernate Idle Timer with 0 through '/sys'  (scsi: ufs: Add support for Auto-Hibernate Idle Timer)? 
> I don't know if this will disable your UFS controller Auto-Hibernate.
> If having a look at UFS host Spec, software writes “0” to disable Auto-Hibernate Idle Timer.
> Sorry I cannot verify this on my platform since it doesn't support auto-hibernate.
> 

Sorry I missed this /sys interface for Auto-Hibernate control.

Yes, I have tested "Auto-Hibernate disabled" case, in this case,
UIC_HIBERNATE_ENTER and UIC_HIBERNATE_EXIT interrupts comes only if
Manual-Hibernate is performed and waiting for completion. Both
interrupts will not be identified as Auto-Hibernate errors by checking
hba->uic_async_done.

As for your concerning, I would like to make "Auto-Hibernate error
detection" more precise in next version: Use below conditions instead of
checking hba->uic_async_done:

As-is:

static inline bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
						u32 intr_mask)
{
	return (ufshcd_is_auto_hibern8_supported(hba) &&
		!hba->uic_async_done &&
		(intr_mask & UFSHCD_UIC_AH8_ERROR_MASK));
}

To-be:

static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
						u32 intr_mask)
{
	if (!ufshcd_is_auto_hibern8_supported(hba))
		return false;

	if (!(intr_mask & UFSHCD_UIC_AH8_ERROR_MASK))
		return false;

	if (hba->active_uic_cmd &&
	    ((hba->active_uic_cmd->command == UIC_CMD_DME_HIBER_ENTER) ||
	    (hba->active_uic_cmd->command == UIC_CMD_DME_HIBER_EXIT)))
		return false;

	return true;
}

What would you think about this change?

> 
> >I agree that checking hba->uic_async_done here does not look so intuitive.
> >However even if auto-hibern8 is disabled, these checks could be safe enough
> >because both "UIC_HIBERNATE_ENTER" and "UIC_HIBERNATE_EXIT" are
> >raised only if "manual-hibernate" is performed, and in this case hba-
> >>uic_async_done shall be true.
> >
> Yes, most of cases ,this is no problem.
> 
> >Anything else or corner case I missed?
> >
> The others are fine. I only concern checking hba->uic_async_done.
> 
> //Bean

Many thanks,
Stanley




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/3] scsi: ufs: use re-factored auto_hibern8 function

[Stanley Chu]

Use re-factored ufshcd_is_auto_hibern8_supported() function
in ufshcd_init() instead to make code more cleaner.

Signed-off-by: Stanley Chu <stanley.chu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.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 e0e3930abc19..17df5913389d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8323,7 +8323,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 						UIC_LINK_HIBERN8_STATE);
 
 	/* Set the default auto-hiberate idle timer value to 150 ms */
-	if (hba->capabilities & MASK_AUTO_HIBERN8_SUPPORT & !hba->ahit) {
+	if (ufshcd_is_auto_hibern8_supported(hba) & !hba->ahit) {
 		hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 150) |
 			    FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3);
 	}
-- 
2.18.0

Re: [PATCH v1 0/3] scsi: ufs: add error handlings of auto-hibern8

[Marc Gonzalez]

On 13/05/2019 16:36, Stanley Chu wrote:

> Currently auto-hibern8 is activated if host supports
> auto-hibern8 capability. However no error handlings are existed thus
> this feature is kind of risky.

This last sentence is not very idiomatic.

I would suggest:
"However, error-handling is not implemented, which makes the feature
somewhat risky."

> If "Hibernate Enter" or "Hibernate Exit" fail happens

I would suggest:
If either "Hibernate Enter" or "Hibernate Exit" fail during ...

> during auto-hibern8 flow, the corresponding interrupt
> "UIC_HIBERNATE_ENTER" or "UIC_HIBERNATE_EXIT" shall be raised
> according to UFS specification.
> 
> This patch adds auto-hibern8 error handlings:

error-handling

> - Monitor "Hibernate Enter" and "Hibernate Exit" interrupts after
>   auto-hibern8 feature is activated.

I just want to take this opportunity to ask a rhetorical question.

Who in the Great Heavens thought it would be a good idea to call the
feature "auto-hibern8" ?

Was it really worth it to save 2 characters by writing "8" instead
of "ate" ?

This bugs me so much that I just might send a patch to fix it up.

Regards.

Re: [PATCH v1 0/3] scsi: ufs: add error handlings of auto-hibern8

[Stanley Chu]

Hi Marc,

Thank you so much for below suggestions. I will fix them all in next
version.

On Mon, 2019-05-13 at 16:51 +0200, Marc Gonzalez wrote:
> On 13/05/2019 16:36, Stanley Chu wrote:
> 
> > Currently auto-hibern8 is activated if host supports
> > auto-hibern8 capability. However no error handlings are existed thus
> > this feature is kind of risky.
> 
> This last sentence is not very idiomatic.
> 
> I would suggest:
> "However, error-handling is not implemented, which makes the feature
> somewhat risky."
> 
> > If "Hibernate Enter" or "Hibernate Exit" fail happens
> 
> I would suggest:
> If either "Hibernate Enter" or "Hibernate Exit" fail during ...
> 
> > during auto-hibern8 flow, the corresponding interrupt
> > "UIC_HIBERNATE_ENTER" or "UIC_HIBERNATE_EXIT" shall be raised
> > according to UFS specification.
> > 
> > This patch adds auto-hibern8 error handlings:
> 
> error-handling
> 
> > - Monitor "Hibernate Enter" and "Hibernate Exit" interrupts after
> >   auto-hibern8 feature is activated.
> 
> I just want to take this opportunity to ask a rhetorical question.
> 
> Who in the Great Heavens thought it would be a good idea to call the
> feature "auto-hibern8" ?
> 
> Was it really worth it to save 2 characters by writing "8" instead
> of "ate" ?
> 
> This bugs me so much that I just might send a patch to fix it up.

As far as I know, the term "HIBERN8" is mentioned in all UFS related
specifications, like UFS, UFSHCI, UniPro and M-PHY. So probably this
abbreviation has existed for a long time.

> 
> Regards.

Thanks,

Stanley

RE: [PATCH v1 0/3] scsi: ufs: add error handlings of auto-hibern8

[Avri Altman]

> 
> On 13/05/2019 16:36, Stanley Chu wrote:
> 
> > Currently auto-hibern8 is activated if host supports
> > auto-hibern8 capability. However no error handlings are existed thus
> > this feature is kind of risky.
> 
> This last sentence is not very idiomatic.
> 
> I would suggest:
> "However, error-handling is not implemented, which makes the feature
> somewhat risky."
> 
> > If "Hibernate Enter" or "Hibernate Exit" fail happens
> 
> I would suggest:
> If either "Hibernate Enter" or "Hibernate Exit" fail during ...
> 
> > during auto-hibern8 flow, the corresponding interrupt
> > "UIC_HIBERNATE_ENTER" or "UIC_HIBERNATE_EXIT" shall be raised
> > according to UFS specification.
> >
> > This patch adds auto-hibern8 error handlings:
> 
> error-handling
> 
> > - Monitor "Hibernate Enter" and "Hibernate Exit" interrupts after
> >   auto-hibern8 feature is activated.
> 
> I just want to take this opportunity to ask a rhetorical question.
> 
> Who in the Great Heavens thought it would be a good idea to call the
> feature "auto-hibern8" ?
> 
> Was it really worth it to save 2 characters by writing "8" instead
> of "ate" ?
> 
> This bugs me so much that I just might send a patch to fix it up.
As strange as it may be, this is not the product of the creative mind
Of the original driver's authors, nor even JEDEC guys which uses it in
their specs (both UFS & HCI).
This strange amalgam dates back to the mipi-unipro terminology.

Thanks,
Avri