diff mbox series

[v2] scsi: ufs: exclude UECxx from SFR dump list

Message ID 1646727118-87159-1-git-send-email-kwmad.kim@samsung.com (mailing list archive)
State Superseded
Headers show
Series [v2] scsi: ufs: exclude UECxx from SFR dump list | expand

Commit Message

Kiwoong Kim March 8, 2022, 8:11 a.m. UTC
v1 -> v2: does skipping only for zero offset

These are ROC type things that means their values
are cleared when the SFRs are read.
They are usually read in ISR when an UIC error occur.
Thus, their values would be zero at many cases. And
there might be a little bit risky when they are read to
be cleared before the ISR reads them, e.g. the case that
a command is timed-out, ufshcd_dump_regs is called in
ufshcd_abort and an UIC error occurs at the nearly
same time. In this case, ISR will be called but UFS error handler
will not be scheduled.
This patch is to make UFS driver not read those SFRs in the
dump function, i.e. ufshcd_dump_regs.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Adrian Hunter March 8, 2022, 8:46 a.m. UTC | #1
On 8.3.2022 10.11, Kiwoong Kim wrote:
> v1 -> v2: does skipping only for zero offset
> 
> These are ROC type things that means their values
> are cleared when the SFRs are read.
> They are usually read in ISR when an UIC error occur.
> Thus, their values would be zero at many cases. And
> there might be a little bit risky when they are read to
> be cleared before the ISR reads them, e.g. the case that
> a command is timed-out, ufshcd_dump_regs is called in
> ufshcd_abort and an UIC error occurs at the nearly
> same time. In this case, ISR will be called but UFS error handler
> will not be scheduled.
> This patch is to make UFS driver not read those SFRs in the
> dump function, i.e. ufshcd_dump_regs.

This is essentially a fix, so perhaps a fixes tag?

Wouldn't hurt to wrap the commit description more nicely.

> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 460d2b4..7f2a1ed 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -115,8 +115,13 @@ int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
>  	if (!regs)
>  		return -ENOMEM;
>  
> -	for (pos = 0; pos < len; pos += 4)
> +	for (pos = 0; pos < len; pos += 4) {
> +		if (offset == 0 &&

So it will still read them if the offset is not zero.  That seems unexpectedly inconsistent.

> +		    pos >= REG_UIC_ERROR_CODE_PHY_ADAPTER_LAYER &&
> +		    pos <= REG_UIC_ERROR_CODE_DME)
> +			continue;
>  		regs[pos / 4] = ufshcd_readl(hba, offset + pos);
> +	}
>  
>  	ufshcd_hex_dump(prefix, regs, len);
>  	kfree(regs);
Kiwoong Kim March 10, 2022, 6:17 a.m. UTC | #2
> > These are ROC type things that means their values are cleared when the
> > SFRs are read.
> > They are usually read in ISR when an UIC error occur.
> > Thus, their values would be zero at many cases. And there might be a
> > little bit risky when they are read to be cleared before the ISR reads
> > them, e.g. the case that a command is timed-out, ufshcd_dump_regs is
> > called in ufshcd_abort and an UIC error occurs at the nearly same
> > time. In this case, ISR will be called but UFS error handler will not
> > be scheduled.
> > This patch is to make UFS driver not read those SFRs in the dump
> > function, i.e. ufshcd_dump_regs.
> 
> This is essentially a fix, so perhaps a fixes tag?
> 
> Wouldn't hurt to wrap the commit description more nicely.


Thank you for your opinion.


Thanks.
Kiwoong Kim
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 460d2b4..7f2a1ed 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -115,8 +115,13 @@  int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 	if (!regs)
 		return -ENOMEM;
 
-	for (pos = 0; pos < len; pos += 4)
+	for (pos = 0; pos < len; pos += 4) {
+		if (offset == 0 &&
+		    pos >= REG_UIC_ERROR_CODE_PHY_ADAPTER_LAYER &&
+		    pos <= REG_UIC_ERROR_CODE_DME)
+			continue;
 		regs[pos / 4] = ufshcd_readl(hba, offset + pos);
+	}
 
 	ufshcd_hex_dump(prefix, regs, len);
 	kfree(regs);