linux-edac.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] edac: synopsys: Fix the wrong value assignment for edac_mode
@ 2021-08-18  7:23 Shubhrajyoti Datta
  2021-08-18  7:23 ` [PATCH 2/2] edac: synopsys: Fix the issue in reporting of the error count Shubhrajyoti Datta
  2021-09-16  8:43 ` [PATCH 1/2] edac: synopsys: Fix the wrong value assignment for edac_mode Borislav Petkov
  0 siblings, 2 replies; 4+ messages in thread
From: Shubhrajyoti Datta @ 2021-08-18  7:23 UTC (permalink / raw)
  To: linux-edac
  Cc: bp, mchehab, michal.simek, git, Sai Krishna Potthuri, Shubhrajyoti Datta

From: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com>

This patch corrected the edac_mode value by using enum instead of bitmask.

Addresses-coverity: enumerated type mixed with another type.
Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 drivers/edac/synopsys_edac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 7e7146b22c16..7d08627e738b 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -782,7 +782,7 @@ static void init_csrows(struct mem_ctl_info *mci)
 
 		for (j = 0; j < csi->nr_channels; j++) {
 			dimm		= csi->channels[j]->dimm;
-			dimm->edac_mode	= EDAC_FLAG_SECDED;
+			dimm->edac_mode	= EDAC_SECDED;
 			dimm->mtype	= p_data->get_mtype(priv->baseaddr);
 			dimm->nr_pages	= (size >> PAGE_SHIFT) / csi->nr_channels;
 			dimm->grain	= SYNPS_EDAC_ERR_GRAIN;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] edac: synopsys: Fix the issue in reporting of the error count
  2021-08-18  7:23 [PATCH 1/2] edac: synopsys: Fix the wrong value assignment for edac_mode Shubhrajyoti Datta
@ 2021-08-18  7:23 ` Shubhrajyoti Datta
  2021-09-16  9:47   ` Borislav Petkov
  2021-09-16  8:43 ` [PATCH 1/2] edac: synopsys: Fix the wrong value assignment for edac_mode Borislav Petkov
  1 sibling, 1 reply; 4+ messages in thread
From: Shubhrajyoti Datta @ 2021-08-18  7:23 UTC (permalink / raw)
  To: linux-edac; +Cc: bp, mchehab, michal.simek, git, Shubhrajyoti Datta

Currently we are reading the error count from status register which
is not correct, this patch fixes the issue by reading the count from
error count register(ERRCNT). Currently we are not reporting the
errors cumulatively.
Also send the cumulative errors to the edac_mc_handle_error.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 drivers/edac/synopsys_edac.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 7d08627e738b..38c03bdc2393 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -163,6 +163,11 @@
 #define ECC_STAT_CECNT_SHIFT		8
 #define ECC_STAT_BITNUM_MASK		0x7F
 
+/* ECC error count register definitions */
+#define ECC_ERRCNT_UECNT_MASK		0xFFFF0000
+#define ECC_ERRCNT_UECNT_SHIFT		16
+#define ECC_ERRCNT_CECNT_MASK		0xFFFF
+
 /* DDR QOS Interrupt register definitions */
 #define DDR_QOS_IRQ_STAT_OFST		0x20200
 #define DDR_QOSUE_MASK			0x4
@@ -418,14 +423,16 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
 	base = priv->baseaddr;
 	p = &priv->stat;
 
+	regval = readl(base + ECC_ERRCNT_OFST);
+	p->ce_cnt = regval & ECC_ERRCNT_CECNT_MASK;
+	p->ue_cnt = (regval & ECC_ERRCNT_UECNT_MASK) >> ECC_ERRCNT_UECNT_SHIFT;
+	if (!p->ce_cnt)
+		goto ue_err;
+
 	regval = readl(base + ECC_STAT_OFST);
 	if (!regval)
 		return 1;
 
-	p->ce_cnt = (regval & ECC_STAT_CECNT_MASK) >> ECC_STAT_CECNT_SHIFT;
-	p->ue_cnt = (regval & ECC_STAT_UECNT_MASK) >> ECC_STAT_UECNT_SHIFT;
-	if (!p->ce_cnt)
-		goto ue_err;
 
 	p->ceinfo.bitpos = (regval & ECC_STAT_BITNUM_MASK);
 
@@ -491,7 +498,7 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
 		}
 
 		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
-				     p->ce_cnt, 0, 0, 0, 0, 0, -1,
+				     priv->ce_cnt, 0, 0, 0, 0, 0, -1,
 				     priv->message, "");
 	}
 
@@ -509,7 +516,7 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
 		}
 
 		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
-				     p->ue_cnt, 0, 0, 0, 0, 0, -1,
+				     priv->ue_cnt, 0, 0, 0, 0, 0, -1,
 				     priv->message, "");
 	}
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] edac: synopsys: Fix the wrong value assignment for edac_mode
  2021-08-18  7:23 [PATCH 1/2] edac: synopsys: Fix the wrong value assignment for edac_mode Shubhrajyoti Datta
  2021-08-18  7:23 ` [PATCH 2/2] edac: synopsys: Fix the issue in reporting of the error count Shubhrajyoti Datta
@ 2021-09-16  8:43 ` Borislav Petkov
  1 sibling, 0 replies; 4+ messages in thread
From: Borislav Petkov @ 2021-09-16  8:43 UTC (permalink / raw)
  To: Shubhrajyoti Datta
  Cc: linux-edac, mchehab, michal.simek, git, Sai Krishna Potthuri

On Wed, Aug 18, 2021 at 12:53:14PM +0530, Shubhrajyoti Datta wrote:
> From: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com>
> 
> This patch corrected the edac_mode value by using enum instead of bitmask.

I've fixed it up now but for the future, please avoid having "This
patch" or "This commit" in the commit message. It is tautologically
useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> Addresses-coverity: enumerated type mixed with another type.
> Signed-off-by: Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@xilinx.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---
>  drivers/edac/synopsys_edac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
> index 7e7146b22c16..7d08627e738b 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -782,7 +782,7 @@ static void init_csrows(struct mem_ctl_info *mci)
>  
>  		for (j = 0; j < csi->nr_channels; j++) {
>  			dimm		= csi->channels[j]->dimm;
> -			dimm->edac_mode	= EDAC_FLAG_SECDED;
> +			dimm->edac_mode	= EDAC_SECDED;
>  			dimm->mtype	= p_data->get_mtype(priv->baseaddr);
>  			dimm->nr_pages	= (size >> PAGE_SHIFT) / csi->nr_channels;
>  			dimm->grain	= SYNPS_EDAC_ERR_GRAIN;
> -- 

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] edac: synopsys: Fix the issue in reporting of the error count
  2021-08-18  7:23 ` [PATCH 2/2] edac: synopsys: Fix the issue in reporting of the error count Shubhrajyoti Datta
@ 2021-09-16  9:47   ` Borislav Petkov
  0 siblings, 0 replies; 4+ messages in thread
From: Borislav Petkov @ 2021-09-16  9:47 UTC (permalink / raw)
  To: Shubhrajyoti Datta; +Cc: linux-edac, mchehab, michal.simek, git

On Wed, Aug 18, 2021 at 12:53:15PM +0530, Shubhrajyoti Datta wrote:
> Currently we are reading the error count from status register which

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.

Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.

> is not correct, this patch fixes the issue by reading the count from

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> error count register(ERRCNT). Currently we are not reporting the
> errors cumulatively.
> Also send the cumulative errors to the edac_mc_handle_error.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---
>  drivers/edac/synopsys_edac.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
> index 7d08627e738b..38c03bdc2393 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -163,6 +163,11 @@
>  #define ECC_STAT_CECNT_SHIFT		8
>  #define ECC_STAT_BITNUM_MASK		0x7F
>  
> +/* ECC error count register definitions */
> +#define ECC_ERRCNT_UECNT_MASK		0xFFFF0000
> +#define ECC_ERRCNT_UECNT_SHIFT		16
> +#define ECC_ERRCNT_CECNT_MASK		0xFFFF
> +
>  /* DDR QOS Interrupt register definitions */
>  #define DDR_QOS_IRQ_STAT_OFST		0x20200
>  #define DDR_QOSUE_MASK			0x4
> @@ -418,14 +423,16 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
>  	base = priv->baseaddr;
>  	p = &priv->stat;
>  
> +	regval = readl(base + ECC_ERRCNT_OFST);
> +	p->ce_cnt = regval & ECC_ERRCNT_CECNT_MASK;
> +	p->ue_cnt = (regval & ECC_ERRCNT_UECNT_MASK) >> ECC_ERRCNT_UECNT_SHIFT;
> +	if (!p->ce_cnt)
> +		goto ue_err;
> +
>  	regval = readl(base + ECC_STAT_OFST);
>  	if (!regval)
>  		return 1;
>  
> -	p->ce_cnt = (regval & ECC_STAT_CECNT_MASK) >> ECC_STAT_CECNT_SHIFT;
> -	p->ue_cnt = (regval & ECC_STAT_UECNT_MASK) >> ECC_STAT_UECNT_SHIFT;
> -	if (!p->ce_cnt)
> -		goto ue_err;
>  
>  	p->ceinfo.bitpos = (regval & ECC_STAT_BITNUM_MASK);
>  

That change looks correct.

> @@ -491,7 +498,7 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
>  		}
>  
>  		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
> -				     p->ce_cnt, 0, 0, 0, 0, 0, -1,
> +				     priv->ce_cnt, 0, 0, 0, 0, 0, -1,
>  				     priv->message, "");
>  	}
>  
> @@ -509,7 +516,7 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
>  		}
>  
>  		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
> -				     p->ue_cnt, 0, 0, 0, 0, 0, -1,
> +				     priv->ue_cnt, 0, 0, 0, 0, 0, -1,
>  				     priv->message, "");
>  	}
>  

That one doesn't. AFIACT, handle_error() is supposed to deal with the
current errors logged which ->get_error_info() has done by putting the
counts into priv->stat which gets passed in.

The cumilative errors are dumped a little bit further down - grep for
"Total error count". Those are debug statements, though.

Also, edac_mc_handle_error() gets the *current* error counts which got
logged by the current ECC interrupt - not the cumulative!

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-09-16  9:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18  7:23 [PATCH 1/2] edac: synopsys: Fix the wrong value assignment for edac_mode Shubhrajyoti Datta
2021-08-18  7:23 ` [PATCH 2/2] edac: synopsys: Fix the issue in reporting of the error count Shubhrajyoti Datta
2021-09-16  9:47   ` Borislav Petkov
2021-09-16  8:43 ` [PATCH 1/2] edac: synopsys: Fix the wrong value assignment for edac_mode Borislav Petkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).