From: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> To: Borislav Petkov <bp@alien8.de> Cc: "Luck, Tony" <tony.luck@intel.com>, Greg KH <gregkh@linuxfoundation.org>, Justin Ernst <justin.ernst@hpe.com>, 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] Raise maximum number of memory controllers Date: Wed, 26 Sep 2018 13:03:40 -0300 [thread overview] Message-ID: <20180926130340.6b22918b@coco.lan> (raw) In-Reply-To: <20180926152752.GG5584@zn.tnic> 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
WARNING: multiple messages have this Message-ID (diff)
From: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> To: Borislav Petkov <bp@alien8.de> Cc: "Luck, Tony" <tony.luck@intel.com>, Greg KH <gregkh@linuxfoundation.org>, Justin Ernst <justin.ernst@hpe.com>, 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: Raise maximum number of memory controllers Date: Wed, 26 Sep 2018 13:03:40 -0300 [thread overview] Message-ID: <20180926130340.6b22918b@coco.lan> (raw) 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
next prev parent reply other threads:[~2018-09-26 16:04 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-09-25 14:34 [PATCH] Raise maximum number of memory controllers Justin Ernst 2018-09-25 14:34 ` Justin Ernst 2018-09-25 15:26 ` [PATCH] " Borislav Petkov 2018-09-25 15:26 ` Borislav Petkov 2018-09-25 17:50 ` [PATCH] " Luck, Tony 2018-09-25 17:50 ` Luck, Tony 2018-09-25 18:07 ` [PATCH] " Borislav Petkov 2018-09-25 18:07 ` Borislav Petkov 2018-09-26 9:35 ` [PATCH] " Borislav Petkov 2018-09-26 9:35 ` Borislav Petkov 2018-09-26 15:27 ` [PATCH] " Borislav Petkov 2018-09-26 15:27 ` Borislav Petkov 2018-09-26 16:03 ` Mauro Carvalho Chehab [this message] 2018-09-26 16:03 ` Mauro Carvalho Chehab 2018-09-26 16:17 ` [PATCH] " Borislav Petkov 2018-09-26 16:17 ` Borislav Petkov 2018-09-26 17:39 ` [PATCH] " Mauro Carvalho Chehab 2018-09-26 17:39 ` Mauro Carvalho Chehab 2018-09-26 18:10 ` [PATCH] " Luck, Tony 2018-09-26 18:10 ` Luck, Tony 2018-09-26 18:23 ` [PATCH] " Russ Anderson 2018-09-26 18:23 ` Russ Anderson 2018-09-26 23:02 ` [PATCH] " Luck, Tony 2018-09-26 23:02 ` Luck, Tony 2018-09-27 4:52 ` [PATCH] " Borislav Petkov 2018-09-27 4:52 ` Borislav Petkov 2018-09-27 21:44 ` [PATCH] " Luck, Tony 2018-09-27 21:44 ` Luck, Tony 2018-09-27 22:03 ` [PATCH] " Borislav Petkov 2018-09-27 22:03 ` Borislav Petkov 2018-09-28 1:10 ` [PATCH] " Mauro Carvalho Chehab 2018-09-28 1:10 ` Mauro Carvalho Chehab 2018-10-01 12:47 ` [PATCH] " Borislav Petkov 2018-10-01 12:47 ` Borislav Petkov 2018-10-01 22:43 ` [PATCH] EDAC: Don't add devices under /sys/bus/edac Luck, Tony 2018-10-01 22:43 ` Luck, Tony 2018-10-02 1:22 ` [PATCH] " Mauro Carvalho Chehab 2018-10-02 1:22 ` Mauro Carvalho Chehab 2018-10-02 15:51 ` [PATCH] " Ernst, Justin 2018-10-02 15:51 ` Justin Ernst 2018-10-02 16:26 ` [PATCH] " Borislav Petkov 2018-10-02 16:26 ` Borislav Petkov 2018-11-06 14:45 ` [PATCH] " Borislav Petkov 2018-11-06 14:45 ` Borislav Petkov 2018-11-13 19:09 ` [PATCH] " Ernst, Justin 2018-11-13 19:09 ` Justin Ernst 2018-11-13 19:15 ` [PATCH] " Borislav Petkov 2018-11-13 19:15 ` Borislav Petkov 2018-09-26 7:55 ` [PATCH] Raise maximum number of memory controllers Zhuo, Qiuxu 2018-09-26 7:55 ` Qiuxu Zhuo 2018-09-26 13:53 ` [PATCH] " Russ Anderson 2018-09-26 13:53 ` Russ Anderson 2018-09-26 16:13 ` [PATCH] " Aristeu Rozanski 2018-09-26 16:13 ` Aristeu Rozanski 2018-09-27 5:56 ` [PATCH] " Borislav Petkov 2018-09-27 5:56 ` Borislav Petkov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180926130340.6b22918b@coco.lan \ --to=mchehab+samsung@kernel.org \ --cc=arozansk@redhat.com \ --cc=bp@alien8.de \ --cc=gregkh@linuxfoundation.org \ --cc=justin.ernst@hpe.com \ --cc=linux-edac@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mchehab@kernel.org \ --cc=russ.anderson@hpe.com \ --cc=tony.luck@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.