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

Luck, Tony
2024-12-05T19:13:25Z

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"?
>
> 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.

> 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"

-Tony