Re: [PATCH v3 50/74] x86/cpu/vfm: Update drivers/hwmon/peci/cputemp.c

From: Guenter Roeck
Date: Tue Apr 16 2024 - 19:37:17 EST


On Tue, Apr 16, 2024 at 11:05:15PM +0000, Luck, Tony wrote:
> Guenter,
>
> Thanks for taking a look at this patch.
>
> > > + case VFM_MODEL(INTEL_ICELAKE_X):
> > > + case VFM_MODEL(INTEL_ICELAKE_D):
> > > + case VFM_MODEL(INTEL_SAPPHIRERAPIDS_X):
> >
> > $ git describe
> > v6.9-rc4-31-g96fca68c4fbf
> > $ git grep VFM_MODEL
> > $
> >
> > So I guess this depends on some other patch ?
> > That might be worth mentioning, especially since
> >
> > $ git describe
> > next-20240416
> > $ git grep VFM_MODEL
> > $
> >
> > doesn't find it either.
>
>
> Yes. This depends on parts 1,2,3 of this series. I should have added
> that to the "---" meta comment part of these patches that I'm spraying
> out to maintainers of random subsystems that use INTEL_FAM6 defines.
>
> > On top of that, it looks like
> >
> > #include <asm/cpu_device_id.h>
> >
> > introduces a dependency on X86 which is not currently the case.
> > CONFIG_PECI is typically used on BMCs. Many of those do not use
> > Intel CPUs. It does not seem appropriate to make support for PECI
> > based hardware monitoring dependent on it running on an Intel CPU.
>
> It seems that some use (or need to know about) Icelake and Sapphire Rapids.
>
> How does this code get the value "peci_dev->info.model" used in this switch?
>

My understanding is that the PECI protocol provides that information.

> Given that it is doing some magic just for some recent Intel CPUs, it seems
> plausible that when non-family 6 CPUs appear in the market, it might have
> to grab both the family and the model to reliably determine what to do?
>
> Simple options to avoid including <asm/cpu_device_id.h> would be
> to either:
>
> 1) provide a copy of the VFM_MODEL macro here.
>
> or
>
> 2) Keep using the old INTEL_FAM6_* #define names, but define those for
> the three CPU models peci needs locally instead of getting them from
> <asm/intel-family.h>. It looks like there is a somewhat convoluted path to
> include that. I see in <linux/peci.cpu.h>
>
> #include "../../arch/x86/include/asm/intel-family.h"
>
>
> Or maybe some better option?
>

If the CPU defines and the new macro are to be kept in architecture code,
maybe include arch/x86/include/asm/cpu_device_id.h from linux/peci.cpu.h.
That would not be worse than today's include of intel-family.h.

Guenter