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

[Kyle Meyer <kyle.meyer@hpe.com>]

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