Re: [PATCH] EDAC/{i10nm,skx,skx_common}: Support multiple clumps

Kyle Meyer
2024-12-05T20:05:58Z

On Thu, Dec 05, 2024 at 11:13:23AM -0800, Luck, Tony wrote:
> On Thu, Dec 05, 2024 at 10:59:54AM -0600, Kyle Meyer wrote:
> > The 3-bit source IDs in PCI configuration space registers are limited to
> > 8 unique IDs, and each ID is local to a clump (UPI/QPI domain).
>
> Is there any better name than "clump"?

Yes, a UPI/QPI domain.

> > Source IDs can not be used to map devices to sockets on systems with
> > multiple clumps because each clump has identical repeating source IDs.
> >
> > Get package IDs instead of source IDs on systems with multiple clumps
> > and use package/source IDs to name IMC information structures.
>
> What would happen if you just assumed the system had clumps and you
> always used package/source? Would that change EDAC naming for
> existing systems? That would be less complexity for the driver.

That works if NUMA is enabled. skx_get_pkg_id() uses NUMA information to
determine the package ID.

> > Signed-off-by: Kyle Meyer <kyle.meyer@hpe.com>
> > ---
> > drivers/edac/i10nm_base.c | 21 +++++++++-------
> > drivers/edac/skx_base.c | 19 ++++++++------
> > drivers/edac/skx_common.c | 52 +++++++++++++++++++++++++++++++++------
> > drivers/edac/skx_common.h | 5 ++--
> > 4 files changed, 71 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/edac/i10nm_base.c b/drivers/edac/i10nm_base.c
> > index 51556c72a967..59384677d025 100644
> > --- a/drivers/edac/i10nm_base.c
> > +++ b/drivers/edac/i10nm_base.c
> > @@ -1010,7 +1010,7 @@ static struct notifier_block i10nm_mce_dec = {
> >
> > static int __init i10nm_init(void)
> > {
> > - u8 mc = 0, src_id = 0, node_id = 0;
> > + u8 mc = 0, src_id = 0;
> > const struct x86_cpu_id *id;
> > struct res_config *cfg;
> > const char *owner;
> > @@ -1018,6 +1018,7 @@ static int __init i10nm_init(void)
> > int rc, i, off[3] = {0xd0, 0xc8, 0xcc};
> > u64 tolm, tohm;
> > int imc_num;
> > + int dup_src_ids = 0;
> >
> > edac_dbg(2, "\n");
> >
> > @@ -1065,24 +1066,26 @@ static int __init i10nm_init(void)
> >
> > imc_num = res_cfg->ddr_imc_num + res_cfg->hbm_imc_num;
> >
> > - list_for_each_entry(d, i10nm_edac_list, list) {
> > - rc = skx_get_src_id(d, 0xf8, &src_id);
> > - if (rc < 0)
> > - goto fail;
> > + rc = dup_src_ids = skx_check_dup_src_ids(0xf8);
>
> Checkpatch complains about this: "multiple assignments should be
> avoided"
>
> > + if (rc < 0)
> > + goto fail;
> >
> > - rc = skx_get_node_id(d, &node_id);
> > + list_for_each_entry(d, i10nm_edac_list, list) {
> > + if (dup_src_ids)
> > + rc = skx_get_pkg_id(d, &src_id);
> > + else
> > + rc = skx_get_src_id(d, 0xf8, &src_id);
> > if (rc < 0)
> > goto fail;
> >
> > - edac_dbg(2, "src_id = %d node_id = %d\n", src_id, node_id);
> > + edac_dbg(2, "src_id = %d\n", src_id);
> > for (i = 0; i < imc_num; i++) {
> > if (!d->imc[i].mdev)
> > continue;
> >
> > d->imc[i].mc = mc++;
> > d->imc[i].lmc = i;
> > - d->imc[i].src_id = src_id;
> > - d->imc[i].node_id = node_id;
> > + d->imc[i].src_id = src_id;
> > if (d->imc[i].hbm_mc) {
> > d->imc[i].chan_mmio_sz = cfg->hbm_chan_mmio_sz;
> > d->imc[i].num_channels = cfg->hbm_chan_num;
> > diff --git a/drivers/edac/skx_base.c b/drivers/edac/skx_base.c
> > index 14cfd394b469..189b8c5a1bda 100644
> > --- a/drivers/edac/skx_base.c
> > +++ b/drivers/edac/skx_base.c
> > @@ -600,8 +600,9 @@ static int __init skx_init(void)
> > const struct munit *m;
> > const char *owner;
> > int rc = 0, i, off[3] = {0xd0, 0xd4, 0xd8};
> > - u8 mc = 0, src_id, node_id;
> > + u8 mc = 0, src_id;
> > struct skx_dev *d;
> > + int dup_src_ids = 0;
> >
> > edac_dbg(2, "\n");
> >
> > @@ -646,19 +647,23 @@ static int __init skx_init(void)
> > }
> > }
> >
> > + rc = dup_src_ids = skx_check_dup_src_ids(0xf0);
>
> Checkpatch complains about this: "multiple assignments should be
> avoided"

That's strange, my scripts/checkpatch.pl didn't complain. I'll avoid that going
forward.

Thanks,
Kyle Meyer