[PATCH] Raise maximum number of memory controllers

[PATCH] Raise maximum number of memory controllers

[Justin Ernst]

We observe an oops in the skx_edac module during boot.
Examining /var/log/messages:
[ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
[ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
...
[ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
[ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
[ 3401.987579] Too many memory controllers: 16
[ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0

We observe there are two memory controllers per socket, with a limit of 16.
Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).

Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Russ Anderson <russ.anderson@hpe.com>
Signed-off-by: Justin Ernst <justin.ernst@hpe.com>
---
 include/linux/edac.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/edac.h b/include/linux/edac.h
index bffb97828ed6..958d69332c1d 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -17,6 +17,7 @@
 #include <linux/completion.h>
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
+#include <linux/numa.h>
 
 #define EDAC_DEVICE_NAME_LEN	31
 
@@ -670,6 +671,6 @@ struct mem_ctl_info {
 /*
  * Maximum number of memory controllers in the coherent fabric.
  */
-#define EDAC_MAX_MCS	16
+#define EDAC_MAX_MCS	2 * MAX_NUMNODES
 
 #endif
-- 
2.12.3

Raise maximum number of memory controllers

[Justin Ernst]

We observe an oops in the skx_edac module during boot.
Examining /var/log/messages:
[ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
[ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
...
[ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
[ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
[ 3401.987579] Too many memory controllers: 16
[ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0

We observe there are two memory controllers per socket, with a limit of 16.
Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).

Cc: Borislav Petkov <bp@alien8.de>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Russ Anderson <russ.anderson@hpe.com>
Signed-off-by: Justin Ernst <justin.ernst@hpe.com>
---
 include/linux/edac.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/edac.h b/include/linux/edac.h
index bffb97828ed6..958d69332c1d 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -17,6 +17,7 @@
 #include <linux/completion.h>
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
+#include <linux/numa.h>
 
 #define EDAC_DEVICE_NAME_LEN	31
 
@@ -670,6 +671,6 @@ struct mem_ctl_info {
 /*
  * Maximum number of memory controllers in the coherent fabric.
  */
-#define EDAC_MAX_MCS	16
+#define EDAC_MAX_MCS	2 * MAX_NUMNODES
 
 #endif

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Tue, Sep 25, 2018 at 09:34:49AM -0500, Justin Ernst wrote:
> We observe an oops in the skx_edac module during boot.
> Examining /var/log/messages:
> [ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
> [ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> [ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> ...
> [ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> [ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
> [ 3401.987579] Too many memory controllers: 16
> [ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0
> 
> We observe there are two memory controllers per socket, with a limit of 16.
> Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).

Tony,

can we read that out from the hardware instead of having this silly
static number?

Leaving in the rest.

> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Russ Anderson <russ.anderson@hpe.com>
> Signed-off-by: Justin Ernst <justin.ernst@hpe.com>
> ---
>  include/linux/edac.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index bffb97828ed6..958d69332c1d 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -17,6 +17,7 @@
>  #include <linux/completion.h>
>  #include <linux/workqueue.h>
>  #include <linux/debugfs.h>
> +#include <linux/numa.h>
>  
>  #define EDAC_DEVICE_NAME_LEN	31
>  
> @@ -670,6 +671,6 @@ struct mem_ctl_info {
>  /*
>   * Maximum number of memory controllers in the coherent fabric.
>   */
> -#define EDAC_MAX_MCS	16
> +#define EDAC_MAX_MCS	2 * MAX_NUMNODES
>  
>  #endif
> -- 
> 2.12.3
> 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Tue, Sep 25, 2018 at 09:34:49AM -0500, Justin Ernst wrote:
> We observe an oops in the skx_edac module during boot.
> Examining /var/log/messages:
> [ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
> [ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> [ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> ...
> [ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> [ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
> [ 3401.987579] Too many memory controllers: 16
> [ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0
> 
> We observe there are two memory controllers per socket, with a limit of 16.
> Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).

Tony,

can we read that out from the hardware instead of having this silly
static number?

Leaving in the rest.

> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Russ Anderson <russ.anderson@hpe.com>
> Signed-off-by: Justin Ernst <justin.ernst@hpe.com>
> ---
>  include/linux/edac.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index bffb97828ed6..958d69332c1d 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -17,6 +17,7 @@
>  #include <linux/completion.h>
>  #include <linux/workqueue.h>
>  #include <linux/debugfs.h>
> +#include <linux/numa.h>
>  
>  #define EDAC_DEVICE_NAME_LEN	31
>  
> @@ -670,6 +671,6 @@ struct mem_ctl_info {
>  /*
>   * Maximum number of memory controllers in the coherent fabric.
>   */
> -#define EDAC_MAX_MCS	16
> +#define EDAC_MAX_MCS	2 * MAX_NUMNODES
>  
>  #endif
> -- 
> 2.12.3
>

Re: [PATCH] Raise maximum number of memory controllers

[Luck, Tony]

On Tue, Sep 25, 2018 at 05:26:59PM +0200, Borislav Petkov wrote:
> On Tue, Sep 25, 2018 at 09:34:49AM -0500, Justin Ernst wrote:
> > We observe an oops in the skx_edac module during boot.
> > Examining /var/log/messages:
> > [ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
> > [ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> > [ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> > ...
> > [ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> > [ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> > [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
> > [ 3401.987579] Too many memory controllers: 16
> > [ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0
> > 
> > We observe there are two memory controllers per socket, with a limit of 16.
> > Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).
> 
> Tony,
> 
> can we read that out from the hardware instead of having this silly
> static number?
> 
> Leaving in the rest.

There are way too many places where we use the identifier "bus"
in the edac core and drivers. But I'm not sure that we need a
static array mc_bus[EDAC_MAX_MCS].

Why can't we:


-	mci->bus = &mc_bus[mci->mc_idx];
+	mci->bus = kmalloc(sizeof *(mci->bus), GFP_KERNEL);

and then figure out where to kfree(mci->bus) on driver removal?

Do we every do arithmetic on different mci->bus pointers that
assume they are all part of a single array?

-Tony

Raise maximum number of memory controllers

[Luck, Tony]

On Tue, Sep 25, 2018 at 05:26:59PM +0200, Borislav Petkov wrote:
> On Tue, Sep 25, 2018 at 09:34:49AM -0500, Justin Ernst wrote:
> > We observe an oops in the skx_edac module during boot.
> > Examining /var/log/messages:
> > [ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
> > [ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> > [ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> > ...
> > [ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> > [ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> > [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
> > [ 3401.987579] Too many memory controllers: 16
> > [ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0
> > 
> > We observe there are two memory controllers per socket, with a limit of 16.
> > Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).
> 
> Tony,
> 
> can we read that out from the hardware instead of having this silly
> static number?
> 
> Leaving in the rest.

There are way too many places where we use the identifier "bus"
in the edac core and drivers. But I'm not sure that we need a
static array mc_bus[EDAC_MAX_MCS].

Why can't we:


-	mci->bus = &mc_bus[mci->mc_idx];
+	mci->bus = kmalloc(sizeof *(mci->bus), GFP_KERNEL);

and then figure out where to kfree(mci->bus) on driver removal?

Do we every do arithmetic on different mci->bus pointers that
assume they are all part of a single array?

-Tony

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Tue, Sep 25, 2018 at 10:50:23AM -0700, Luck, Tony wrote:
> There are way too many places where we use the identifier "bus"
> in the edac core and drivers. But I'm not sure that we need a
> static array mc_bus[EDAC_MAX_MCS].

That, of course, is another way of looking at it which I didn't think
of.

> Why can't we:
> 
> 
> -	mci->bus = &mc_bus[mci->mc_idx];
> +	mci->bus = kmalloc(sizeof *(mci->bus), GFP_KERNEL);
> 
> and then figure out where to kfree(mci->bus) on driver removal?

AFAICT, in _edac_mc_free(). We free there mci itself so kfree(mci->bus)
can happen directly before it.

> Do we every do arithmetic on different mci->bus pointers that
> assume they are all part of a single array?

AFAICT, we use that thing for the bus_reg/unreg functions and we hand it
back'n'forth in edac_mc_sysfs.c, see

$ git grep -E "mci.*bus" drivers/edac/
drivers/edac/edac_mc.c:763:     mci->bus = &mc_bus[mci->mc_idx];
drivers/edac/edac_mc_sysfs.c:408:       csrow->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:639:       dimm->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:928:       mci->bus->name = name;
drivers/edac/edac_mc_sysfs.c:930:       edac_dbg(0, "creating bus %s\n", mci->bus->name);
drivers/edac/edac_mc_sysfs.c:932:       err = bus_register(mci->bus);
drivers/edac/edac_mc_sysfs.c:943:       mci->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:1002:      bus_unregister(mci->bus);
drivers/edac/edac_mc_sysfs.c:1035:      struct bus_type *bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:1036:      const char *name = mci->bus->name;
drivers/edac/edac_mc_sysfs.c:1071:      mci_pdev->bus = edac_get_sysfs_subsys();
drivers/edac/i5100_edac.c:967:  priv->debugfs = edac_debugfs_create_dir_at(mci->bus->name, i5100_debugfs);
drivers/edac/i7core_edac.c:1170:        pvt->addrmatch_dev->bus = mci->dev.bus;
drivers/edac/i7core_edac.c:1191:                pvt->chancounts_dev->bus = mci->dev.bus;

HOWEVER, look at

  88d84ac97378 ("EDAC: Fix lockdep splat")

Now I remember. I did that for lockdep because it wants statically
allocated memory. I'll try to think of something tomorrow.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Tue, Sep 25, 2018 at 10:50:23AM -0700, Luck, Tony wrote:
> There are way too many places where we use the identifier "bus"
> in the edac core and drivers. But I'm not sure that we need a
> static array mc_bus[EDAC_MAX_MCS].

That, of course, is another way of looking at it which I didn't think
of.

> Why can't we:
> 
> 
> -	mci->bus = &mc_bus[mci->mc_idx];
> +	mci->bus = kmalloc(sizeof *(mci->bus), GFP_KERNEL);
> 
> and then figure out where to kfree(mci->bus) on driver removal?

AFAICT, in _edac_mc_free(). We free there mci itself so kfree(mci->bus)
can happen directly before it.

> Do we every do arithmetic on different mci->bus pointers that
> assume they are all part of a single array?

AFAICT, we use that thing for the bus_reg/unreg functions and we hand it
back'n'forth in edac_mc_sysfs.c, see

$ git grep -E "mci.*bus" drivers/edac/
drivers/edac/edac_mc.c:763:     mci->bus = &mc_bus[mci->mc_idx];
drivers/edac/edac_mc_sysfs.c:408:       csrow->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:639:       dimm->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:928:       mci->bus->name = name;
drivers/edac/edac_mc_sysfs.c:930:       edac_dbg(0, "creating bus %s\n", mci->bus->name);
drivers/edac/edac_mc_sysfs.c:932:       err = bus_register(mci->bus);
drivers/edac/edac_mc_sysfs.c:943:       mci->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:1002:      bus_unregister(mci->bus);
drivers/edac/edac_mc_sysfs.c:1035:      struct bus_type *bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:1036:      const char *name = mci->bus->name;
drivers/edac/edac_mc_sysfs.c:1071:      mci_pdev->bus = edac_get_sysfs_subsys();
drivers/edac/i5100_edac.c:967:  priv->debugfs = edac_debugfs_create_dir_at(mci->bus->name, i5100_debugfs);
drivers/edac/i7core_edac.c:1170:        pvt->addrmatch_dev->bus = mci->dev.bus;
drivers/edac/i7core_edac.c:1191:                pvt->chancounts_dev->bus = mci->dev.bus;

HOWEVER, look at

  88d84ac97378 ("EDAC: Fix lockdep splat")

Now I remember. I did that for lockdep because it wants statically
allocated memory. I'll try to think of something tomorrow.

Thx.

RE: [PATCH] Raise maximum number of memory controllers

[Zhuo, Qiuxu]

Hi Justin,

> [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
    Just curious, has the system(two memory controllers per socket) got more than 8 sockets?
    Normally, the number "1" in the above string "Skylake Socekt#1 IMC#1" should be 7 (that was 15/2), but it was 1 here.

Thanks!
-Qiuxu

Raise maximum number of memory controllers

[Qiuxu Zhuo]

Hi Justin,

> [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
    Just curious, has the system(two memory controllers per socket) got more than 8 sockets?
    Normally, the number "1" in the above string "Skylake Socekt#1 IMC#1" should be 7 (that was 15/2), but it was 1 here.

Thanks!
-Qiuxu

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Tue, Sep 25, 2018 at 08:07:33PM +0200, Borislav Petkov wrote:
> Now I remember. I did that for lockdep because it wants statically
> allocated memory. I'll try to think of something tomorrow.

Some more info after some staring:

We could've made the lock_class_key only static storage so that lockdep
is happy but then struct bus_type embeds the *whole* lock_class_key and
not a pointer to it.

Which leaves us with either:

* this fix postponing the problem

* or Greg coming and saying, you're using bus_type all wrong and you
shouldn't and you should remove it completely! :-)

Which would be much better.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Tue, Sep 25, 2018 at 08:07:33PM +0200, Borislav Petkov wrote:
> Now I remember. I did that for lockdep because it wants statically
> allocated memory. I'll try to think of something tomorrow.

Some more info after some staring:

We could've made the lock_class_key only static storage so that lockdep
is happy but then struct bus_type embeds the *whole* lock_class_key and
not a pointer to it.

Which leaves us with either:

* this fix postponing the problem

* or Greg coming and saying, you're using bus_type all wrong and you
shouldn't and you should remove it completely! :-)

Which would be much better.

Thx.

Re: [PATCH] Raise maximum number of memory controllers

[Russ Anderson]

On Wed, Sep 26, 2018 at 07:55:39AM +0000, Zhuo, Qiuxu wrote:
> Hi Justin,
> 
> > [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
>     Just curious, has the system(two memory controllers per socket) got more than 8 sockets?
>     Normally, the number "1" in the above string "Skylake Socekt#1 IMC#1" should be 7 (that was 15/2), but it was 1 here.

Yes, that is from a 32 socket system.

Thanks.
-- 
Russ Anderson,  SuperDome Flex Linux Kernel Group Manager
HPE - Hewlett Packard Enterprise (formerly SGI)  rja@hpe.com

Raise maximum number of memory controllers

[Russ Anderson]

On Wed, Sep 26, 2018 at 07:55:39AM +0000, Zhuo, Qiuxu wrote:
> Hi Justin,
> 
> > [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
>     Just curious, has the system(two memory controllers per socket) got more than 8 sockets?
>     Normally, the number "1" in the above string "Skylake Socekt#1 IMC#1" should be 7 (that was 15/2), but it was 1 here.

Yes, that is from a 32 socket system.

Thanks.

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Wed, Sep 26, 2018 at 11:35:11AM +0200, Borislav Petkov wrote:
> * or Greg coming and saying, you're using bus_type all wrong and you
> shouldn't and you should remove it completely! :-)

Yap, and so he did! :-)

It looks like we can remove the whole per-MC bus thing, see below. Patch
seems to work here and grepping sysfs hierarchy before and after:

find /sys/ | grep -i edac > ...

doesn't show any differences.

Tony, I'd appreciate running this on one of your big boxes to check
nothing is missing in sysfs...

Thx.

---
 drivers/edac/edac_mc.c       |  9 +--------
 drivers/edac/edac_mc_sysfs.c | 30 ++----------------------------
 include/linux/edac.h         |  6 ------
 3 files changed, 3 insertions(+), 42 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 7d3edd713932..13594ffadcb3 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -55,8 +55,6 @@ static LIST_HEAD(mc_devices);
  */
 static const char *edac_mc_owner;
 
-static struct bus_type mc_bus[EDAC_MAX_MCS];
-
 int edac_get_report_status(void)
 {
 	return edac_report;
@@ -716,11 +714,6 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 	int ret = -EINVAL;
 	edac_dbg(0, "\n");
 
-	if (mci->mc_idx >= EDAC_MAX_MCS) {
-		pr_warn_once("Too many memory controllers: %d\n", mci->mc_idx);
-		return -ENODEV;
-	}
-
 #ifdef CONFIG_EDAC_DEBUG
 	if (edac_debug_level >= 3)
 		edac_mc_dump_mci(mci);
@@ -760,7 +753,7 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 	/* set load time so that error rate can be tracked */
 	mci->start_time = jiffies;
 
-	mci->bus = &mc_bus[mci->mc_idx];
+	mci->bus = edac_get_sysfs_subsys();
 
 	if (edac_create_sysfs_mci_device(mci, groups)) {
 		edac_mc_printk(mci, KERN_WARNING,
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 20374b8248f0..2ca2012f2857 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -914,27 +914,8 @@ static const struct device_type mci_attr_type = {
 int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 				 const struct attribute_group **groups)
 {
-	char *name;
 	int i, err;
 
-	/*
-	 * The memory controller needs its own bus, in order to avoid
-	 * namespace conflicts at /sys/bus/edac.
-	 */
-	name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
-	if (!name)
-		return -ENOMEM;
-
-	mci->bus->name = name;
-
-	edac_dbg(0, "creating bus %s\n", mci->bus->name);
-
-	err = bus_register(mci->bus);
-	if (err < 0) {
-		kfree(name);
-		return err;
-	}
-
 	/* get the /sys/devices/system/edac subsys reference */
 	mci->dev.type = &mci_attr_type;
 	device_initialize(&mci->dev);
@@ -950,7 +931,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	err = device_add(&mci->dev);
 	if (err < 0) {
 		edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
-		goto fail_unregister_bus;
+		goto out;
 	}
 
 	/*
@@ -998,10 +979,8 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 		device_unregister(&dimm->dev);
 	}
 	device_unregister(&mci->dev);
-fail_unregister_bus:
-	bus_unregister(mci->bus);
-	kfree(name);
 
+out:
 	return err;
 }
 
@@ -1032,13 +1011,8 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
 
 void edac_unregister_sysfs(struct mem_ctl_info *mci)
 {
-	struct bus_type *bus = mci->bus;
-	const char *name = mci->bus->name;
-
 	edac_dbg(1, "Unregistering device %s\n", dev_name(&mci->dev));
 	device_unregister(&mci->dev);
-	bus_unregister(bus);
-	kfree(name);
 }
 
 static void mc_attr_release(struct device *dev)
diff --git a/include/linux/edac.h b/include/linux/edac.h
index a45ce1f84bfc..dd8d006f1837 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -668,10 +668,4 @@ struct mem_ctl_info {
 	bool fake_inject_ue;
 	u16 fake_inject_count;
 };
-
-/*
- * Maximum number of memory controllers in the coherent fabric.
- */
-#define EDAC_MAX_MCS	16
-
 #endif
-- 
2.17.0.582.gccdcbd54c

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Wed, Sep 26, 2018 at 11:35:11AM +0200, Borislav Petkov wrote:
> * or Greg coming and saying, you're using bus_type all wrong and you
> shouldn't and you should remove it completely! :-)

Yap, and so he did! :-)

It looks like we can remove the whole per-MC bus thing, see below. Patch
seems to work here and grepping sysfs hierarchy before and after:

find /sys/ | grep -i edac > ...

doesn't show any differences.

Tony, I'd appreciate running this on one of your big boxes to check
nothing is missing in sysfs...

Thx.
---
 drivers/edac/edac_mc.c       |  9 +--------
 drivers/edac/edac_mc_sysfs.c | 30 ++----------------------------
 include/linux/edac.h         |  6 ------
 3 files changed, 3 insertions(+), 42 deletions(-)

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 7d3edd713932..13594ffadcb3 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -55,8 +55,6 @@ static LIST_HEAD(mc_devices);
  */
 static const char *edac_mc_owner;
 
-static struct bus_type mc_bus[EDAC_MAX_MCS];
-
 int edac_get_report_status(void)
 {
 	return edac_report;
@@ -716,11 +714,6 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 	int ret = -EINVAL;
 	edac_dbg(0, "\n");
 
-	if (mci->mc_idx >= EDAC_MAX_MCS) {
-		pr_warn_once("Too many memory controllers: %d\n", mci->mc_idx);
-		return -ENODEV;
-	}
-
 #ifdef CONFIG_EDAC_DEBUG
 	if (edac_debug_level >= 3)
 		edac_mc_dump_mci(mci);
@@ -760,7 +753,7 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 	/* set load time so that error rate can be tracked */
 	mci->start_time = jiffies;
 
-	mci->bus = &mc_bus[mci->mc_idx];
+	mci->bus = edac_get_sysfs_subsys();
 
 	if (edac_create_sysfs_mci_device(mci, groups)) {
 		edac_mc_printk(mci, KERN_WARNING,
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 20374b8248f0..2ca2012f2857 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -914,27 +914,8 @@ static const struct device_type mci_attr_type = {
 int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 				 const struct attribute_group **groups)
 {
-	char *name;
 	int i, err;
 
-	/*
-	 * The memory controller needs its own bus, in order to avoid
-	 * namespace conflicts at /sys/bus/edac.
-	 */
-	name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
-	if (!name)
-		return -ENOMEM;
-
-	mci->bus->name = name;
-
-	edac_dbg(0, "creating bus %s\n", mci->bus->name);
-
-	err = bus_register(mci->bus);
-	if (err < 0) {
-		kfree(name);
-		return err;
-	}
-
 	/* get the /sys/devices/system/edac subsys reference */
 	mci->dev.type = &mci_attr_type;
 	device_initialize(&mci->dev);
@@ -950,7 +931,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	err = device_add(&mci->dev);
 	if (err < 0) {
 		edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
-		goto fail_unregister_bus;
+		goto out;
 	}
 
 	/*
@@ -998,10 +979,8 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 		device_unregister(&dimm->dev);
 	}
 	device_unregister(&mci->dev);
-fail_unregister_bus:
-	bus_unregister(mci->bus);
-	kfree(name);
 
+out:
 	return err;
 }
 
@@ -1032,13 +1011,8 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
 
 void edac_unregister_sysfs(struct mem_ctl_info *mci)
 {
-	struct bus_type *bus = mci->bus;
-	const char *name = mci->bus->name;
-
 	edac_dbg(1, "Unregistering device %s\n", dev_name(&mci->dev));
 	device_unregister(&mci->dev);
-	bus_unregister(bus);
-	kfree(name);
 }
 
 static void mc_attr_release(struct device *dev)
diff --git a/include/linux/edac.h b/include/linux/edac.h
index a45ce1f84bfc..dd8d006f1837 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -668,10 +668,4 @@ struct mem_ctl_info {
 	bool fake_inject_ue;
 	u16 fake_inject_count;
 };
-
-/*
- * Maximum number of memory controllers in the coherent fabric.
- */
-#define EDAC_MAX_MCS	16
-
 #endif

Re: [PATCH] Raise maximum number of memory controllers

[Mauro Carvalho Chehab]

Em Wed, 26 Sep 2018 17:27:52 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Wed, Sep 26, 2018 at 11:35:11AM +0200, Borislav Petkov wrote:
> > * or Greg coming and saying, you're using bus_type all wrong and you
> > shouldn't and you should remove it completely! :-)  
> 
> Yap, and so he did! :-)
> 
> It looks like we can remove the whole per-MC bus thing, see below. 

I guess this is/was needed to create things like this:

	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc

(I don't test this logic for a while).

> Patch
> seems to work here and grepping sysfs hierarchy before and after:
> 
> find /sys/ | grep -i edac > ...
> 
> doesn't show any differences.
> 
> Tony, I'd appreciate running this on one of your big boxes to check
> nothing is missing in sysfs...

There are a few EDAC drivers for old server chipsets that create
error report mechanisms for PCI bus controllers. I don't remember
the specifics anymore. I *suspect* those are the drivers that
do such usage:

	$ git grep -l edac_pci_add_device
	drivers/edac/amd8111_edac.c
	drivers/edac/amd8131_edac.c
	drivers/edac/edac_pci.c
	drivers/edac/edac_pci.h
	drivers/edac/mpc85xx_edac.c
	drivers/edac/mv64x60_edac.c
	drivers/edac/octeon_edac-pci.c

None of them use Intel chipsets.

Last time I tried to test it (more than 5 years ago), it was
hard to find such systems at the labs I had access on that time.

Anyway, as this is a more unusual usage of EDAC API, it could be
worth to double-check if this patch won't break it, maybe doing
some test on such machines before/after this patch in order to 
verify that this won't cause regressions there.

In thesis, it shouldn't affect, as the changes are happening at
edac_mc*.c, but I won't doubt that it might have a hidden dependency
somewhere.

> 
> Thx.
> 
> ---
>  drivers/edac/edac_mc.c       |  9 +--------
>  drivers/edac/edac_mc_sysfs.c | 30 ++----------------------------
>  include/linux/edac.h         |  6 ------
>  3 files changed, 3 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 7d3edd713932..13594ffadcb3 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -55,8 +55,6 @@ static LIST_HEAD(mc_devices);
>   */
>  static const char *edac_mc_owner;
>  
> -static struct bus_type mc_bus[EDAC_MAX_MCS];
> -
>  int edac_get_report_status(void)
>  {
>  	return edac_report;
> @@ -716,11 +714,6 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
>  	int ret = -EINVAL;
>  	edac_dbg(0, "\n");
>  
> -	if (mci->mc_idx >= EDAC_MAX_MCS) {
> -		pr_warn_once("Too many memory controllers: %d\n", mci->mc_idx);
> -		return -ENODEV;
> -	}
> -
>  #ifdef CONFIG_EDAC_DEBUG
>  	if (edac_debug_level >= 3)
>  		edac_mc_dump_mci(mci);
> @@ -760,7 +753,7 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
>  	/* set load time so that error rate can be tracked */
>  	mci->start_time = jiffies;
>  
> -	mci->bus = &mc_bus[mci->mc_idx];
> +	mci->bus = edac_get_sysfs_subsys();
>  
>  	if (edac_create_sysfs_mci_device(mci, groups)) {
>  		edac_mc_printk(mci, KERN_WARNING,
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 20374b8248f0..2ca2012f2857 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -914,27 +914,8 @@ static const struct device_type mci_attr_type = {
>  int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
>  				 const struct attribute_group **groups)
>  {
> -	char *name;
>  	int i, err;
>  
> -	/*
> -	 * The memory controller needs its own bus, in order to avoid
> -	 * namespace conflicts at /sys/bus/edac.
> -	 */
> -	name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
> -	if (!name)
> -		return -ENOMEM;
> -
> -	mci->bus->name = name;
> -
> -	edac_dbg(0, "creating bus %s\n", mci->bus->name);
> -
> -	err = bus_register(mci->bus);
> -	if (err < 0) {
> -		kfree(name);
> -		return err;
> -	}
> -
>  	/* get the /sys/devices/system/edac subsys reference */
>  	mci->dev.type = &mci_attr_type;
>  	device_initialize(&mci->dev);
> @@ -950,7 +931,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
>  	err = device_add(&mci->dev);
>  	if (err < 0) {
>  		edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
> -		goto fail_unregister_bus;
> +		goto out;
>  	}
>  
>  	/*
> @@ -998,10 +979,8 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
>  		device_unregister(&dimm->dev);
>  	}
>  	device_unregister(&mci->dev);
> -fail_unregister_bus:
> -	bus_unregister(mci->bus);
> -	kfree(name);
>  
> +out:
>  	return err;
>  }
>  
> @@ -1032,13 +1011,8 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
>  
>  void edac_unregister_sysfs(struct mem_ctl_info *mci)
>  {
> -	struct bus_type *bus = mci->bus;
> -	const char *name = mci->bus->name;
> -
>  	edac_dbg(1, "Unregistering device %s\n", dev_name(&mci->dev));
>  	device_unregister(&mci->dev);
> -	bus_unregister(bus);
> -	kfree(name);
>  }
>  
>  static void mc_attr_release(struct device *dev)
> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index a45ce1f84bfc..dd8d006f1837 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -668,10 +668,4 @@ struct mem_ctl_info {
>  	bool fake_inject_ue;
>  	u16 fake_inject_count;
>  };
> -
> -/*
> - * Maximum number of memory controllers in the coherent fabric.
> - */
> -#define EDAC_MAX_MCS	16
> -
>  #endif



Thanks,
Mauro

Raise maximum number of memory controllers

[Mauro Carvalho Chehab]

Em Wed, 26 Sep 2018 17:27:52 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Wed, Sep 26, 2018 at 11:35:11AM +0200, Borislav Petkov wrote:
> > * or Greg coming and saying, you're using bus_type all wrong and you
> > shouldn't and you should remove it completely! :-)  
> 
> Yap, and so he did! :-)
> 
> It looks like we can remove the whole per-MC bus thing, see below. 

I guess this is/was needed to create things like this:

	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc

(I don't test this logic for a while).

> Patch
> seems to work here and grepping sysfs hierarchy before and after:
> 
> find /sys/ | grep -i edac > ...
> 
> doesn't show any differences.
> 
> Tony, I'd appreciate running this on one of your big boxes to check
> nothing is missing in sysfs...

There are a few EDAC drivers for old server chipsets that create
error report mechanisms for PCI bus controllers. I don't remember
the specifics anymore. I *suspect* those are the drivers that
do such usage:

	$ git grep -l edac_pci_add_device
	drivers/edac/amd8111_edac.c
	drivers/edac/amd8131_edac.c
	drivers/edac/edac_pci.c
	drivers/edac/edac_pci.h
	drivers/edac/mpc85xx_edac.c
	drivers/edac/mv64x60_edac.c
	drivers/edac/octeon_edac-pci.c

None of them use Intel chipsets.

Last time I tried to test it (more than 5 years ago), it was
hard to find such systems at the labs I had access on that time.

Anyway, as this is a more unusual usage of EDAC API, it could be
worth to double-check if this patch won't break it, maybe doing
some test on such machines before/after this patch in order to 
verify that this won't cause regressions there.

In thesis, it shouldn't affect, as the changes are happening at
edac_mc*.c, but I won't doubt that it might have a hidden dependency
somewhere.

> 
> Thx.
> 
> ---
>  drivers/edac/edac_mc.c       |  9 +--------
>  drivers/edac/edac_mc_sysfs.c | 30 ++----------------------------
>  include/linux/edac.h         |  6 ------
>  3 files changed, 3 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 7d3edd713932..13594ffadcb3 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -55,8 +55,6 @@ static LIST_HEAD(mc_devices);
>   */
>  static const char *edac_mc_owner;
>  
> -static struct bus_type mc_bus[EDAC_MAX_MCS];
> -
>  int edac_get_report_status(void)
>  {
>  	return edac_report;
> @@ -716,11 +714,6 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
>  	int ret = -EINVAL;
>  	edac_dbg(0, "\n");
>  
> -	if (mci->mc_idx >= EDAC_MAX_MCS) {
> -		pr_warn_once("Too many memory controllers: %d\n", mci->mc_idx);
> -		return -ENODEV;
> -	}
> -
>  #ifdef CONFIG_EDAC_DEBUG
>  	if (edac_debug_level >= 3)
>  		edac_mc_dump_mci(mci);
> @@ -760,7 +753,7 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
>  	/* set load time so that error rate can be tracked */
>  	mci->start_time = jiffies;
>  
> -	mci->bus = &mc_bus[mci->mc_idx];
> +	mci->bus = edac_get_sysfs_subsys();
>  
>  	if (edac_create_sysfs_mci_device(mci, groups)) {
>  		edac_mc_printk(mci, KERN_WARNING,
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 20374b8248f0..2ca2012f2857 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -914,27 +914,8 @@ static const struct device_type mci_attr_type = {
>  int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
>  				 const struct attribute_group **groups)
>  {
> -	char *name;
>  	int i, err;
>  
> -	/*
> -	 * The memory controller needs its own bus, in order to avoid
> -	 * namespace conflicts at /sys/bus/edac.
> -	 */
> -	name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
> -	if (!name)
> -		return -ENOMEM;
> -
> -	mci->bus->name = name;
> -
> -	edac_dbg(0, "creating bus %s\n", mci->bus->name);
> -
> -	err = bus_register(mci->bus);
> -	if (err < 0) {
> -		kfree(name);
> -		return err;
> -	}
> -
>  	/* get the /sys/devices/system/edac subsys reference */
>  	mci->dev.type = &mci_attr_type;
>  	device_initialize(&mci->dev);
> @@ -950,7 +931,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
>  	err = device_add(&mci->dev);
>  	if (err < 0) {
>  		edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
> -		goto fail_unregister_bus;
> +		goto out;
>  	}
>  
>  	/*
> @@ -998,10 +979,8 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
>  		device_unregister(&dimm->dev);
>  	}
>  	device_unregister(&mci->dev);
> -fail_unregister_bus:
> -	bus_unregister(mci->bus);
> -	kfree(name);
>  
> +out:
>  	return err;
>  }
>  
> @@ -1032,13 +1011,8 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
>  
>  void edac_unregister_sysfs(struct mem_ctl_info *mci)
>  {
> -	struct bus_type *bus = mci->bus;
> -	const char *name = mci->bus->name;
> -
>  	edac_dbg(1, "Unregistering device %s\n", dev_name(&mci->dev));
>  	device_unregister(&mci->dev);
> -	bus_unregister(bus);
> -	kfree(name);
>  }
>  
>  static void mc_attr_release(struct device *dev)
> diff --git a/include/linux/edac.h b/include/linux/edac.h
> index a45ce1f84bfc..dd8d006f1837 100644
> --- a/include/linux/edac.h
> +++ b/include/linux/edac.h
> @@ -668,10 +668,4 @@ struct mem_ctl_info {
>  	bool fake_inject_ue;
>  	u16 fake_inject_count;
>  };
> -
> -/*
> - * Maximum number of memory controllers in the coherent fabric.
> - */
> -#define EDAC_MAX_MCS	16
> -
>  #endif



Thanks,
Mauro

Re: [PATCH] Raise maximum number of memory controllers

[Aristeu Rozanski]

On Tue, Sep 25, 2018 at 09:34:49AM -0500, Justin Ernst wrote:
> We observe an oops in the skx_edac module during boot.

That's happening also on sb_edac too and the oops comes from memory corruption
after trying to load the module several times during boot.

-- 
Aristeu

Raise maximum number of memory controllers

[Aristeu Rozanski]

On Tue, Sep 25, 2018 at 09:34:49AM -0500, Justin Ernst wrote:
> We observe an oops in the skx_edac module during boot.

That's happening also on sb_edac too and the oops comes from memory corruption
after trying to load the module several times during boot.

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Wed, Sep 26, 2018 at 01:03:40PM -0300, Mauro Carvalho Chehab wrote:
> I guess this is/was needed to create things like this:
> 
> 	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc

They're still there:

$ ls -l /sys/bus/edac/devices/
total 0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Wed, Sep 26, 2018 at 01:03:40PM -0300, Mauro Carvalho Chehab wrote:
> I guess this is/was needed to create things like this:
> 
> 	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc

They're still there:

$ ls -l /sys/bus/edac/devices/
total 0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0

Re: [PATCH] Raise maximum number of memory controllers

[Mauro Carvalho Chehab]

Em Wed, 26 Sep 2018 18:17:49 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Wed, Sep 26, 2018 at 01:03:40PM -0300, Mauro Carvalho Chehab wrote:
> > I guess this is/was needed to create things like this:
> > 
> > 	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc  
> 
> They're still there:
> 
> $ ls -l /sys/bus/edac/devices/
> total 0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0
> 

Ok. Yeah, probably some changes at the driver's core made that bus
thing obsolete. Or maybe it is there to allow unbind/remove
the edac driver. Had you test it with KASAN and tried to unbind/remove
the EDAC driver?

Thanks,
Mauro

Raise maximum number of memory controllers

[Mauro Carvalho Chehab]

Em Wed, 26 Sep 2018 18:17:49 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Wed, Sep 26, 2018 at 01:03:40PM -0300, Mauro Carvalho Chehab wrote:
> > I guess this is/was needed to create things like this:
> > 
> > 	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc  
> 
> They're still there:
> 
> $ ls -l /sys/bus/edac/devices/
> total 0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0
> 

Ok. Yeah, probably some changes at the driver's core made that bus
thing obsolete. Or maybe it is there to allow unbind/remove
the edac driver. Had you test it with KASAN and tried to unbind/remove
the EDAC driver?

Thanks,
Mauro

Re: [PATCH] Raise maximum number of memory controllers

[Luck, Tony]

On Wed, Sep 26, 2018 at 06:17:49PM +0200, Borislav Petkov wrote:
> On Wed, Sep 26, 2018 at 01:03:40PM -0300, Mauro Carvalho Chehab wrote:
> > I guess this is/was needed to create things like this:
> > 
> > 	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc
> 
> They're still there:
> 
> $ ls -l /sys/bus/edac/devices/
> total 0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0

I ran into trouble on my 4 socket broadwell server (so 8 memory controllers,
a whole pile of DIMMs, running from sb_edac.c)

Things start going wrong with:

[   45.216657] sysfs: cannot create duplicate filename '/bus/edac/devices/dimm0'
[   45.216663] CPU: 37 PID: 2034 Comm: systemd-udevd Not tainted 4.19.0-rc5 #1
[   45.216665] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRBDXSD1.86B.0338.V01.1603162127 03/16/2016
[   45.216667] Call Trace:
[   45.216688]  dump_stack+0x5c/0x7b
[   45.216697]  sysfs_warn_dup+0x56/0x70
[   45.216702]  sysfs_do_create_link_sd.isra.2+0x98/0xb0
[   45.216714]  bus_add_device+0x77/0x160
[   45.216720]  device_add+0x424/0x660
[   45.216731]  edac_create_sysfs_mci_device+0xb9/0x2f0
[   45.216738]  edac_mc_add_mc_with_groups+0x111/0x2b0
[   45.216747]  sbridge_init+0x13c9/0x2000 [sb_edac]
[   45.216757]  ? _raw_spin_lock+0x1d/0x20
[   45.216765]  ? free_pcppages_bulk+0x2ca/0x630
[   45.216769]  ? 0xffffffffc050f000
[   45.216779]  do_one_initcall+0x46/0x1c8
[   45.216784]  ? free_unref_page_commit+0x95/0x120
[   45.216791]  ? _cond_resched+0x15/0x40
[   45.216798]  ? kmem_cache_alloc_trace+0x153/0x1c0
[   45.216805]  do_init_module+0x5b/0x208
[   45.216826]  load_module+0x1a2d/0x1fb0
[   45.216835]  ? __do_sys_finit_module+0xe9/0x110
[   45.216840]  __do_sys_finit_module+0xe9/0x110
[   45.216847]  do_syscall_64+0x5b/0x180
[   45.216852]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   45.216856] RIP: 0033:0x7fcdec618bd9

and fell off a cliff after that.

Going back to the old code I have a "dimm0" on each of the eight controllers:

# find /sys -name dimm0
/sys/devices/system/edac/mc/mc6/dimm0
/sys/devices/system/edac/mc/mc4/dimm0
/sys/devices/system/edac/mc/mc2/dimm0
/sys/devices/system/edac/mc/mc0/dimm0
/sys/devices/system/edac/mc/mc7/dimm0
/sys/devices/system/edac/mc/mc5/dimm0
/sys/devices/system/edac/mc/mc3/dimm0
/sys/devices/system/edac/mc/mc1/dimm0
/sys/bus/mc6/devices/dimm0
/sys/bus/mc4/devices/dimm0
/sys/bus/mc2/devices/dimm0
/sys/bus/mc0/devices/dimm0
/sys/bus/mc7/devices/dimm0
/sys/bus/mc5/devices/dimm0
/sys/bus/mc3/devices/dimm0
/sys/bus/mc1/devices/dimm0
# ls -l /sys/bus/mc0/devices
total 0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 mc0 -> ../../../devices/system/edac/mc/mc0

It looks like the new code isn't trying to place the dimm symlinks
in the proper subdirectories.

-Tony

Raise maximum number of memory controllers

[Luck, Tony]

On Wed, Sep 26, 2018 at 06:17:49PM +0200, Borislav Petkov wrote:
> On Wed, Sep 26, 2018 at 01:03:40PM -0300, Mauro Carvalho Chehab wrote:
> > I guess this is/was needed to create things like this:
> > 
> > 	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc
> 
> They're still there:
> 
> $ ls -l /sys/bus/edac/devices/
> total 0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
> lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0

I ran into trouble on my 4 socket broadwell server (so 8 memory controllers,
a whole pile of DIMMs, running from sb_edac.c)

Things start going wrong with:

[   45.216657] sysfs: cannot create duplicate filename '/bus/edac/devices/dimm0'
[   45.216663] CPU: 37 PID: 2034 Comm: systemd-udevd Not tainted 4.19.0-rc5 #1
[   45.216665] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRBDXSD1.86B.0338.V01.1603162127 03/16/2016
[   45.216667] Call Trace:
[   45.216688]  dump_stack+0x5c/0x7b
[   45.216697]  sysfs_warn_dup+0x56/0x70
[   45.216702]  sysfs_do_create_link_sd.isra.2+0x98/0xb0
[   45.216714]  bus_add_device+0x77/0x160
[   45.216720]  device_add+0x424/0x660
[   45.216731]  edac_create_sysfs_mci_device+0xb9/0x2f0
[   45.216738]  edac_mc_add_mc_with_groups+0x111/0x2b0
[   45.216747]  sbridge_init+0x13c9/0x2000 [sb_edac]
[   45.216757]  ? _raw_spin_lock+0x1d/0x20
[   45.216765]  ? free_pcppages_bulk+0x2ca/0x630
[   45.216769]  ? 0xffffffffc050f000
[   45.216779]  do_one_initcall+0x46/0x1c8
[   45.216784]  ? free_unref_page_commit+0x95/0x120
[   45.216791]  ? _cond_resched+0x15/0x40
[   45.216798]  ? kmem_cache_alloc_trace+0x153/0x1c0
[   45.216805]  do_init_module+0x5b/0x208
[   45.216826]  load_module+0x1a2d/0x1fb0
[   45.216835]  ? __do_sys_finit_module+0xe9/0x110
[   45.216840]  __do_sys_finit_module+0xe9/0x110
[   45.216847]  do_syscall_64+0x5b/0x180
[   45.216852]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   45.216856] RIP: 0033:0x7fcdec618bd9

and fell off a cliff after that.

Going back to the old code I have a "dimm0" on each of the eight controllers:

# find /sys -name dimm0
/sys/devices/system/edac/mc/mc6/dimm0
/sys/devices/system/edac/mc/mc4/dimm0
/sys/devices/system/edac/mc/mc2/dimm0
/sys/devices/system/edac/mc/mc0/dimm0
/sys/devices/system/edac/mc/mc7/dimm0
/sys/devices/system/edac/mc/mc5/dimm0
/sys/devices/system/edac/mc/mc3/dimm0
/sys/devices/system/edac/mc/mc1/dimm0
/sys/bus/mc6/devices/dimm0
/sys/bus/mc4/devices/dimm0
/sys/bus/mc2/devices/dimm0
/sys/bus/mc0/devices/dimm0
/sys/bus/mc7/devices/dimm0
/sys/bus/mc5/devices/dimm0
/sys/bus/mc3/devices/dimm0
/sys/bus/mc1/devices/dimm0
# ls -l /sys/bus/mc0/devices
total 0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 mc0 -> ../../../devices/system/edac/mc/mc0

It looks like the new code isn't trying to place the dimm symlinks
in the proper subdirectories.

-Tony

Re: [PATCH] Raise maximum number of memory controllers

[Russ Anderson]

On Wed, Sep 26, 2018 at 11:10:35AM -0700, Luck, Tony wrote:
> On Wed, Sep 26, 2018 at 06:17:49PM +0200, Borislav Petkov wrote:
> > On Wed, Sep 26, 2018 at 01:03:40PM -0300, Mauro Carvalho Chehab wrote:
> > > I guess this is/was needed to create things like this:
> > > 
> > > 	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc
> > 
> > They're still there:
> > 
> > $ ls -l /sys/bus/edac/devices/
> > total 0
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0
> 
> I ran into trouble on my 4 socket broadwell server (so 8 memory controllers,
> a whole pile of DIMMs, running from sb_edac.c)

We are also having trouble on a 32 socket system.

---------------------------------------------------------------------------------------------
[  OK  ] Started Load kdump kernel early on startup.
[   ***] (2 of 2) A start job is running for...work interfaces (18s / no limit)[  132.638611] BUG: unable to handle kernel paging request at ffff8c7efeebefff
[  132.640895] PGD 5fec3fdd067 P4D 5fec3fdd067 PUD 5fec3fda067 PMD 0 
[  132.640895] Oops: 0002 [#1] SMP PTI
[  132.640895] CPU: 650 PID: 9884 Comm: kworker/650:1 Kdump: loaded Tainted: G            E     4.19.0-rc4-ernstj+ #6
[  132.640895] Hardware name: HPE Superdome Flex/Superdome Flex, BIOS Bundle:3.0.196 SFW:IP147.007.000.071.000.1809242200 09/24/2018
[  132.640895] Workqueue: events cache_reap
[  132.640895] RIP: 0010:free_block+0x11c/0x1e0
[  132.640895] Code: ea 20 45 29 d7 41 d3 ef 0f b6 4f 1d 45 01 fa 41 d3 ea 8b 48 30 44 8d 79 ff 48 8b 48 20 44 89 78 30 48 85 c9 0f 84 a5 00 00 00 <46> 88 14 39 8b 48 30 85 c9 0f 84 32 ff ff ff 49 8b 49 10 4c 8d 50
[  132.640895] RSP: 0018:ffffc9004b0c7d90 EFLAGS: 00010086
[  132.640895] RAX: ffffea11f7fbafc0 RBX: 0000000000000002 RCX: ffff8c7dfeebf000
[  132.640895] RDX: 0000000000000005 RSI: ffff8c7e08da9328 RDI: ffff880147c02000
[  132.640895] RBP: 0000000080000000 R08: ffff8c5047c004a8 R09: ffff8c5047c00480
[  132.640895] R10: 0000000000000001 R11: ffff8c7dfeebf800 R12: ffffea0000000000
[  132.640895] R13: 000077ff80000000 R14: ffff8c5047c00488 R15: 00000000ffffffff
[  132.640895] FS:  0000000000000000(0000) GS:ffff8c7e08d80000(0000) knlGS:0000000000000000
[  132.640895] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  132.640895] CR2: ffff8c7efeebefff CR3: 000000000200a006 CR4: 00000000007606e0
[  132.640895] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  132.640895] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  132.640895] PKRU: 55555554
[  132.640895] Call Trace:
[  132.640895]  drain_array_locked+0x5b/0x80
[  132.640895]  drain_array+0x63/0x90
[  132.640895]  cache_reap+0x68/0x1f0
[  132.640895]  process_one_work+0x165/0x360
[  132.640895]  worker_thread+0x49/0x3e0
[  132.640895]  kthread+0xf8/0x130
[  132.640895]  ? max_active_store+0x60/0x60
[  132.640895]  ? kthread_bind+0x10/0x10
[  132.640895]  ret_from_fork+0x35/0x40
[  132.640895] Modules linked in: acpi_cpufreq(E-) skx_edac(E+) intel_rapl(E) nfit(E) libnvdimm(E) x86_pkg_temp_thermal(E) intel_powerclamp(E) coretemp(E) kvm_intel(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) pcbc(E) aesni_intel(E) aes_x86_64(E) iscsi_ibft(E) crypto_simd(E) iscsi_boot_sysfs(E) cryptd(E) glue_helper(E) pcspkr(E) nls_iso8859_1(E) nls_cp437(E) vfat(E) fat(E) joydev(E) i40e(E) ipmi_ssif(E) lpc_ich(E) i2c_i801(E) mfd_core(E) wmi(E) ipmi_si(E) ipmi_devintf(E) ipmi_msghandler(E) button(E) xfs(E) libcrc32c(E) hid_generic(E) usbhid(E) sd_mod(E) mgag200(E) i2c_algo_bit(E) drm_kms_helper(E) syscopyarea(E) crc32c_intel(E) sysfillrect(E) xhci_pci(E) sysimgblt(E) fb_sys_fops(E) xhci_hcd(E) ahci(E) libahci(E) ttm(E) drm(E) libata(E) usbcore(E) sg(E) dm_multipath(E)
[  132.916934]  dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E) msr(E) efivarfs(E) autofs4(E)
[  132.916934] CR2: ffff8c7efeebefff
[  132.916934] ---[ end trace 9ee1381bf4bae01f ]---
[  *** ] (2 of 2) A start job is running for.[  132.916934] RIP: 0010:free_block+0x11c/0x1e0
[  132.916934] Code: ea 20 45 29 d7 41 d3 ef 0f b6 4f 1d 45 01 fa 41 d3 ea 8b 48 30 44 8d 79 ff 48 8b 48 20 44 89 78 30 48 85 c9 0f 84 a5 00 00 00 <46> 88 14 39 8b 48 30 85 c9 0f 84 32 ff ff ff 49 8b 49 10 4c 8d 50
[  132.916934] RSP: 0018:ffffc9004b0c7d90 EFLAGS: 00010086
..work interface[  132.977236] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0: DEV 0000:80:0a.0
[  132.916934] RAX: ffffea11f7fbafc0 RBX: 0000000000000002 RCX: ffff8c7dfeebf000
[  132.916934] RDX: 0000000000000005 RSI: ffff8c7e08da9328 RDI: ffff880147c02000
s (19s / no limi[  133.004953] RBP: 0000000080000000 R08: ffff8c5047c004a8 R09: ffff8c5047c00480
[  133.004953] R10: 0000000000000001 R11: ffff8c7dfeebf800 R12: ffffea0000000000
[  133.004953] R13: 000077ff80000000 R14: ffff8c5047c00488 R15: 00000000ffffffff
[  133.004953] FS:  0000000000000000(0000) GS:ffff8c7e08d80000(0000) knlGS:0000000000000000
[  133.004953] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  133.004953] CR2: ffff8c7efeebefff CR3: 000000000200a006 CR4: 00000000007606e0
[  133.004953] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  133.004953] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  133.004953] PKRU: 55555554
[  133.004953] Kernel panic - not syncing: Fatal exception
---------------------------------------------------------------------------------------------

> Things start going wrong with:
> 
> [   45.216657] sysfs: cannot create duplicate filename '/bus/edac/devices/dimm0'
> [   45.216663] CPU: 37 PID: 2034 Comm: systemd-udevd Not tainted 4.19.0-rc5 #1
> [   45.216665] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRBDXSD1.86B.0338.V01.1603162127 03/16/2016
> [   45.216667] Call Trace:
> [   45.216688]  dump_stack+0x5c/0x7b
> [   45.216697]  sysfs_warn_dup+0x56/0x70
> [   45.216702]  sysfs_do_create_link_sd.isra.2+0x98/0xb0
> [   45.216714]  bus_add_device+0x77/0x160
> [   45.216720]  device_add+0x424/0x660
> [   45.216731]  edac_create_sysfs_mci_device+0xb9/0x2f0
> [   45.216738]  edac_mc_add_mc_with_groups+0x111/0x2b0
> [   45.216747]  sbridge_init+0x13c9/0x2000 [sb_edac]
> [   45.216757]  ? _raw_spin_lock+0x1d/0x20
> [   45.216765]  ? free_pcppages_bulk+0x2ca/0x630
> [   45.216769]  ? 0xffffffffc050f000
> [   45.216779]  do_one_initcall+0x46/0x1c8
> [   45.216784]  ? free_unref_page_commit+0x95/0x120
> [   45.216791]  ? _cond_resched+0x15/0x40
> [   45.216798]  ? kmem_cache_alloc_trace+0x153/0x1c0
> [   45.216805]  do_init_module+0x5b/0x208
> [   45.216826]  load_module+0x1a2d/0x1fb0
> [   45.216835]  ? __do_sys_finit_module+0xe9/0x110
> [   45.216840]  __do_sys_finit_module+0xe9/0x110
> [   45.216847]  do_syscall_64+0x5b/0x180
> [   45.216852]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   45.216856] RIP: 0033:0x7fcdec618bd9
> 
> and fell off a cliff after that.
> 
> Going back to the old code I have a "dimm0" on each of the eight controllers:
> 
> # find /sys -name dimm0
> /sys/devices/system/edac/mc/mc6/dimm0
> /sys/devices/system/edac/mc/mc4/dimm0
> /sys/devices/system/edac/mc/mc2/dimm0
> /sys/devices/system/edac/mc/mc0/dimm0
> /sys/devices/system/edac/mc/mc7/dimm0
> /sys/devices/system/edac/mc/mc5/dimm0
> /sys/devices/system/edac/mc/mc3/dimm0
> /sys/devices/system/edac/mc/mc1/dimm0
> /sys/bus/mc6/devices/dimm0
> /sys/bus/mc4/devices/dimm0
> /sys/bus/mc2/devices/dimm0
> /sys/bus/mc0/devices/dimm0
> /sys/bus/mc7/devices/dimm0
> /sys/bus/mc5/devices/dimm0
> /sys/bus/mc3/devices/dimm0
> /sys/bus/mc1/devices/dimm0
> # ls -l /sys/bus/mc0/devices
> total 0
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 mc0 -> ../../../devices/system/edac/mc/mc0
> 
> It looks like the new code isn't trying to place the dimm symlinks
> in the proper subdirectories.
> 
> -Tony

-- 
Russ Anderson,  SuperDome Flex Linux Kernel Group Manager
HPE - Hewlett Packard Enterprise (formerly SGI)  rja@hpe.com

Raise maximum number of memory controllers

[Russ Anderson]

On Wed, Sep 26, 2018 at 11:10:35AM -0700, Luck, Tony wrote:
> On Wed, Sep 26, 2018 at 06:17:49PM +0200, Borislav Petkov wrote:
> > On Wed, Sep 26, 2018 at 01:03:40PM -0300, Mauro Carvalho Chehab wrote:
> > > I guess this is/was needed to create things like this:
> > > 
> > > 	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc
> > 
> > They're still there:
> > 
> > $ ls -l /sys/bus/edac/devices/
> > total 0
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
> > lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0
> 
> I ran into trouble on my 4 socket broadwell server (so 8 memory controllers,
> a whole pile of DIMMs, running from sb_edac.c)

We are also having trouble on a 32 socket system.

---------------------------------------------------------------------------------------------
[  OK  ] Started Load kdump kernel early on startup.
[   ***] (2 of 2) A start job is running for...work interfaces (18s / no limit)[  132.638611] BUG: unable to handle kernel paging request at ffff8c7efeebefff
[  132.640895] PGD 5fec3fdd067 P4D 5fec3fdd067 PUD 5fec3fda067 PMD 0 
[  132.640895] Oops: 0002 [#1] SMP PTI
[  132.640895] CPU: 650 PID: 9884 Comm: kworker/650:1 Kdump: loaded Tainted: G            E     4.19.0-rc4-ernstj+ #6
[  132.640895] Hardware name: HPE Superdome Flex/Superdome Flex, BIOS Bundle:3.0.196 SFW:IP147.007.000.071.000.1809242200 09/24/2018
[  132.640895] Workqueue: events cache_reap
[  132.640895] RIP: 0010:free_block+0x11c/0x1e0
[  132.640895] Code: ea 20 45 29 d7 41 d3 ef 0f b6 4f 1d 45 01 fa 41 d3 ea 8b 48 30 44 8d 79 ff 48 8b 48 20 44 89 78 30 48 85 c9 0f 84 a5 00 00 00 <46> 88 14 39 8b 48 30 85 c9 0f 84 32 ff ff ff 49 8b 49 10 4c 8d 50
[  132.640895] RSP: 0018:ffffc9004b0c7d90 EFLAGS: 00010086
[  132.640895] RAX: ffffea11f7fbafc0 RBX: 0000000000000002 RCX: ffff8c7dfeebf000
[  132.640895] RDX: 0000000000000005 RSI: ffff8c7e08da9328 RDI: ffff880147c02000
[  132.640895] RBP: 0000000080000000 R08: ffff8c5047c004a8 R09: ffff8c5047c00480
[  132.640895] R10: 0000000000000001 R11: ffff8c7dfeebf800 R12: ffffea0000000000
[  132.640895] R13: 000077ff80000000 R14: ffff8c5047c00488 R15: 00000000ffffffff
[  132.640895] FS:  0000000000000000(0000) GS:ffff8c7e08d80000(0000) knlGS:0000000000000000
[  132.640895] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  132.640895] CR2: ffff8c7efeebefff CR3: 000000000200a006 CR4: 00000000007606e0
[  132.640895] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  132.640895] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  132.640895] PKRU: 55555554
[  132.640895] Call Trace:
[  132.640895]  drain_array_locked+0x5b/0x80
[  132.640895]  drain_array+0x63/0x90
[  132.640895]  cache_reap+0x68/0x1f0
[  132.640895]  process_one_work+0x165/0x360
[  132.640895]  worker_thread+0x49/0x3e0
[  132.640895]  kthread+0xf8/0x130
[  132.640895]  ? max_active_store+0x60/0x60
[  132.640895]  ? kthread_bind+0x10/0x10
[  132.640895]  ret_from_fork+0x35/0x40
[  132.640895] Modules linked in: acpi_cpufreq(E-) skx_edac(E+) intel_rapl(E) nfit(E) libnvdimm(E) x86_pkg_temp_thermal(E) intel_powerclamp(E) coretemp(E) kvm_intel(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) pcbc(E) aesni_intel(E) aes_x86_64(E) iscsi_ibft(E) crypto_simd(E) iscsi_boot_sysfs(E) cryptd(E) glue_helper(E) pcspkr(E) nls_iso8859_1(E) nls_cp437(E) vfat(E) fat(E) joydev(E) i40e(E) ipmi_ssif(E) lpc_ich(E) i2c_i801(E) mfd_core(E) wmi(E) ipmi_si(E) ipmi_devintf(E) ipmi_msghandler(E) button(E) xfs(E) libcrc32c(E) hid_generic(E) usbhid(E) sd_mod(E) mgag200(E) i2c_algo_bit(E) drm_kms_helper(E) syscopyarea(E) crc32c_intel(E) sysfillrect(E) xhci_pci(E) sysimgblt(E) fb_sys_fops(E) xhci_hcd(E) ahci(E) libahci(E) ttm(E) drm(E) libata(E) usbcore(E) sg(E) dm_multipath(E)
[  132.916934]  dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E) msr(E) efivarfs(E) autofs4(E)
[  132.916934] CR2: ffff8c7efeebefff
[  132.916934] ---[ end trace 9ee1381bf4bae01f ]---
[  *** ] (2 of 2) A start job is running for.[  132.916934] RIP: 0010:free_block+0x11c/0x1e0
[  132.916934] Code: ea 20 45 29 d7 41 d3 ef 0f b6 4f 1d 45 01 fa 41 d3 ea 8b 48 30 44 8d 79 ff 48 8b 48 20 44 89 78 30 48 85 c9 0f 84 a5 00 00 00 <46> 88 14 39 8b 48 30 85 c9 0f 84 32 ff ff ff 49 8b 49 10 4c 8d 50
[  132.916934] RSP: 0018:ffffc9004b0c7d90 EFLAGS: 00010086
..work interface[  132.977236] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0: DEV 0000:80:0a.0
[  132.916934] RAX: ffffea11f7fbafc0 RBX: 0000000000000002 RCX: ffff8c7dfeebf000
[  132.916934] RDX: 0000000000000005 RSI: ffff8c7e08da9328 RDI: ffff880147c02000
s (19s / no limi[  133.004953] RBP: 0000000080000000 R08: ffff8c5047c004a8 R09: ffff8c5047c00480
[  133.004953] R10: 0000000000000001 R11: ffff8c7dfeebf800 R12: ffffea0000000000
[  133.004953] R13: 000077ff80000000 R14: ffff8c5047c00488 R15: 00000000ffffffff
[  133.004953] FS:  0000000000000000(0000) GS:ffff8c7e08d80000(0000) knlGS:0000000000000000
[  133.004953] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  133.004953] CR2: ffff8c7efeebefff CR3: 000000000200a006 CR4: 00000000007606e0
[  133.004953] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  133.004953] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  133.004953] PKRU: 55555554
[  133.004953] Kernel panic - not syncing: Fatal exception
---------------------------------------------------------------------------------------------

> Things start going wrong with:
> 
> [   45.216657] sysfs: cannot create duplicate filename '/bus/edac/devices/dimm0'
> [   45.216663] CPU: 37 PID: 2034 Comm: systemd-udevd Not tainted 4.19.0-rc5 #1
> [   45.216665] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRBDXSD1.86B.0338.V01.1603162127 03/16/2016
> [   45.216667] Call Trace:
> [   45.216688]  dump_stack+0x5c/0x7b
> [   45.216697]  sysfs_warn_dup+0x56/0x70
> [   45.216702]  sysfs_do_create_link_sd.isra.2+0x98/0xb0
> [   45.216714]  bus_add_device+0x77/0x160
> [   45.216720]  device_add+0x424/0x660
> [   45.216731]  edac_create_sysfs_mci_device+0xb9/0x2f0
> [   45.216738]  edac_mc_add_mc_with_groups+0x111/0x2b0
> [   45.216747]  sbridge_init+0x13c9/0x2000 [sb_edac]
> [   45.216757]  ? _raw_spin_lock+0x1d/0x20
> [   45.216765]  ? free_pcppages_bulk+0x2ca/0x630
> [   45.216769]  ? 0xffffffffc050f000
> [   45.216779]  do_one_initcall+0x46/0x1c8
> [   45.216784]  ? free_unref_page_commit+0x95/0x120
> [   45.216791]  ? _cond_resched+0x15/0x40
> [   45.216798]  ? kmem_cache_alloc_trace+0x153/0x1c0
> [   45.216805]  do_init_module+0x5b/0x208
> [   45.216826]  load_module+0x1a2d/0x1fb0
> [   45.216835]  ? __do_sys_finit_module+0xe9/0x110
> [   45.216840]  __do_sys_finit_module+0xe9/0x110
> [   45.216847]  do_syscall_64+0x5b/0x180
> [   45.216852]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   45.216856] RIP: 0033:0x7fcdec618bd9
> 
> and fell off a cliff after that.
> 
> Going back to the old code I have a "dimm0" on each of the eight controllers:
> 
> # find /sys -name dimm0
> /sys/devices/system/edac/mc/mc6/dimm0
> /sys/devices/system/edac/mc/mc4/dimm0
> /sys/devices/system/edac/mc/mc2/dimm0
> /sys/devices/system/edac/mc/mc0/dimm0
> /sys/devices/system/edac/mc/mc7/dimm0
> /sys/devices/system/edac/mc/mc5/dimm0
> /sys/devices/system/edac/mc/mc3/dimm0
> /sys/devices/system/edac/mc/mc1/dimm0
> /sys/bus/mc6/devices/dimm0
> /sys/bus/mc4/devices/dimm0
> /sys/bus/mc2/devices/dimm0
> /sys/bus/mc0/devices/dimm0
> /sys/bus/mc7/devices/dimm0
> /sys/bus/mc5/devices/dimm0
> /sys/bus/mc3/devices/dimm0
> /sys/bus/mc1/devices/dimm0
> # ls -l /sys/bus/mc0/devices
> total 0
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
> lrwxrwxrwx. 1 root root 0 Sep 26 11:08 mc0 -> ../../../devices/system/edac/mc/mc0
> 
> It looks like the new code isn't trying to place the dimm symlinks
> in the proper subdirectories.
> 
> -Tony

Re: [PATCH] Raise maximum number of memory controllers

[Luck, Tony]

This issue has made me look a bit more at what EDAC puts in sysfs.
It seems like the current code inherits some useless baggage
from the device calls it makes.

E.g. all the "power" subdirectories:

$ find /sys/devices/system/edac -name power
/sys/devices/system/edac/power
/sys/devices/system/edac/mc/mc6/dimm3/power
/sys/devices/system/edac/mc/mc6/power
/sys/devices/system/edac/mc/mc6/csrow0/power
/sys/devices/system/edac/mc/mc6/dimm6/power
/sys/devices/system/edac/mc/mc6/dimm0/power
/sys/devices/system/edac/mc/mc6/dimm9/power
/sys/devices/system/edac/mc/mc4/dimm3/power
/sys/devices/system/edac/mc/mc4/power
... total of 50 of these ...

$ grep -r . /sys/devices/system/edac/mc/mc6/dimm0/power
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_active_time:0
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_status:unsupported
grep: /sys/devices/system/edac/mc/mc6/dimm0/power/autosuspend_delay_ms: Input/output error
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_suspended_time:0
/sys/devices/system/edac/mc/mc6/dimm0/power/control:auto

We don't have stats, nor control of power on a per memory controller
or per dimm basis. So all these files are just noise.


But ... we are at -rc5. Not sure that we'll figure out, write, test & debug
the proper solution in the next 3-4 weeks. So perhaps we should apply

-#define EDAC_MAX_MCS   16
+#define EDAC_MAX_MCS   64

as a temporary band-aid to get HPE's 32-socket machine running while
we work on the proper fix?

-Tony

Raise maximum number of memory controllers

[Luck, Tony]

This issue has made me look a bit more at what EDAC puts in sysfs.
It seems like the current code inherits some useless baggage
from the device calls it makes.

E.g. all the "power" subdirectories:

$ find /sys/devices/system/edac -name power
/sys/devices/system/edac/power
/sys/devices/system/edac/mc/mc6/dimm3/power
/sys/devices/system/edac/mc/mc6/power
/sys/devices/system/edac/mc/mc6/csrow0/power
/sys/devices/system/edac/mc/mc6/dimm6/power
/sys/devices/system/edac/mc/mc6/dimm0/power
/sys/devices/system/edac/mc/mc6/dimm9/power
/sys/devices/system/edac/mc/mc4/dimm3/power
/sys/devices/system/edac/mc/mc4/power
... total of 50 of these ...

$ grep -r . /sys/devices/system/edac/mc/mc6/dimm0/power
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_active_time:0
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_status:unsupported
grep: /sys/devices/system/edac/mc/mc6/dimm0/power/autosuspend_delay_ms: Input/output error
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_suspended_time:0
/sys/devices/system/edac/mc/mc6/dimm0/power/control:auto

We don't have stats, nor control of power on a per memory controller
or per dimm basis. So all these files are just noise.


But ... we are at -rc5. Not sure that we'll figure out, write, test & debug
the proper solution in the next 3-4 weeks. So perhaps we should apply

-#define EDAC_MAX_MCS   16
+#define EDAC_MAX_MCS   64

as a temporary band-aid to get HPE's 32-socket machine running while
we work on the proper fix?

-Tony

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Wed, Sep 26, 2018 at 04:02:57PM -0700, Luck, Tony wrote:
> We don't have stats, nor control of power on a per memory controller
> or per dimm basis. So all these files are just noise.

Yeah, and also, looking at your previous mail, stuff like:

/sys/bus/mc6/devices/dimm0
/sys/bus/mc4/devices/dimm0

doesn't make any sense: why is mc* directly under bus? It should be
under ...bus/edac/mc/...

We'll have to clean it up carefully, when there's time.

> But ... we are at -rc5. Not sure that we'll figure out, write, test & debug
> the proper solution in the next 3-4 weeks. So perhaps we should apply
> 
> -#define EDAC_MAX_MCS   16
> +#define EDAC_MAX_MCS   64
> 
> as a temporary band-aid to get HPE's 32-socket machine running while
> we work on the proper fix?

Yeah, after sleeping on it I see it the same way - band-aid it now and
clean it up properly later.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Wed, Sep 26, 2018 at 04:02:57PM -0700, Luck, Tony wrote:
> We don't have stats, nor control of power on a per memory controller
> or per dimm basis. So all these files are just noise.

Yeah, and also, looking at your previous mail, stuff like:

/sys/bus/mc6/devices/dimm0
/sys/bus/mc4/devices/dimm0

doesn't make any sense: why is mc* directly under bus? It should be
under ...bus/edac/mc/...

We'll have to clean it up carefully, when there's time.

> But ... we are at -rc5. Not sure that we'll figure out, write, test & debug
> the proper solution in the next 3-4 weeks. So perhaps we should apply
> 
> -#define EDAC_MAX_MCS   16
> +#define EDAC_MAX_MCS   64
> 
> as a temporary band-aid to get HPE's 32-socket machine running while
> we work on the proper fix?

Yeah, after sleeping on it I see it the same way - band-aid it now and
clean it up properly later.

Thx.

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Tue, Sep 25, 2018 at 09:34:49AM -0500, Justin Ernst wrote:
> We observe an oops in the skx_edac module during boot.
> Examining /var/log/messages:
> [ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
> [ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> [ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> ...
> [ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> [ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
> [ 3401.987579] Too many memory controllers: 16
> [ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0
> 
> We observe there are two memory controllers per socket, with a limit of 16.
> Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Russ Anderson <russ.anderson@hpe.com>
> Signed-off-by: Justin Ernst <justin.ernst@hpe.com>
> ---
>  include/linux/edac.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Tue, Sep 25, 2018 at 09:34:49AM -0500, Justin Ernst wrote:
> We observe an oops in the skx_edac module during boot.
> Examining /var/log/messages:
> [ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
> [ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> [ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> ...
> [ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
> [ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
> [ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
> [ 3401.987579] Too many memory controllers: 16
> [ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0
> 
> We observe there are two memory controllers per socket, with a limit of 16.
> Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Russ Anderson <russ.anderson@hpe.com>
> Signed-off-by: Justin Ernst <justin.ernst@hpe.com>
> ---
>  include/linux/edac.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Applied, thanks.

Re: [PATCH] Raise maximum number of memory controllers

[Luck, Tony]

On Thu, Sep 27, 2018 at 06:52:44AM +0200, Borislav Petkov wrote:
> On Wed, Sep 26, 2018 at 04:02:57PM -0700, Luck, Tony wrote:
> > But ... we are at -rc5. Not sure that we'll figure out, write, test & debug
> > the proper solution in the next 3-4 weeks. So perhaps we should apply
> > 
> > -#define EDAC_MAX_MCS   16
> > +#define EDAC_MAX_MCS   64
> > 
> > as a temporary band-aid to get HPE's 32-socket machine running while
> > we work on the proper fix?
> 
> Yeah, after sleeping on it I see it the same way - band-aid it now and
> clean it up properly later.

The problem with your patch that gets rid of EDAC_MAX_MCS is making
device links under /sys/bus/edac.  Which is hinted at in some of the
code your patch deleted:

-       /*
-        * The memory controller needs its own bus, in order to avoid
-        * namespace conflicts at /sys/bus/edac.
-        */
-       name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
-       if (!name)
-               return -ENOMEM;
-
-       mci->bus->name = name;
-
-       edac_dbg(0, "creating bus %s\n", mci->bus->name);
-
-       err = bus_register(mci->bus);

Just to see if there was anything else wrong I added a patch to
make the names unique:


diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 2ca2012f2857..6ec6d8a2adb8 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -410,7 +410,7 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;
 	csrow->mci = mci;
-	dev_set_name(&csrow->dev, "csrow%d", index);
+	dev_set_name(&csrow->dev, "mci%d_csrow%d", mci->mc_idx, index);
 	dev_set_drvdata(&csrow->dev, csrow);
 
 	edac_dbg(0, "creating (virtual) csrow node %s\n",
@@ -641,9 +641,9 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 
 	dimm->dev.parent = &mci->dev;
 	if (mci->csbased)
-		dev_set_name(&dimm->dev, "rank%d", index);
+		dev_set_name(&dimm->dev, "mci%d_rank%d", mci->mc_idx, index);
 	else
-		dev_set_name(&dimm->dev, "dimm%d", index);
+		dev_set_name(&dimm->dev, "mci%d_dimm%d", mci->mc_idx, index);
 	dev_set_drvdata(&dimm->dev, dimm);
 	pm_runtime_forbid(&mci->dev);
 

which seemed to work.  But then I began wondering what are ABI expectations
from applications that read the EDAC /sys files?

Is this this current source repository?  https://github.com/grondo/edac-utils

This code doesn't seem to know about the "dimm*" directories below the
"mc*" level. It just looks for the csrow* entries.

-Tony

Raise maximum number of memory controllers

[Luck, Tony]

On Thu, Sep 27, 2018 at 06:52:44AM +0200, Borislav Petkov wrote:
> On Wed, Sep 26, 2018 at 04:02:57PM -0700, Luck, Tony wrote:
> > But ... we are at -rc5. Not sure that we'll figure out, write, test & debug
> > the proper solution in the next 3-4 weeks. So perhaps we should apply
> > 
> > -#define EDAC_MAX_MCS   16
> > +#define EDAC_MAX_MCS   64
> > 
> > as a temporary band-aid to get HPE's 32-socket machine running while
> > we work on the proper fix?
> 
> Yeah, after sleeping on it I see it the same way - band-aid it now and
> clean it up properly later.

The problem with your patch that gets rid of EDAC_MAX_MCS is making
device links under /sys/bus/edac.  Which is hinted at in some of the
code your patch deleted:

-       /*
-        * The memory controller needs its own bus, in order to avoid
-        * namespace conflicts at /sys/bus/edac.
-        */
-       name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
-       if (!name)
-               return -ENOMEM;
-
-       mci->bus->name = name;
-
-       edac_dbg(0, "creating bus %s\n", mci->bus->name);
-
-       err = bus_register(mci->bus);

Just to see if there was anything else wrong I added a patch to
make the names unique:



which seemed to work.  But then I began wondering what are ABI expectations
from applications that read the EDAC /sys files?

Is this this current source repository?  https://github.com/grondo/edac-utils

This code doesn't seem to know about the "dimm*" directories below the
"mc*" level. It just looks for the csrow* entries.

-Tony

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 2ca2012f2857..6ec6d8a2adb8 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -410,7 +410,7 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;
 	csrow->mci = mci;
-	dev_set_name(&csrow->dev, "csrow%d", index);
+	dev_set_name(&csrow->dev, "mci%d_csrow%d", mci->mc_idx, index);
 	dev_set_drvdata(&csrow->dev, csrow);
 
 	edac_dbg(0, "creating (virtual) csrow node %s\n",
@@ -641,9 +641,9 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 
 	dimm->dev.parent = &mci->dev;
 	if (mci->csbased)
-		dev_set_name(&dimm->dev, "rank%d", index);
+		dev_set_name(&dimm->dev, "mci%d_rank%d", mci->mc_idx, index);
 	else
-		dev_set_name(&dimm->dev, "dimm%d", index);
+		dev_set_name(&dimm->dev, "mci%d_dimm%d", mci->mc_idx, index);
 	dev_set_drvdata(&dimm->dev, dimm);
 	pm_runtime_forbid(&mci->dev);
 

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Thu, Sep 27, 2018 at 02:44:01PM -0700, Luck, Tony wrote:
> The problem with your patch that gets rid of EDAC_MAX_MCS is making
> device links under /sys/bus/edac.  Which is hinted at in some of the
> code your patch deleted:
> 
> -       /*
> -        * The memory controller needs its own bus, in order to avoid
> -        * namespace conflicts at /sys/bus/edac.
> -        */
> -       name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
> -       if (!name)
> -               return -ENOMEM;
> -
> -       mci->bus->name = name;

Yes, and that needed to go because I am using a single bus. Which kinda
makes sense because you want to have a single bus and multiple devices
on it. I mean, if we *have* to have a bus.

I think this whole /sys/bus/edac thing is crap and needs to go. We
have a perfectly fine hierarchy under /sys/devices/system/edac and
duplicating it under /sys/bus/edac is just bollocks. IMHO. Feel free to
correct me with, but but, this is useful for...

> which seemed to work.

Right.

> But then I began wondering what are ABI expectations
> from applications that read the EDAC /sys files?
> 
> Is this this current source repository?  https://github.com/grondo/edac-utils
> 
> This code doesn't seem to know about the "dimm*" directories below the
> "mc*" level. It just looks for the csrow* entries.

I guess this is a question for Mauro. I never really needed any special
edac tool to get info and if you ask me, we probably should try to keep
it simple and grep sysfs. So that you can always get the info without
having to install any special tools. Like ftrace works on every system
with just a shell and basic tools. I think this is very powerful. But
this is old spartan me only thinking out loud.

In any case, I'm more than fine with dropping the bus hierarchy if
nothing uses it.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Thu, Sep 27, 2018 at 02:44:01PM -0700, Luck, Tony wrote:
> The problem with your patch that gets rid of EDAC_MAX_MCS is making
> device links under /sys/bus/edac.  Which is hinted at in some of the
> code your patch deleted:
> 
> -       /*
> -        * The memory controller needs its own bus, in order to avoid
> -        * namespace conflicts at /sys/bus/edac.
> -        */
> -       name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
> -       if (!name)
> -               return -ENOMEM;
> -
> -       mci->bus->name = name;

Yes, and that needed to go because I am using a single bus. Which kinda
makes sense because you want to have a single bus and multiple devices
on it. I mean, if we *have* to have a bus.

I think this whole /sys/bus/edac thing is crap and needs to go. We
have a perfectly fine hierarchy under /sys/devices/system/edac and
duplicating it under /sys/bus/edac is just bollocks. IMHO. Feel free to
correct me with, but but, this is useful for...

> which seemed to work.

Right.

> But then I began wondering what are ABI expectations
> from applications that read the EDAC /sys files?
> 
> Is this this current source repository?  https://github.com/grondo/edac-utils
> 
> This code doesn't seem to know about the "dimm*" directories below the
> "mc*" level. It just looks for the csrow* entries.

I guess this is a question for Mauro. I never really needed any special
edac tool to get info and if you ask me, we probably should try to keep
it simple and grep sysfs. So that you can always get the info without
having to install any special tools. Like ftrace works on every system
with just a shell and basic tools. I think this is very powerful. But
this is old spartan me only thinking out loud.

In any case, I'm more than fine with dropping the bus hierarchy if
nothing uses it.

Thx.

Re: [PATCH] Raise maximum number of memory controllers

[Mauro Carvalho Chehab]

Em Fri, 28 Sep 2018 00:03:55 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Thu, Sep 27, 2018 at 02:44:01PM -0700, Luck, Tony wrote:
> > The problem with your patch that gets rid of EDAC_MAX_MCS is making
> > device links under /sys/bus/edac.  Which is hinted at in some of the
> > code your patch deleted:
> > 
> > -       /*
> > -        * The memory controller needs its own bus, in order to avoid
> > -        * namespace conflicts at /sys/bus/edac.
> > -        */
> > -       name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
> > -       if (!name)
> > -               return -ENOMEM;
> > -
> > -       mci->bus->name = name;  
> 
> Yes, and that needed to go because I am using a single bus. Which kinda
> makes sense because you want to have a single bus and multiple devices
> on it. I mean, if we *have* to have a bus.
> 
> I think this whole /sys/bus/edac thing is crap and needs to go. We
> have a perfectly fine hierarchy under /sys/devices/system/edac and
> duplicating it under /sys/bus/edac is just bollocks. IMHO. Feel free to
> correct me with, but but, this is useful for...
> 
> > which seemed to work.  
> 
> Right.
> 
> > But then I began wondering what are ABI expectations
> > from applications that read the EDAC /sys files?
> > 
> > Is this this current source repository?  https://github.com/grondo/edac-utils
> > 
> > This code doesn't seem to know about the "dimm*" directories below the
> > "mc*" level. It just looks for the csrow* entries.  
> 
> I guess this is a question for Mauro. I never really needed any special
> edac tool to get info and if you ask me, we probably should try to keep
> it simple and grep sysfs. So that you can always get the info without
> having to install any special tools. Like ftrace works on every system
> with just a shell and basic tools. I think this is very powerful. But
> this is old spartan me only thinking out loud.
> 
> In any case, I'm more than fine with dropping the bus hierarchy if
> nothing uses it.

I don't remember about any rationale behind /sys/bus/edac. It was
there already before I start working on EDAC about 10 years ago.
I guess it was used in the past by edac-utils (or maybe it is just a
side effect of the need to create a bus on some past).

Btw, The documented EDAC ABI is /sys/devices/system/edac, as
described at Documentation/ABI/testing/sysfs-devices-edac.

So, I suspect it should be safe to get rid of /sys/bus/edac,
provided that it won't cause side effects at /sys/devices/system/edac.

Why I think it is safe to get rid of /sys/bus/edac?
---------------------------------------------------

As far as I can tell, there are only two toolsets: the legacy edac-utils
and the rasdaemon. At least on Fedora 28, both applications are 
packaged (meaning that there are probably people using both).

The edac-utils uses the old sysfs entries (the ones whose entries
are dated up to 2007). I don't see any changes upstream for it
since 2008:

	https://sourceforge.net/projects/edac-utils/

I did a grep on its source code (on its version 0.16, from 2018). It seems
that it uses only /sys/devices/system/edac.

The rasdaemon uses also the new sysfs entries (the ones dated as
2012 and 2016). I'm maintaining it. Rastool not only receive traces,
but it can also store them on a database and even generate ABRT events.
It also uses only /sys/devices/system/edac.

On both toolsets, the sysfs entries there are important, in order to 
not only list the memory layout and error counts, but also to store
the dimm labels.

The rasdaemon itself uses perf trace events, although Aris added
support for it to work on non-daemon mode, where it just reads
the counters via sysfs, at /sys/devices/system/edac.

Thanks,
Mauro

Raise maximum number of memory controllers

[Mauro Carvalho Chehab]

Em Fri, 28 Sep 2018 00:03:55 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Thu, Sep 27, 2018 at 02:44:01PM -0700, Luck, Tony wrote:
> > The problem with your patch that gets rid of EDAC_MAX_MCS is making
> > device links under /sys/bus/edac.  Which is hinted at in some of the
> > code your patch deleted:
> > 
> > -       /*
> > -        * The memory controller needs its own bus, in order to avoid
> > -        * namespace conflicts at /sys/bus/edac.
> > -        */
> > -       name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
> > -       if (!name)
> > -               return -ENOMEM;
> > -
> > -       mci->bus->name = name;  
> 
> Yes, and that needed to go because I am using a single bus. Which kinda
> makes sense because you want to have a single bus and multiple devices
> on it. I mean, if we *have* to have a bus.
> 
> I think this whole /sys/bus/edac thing is crap and needs to go. We
> have a perfectly fine hierarchy under /sys/devices/system/edac and
> duplicating it under /sys/bus/edac is just bollocks. IMHO. Feel free to
> correct me with, but but, this is useful for...
> 
> > which seemed to work.  
> 
> Right.
> 
> > But then I began wondering what are ABI expectations
> > from applications that read the EDAC /sys files?
> > 
> > Is this this current source repository?  https://github.com/grondo/edac-utils
> > 
> > This code doesn't seem to know about the "dimm*" directories below the
> > "mc*" level. It just looks for the csrow* entries.  
> 
> I guess this is a question for Mauro. I never really needed any special
> edac tool to get info and if you ask me, we probably should try to keep
> it simple and grep sysfs. So that you can always get the info without
> having to install any special tools. Like ftrace works on every system
> with just a shell and basic tools. I think this is very powerful. But
> this is old spartan me only thinking out loud.
> 
> In any case, I'm more than fine with dropping the bus hierarchy if
> nothing uses it.

I don't remember about any rationale behind /sys/bus/edac. It was
there already before I start working on EDAC about 10 years ago.
I guess it was used in the past by edac-utils (or maybe it is just a
side effect of the need to create a bus on some past).

Btw, The documented EDAC ABI is /sys/devices/system/edac, as
described at Documentation/ABI/testing/sysfs-devices-edac.

So, I suspect it should be safe to get rid of /sys/bus/edac,
provided that it won't cause side effects at /sys/devices/system/edac.

Why I think it is safe to get rid of /sys/bus/edac?
---------------------------------------------------

As far as I can tell, there are only two toolsets: the legacy edac-utils
and the rasdaemon. At least on Fedora 28, both applications are 
packaged (meaning that there are probably people using both).

The edac-utils uses the old sysfs entries (the ones whose entries
are dated up to 2007). I don't see any changes upstream for it
since 2008:

	https://sourceforge.net/projects/edac-utils/

I did a grep on its source code (on its version 0.16, from 2018). It seems
that it uses only /sys/devices/system/edac.

The rasdaemon uses also the new sysfs entries (the ones dated as
2012 and 2016). I'm maintaining it. Rastool not only receive traces,
but it can also store them on a database and even generate ABRT events.
It also uses only /sys/devices/system/edac.

On both toolsets, the sysfs entries there are important, in order to 
not only list the memory layout and error counts, but also to store
the dimm labels.

The rasdaemon itself uses perf trace events, although Aris added
support for it to work on non-daemon mode, where it just reads
the counters via sysfs, at /sys/devices/system/edac.

Thanks,
Mauro

Re: [PATCH] Raise maximum number of memory controllers

[Borislav Petkov]

On Thu, Sep 27, 2018 at 10:10:54PM -0300, Mauro Carvalho Chehab wrote:
> I don't remember about any rationale behind /sys/bus/edac. It was
> there already before I start working on EDAC about 10 years ago.
> I guess it was used in the past by edac-utils (or maybe it is just a
> side effect of the need to create a bus on some past).
> 
> Btw, The documented EDAC ABI is /sys/devices/system/edac, as
> described at Documentation/ABI/testing/sysfs-devices-edac.
> 
> So, I suspect it should be safe to get rid of /sys/bus/edac,
> provided that it won't cause side effects at /sys/devices/system/edac.
> 
> Why I think it is safe to get rid of /sys/bus/edac?
> ---------------------------------------------------

...

Thanks for the analysis. So yap, I think we should try to rip out the
whole bus hierarchy then, when we have a quiet minute and whoever does
this, should add your analysis to the commit message so that we know.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Raise maximum number of memory controllers

[Borislav Petkov]

On Thu, Sep 27, 2018 at 10:10:54PM -0300, Mauro Carvalho Chehab wrote:
> I don't remember about any rationale behind /sys/bus/edac. It was
> there already before I start working on EDAC about 10 years ago.
> I guess it was used in the past by edac-utils (or maybe it is just a
> side effect of the need to create a bus on some past).
> 
> Btw, The documented EDAC ABI is /sys/devices/system/edac, as
> described at Documentation/ABI/testing/sysfs-devices-edac.
> 
> So, I suspect it should be safe to get rid of /sys/bus/edac,
> provided that it won't cause side effects at /sys/devices/system/edac.
> 
> Why I think it is safe to get rid of /sys/bus/edac?
> ---------------------------------------------------

...

Thanks for the analysis. So yap, I think we should try to rip out the
whole bus hierarchy then, when we have a quiet minute and whoever does
this, should add your analysis to the commit message so that we know.

Thx.

[PATCH] EDAC: Don't add devices under /sys/bus/edac

[Luck, Tony]

Nobody(*) uses them.  Dropping this will allow us to make the total
number of memory controllers configurable (as we won't have to
worry about duplicated device names under this directory).

(*) https://marc.info/?l=linux-edac&m=153809709903987&w=2

Signed-off-by: Tony Luck <tony.luck@intel.com>
---

Boris: Apply this, then your earlier patch to get rid of the
hard coded limit on the number of memory controllers:
  https://marc.info/?l=linux-edac&m=153797567628947&w=2
the combination works on my 4 socket machine. Perhaps HPE
can test on their superdome.

 drivers/edac/edac_mc_sysfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 20374b8248f0..4c1bee59c2e6 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 				    struct csrow_info *csrow, int index)
 {
 	csrow->dev.type = &csrow_attr_type;
-	csrow->dev.bus = mci->bus;
 	csrow->dev.groups = csrow_dev_groups;
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;
@@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 	dimm->mci = mci;
 
 	dimm->dev.type = &dimm_attr_type;
-	dimm->dev.bus = mci->bus;
 	device_initialize(&dimm->dev);
 
 	dimm->dev.parent = &mci->dev;
@@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	device_initialize(&mci->dev);
 
 	mci->dev.parent = mci_pdev;
-	mci->dev.bus = mci->bus;
 	mci->dev.groups = groups;
 	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
 	dev_set_drvdata(&mci->dev, mci);
-- 
2.17.1

EDAC: Don't add devices under /sys/bus/edac

[Luck, Tony]

Nobody(*) uses them.  Dropping this will allow us to make the total
number of memory controllers configurable (as we won't have to
worry about duplicated device names under this directory).

(*) https://marc.info/?l=linux-edac&m=153809709903987&w=2

Signed-off-by: Tony Luck <tony.luck@intel.com>
---

Boris: Apply this, then your earlier patch to get rid of the
hard coded limit on the number of memory controllers:
  https://marc.info/?l=linux-edac&m=153797567628947&w=2
the combination works on my 4 socket machine. Perhaps HPE
can test on their superdome.

 drivers/edac/edac_mc_sysfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 20374b8248f0..4c1bee59c2e6 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 				    struct csrow_info *csrow, int index)
 {
 	csrow->dev.type = &csrow_attr_type;
-	csrow->dev.bus = mci->bus;
 	csrow->dev.groups = csrow_dev_groups;
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;
@@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 	dimm->mci = mci;
 
 	dimm->dev.type = &dimm_attr_type;
-	dimm->dev.bus = mci->bus;
 	device_initialize(&dimm->dev);
 
 	dimm->dev.parent = &mci->dev;
@@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	device_initialize(&mci->dev);
 
 	mci->dev.parent = mci_pdev;
-	mci->dev.bus = mci->bus;
 	mci->dev.groups = groups;
 	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
 	dev_set_drvdata(&mci->dev, mci);

Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac

[Mauro Carvalho Chehab]

Em Mon, 1 Oct 2018 15:43:13 -0700
"Luck, Tony" <tony.luck@intel.com> escreveu:

> Nobody(*) uses them.  Dropping this will allow us to make the total
> number of memory controllers configurable (as we won't have to
> worry about duplicated device names under this directory).
> 
> (*) https://marc.info/?l=linux-edac&m=153809709903987&w=2
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 
> Boris: Apply this, then your earlier patch to get rid of the
> hard coded limit on the number of memory controllers:
>   https://marc.info/?l=linux-edac&m=153797567628947&w=2
> the combination works on my 4 socket machine. Perhaps HPE
> can test on their superdome.
> 

For both this and the referred patch:

Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

>  drivers/edac/edac_mc_sysfs.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 20374b8248f0..4c1bee59c2e6 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
>  				    struct csrow_info *csrow, int index)
>  {
>  	csrow->dev.type = &csrow_attr_type;
> -	csrow->dev.bus = mci->bus;
>  	csrow->dev.groups = csrow_dev_groups;
>  	device_initialize(&csrow->dev);
>  	csrow->dev.parent = &mci->dev;
> @@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
>  	dimm->mci = mci;
>  
>  	dimm->dev.type = &dimm_attr_type;
> -	dimm->dev.bus = mci->bus;
>  	device_initialize(&dimm->dev);
>  
>  	dimm->dev.parent = &mci->dev;
> @@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
>  	device_initialize(&mci->dev);
>  
>  	mci->dev.parent = mci_pdev;
> -	mci->dev.bus = mci->bus;
>  	mci->dev.groups = groups;
>  	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
>  	dev_set_drvdata(&mci->dev, mci);



Thanks,
Mauro

EDAC: Don't add devices under /sys/bus/edac

[Mauro Carvalho Chehab]

Em Mon, 1 Oct 2018 15:43:13 -0700
"Luck, Tony" <tony.luck@intel.com> escreveu:

> Nobody(*) uses them.  Dropping this will allow us to make the total
> number of memory controllers configurable (as we won't have to
> worry about duplicated device names under this directory).
> 
> (*) https://marc.info/?l=linux-edac&m=153809709903987&w=2
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 
> Boris: Apply this, then your earlier patch to get rid of the
> hard coded limit on the number of memory controllers:
>   https://marc.info/?l=linux-edac&m=153797567628947&w=2
> the combination works on my 4 socket machine. Perhaps HPE
> can test on their superdome.
> 

For both this and the referred patch:

Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

>  drivers/edac/edac_mc_sysfs.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 20374b8248f0..4c1bee59c2e6 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
>  				    struct csrow_info *csrow, int index)
>  {
>  	csrow->dev.type = &csrow_attr_type;
> -	csrow->dev.bus = mci->bus;
>  	csrow->dev.groups = csrow_dev_groups;
>  	device_initialize(&csrow->dev);
>  	csrow->dev.parent = &mci->dev;
> @@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
>  	dimm->mci = mci;
>  
>  	dimm->dev.type = &dimm_attr_type;
> -	dimm->dev.bus = mci->bus;
>  	device_initialize(&dimm->dev);
>  
>  	dimm->dev.parent = &mci->dev;
> @@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
>  	device_initialize(&mci->dev);
>  
>  	mci->dev.parent = mci_pdev;
> -	mci->dev.bus = mci->bus;
>  	mci->dev.groups = groups;
>  	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
>  	dev_set_drvdata(&mci->dev, mci);



Thanks,
Mauro

RE: [PATCH] EDAC: Don't add devices under /sys/bus/edac

[Ernst, Justin]

The combined patches work on a 20 socket system. 
Thanks!
-Justin

> -----Original Message-----
> From: Mauro Carvalho Chehab [mailto:mchehab+samsung@kernel.org]
> Sent: Monday, October 1, 2018 8:23 PM
> To: Luck, Tony <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>; Anderson, Russ
> <russ.anderson@hpe.com>; Greg KH <gregkh@linuxfoundation.org>; Ernst,
> Justin <justin.ernst@hpe.com>; Anderson, Russ <russ.anderson@hpe.com>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> edac@vger.kernel.org; linux-kernel@vger.kernel.org; Aristeu Rozanski Filho
> <arozansk@redhat.com>
> Subject: Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac
> 
> Em Mon, 1 Oct 2018 15:43:13 -0700
> "Luck, Tony" <tony.luck@intel.com> escreveu:
> 
> > Nobody(*) uses them.  Dropping this will allow us to make the total
> > number of memory controllers configurable (as we won't have to
> > worry about duplicated device names under this directory).
> >
> > (*) https://marc.info/?l=linux-edac&m=153809709903987&w=2
> >
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >
> > Boris: Apply this, then your earlier patch to get rid of the
> > hard coded limit on the number of memory controllers:
> >   https://marc.info/?l=linux-edac&m=153797567628947&w=2
> > the combination works on my 4 socket machine. Perhaps HPE
> > can test on their superdome.
> >
> 
> For both this and the referred patch:
> 
> Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> 
> >  drivers/edac/edac_mc_sysfs.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> > index 20374b8248f0..4c1bee59c2e6 100644
> > --- a/drivers/edac/edac_mc_sysfs.c
> > +++ b/drivers/edac/edac_mc_sysfs.c
> > @@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct
> mem_ctl_info *mci,
> >  				    struct csrow_info *csrow, int index)
> >  {
> >  	csrow->dev.type = &csrow_attr_type;
> > -	csrow->dev.bus = mci->bus;
> >  	csrow->dev.groups = csrow_dev_groups;
> >  	device_initialize(&csrow->dev);
> >  	csrow->dev.parent = &mci->dev;
> > @@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct
> mem_ctl_info *mci,
> >  	dimm->mci = mci;
> >
> >  	dimm->dev.type = &dimm_attr_type;
> > -	dimm->dev.bus = mci->bus;
> >  	device_initialize(&dimm->dev);
> >
> >  	dimm->dev.parent = &mci->dev;
> > @@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct
> mem_ctl_info *mci,
> >  	device_initialize(&mci->dev);
> >
> >  	mci->dev.parent = mci_pdev;
> > -	mci->dev.bus = mci->bus;
> >  	mci->dev.groups = groups;
> >  	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
> >  	dev_set_drvdata(&mci->dev, mci);
> 
> 
> 
> Thanks,
> Mauro

EDAC: Don't add devices under /sys/bus/edac

[Justin Ernst]

The combined patches work on a 20 socket system. 
Thanks!
-Justin

> -----Original Message-----
> From: Mauro Carvalho Chehab [mailto:mchehab+samsung@kernel.org]
> Sent: Monday, October 1, 2018 8:23 PM
> To: Luck, Tony <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>; Anderson, Russ
> <russ.anderson@hpe.com>; Greg KH <gregkh@linuxfoundation.org>; Ernst,
> Justin <justin.ernst@hpe.com>; Anderson, Russ <russ.anderson@hpe.com>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; linux-
> edac@vger.kernel.org; linux-kernel@vger.kernel.org; Aristeu Rozanski Filho
> <arozansk@redhat.com>
> Subject: Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac
> 
> Em Mon, 1 Oct 2018 15:43:13 -0700
> "Luck, Tony" <tony.luck@intel.com> escreveu:
> 
> > Nobody(*) uses them.  Dropping this will allow us to make the total
> > number of memory controllers configurable (as we won't have to
> > worry about duplicated device names under this directory).
> >
> > (*) https://marc.info/?l=linux-edac&m=153809709903987&w=2
> >
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >
> > Boris: Apply this, then your earlier patch to get rid of the
> > hard coded limit on the number of memory controllers:
> >   https://marc.info/?l=linux-edac&m=153797567628947&w=2
> > the combination works on my 4 socket machine. Perhaps HPE
> > can test on their superdome.
> >
> 
> For both this and the referred patch:
> 
> Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> 
> >  drivers/edac/edac_mc_sysfs.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> > index 20374b8248f0..4c1bee59c2e6 100644
> > --- a/drivers/edac/edac_mc_sysfs.c
> > +++ b/drivers/edac/edac_mc_sysfs.c
> > @@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct
> mem_ctl_info *mci,
> >  				    struct csrow_info *csrow, int index)
> >  {
> >  	csrow->dev.type = &csrow_attr_type;
> > -	csrow->dev.bus = mci->bus;
> >  	csrow->dev.groups = csrow_dev_groups;
> >  	device_initialize(&csrow->dev);
> >  	csrow->dev.parent = &mci->dev;
> > @@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct
> mem_ctl_info *mci,
> >  	dimm->mci = mci;
> >
> >  	dimm->dev.type = &dimm_attr_type;
> > -	dimm->dev.bus = mci->bus;
> >  	device_initialize(&dimm->dev);
> >
> >  	dimm->dev.parent = &mci->dev;
> > @@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct
> mem_ctl_info *mci,
> >  	device_initialize(&mci->dev);
> >
> >  	mci->dev.parent = mci_pdev;
> > -	mci->dev.bus = mci->bus;
> >  	mci->dev.groups = groups;
> >  	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
> >  	dev_set_drvdata(&mci->dev, mci);
> 
> 
> 
> Thanks,
> Mauro

Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac

[Borislav Petkov]

On Tue, Oct 02, 2018 at 03:51:41PM +0000, Ernst, Justin wrote:
> The combined patches work on a 20 socket system. 
> Thanks!

Cool, thanks for testing.

Nevertheless, I'll queue them for 4.21 so that we have a full cycle of
testing before we really kill the bus thing.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

EDAC: Don't add devices under /sys/bus/edac

[Borislav Petkov]

On Tue, Oct 02, 2018 at 03:51:41PM +0000, Ernst, Justin wrote:
> The combined patches work on a 20 socket system. 
> Thanks!

Cool, thanks for testing.

Nevertheless, I'll queue them for 4.21 so that we have a full cycle of
testing before we really kill the bus thing.

Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac

[Borislav Petkov]

On Tue, Oct 02, 2018 at 06:26:08PM +0200, Borislav Petkov wrote:
> On Tue, Oct 02, 2018 at 03:51:41PM +0000, Ernst, Justin wrote:
> > The combined patches work on a 20 socket system. 
> > Thanks!
> 
> Cool, thanks for testing.
> 
> Nevertheless, I'll queue them for 4.21 so that we have a full cycle of
> testing before we really kill the bus thing.

Ok, I've pushed the two patches ontop of EDAC's for-next branch, here:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=edac-for-4.21-bus

and I'd appreciate testing them one more time on the big boxes you guys have.

Diffing sysfs here shows this:

--- edac.before	2018-11-06 13:37:37.925448609 +0100
+++ edac.after	2018-11-06 15:36:11.229497795 +0100
@@ -37,7 +37,6 @@
 /sys/devices/system/edac/mc/mc0/dimm3/power/control
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_dev_type
 /sys/devices/system/edac/mc/mc0/dimm3/size
-/sys/devices/system/edac/mc/mc0/dimm3/subsystem
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_ce_count
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_label
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_location

which is the bus symlink:

subsystem -> ../../../../../../bus/mc0

and I think that's ok as nothing should be using it.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

EDAC: Don't add devices under /sys/bus/edac

[Borislav Petkov]

On Tue, Oct 02, 2018 at 06:26:08PM +0200, Borislav Petkov wrote:
> On Tue, Oct 02, 2018 at 03:51:41PM +0000, Ernst, Justin wrote:
> > The combined patches work on a 20 socket system. 
> > Thanks!
> 
> Cool, thanks for testing.
> 
> Nevertheless, I'll queue them for 4.21 so that we have a full cycle of
> testing before we really kill the bus thing.

Ok, I've pushed the two patches ontop of EDAC's for-next branch, here:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=edac-for-4.21-bus

and I'd appreciate testing them one more time on the big boxes you guys have.

Diffing sysfs here shows this:


which is the bus symlink:

subsystem -> ../../../../../../bus/mc0

and I think that's ok as nothing should be using it.

Thx.

--- edac.before	2018-11-06 13:37:37.925448609 +0100
+++ edac.after	2018-11-06 15:36:11.229497795 +0100
@@ -37,7 +37,6 @@
 /sys/devices/system/edac/mc/mc0/dimm3/power/control
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_dev_type
 /sys/devices/system/edac/mc/mc0/dimm3/size
-/sys/devices/system/edac/mc/mc0/dimm3/subsystem
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_ce_count
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_label
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_location

RE: [PATCH] EDAC: Don't add devices under /sys/bus/edac

[Ernst, Justin]

Looks good on a 32 socket system. All 64 memory controllers show up and I'm able to see the same sysfs diff.
Thanks
-Justin

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, November 6, 2018 8:46 AM
> To: Ernst, Justin <justin.ernst@hpe.com>; Luck, Tony <tony.luck@intel.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>; Anderson, Russ
> <russ.anderson@hpe.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; linux-edac@vger.kernel.org; linux-
> kernel@vger.kernel.org; Aristeu Rozanski Filho <arozansk@redhat.com>
> Subject: Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac
> 
> On Tue, Oct 02, 2018 at 06:26:08PM +0200, Borislav Petkov wrote:
> > On Tue, Oct 02, 2018 at 03:51:41PM +0000, Ernst, Justin wrote:
> > > The combined patches work on a 20 socket system.
> > > Thanks!
> >
> > Cool, thanks for testing.
> >
> > Nevertheless, I'll queue them for 4.21 so that we have a full cycle of
> > testing before we really kill the bus thing.
> 
> Ok, I've pushed the two patches ontop of EDAC's for-next branch, here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=edac-for-
> 4.21-bus
> 
> and I'd appreciate testing them one more time on the big boxes you guys
> have.
> 
> Diffing sysfs here shows this:
> 
> --- edac.before	2018-11-06 13:37:37.925448609 +0100
> +++ edac.after	2018-11-06 15:36:11.229497795 +0100
> @@ -37,7 +37,6 @@
>  /sys/devices/system/edac/mc/mc0/dimm3/power/control
>  /sys/devices/system/edac/mc/mc0/dimm3/dimm_dev_type
>  /sys/devices/system/edac/mc/mc0/dimm3/size
> -/sys/devices/system/edac/mc/mc0/dimm3/subsystem
>  /sys/devices/system/edac/mc/mc0/dimm3/dimm_ce_count
>  /sys/devices/system/edac/mc/mc0/dimm3/dimm_label
>  /sys/devices/system/edac/mc/mc0/dimm3/dimm_location
> 
> which is the bus symlink:
> 
> subsystem -> ../../../../../../bus/mc0
> 
> and I think that's ok as nothing should be using it.
> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

EDAC: Don't add devices under /sys/bus/edac

[Justin Ernst]

Looks good on a 32 socket system. All 64 memory controllers show up and I'm able to see the same sysfs diff.
Thanks
-Justin

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, November 6, 2018 8:46 AM
> To: Ernst, Justin <justin.ernst@hpe.com>; Luck, Tony <tony.luck@intel.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>; Anderson, Russ
> <russ.anderson@hpe.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; linux-edac@vger.kernel.org; linux-
> kernel@vger.kernel.org; Aristeu Rozanski Filho <arozansk@redhat.com>
> Subject: Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac
> 
> On Tue, Oct 02, 2018 at 06:26:08PM +0200, Borislav Petkov wrote:
> > On Tue, Oct 02, 2018 at 03:51:41PM +0000, Ernst, Justin wrote:
> > > The combined patches work on a 20 socket system.
> > > Thanks!
> >
> > Cool, thanks for testing.
> >
> > Nevertheless, I'll queue them for 4.21 so that we have a full cycle of
> > testing before we really kill the bus thing.
> 
> Ok, I've pushed the two patches ontop of EDAC's for-next branch, here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=edac-for-
> 4.21-bus
> 
> and I'd appreciate testing them one more time on the big boxes you guys
> have.
> 
> Diffing sysfs here shows this:
> 
> --- edac.before	2018-11-06 13:37:37.925448609 +0100
> +++ edac.after	2018-11-06 15:36:11.229497795 +0100
> @@ -37,7 +37,6 @@
>  /sys/devices/system/edac/mc/mc0/dimm3/power/control
>  /sys/devices/system/edac/mc/mc0/dimm3/dimm_dev_type
>  /sys/devices/system/edac/mc/mc0/dimm3/size
> -/sys/devices/system/edac/mc/mc0/dimm3/subsystem
>  /sys/devices/system/edac/mc/mc0/dimm3/dimm_ce_count
>  /sys/devices/system/edac/mc/mc0/dimm3/dimm_label
>  /sys/devices/system/edac/mc/mc0/dimm3/dimm_location
> 
> which is the bus symlink:
> 
> subsystem -> ../../../../../../bus/mc0
> 
> and I think that's ok as nothing should be using it.
> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac

[Borislav Petkov]

On Tue, Nov 13, 2018 at 07:09:24PM +0000, Ernst, Justin wrote:
> Looks good on a 32 socket system. All 64 memory controllers show up
> and I'm able to see the same sysfs diff. Thanks

Thanks for testing, much appreciated!

I'm queuing the stuff for 4.21.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

EDAC: Don't add devices under /sys/bus/edac

[Borislav Petkov]

On Tue, Nov 13, 2018 at 07:09:24PM +0000, Ernst, Justin wrote:
> Looks good on a 32 socket system. All 64 memory controllers show up
> and I'm able to see the same sysfs diff. Thanks

Thanks for testing, much appreciated!

I'm queuing the stuff for 4.21.