[PATCH 1/1] EDAC/skx: Fix overflows on the DRAM row address mapping arrays

[PATCH 1/1] EDAC/skx: Fix overflows on the DRAM row address mapping arrays

[Qiuxu Zhuo]

The current DRAM row address mapping arrays skx_{open,close}_row[]
only support ranks with sizes up to 16G. Decoding a rank address
to a DRAM row address for a 32G rank by using either one of the
above arrays by the skx_edac driver, will result in an overflow on
the array.

For a 32G rank, the most significant DRAM row address bit (the
bit17) is mapped from the bit34 of the rank address. Add this new
mapping item to both arrays to fix the overflow issue.

Fixes: 98f2fc829e3b ("EDAC, skx_edac: Delete duplicated code")
Reported-by: Feng Xu <feng.f.xu@intel.com>
Tested-by: Feng Xu <feng.f.xu@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
 drivers/edac/skx_base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/skx_base.c b/drivers/edac/skx_base.c
index 9397abb42c49..0a862336a7ce 100644
--- a/drivers/edac/skx_base.c
+++ b/drivers/edac/skx_base.c
@@ -510,7 +510,7 @@ static bool skx_rir_decode(struct decoded_addr *res)
 }
 
 static u8 skx_close_row[] = {
-	15, 16, 17, 18, 20, 21, 22, 28, 10, 11, 12, 13, 29, 30, 31, 32, 33
+	15, 16, 17, 18, 20, 21, 22, 28, 10, 11, 12, 13, 29, 30, 31, 32, 33, 34
 };
 
 static u8 skx_close_column[] = {
@@ -518,7 +518,7 @@ static u8 skx_close_column[] = {
 };
 
 static u8 skx_open_row[] = {
-	14, 15, 16, 20, 28, 21, 22, 23, 24, 25, 26, 27, 29, 30, 31, 32, 33
+	14, 15, 16, 20, 28, 21, 22, 23, 24, 25, 26, 27, 29, 30, 31, 32, 33, 34
 };
 
 static u8 skx_open_column[] = {
-- 
2.17.1

RE: [PATCH 1/1] EDAC/skx: Fix overflows on the DRAM row address mapping arrays

[Luck, Tony]

> Fixes: 98f2fc829e3b ("EDAC, skx_edac: Delete duplicated code")

Please explain this Fixes tag. Looking at that commit I see that the 
skx_close_row[] and skx_open_row[] arrays were moved from one
file to another in that commit. But neither had the "34" entry that
you are adding in this patch. They both stopped at "33".

> static u8 skx_close_row[] = {
> -	15, 16, 17, 18, 20, 21, 22, 28, 10, 11, 12, 13, 29, 30, 31, 32, 33
> +	15, 16, 17, 18, 20, 21, 22, 28, 10, 11, 12, 13, 29, 30, 31, 32, 33, 34
>  };
 
> static u8 skx_open_row[] = {
> -	14, 15, 16, 20, 28, 21, 22, 23, 24, 25, 26, 27, 29, 30, 31, 32, 33
> +	14, 15, 16, 20, 28, 21, 22, 23, 24, 25, 26, 27, 29, 30, 31, 32, 33, 34
>  };

-Tony

RE: [PATCH 1/1] EDAC/skx: Fix overflows on the DRAM row address mapping arrays

[Zhuo, Qiuxu]

> From: Luck, Tony <tony.luck@intel.com>
> Sent: Friday, February 10, 2023 1:30 AM
> To: Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>; Aristeu Rozanski <aris@redhat.com>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Xu, Feng F
> <feng.f.xu@intel.com>; linux-edac@vger.kernel.org; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: RE: [PATCH 1/1] EDAC/skx: Fix overflows on the DRAM row address
> mapping arrays
> 
> > Fixes: 98f2fc829e3b ("EDAC, skx_edac: Delete duplicated code")
> 
> Please explain this Fixes tag. Looking at that commit I see that the
> skx_close_row[] and skx_open_row[] arrays were moved from one file to
> another in that commit. But neither had the "34" entry that you are adding in
> this patch. They both stopped at "33".

Hi Tony,

I thought the commit "98f2fc829e3b" was the first commit of the new file skx_base.c and then chose it as the Fixes tag for this patch. But as your concern, this Fixes tag made people confused due to no changes on the two arrays by the commit "98f2fc829e3b" [1]. 

Do you think the original commit "4ec656bdf43a" of the file skx_edac.c (though it was removed) is the right commit to be used as the Fixes tag for this patch?

      Fixes: 4ec656bdf43a ("EDAC, skx_edac: Add EDAC driver for Skylake")

If yes, I’ll replace the Fixes tag with the new Fixes tag above in v2. 
If no, any suggestions for me? 😊 Thanks!

[1] The commit “98f2fc829e3b” only moved the skx_close_row[] and skx_open_row[] arrays from skx_edac.c to skx_base.c but made no changes on them.

-Qiuxu


> > static u8 skx_close_row[] = {
> > -	15, 16, 17, 18, 20, 21, 22, 28, 10, 11, 12, 13, 29, 30, 31, 32, 33
> > +	15, 16, 17, 18, 20, 21, 22, 28, 10, 11, 12, 13, 29, 30, 31, 32, 33,
> > +34
> >  };
> 
> > static u8 skx_open_row[] = {
> > -	14, 15, 16, 20, 28, 21, 22, 23, 24, 25, 26, 27, 29, 30, 31, 32, 33
> > +	14, 15, 16, 20, 28, 21, 22, 23, 24, 25, 26, 27, 29, 30, 31, 32, 33,
> > +34
> >  };
> 
> -Tony

RE: [PATCH 1/1] EDAC/skx: Fix overflows on the DRAM row address mapping arrays

[Luck, Tony]

> Do you think the original commit "4ec656bdf43a" of the file skx_edac.c (though it was removed) is the right commit to be used as the Fixes tag for this patch?
>
>      Fixes: 4ec656bdf43a ("EDAC, skx_edac: Add EDAC driver for Skylake")

This is the correct tag (this problem goes all the way back to the very first version of this driver).

-Tony

RE: [PATCH 1/1] EDAC/skx: Fix overflows on the DRAM row address mapping arrays

[Zhuo, Qiuxu]

> From: Luck, Tony <tony.luck@intel.com>
> Sent: Saturday, February 11, 2023 1:02 AM
> ...
> > Do you think the original commit "4ec656bdf43a" of the file skx_edac.c
> > (though it was removed) is the right commit to be used as the Fixes tag for this
>> patch?
> >
> >      Fixes: 4ec656bdf43a ("EDAC, skx_edac: Add EDAC driver for Skylake")
> 
> This is the correct tag (this problem goes all the way back to the very first
> version of this driver).

Thank you for the correction. Will update it in v2.

-Qiuxu

[PATCH v2 1/1] EDAC/skx: Fix overflows on the DRAM row address mapping arrays

[Qiuxu Zhuo]

The current DRAM row address mapping arrays skx_{open,close}_row[]
only support ranks with sizes up to 16G. Decoding a rank address
to a DRAM row address for a 32G rank by using either one of the
above arrays by the skx_edac driver, will result in an overflow on
the array.

For a 32G rank, the most significant DRAM row address bit (the
bit17) is mapped from the bit34 of the rank address. Add this new
mapping item to both arrays to fix the overflow issue.

Fixes: 4ec656bdf43a ("EDAC, skx_edac: Add EDAC driver for Skylake")
Reported-by: Feng Xu <feng.f.xu@intel.com>
Tested-by: Feng Xu <feng.f.xu@intel.com>
Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
---
v1->v2:
    Fix the Fixes tag in the commit message.

 drivers/edac/skx_base.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/skx_base.c b/drivers/edac/skx_base.c
index 9397abb42c49..0a862336a7ce 100644
--- a/drivers/edac/skx_base.c
+++ b/drivers/edac/skx_base.c
@@ -510,7 +510,7 @@ static bool skx_rir_decode(struct decoded_addr *res)
 }
 
 static u8 skx_close_row[] = {
-	15, 16, 17, 18, 20, 21, 22, 28, 10, 11, 12, 13, 29, 30, 31, 32, 33
+	15, 16, 17, 18, 20, 21, 22, 28, 10, 11, 12, 13, 29, 30, 31, 32, 33, 34
 };
 
 static u8 skx_close_column[] = {
@@ -518,7 +518,7 @@ static u8 skx_close_column[] = {
 };
 
 static u8 skx_open_row[] = {
-	14, 15, 16, 20, 28, 21, 22, 23, 24, 25, 26, 27, 29, 30, 31, 32, 33
+	14, 15, 16, 20, 28, 21, 22, 23, 24, 25, 26, 27, 29, 30, 31, 32, 33, 34
 };
 
 static u8 skx_open_column[] = {
-- 
2.17.1

RE: [PATCH v2 1/1] EDAC/skx: Fix overflows on the DRAM row address mapping arrays

[Luck, Tony]

> For a 32G rank, the most significant DRAM row address bit (the
> bit17) is mapped from the bit34 of the rank address. Add this new
> mapping item to both arrays to fix the overflow issue.

Applied to edac-drivers branch of the RAS tree.

Thanks

-Tony