[PATCH v1 0/3] scsi: ufs: fix error history and complete device reset history

[PATCH v1 0/3] scsi: ufs: fix error history and complete device reset history

[Stanley Chu]

Hi,

This series targets on UFS error history fixes and feature add-on,

1. Fix empty check logic while outputing error history.
2. Add device reset history events for vendor's implementations.
3. Remove dummy word in output format.

Stanley Chu (3):
  scsi: ufs: fix empty check of error history
  scsi: ufs: add device reset history for vendor implementations
  scsi: ufs: remove "errors" word in ufshcd_print_err_hist()

 drivers/scsi/ufs/ufshcd.c | 9 +++++----
 drivers/scsi/ufs/ufshcd.h | 6 +++++-
 2 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.18.0

[PATCH v1 1/3] scsi: ufs: fix empty check of error history

[Stanley Chu]

Currently checking if an error history element is empty or
not is by its "value". In most cases, value is error code.

However this checking is not correct because some errors or
events do not specify any values in error history so values
remain as 0, and this will lead to incorrect empty checking.

Fix it by checking "timestamp" instead of "value" because
timestamp will be always assigned for all history elements

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
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 1b97f2dc0b63..bae43da00bb6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -385,7 +385,7 @@ static void ufshcd_print_err_hist(struct ufs_hba *hba,
 	for (i = 0; i < UFS_ERR_REG_HIST_LENGTH; i++) {
 		int p = (i + err_hist->pos) % UFS_ERR_REG_HIST_LENGTH;
 
-		if (err_hist->reg[p] == 0)
+		if (err_hist->tstamp[p] == 0)
 			continue;
 		dev_err(hba->dev, "%s[%d] = 0x%x at %lld us\n", err_name, p,
 			err_hist->reg[p], ktime_to_us(err_hist->tstamp[p]));
-- 
2.18.0

Re: [PATCH v1 1/3] scsi: ufs: fix empty check of error history

[asutoshd]

On 2020-01-04 06:26, Stanley Chu wrote:
> Currently checking if an error history element is empty or
> not is by its "value". In most cases, value is error code.
> 
> However this checking is not correct because some errors or
> events do not specify any values in error history so values
> remain as 0, and this will lead to incorrect empty checking.
> 
> Fix it by checking "timestamp" instead of "value" because
> timestamp will be always assigned for all history elements
> 
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> 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 1b97f2dc0b63..bae43da00bb6 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -385,7 +385,7 @@ static void ufshcd_print_err_hist(struct ufs_hba 
> *hba,
>  	for (i = 0; i < UFS_ERR_REG_HIST_LENGTH; i++) {
>  		int p = (i + err_hist->pos) % UFS_ERR_REG_HIST_LENGTH;
> 
> -		if (err_hist->reg[p] == 0)
> +		if (err_hist->tstamp[p] == 0)
>  			continue;
>  		dev_err(hba->dev, "%s[%d] = 0x%x at %lld us\n", err_name, p,
>  			err_hist->reg[p], ktime_to_us(err_hist->tstamp[p]));

Looks good to me.

Reviewed by:- Asutosh Das <asutoshd@codeaurora.org>

RE: [EXT] [PATCH v1 1/3] scsi: ufs: fix empty check of error history

[Bean Huo (beanhuo)]

Hi, Stanley

> 
> Currently checking if an error history element is empty or not is by its "value". In
> most cases, value is error code.
> 
> However this checking is not correct because some errors or events do not
> specify any values in error history so values remain as 0, and this will lead to
> incorrect empty checking.
> 
Do you think this is a bug of UFS host controller? According to the UFS host Spec, 
If there had error detected in each layer, at least bit31 in its error code register
Should be set to 1.

Why there was an error happening, but host didn't set this bit31?

Thanks.
//Bean

RE: [EXT] [PATCH v1 1/3] scsi: ufs: fix empty check of error history

[Stanley Chu]

Hi Bean,

On Mon, 2020-01-13 at 09:28 +0000, Bean Huo (beanhuo) wrote:
> Hi, Stanley
> 
> > 
> > Currently checking if an error history element is empty or not is by its "value". In
> > most cases, value is error code.
> > 
> > However this checking is not correct because some errors or events do not
> > specify any values in error history so values remain as 0, and this will lead to
> > incorrect empty checking.
> > 
> Do you think this is a bug of UFS host controller? According to the UFS host Spec, 
> If there had error detected in each layer, at least bit31 in its error code register
> Should be set to 1.
> 
> Why there was an error happening, but host didn't set this bit31?
> 

Thanks so much for review.

Yes, the case bit[31] set is true for UIC errors.

However the users of UFS error history, i.e., users of
ufshcd_update_reg_hlist(), are not only UIC errors. Some other essential
events will update history too, for example, device reset events and
abort events.

Take "device reset events" as example: parameter "val" may be 0 while
invoking ufshcd_update_reg_hlist(). If this happens, the device reset
event will not be printed out because its err_hist->reg[p] is 0
according to the original logic in ufshcd_print_err_hist().

Feel free to correct above description if it is wrong.

Thanks,
Stanley

RE: [EXT] [PATCH v1 1/3] scsi: ufs: fix empty check of error history

[Bean Huo (beanhuo)]

> Subject: RE: [EXT] [PATCH v1 1/3] scsi: ufs: fix empty check of error history
> 
> Hi Bean,
> 
> On Mon, 2020-01-13 at 09:28 +0000, Bean Huo (beanhuo) wrote:
> > Hi, Stanley
> >
> > >
> > > Currently checking if an error history element is empty or not is by
> > > its "value". In most cases, value is error code.
> > >
> > > However this checking is not correct because some errors or events
> > > do not specify any values in error history so values remain as 0,
> > > and this will lead to incorrect empty checking.
> > >
> > Do you think this is a bug of UFS host controller? According to the
> > UFS host Spec, If there had error detected in each layer, at least
> > bit31 in its error code register Should be set to 1.
> >
> > Why there was an error happening, but host didn't set this bit31?
> >
> 
> Thanks so much for review.
> 
> Yes, the case bit[31] set is true for UIC errors.
> 
> However the users of UFS error history, i.e., users of ufshcd_update_reg_hlist(),
> are not only UIC errors. Some other essential events will update history too, for
> example, device reset events and abort events.
> 
> Take "device reset events" as example: parameter "val" may be 0 while invoking
> ufshcd_update_reg_hlist(). If this happens, the device reset event will not be
> printed out because its err_hist->reg[p] is 0 according to the original logic in
> ufshcd_print_err_hist().
> 
> Feel free to correct above description if it is wrong.
> 
> Thanks,
> Stanley

Hi, Stanley
Thanks, now clear, it is not controller negative act issue.

Reviewed-by: Bean Huo <beanhuo@micron.com>

[PATCH v1 2/3] scsi: ufs: add device reset history for vendor implementations

[Stanley Chu]

Device reset history shall be also added for vendor's device
reset variant operation implementation.

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 5 +++--
 drivers/scsi/ufs/ufshcd.h | 6 +++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index bae43da00bb6..29e3d50aabfb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4346,13 +4346,14 @@ static inline int ufshcd_disable_device_tx_lcc(struct ufs_hba *hba)
 	return ufshcd_disable_tx_lcc(hba, true);
 }
 
-static void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
-				   u32 reg)
+void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
+			    u32 reg)
 {
 	reg_hist->reg[reg_hist->pos] = reg;
 	reg_hist->tstamp[reg_hist->pos] = ktime_get();
 	reg_hist->pos = (reg_hist->pos + 1) % UFS_ERR_REG_HIST_LENGTH;
 }
+EXPORT_SYMBOL_GPL(ufshcd_update_reg_hist);
 
 /**
  * ufshcd_link_startup - Initialize unipro link startup
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e05cafddc87b..de1be6a862b0 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -805,6 +805,8 @@ int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
 				u32 val, unsigned long interval_us,
 				unsigned long timeout_ms, bool can_sleep);
 void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk *refclk);
+void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
+			    u32 reg);
 
 static inline void check_upiu_size(void)
 {
@@ -1083,8 +1085,10 @@ static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
 
 static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
 {
-	if (hba->vops && hba->vops->device_reset)
+	if (hba->vops && hba->vops->device_reset) {
 		hba->vops->device_reset(hba);
+		ufshcd_update_reg_hist(&hba->ufs_stats.dev_reset, 0);
+	}
 }
 
 extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
-- 
2.18.0

RE: [EXT] [PATCH v1 2/3] scsi: ufs: add device reset history for vendor implementations

[Bean Huo (beanhuo)]

As for me, no further question about this patch.

> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>

Re: [PATCH v1 2/3] scsi: ufs: add device reset history for vendor implementations

[asutoshd]

On 2020-01-04 06:26, Stanley Chu wrote:
> Device reset history shall be also added for vendor's device
> reset variant operation implementation.
> 
> Cc: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 5 +++--
>  drivers/scsi/ufs/ufshcd.h | 6 +++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index bae43da00bb6..29e3d50aabfb 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4346,13 +4346,14 @@ static inline int
> ufshcd_disable_device_tx_lcc(struct ufs_hba *hba)
>  	return ufshcd_disable_tx_lcc(hba, true);
>  }
> 
> -static void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
> -				   u32 reg)
> +void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
> +			    u32 reg)
>  {
>  	reg_hist->reg[reg_hist->pos] = reg;
>  	reg_hist->tstamp[reg_hist->pos] = ktime_get();
>  	reg_hist->pos = (reg_hist->pos + 1) % UFS_ERR_REG_HIST_LENGTH;
>  }
> +EXPORT_SYMBOL_GPL(ufshcd_update_reg_hist);
> 
>  /**
>   * ufshcd_link_startup - Initialize unipro link startup
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index e05cafddc87b..de1be6a862b0 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -805,6 +805,8 @@ int ufshcd_wait_for_register(struct ufs_hba *hba,
> u32 reg, u32 mask,
>  				u32 val, unsigned long interval_us,
>  				unsigned long timeout_ms, bool can_sleep);
>  void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk 
> *refclk);
> +void ufshcd_update_reg_hist(struct ufs_err_reg_hist *reg_hist,
> +			    u32 reg);
> 
>  static inline void check_upiu_size(void)
>  {
> @@ -1083,8 +1085,10 @@ static inline void
> ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
> 
>  static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
>  {
> -	if (hba->vops && hba->vops->device_reset)
> +	if (hba->vops && hba->vops->device_reset) {
>  		hba->vops->device_reset(hba);
> +		ufshcd_update_reg_hist(&hba->ufs_stats.dev_reset, 0);
> +	}
>  }
> 
>  extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

[PATCH v1 3/3] scsi: ufs: remove "errors" word in ufshcd_print_err_hist()

[Stanley Chu]

Remove "errors" word in output string by ufshcd_print_err_hist()
since not all printed targets are "errors". Sometimes they are
just "events".

In addition, all events which can be treated as "errors" already
have "err" or "fail" words in their names.

Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Asutosh Das <asutoshd@codeaurora.org>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Can Guo <cang@codeaurora.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
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 29e3d50aabfb..d847426507e7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -393,7 +393,7 @@ static void ufshcd_print_err_hist(struct ufs_hba *hba,
 	}
 
 	if (!found)
-		dev_err(hba->dev, "No record of %s errors\n", err_name);
+		dev_err(hba->dev, "No record of %s\n", err_name);
 }
 
 static void ufshcd_print_host_regs(struct ufs_hba *hba)
-- 
2.18.0

RE: [EXT] [PATCH v1 3/3] scsi: ufs: remove "errors" word in ufshcd_print_err_hist()

[Bean Huo (beanhuo)]

> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>

Re: [PATCH v1 0/3] scsi: ufs: fix error history and complete device reset history

[Stanley Chu]

Hi,

Gentle ping for this set.

On Sat, 2020-01-04 at 22:26 +0800, Stanley Chu wrote:
> Hi,
> 
> This series targets on UFS error history fixes and feature add-on,
> 
> 1. Fix empty check logic while outputing error history.
> 2. Add device reset history events for vendor's implementations.
> 3. Remove dummy word in output format.
> 
> Stanley Chu (3):
>   scsi: ufs: fix empty check of error history
>   scsi: ufs: add device reset history for vendor implementations
>   scsi: ufs: remove "errors" word in ufshcd_print_err_hist()
> 
>  drivers/scsi/ufs/ufshcd.c | 9 +++++----
>  drivers/scsi/ufs/ufshcd.h | 6 +++++-
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 

Re: [PATCH v1 0/3] scsi: ufs: fix error history and complete device reset history

[Martin K. Petersen]

Stanley,

> This series targets on UFS error history fixes and feature add-on,
>
> 1. Fix empty check logic while outputing error history.
> 2. Add device reset history events for vendor's implementations.
> 3. Remove dummy word in output format.

Applied to 5.6/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering