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

[Luck, Tony]

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@xxxxxxx>
> ---
>  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