Re: [PATCH v5 10/16] platform/x86/intel/pmc/ssram: Use fixed-size static pmc array

[David Box]

On Fri, May 22, 2026 at 12:49:43PM -0700, David Box wrote:
> On Fri, May 22, 2026 at 01:37:07PM +0300, Ilpo Järvinen wrote:
> > On Thu, 21 May 2026, David E. Box wrote:
> > 
> > > From: Xi Pardee <xi.pardee@xxxxxxxxxxxxxxx>
> > > 
> > > Switch pmc_ssram_telems from a devm-allocated pointer to a fixed-size
> > > static array, eliminating per-probe allocation overhead and simplifying
> > > lifetime management.
> > > 
> > > Correspondingly simplify pmc_ssram_telemetry_get_pmc_info() validation to
> > > check devid availability and tighten input bounds checking. Drop
> > > null-pointer checks now that the storage is static.
> > > 
> > > Signed-off-by: Xi Pardee <xi.pardee@xxxxxxxxxxxxxxx>
> > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > ---
> > 
> > Sashiko has noted some corner case issues related probe/init paths and 
> > missing cleanup which may have some merit, especially if this series makes 
> > the issue worse (for patches 10-14 but all those seem to fundamendally 
> > relate to same reprobe corner case and leaks of stale setups through 
> > static vars).
> > 
> > It has some false positives too, such as not realizing smp_*mb() implies 
> > a compiler barrier so those READ/WRITE_ONCE() requests look bogus to me.
> 
> Okay thanks. This will be the first time I try it myself.
> 

I couldn't get Sashiko to rerun with my Claude setup, but I did manually use
Claude to find what I think is the same issue.

On Fri, May 22, 2026 at 12:49:43PM -0700, David Box wrote:
> On Fri, May 22, 2026 at 01:37:07PM +0300, Ilpo Järvinen wrote:
> > On Thu, 21 May 2026, David E. Box wrote:
> > 
> > > From: Xi Pardee <xi.pardee@xxxxxxxxxxxxxxx>
> > > 
> > > Switch pmc_ssram_telems from a devm-allocated pointer to a fixed-size
> > > static array, eliminating per-probe allocation overhead and simplifying
> > > lifetime management.
> > > 
> > > Correspondingly simplify pmc_ssram_telemetry_get_pmc_info() validation to
> > > check devid availability and tighten input bounds checking. Drop
> > > null-pointer checks now that the storage is static.
> > > 
> > > Signed-off-by: Xi Pardee <xi.pardee@xxxxxxxxxxxxxxx>
> > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > ---
> > 
> > Sashiko has noted some corner case issues related probe/init paths and 
> > missing cleanup which may have some merit, especially if this series makes 
> > the issue worse (for patches 10-14 but all those seem to fundamendally 
> > relate to same reprobe corner case and leaks of stale setups through 
> > static vars).
> > 
> > It has some false positives too, such as not realizing smp_*mb() implies 
> > a compiler barrier so those READ/WRITE_ONCE() requests look bogus to me.
> 
> Okay thanks. This will be the first time I try it myself.
> 

I couldn't get Sashiko to rerun with my Claude setup, but I did manually use
Claude to find what I think is the same issue.

First, I realize the commit message doesn't capture why we eliminated the devm
allocation. Up to this point the driver probes all PMCs from a single PCI
device probe, but the following patches move toward probing once per device.
That means a per-device allocated PMC tracker no longer fits the later model.
I'll update the commit message with that information.

The bug Claude found in this patch is that the original devm allocation made
pmc_ssram_telems freshly zeroed on every probe attempt, while switching to a
static array means it's only zeroed at module load time.  If we fail and have to
reprobe the data is stale. I fixed this temporarily by zeroing the array at the
top of probe. It will be removed in the next patch when we start probing per
device.

David

> 
> > 
> > -- 
> >  i.
> > 
> > > V5 - No changes
> > > 
> > > V4 - No changes
> > > 
> > > V3 - No changes
> > > 
> > > V2 changes:
> > >   - Replaced hardcoded array size [3] with MAX_NUM_PMC constant
> > > 
> > >  drivers/platform/x86/intel/pmc/ssram_telemetry.c | 10 ++--------
> > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/pmc/ssram_telemetry.c b/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > > index 1deb4d71da3f..4bfe60ee55ca 100644
> > > --- a/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > > +++ b/drivers/platform/x86/intel/pmc/ssram_telemetry.c
> > > @@ -24,7 +24,7 @@
> > >  
> > >  DEFINE_FREE(pmc_ssram_telemetry_iounmap, void __iomem *, if (_T) iounmap(_T))
> > >  
> > > -static struct pmc_ssram_telemetry *pmc_ssram_telems;
> > > +static struct pmc_ssram_telemetry pmc_ssram_telems[MAX_NUM_PMC];
> > >  static bool device_probed;
> > >  
> > >  static int
> > > @@ -140,7 +140,7 @@ int pmc_ssram_telemetry_get_pmc_info(unsigned int pmc_idx,
> > >  	if (pmc_idx >= MAX_NUM_PMC)
> > >  		return -EINVAL;
> > >  
> > > -	if (!pmc_ssram_telems || !pmc_ssram_telems[pmc_idx].devid)
> > > +	if (!pmc_ssram_telems[pmc_idx].devid)
> > >  		return -ENODEV;
> > >  
> > >  	pmc_ssram_telemetry->devid = pmc_ssram_telems[pmc_idx].devid;
> > > @@ -153,12 +153,6 @@ static int pmc_ssram_telemetry_probe(struct pci_dev *pcidev, const struct pci_de
> > >  {
> > >  	int ret;
> > >  
> > > -	pmc_ssram_telems = devm_kzalloc(&pcidev->dev, sizeof(*pmc_ssram_telems) * MAX_NUM_PMC,
> > > -					GFP_KERNEL);
> > > -	if (!pmc_ssram_telems) {
> > > -		ret = -ENOMEM;
> > > -		goto probe_finish;
> > > -	}
> > >  
> > >  	ret = pcim_enable_device(pcidev);
> > >  	if (ret) {
> > > 
> 
>