Received-SPF: pass (google.com: domain of linux-kernel+bounces-147754-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Sender: Guenter Roeck <groeck7@gmail.com>
Date: Tue, 16 Apr 2024 16:37:05 -0700
From: Guenter Roeck <linux@roeck-us.net>
To: "Luck, Tony" <tony.luck@intel.com>
Cc: Jean Delvare <jdelvare@suse.com>,
	"Winiarska, Iwona" <iwona.winiarska@intel.com>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"patches@lists.linux.dev" <patches@lists.linux.dev>
Subject: Re: [PATCH v3 50/74] x86/cpu/vfm: Update drivers/hwmon/peci/cputemp.c
Message-ID: <8690bcea-3ff4-49f1-a671-16583b8d241e@roeck-us.net>
References: <20240416211941.9369-1-tony.luck@intel.com>
 <20240416212216.9605-1-tony.luck@intel.com>
 <5869d164-25c4-42e5-bf87-c4aeeac57388@roeck-us.net>
 <SJ1PR11MB6083D0DFBC7DE286F986A453FC082@SJ1PR11MB6083.namprd11.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SJ1PR11MB6083D0DFBC7DE286F986A453FC082@SJ1PR11MB6083.namprd11.prod.outlook.com>

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