[v3] EDAC/fsl-ddr: Add imx9 support

[PATCH v3 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

Posted by Frank Li 1 month, 1 week ago

From: Priyanka Singh <priyanka.singh@nxp.com>

Fix undefined behavior caused by left-shifting a negative value in the
expression:

    cap_high ^ (1 << (bad_data_bit - 32))

The variable 'bad_data_bit' ranges from 0 to 63. When 'bad_data_bit' is
less than 32, 'bad_data_bit - 32' becomes negative, and left-shifting by a
negative value in C is undefined behavior.

Fix this by combining 'cap_high' and 'cap_low' into a 64-bit variable.

Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Li Yang <leoyang.li@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/edac/fsl_ddr_edac.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index 7a9fb1202f1a0..846a4ba25342a 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -328,6 +328,9 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 	 * TODO: Add support for 32-bit wide buses
 	 */
 	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
+		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
+		u32 s = syndrome;
+
 		sbe_ecc_decode(cap_high, cap_low, syndrome,
 				&bad_data_bit, &bad_ecc_bit);
 
@@ -338,11 +341,15 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 			fsl_mc_printk(mci, KERN_ERR,
 				"Faulty ECC bit: %d\n", bad_ecc_bit);
 
+		if (bad_data_bit >= 0)
+			cap ^= 1ULL << bad_data_bit;
+
+		if (bad_ecc_bit >= 0)
+			s ^= 1 << bad_ecc_bit;
+
 		fsl_mc_printk(mci, KERN_ERR,
 			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
-			cap_high ^ (1 << (bad_data_bit - 32)),
-			cap_low ^ (1 << bad_data_bit),
-			syndrome ^ (1 << bad_ecc_bit));
+			upper_32_bits(cap), lower_32_bits(cap), s);
 	}
 
 	fsl_mc_printk(mci, KERN_ERR,

-- 
2.34.1

Re: [PATCH v3 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

Posted by Borislav Petkov 1 month ago

On Wed, Oct 16, 2024 at 04:31:11PM -0400, Frank Li wrote:
> From: Priyanka Singh <priyanka.singh@nxp.com>
> 
> Fix undefined behavior caused by left-shifting a negative value in the
> expression:
> 
>     cap_high ^ (1 << (bad_data_bit - 32))
> 
> The variable 'bad_data_bit' ranges from 0 to 63. When 'bad_data_bit' is
> less than 32, 'bad_data_bit - 32' becomes negative, and left-shifting by a
> negative value in C is undefined behavior.
> 
> Fix this by combining 'cap_high' and 'cap_low' into a 64-bit variable.
> 
> Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
> Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
> Reviewed-by: Sherry Sun <sherry.sun@nxp.com>

You can't keep Reviewed-by tags when you change a patch considerably: Documentation/process/submitting-patches.rst

> Signed-off-by: Li Yang <leoyang.li@nxp.com>

What does that SOB tag mean?

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/edac/fsl_ddr_edac.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 7a9fb1202f1a0..846a4ba25342a 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -328,6 +328,9 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  	 * TODO: Add support for 32-bit wide buses
>  	 */
>  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> +		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> +		u32 s = syndrome;
> +
>  		sbe_ecc_decode(cap_high, cap_low, syndrome,
>  				&bad_data_bit, &bad_ecc_bit);
>  
> @@ -338,11 +341,15 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  			fsl_mc_printk(mci, KERN_ERR,
>  				"Faulty ECC bit: %d\n", bad_ecc_bit);
>  
> +		if (bad_data_bit >= 0)

>= 0 implies != -1, right?

IOW?

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index 846a4ba25342..fe822cb9b562 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -328,24 +328,21 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 	 * TODO: Add support for 32-bit wide buses
 	 */
 	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
-		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
+		u64 cap = (u64)cap_high << 32 | cap_low;
 		u32 s = syndrome;
 
 		sbe_ecc_decode(cap_high, cap_low, syndrome,
 				&bad_data_bit, &bad_ecc_bit);
 
-		if (bad_data_bit != -1)
-			fsl_mc_printk(mci, KERN_ERR,
-				"Faulty Data bit: %d\n", bad_data_bit);
-		if (bad_ecc_bit != -1)
-			fsl_mc_printk(mci, KERN_ERR,
-				"Faulty ECC bit: %d\n", bad_ecc_bit);
-
-		if (bad_data_bit >= 0)
+		if (bad_data_bit >= 0) {
+			fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
 			cap ^= 1ULL << bad_data_bit;
+		}
 
-		if (bad_ecc_bit >= 0)
+		if (bad_ecc_bit >= 0) {
+			fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
 			s ^= 1 << bad_ecc_bit;
+		}
 
 		fsl_mc_printk(mci, KERN_ERR,
 			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v3 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

Posted by Frank Li 1 month ago

On Tue, Oct 22, 2024 at 11:44:29AM +0200, Borislav Petkov wrote:
> On Wed, Oct 16, 2024 at 04:31:11PM -0400, Frank Li wrote:
> > From: Priyanka Singh <priyanka.singh@nxp.com>
> >
> > Fix undefined behavior caused by left-shifting a negative value in the
> > expression:
> >
> >     cap_high ^ (1 << (bad_data_bit - 32))
> >
> > The variable 'bad_data_bit' ranges from 0 to 63. When 'bad_data_bit' is
> > less than 32, 'bad_data_bit - 32' becomes negative, and left-shifting by a
> > negative value in C is undefined behavior.
> >
> > Fix this by combining 'cap_high' and 'cap_low' into a 64-bit variable.
> >
> > Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
> > Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
> > Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
>
> You can't keep Reviewed-by tags when you change a patch considerably: Documentation/process/submitting-patches.rst
>
> > Signed-off-by: Li Yang <leoyang.li@nxp.com>
>
> What does that SOB tag mean?
>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/edac/fsl_ddr_edac.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 7a9fb1202f1a0..846a4ba25342a 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -328,6 +328,9 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  	 * TODO: Add support for 32-bit wide buses
> >  	 */
> >  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> > +		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> > +		u32 s = syndrome;
> > +
> >  		sbe_ecc_decode(cap_high, cap_low, syndrome,
> >  				&bad_data_bit, &bad_ecc_bit);
> >
> > @@ -338,11 +341,15 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  			fsl_mc_printk(mci, KERN_ERR,
> >  				"Faulty ECC bit: %d\n", bad_ecc_bit);
> >
> > +		if (bad_data_bit >= 0)
>
> >= 0 implies != -1, right?
>
> IOW?

Yes, Your code is better.

>
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 846a4ba25342..fe822cb9b562 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -328,24 +328,21 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  	 * TODO: Add support for 32-bit wide buses
>  	 */
>  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> -		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> +		u64 cap = (u64)cap_high << 32 | cap_low;
>  		u32 s = syndrome;
>
>  		sbe_ecc_decode(cap_high, cap_low, syndrome,
>  				&bad_data_bit, &bad_ecc_bit);
>
> -		if (bad_data_bit != -1)
> -			fsl_mc_printk(mci, KERN_ERR,
> -				"Faulty Data bit: %d\n", bad_data_bit);
> -		if (bad_ecc_bit != -1)
> -			fsl_mc_printk(mci, KERN_ERR,
> -				"Faulty ECC bit: %d\n", bad_ecc_bit);
> -
> -		if (bad_data_bit >= 0)
> +		if (bad_data_bit >= 0) {
> +			fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
>  			cap ^= 1ULL << bad_data_bit;
> +		}
>
> -		if (bad_ecc_bit >= 0)
> +		if (bad_ecc_bit >= 0) {
> +			fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
>  			s ^= 1 << bad_ecc_bit;
> +		}
>
>  		fsl_mc_printk(mci, KERN_ERR,
>  			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

Posted by Frank Li 1 month ago

On Tue, Oct 22, 2024 at 11:44:29AM +0200, Borislav Petkov wrote:
> On Wed, Oct 16, 2024 at 04:31:11PM -0400, Frank Li wrote:
> > From: Priyanka Singh <priyanka.singh@nxp.com>
> >
> > Fix undefined behavior caused by left-shifting a negative value in the
> > expression:
> >
> >     cap_high ^ (1 << (bad_data_bit - 32))
> >
> > The variable 'bad_data_bit' ranges from 0 to 63. When 'bad_data_bit' is
> > less than 32, 'bad_data_bit - 32' becomes negative, and left-shifting by a
> > negative value in C is undefined behavior.
> >
> > Fix this by combining 'cap_high' and 'cap_low' into a 64-bit variable.
> >
> > Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
> > Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
> > Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
>
> You can't keep Reviewed-by tags when you change a patch considerably: Documentation/process/submitting-patches.rst

Sorry, I omitted it since it is nxp internal reviewer. Do I need repost
it?

>
> > Signed-off-by: Li Yang <leoyang.li@nxp.com>
>
> What does that SOB tag mean?

It is original nxp layerscape platform maintainer. He leave NXP recently
and some item in MAINTANERS already been removed. It intents to fix
layerscape platform problem at beginning. And orginal patch have his SOB.
So I kept as original one.

>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/edac/fsl_ddr_edac.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 7a9fb1202f1a0..846a4ba25342a 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -328,6 +328,9 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  	 * TODO: Add support for 32-bit wide buses
> >  	 */
> >  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> > +		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> > +		u32 s = syndrome;
> > +
> >  		sbe_ecc_decode(cap_high, cap_low, syndrome,
> >  				&bad_data_bit, &bad_ecc_bit);
> >
> > @@ -338,11 +341,15 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  			fsl_mc_printk(mci, KERN_ERR,
> >  				"Faulty ECC bit: %d\n", bad_ecc_bit);
> >
> > +		if (bad_data_bit >= 0)
>
> >= 0 implies != -1, right?
>
> IOW?
>
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 846a4ba25342..fe822cb9b562 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -328,24 +328,21 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  	 * TODO: Add support for 32-bit wide buses
>  	 */
>  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> -		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> +		u64 cap = (u64)cap_high << 32 | cap_low;
>  		u32 s = syndrome;
>
>  		sbe_ecc_decode(cap_high, cap_low, syndrome,
>  				&bad_data_bit, &bad_ecc_bit);
>
> -		if (bad_data_bit != -1)
> -			fsl_mc_printk(mci, KERN_ERR,
> -				"Faulty Data bit: %d\n", bad_data_bit);
> -		if (bad_ecc_bit != -1)
> -			fsl_mc_printk(mci, KERN_ERR,
> -				"Faulty ECC bit: %d\n", bad_ecc_bit);
> -
> -		if (bad_data_bit >= 0)
> +		if (bad_data_bit >= 0) {
> +			fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
>  			cap ^= 1ULL << bad_data_bit;
> +		}
>
> -		if (bad_ecc_bit >= 0)
> +		if (bad_ecc_bit >= 0) {
> +			fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
>  			s ^= 1 << bad_ecc_bit;
> +		}
>
>  		fsl_mc_printk(mci, KERN_ERR,
>  			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

Posted by Borislav Petkov 1 month ago

On Tue, Oct 22, 2024 at 11:29:03AM -0400, Frank Li wrote:
> > You can't keep Reviewed-by tags when you change a patch considerably: Documentation/process/submitting-patches.rst
> 
> Sorry, I omitted it since it is nxp internal reviewer. Do I need repost
> it?

No, I can zap it.

> > > Signed-off-by: Li Yang <leoyang.li@nxp.com>
> >
> > What does that SOB tag mean?
> 
> It is original nxp layerscape platform maintainer. He leave NXP recently
> and some item in MAINTANERS already been removed. It intents to fix
> layerscape platform problem at beginning. And orginal patch have his SOB.
> So I kept as original one.

Ok.

You forgot to comment on this part:

> > >= 0 implies != -1, right?
> >
> > IOW?
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 846a4ba25342..fe822cb9b562 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -328,24 +328,21 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  	 * TODO: Add support for 32-bit wide buses
> >  	 */
> >  	if ((err_detect & DDR_EDE_SBE) && (bus_width == 64)) {
> > -		u64 cap = (u64)cap_high << 32 | (u64)cap_low;
> > +		u64 cap = (u64)cap_high << 32 | cap_low;
> >  		u32 s = syndrome;
> >
> >  		sbe_ecc_decode(cap_high, cap_low, syndrome,
> >  				&bad_data_bit, &bad_ecc_bit);
> >
> > -		if (bad_data_bit != -1)
> > -			fsl_mc_printk(mci, KERN_ERR,
> > -				"Faulty Data bit: %d\n", bad_data_bit);
> > -		if (bad_ecc_bit != -1)
> > -			fsl_mc_printk(mci, KERN_ERR,
> > -				"Faulty ECC bit: %d\n", bad_ecc_bit);
> > -
> > -		if (bad_data_bit >= 0)
> > +		if (bad_data_bit >= 0) {
> > +			fsl_mc_printk(mci, KERN_ERR, "Faulty Data bit: %d\n", bad_data_bit);
> >  			cap ^= 1ULL << bad_data_bit;
> > +		}
> >
> > -		if (bad_ecc_bit >= 0)
> > +		if (bad_ecc_bit >= 0) {
> > +			fsl_mc_printk(mci, KERN_ERR, "Faulty ECC bit: %d\n", bad_ecc_bit);
> >  			s ^= 1 << bad_ecc_bit;
> > +		}
> >
> >  		fsl_mc_printk(mci, KERN_ERR,
> >  			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> >

-- 
Regards/Gruss,
    Boris.

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

[PATCH v3 1/6] EDAC/fsl_ddr: Pass down fsl_mc_pdata in ddr_in32() and ddr_out32()
[PATCH v3 2/6] EDAC/fsl_ddr: Move global variables into struct fsl_mc_pdata
[PATCH v3 3/6] EDAC/fsl_ddr: Fix bad bit shift operations
[PATCH v3 4/6] dt-bindings: memory: fsl: Add compatible string nxp,imx9-memory-controller
[PATCH v3 5/6] EDAC/fsl_ddr: Add support for i.MX9 DDR controller
[PATCH v3 6/6] arm64: dts: imx93: add ddr edac support