[RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Kiwoong Kim]

v2 -> v3: add fixes
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.

Fixes: d67247566450 ("scsi: ufs: Use explicit access size in 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(-)

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);
-- 
2.7.4

Re: [RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Bart Van Assche]

On 3/15/22 02:56, Kiwoong Kim wrote:
> These are ROC type things that means their values
> are cleared when the SFRs are read.

Is this behavior specific to the Exynos controller or is this behavior 
required by the UFSHCI specification? In the latter case, can you tell 
me where to find this requirement in the UFSHCI specification? I haven't 
found that requirement yet. Maybe this means that I overlooked something?

Thanks,

Bart.

RE: [RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Kiwoong Kim]

> Is this behavior specific to the Exynos controller or is this behavior
> required by the UFSHCI specification? In the latter case, can you tell me
> where to find this requirement in the UFSHCI specification? I haven't
> found that requirement yet. Maybe this means that I overlooked something?
> 
> Thanks,
> 
> Bart. 

This is needed because those SFRs are ROC (Read to Clear) type.
That means reading causes clearing contexts.
The SFRs are mainly read in interrupt context but the reading is also done in dump.

Besides, I think reading them in dump is not proper because reading them is not just 'reading'

Thanks.
Kiwoong Kim

RE: [RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Kiwoong Kim]

> Is this behavior specific to the Exynos controller or is this behavior
> required by the UFSHCI specification? In the latter case, can you tell me
> where to find this requirement in the UFSHCI specification? I haven't
> found that requirement yet. Maybe this means that I overlooked something?
> 
> Thanks,
> 
> Bart.

This is needed because those SFRs are ROC (Read to Clear) type.
That means reading causes clearing contexts.
The SFRs are mainly read in interrupt context but the reading is also done in dump.

Besides, I think reading them in dump is not proper because reading them is not just 'reading'

Thanks.
Kiwoong Kim

Re: [RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Bart Van Assche]

On 7/10/22 19:02, Kiwoong Kim wrote:
>> Is this behavior specific to the Exynos controller or is this behavior
>> required by the UFSHCI specification? In the latter case, can you tell me
>> where to find this requirement in the UFSHCI specification? I haven't
>> found that requirement yet. Maybe this means that I overlooked something?
>
> This is needed because those SFRs are ROC (Read to Clear) type.
> That means reading causes clearing contexts.
> The SFRs are mainly read in interrupt context but the reading is also done in dump.
The above repeats what I can find in the patch description but doesn't 
answer my question :-(

Bart.

RE: [RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Kiwoong Kim]

> >> Is this behavior specific to the Exynos controller or is this
> >> behavior required by the UFSHCI specification? In the latter case,
> >> can you tell me where to find this requirement in the UFSHCI
> >> specification? I haven't found that requirement yet. Maybe this means
> that I overlooked something?
> >
> > This is needed because those SFRs are ROC (Read to Clear) type.
> > That means reading causes clearing contexts.
> > The SFRs are mainly read in interrupt context but the reading is also
> done in dump.
> The above repeats what I can find in the patch description but doesn't
> answer my question :-(
> 
> Bart.

It's definitely not Exynos specific
because the fact that those SFRs are ROC type is written in UFSHCI.
I don't know more things to explain why this patch is needed.

You just want to know whether any additional descriptions about ROC is written ?

Thanks.
Kiwoong Kim

Re: [RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Bart Van Assche]

On 7/10/22 21:01, Kiwoong Kim wrote:
> It's definitely not Exynos specific
> because the fact that those SFRs are ROC type is written in UFSHCI.
> I don't know more things to explain why this patch is needed.
> 
> You just want to know whether any additional descriptions about ROC is written ?

Hi Kiwoong,

I had overlooked the "ROC" text in the "Type" column in the UFSHCI 
specification. Thank you for having taken the time to answer my questions.

Bart.

[RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Kiwoong Kim]

v2 -> v3: add fixes
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.

Fixes: d67247566450 ("scsi: ufs: Use explicit access size in 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(-)

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);
-- 
2.7.4

Re: [RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Martin K. Petersen]

Kiwoong,

> This patch is to make UFS driver not read those SFRs in the dump
> function, i.e. ufshcd_dump_regs.

Applied to 5.19/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [RESEND PATCH v3] scsi: ufs: exclude UECxx from SFR dump list

[Martin K. Petersen]

On Thu, 31 Mar 2022 10:24:05 +0900, Kiwoong Kim wrote:

> v2 -> v3: add fixes
> 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.
> 
> [...]

Applied to 5.19/scsi-queue, thanks!

[1/1] scsi: ufs: exclude UECxx from SFR dump list
      https://git.kernel.org/mkp/scsi/c/ef60031022eb

-- 
Martin K. Petersen	Oracle Linux Engineering