Re: [PATCH] platform/x86: intel_pmc_core: Fix uninitialized pmc/map in pmc_core_send_ltr_ignore

[Purva Yeshi]

On 17/04/25 18:43, Ilpo Järvinen wrote:
> On Thu, 17 Apr 2025, Purva Yeshi wrote:
>
> > Fix Smatch-detected issue:
> >
> > drivers/platform/x86/intel/pmc/core.c:501 pmc_core_send_ltr_ignore()
> > error: uninitialized symbol 'pmc'.
> >
> > drivers/platform/x86/intel/pmc/core.c:501 pmc_core_send_ltr_ignore()
> > error: uninitialized symbol 'map'.
> >
> > drivers/platform/x86/intel/pmc/core.c:501 pmc_core_send_ltr_ignore()
> > error: we previously assumed 'pmc' could be null (see line 479)
> >
> >
> > Prevents uninitialized symbol warnings detected by smatch.
> >
> > Ensures map is not accessed if pmc is NULL, preventing dereferencing
> > of uninitialized pointers
> >
> > Add defensive check for pmc and map to catch any unexpected edge cases
> > and ensure all required pointers are valid.
> >
> > Signed-off-by: Purva Yeshi <purvayeshi550@xxxxxxxxx>
> > ---
> >   drivers/platform/x86/intel/pmc/core.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> > index 7a1d11f2914f..e674b940e29e 100644
> > --- a/drivers/platform/x86/intel/pmc/core.c
> > +++ b/drivers/platform/x86/intel/pmc/core.c
> > @@ -462,8 +462,8 @@ DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
> >   
> >   int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore)
> >   {
> > -	struct pmc *pmc;
> > -	const struct pmc_reg_map *map;
> > +	struct pmc *pmc = NULL;
> > +	const struct pmc_reg_map *map = NULL;
> >   	u32 reg;
> >   	unsigned int pmc_index;
> >   	int ltr_index;
> > @@ -480,6 +480,9 @@ int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore)

> >   			continue;
> >   
> >   		map = pmc->map;
> > +		if (!map)
> > +			continue;
>
> How can this happen?? If pmc is created, it should have a valid ->map
> AFAICT. Did you even read that code at all???

Hi,

Thanks for the feedback.

Yes, I did read through the code and I understand your point.

The motivation behind the patch was a Smatch warning about possible 
uninitialized use of map and pmc, even though they are logically 
guarded. I now see that these checks may not be necessary given the 
existing control flow.

> > +
> >   		if (ltr_index <= map->ltr_ignore_max)
> >   			break;
> >   
> > @@ -491,7 +494,7 @@ int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore)
> >   		ltr_index = ltr_index - (map->ltr_ignore_max + 2) - 1;
> >   	}
> >   
> > -	if (pmc_index >= ARRAY_SIZE(pmcdev->pmcs) || ltr_index < 0)
> > +	if (pmc_index >= ARRAY_SIZE(pmcdev->pmcs) || ltr_index < 0 || !pmc || !map)
>
> What are the situations pmc_index >= ARRAY_SIZE(pmcdev->pmcs) check
> didn't catch where these new checks do something useful??
>
> Lots of noise but little real substance in this patch?

You're right, if pmc is non-NULL, then map should also be valid, and the 
bounds check on pmc_index already prevents out-of-bounds access. Adding 
further checks might just add noise.

I'll drop the patch unless there's a cleaner way to restructure the 
logic to make Smatch silence without redundant checks.

Thanks again for the clarification!

Best regards,
Purva

>