| Message ID | 20230215123249.4473-1-sumeet.r.pawnikar@intel.com |
|---|---|
| State | Accepted |
| Commit | eb52bc2ae5b8413619a326f47ed635365883343a |
| Headers | show |
| Series | powercap: RAPL: Add Power Limit4 support for Meteor Lake SoC | expand |
On Wed, Feb 15, 2023 at 1:46 PM Sumeet Pawnikar <sumeet.r.pawnikar@intel.com> wrote: > > Add Meteor Lake SoC to the list of processor models for which > Power Limit4 is supported by the Intel RAPL driver. > > Signed-off-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com> > --- > drivers/powercap/intel_rapl_msr.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c > index bc6adda58883..a27673706c3d 100644 > --- a/drivers/powercap/intel_rapl_msr.c > +++ b/drivers/powercap/intel_rapl_msr.c > @@ -143,6 +143,8 @@ static const struct x86_cpu_id pl4_support_ids[] = { > { X86_VENDOR_INTEL, 6, INTEL_FAM6_ALDERLAKE_N, X86_FEATURE_ANY }, > { X86_VENDOR_INTEL, 6, INTEL_FAM6_RAPTORLAKE, X86_FEATURE_ANY }, > { X86_VENDOR_INTEL, 6, INTEL_FAM6_RAPTORLAKE_P, X86_FEATURE_ANY }, > + { X86_VENDOR_INTEL, 6, INTEL_FAM6_METEORLAKE, X86_FEATURE_ANY }, > + { X86_VENDOR_INTEL, 6, INTEL_FAM6_METEORLAKE_L, X86_FEATURE_ANY }, > {} > }; > > -- Applied as 6.3-rc material, thanks!
On 2/15/23 04:32, Sumeet Pawnikar wrote: > diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c > index bc6adda58883..a27673706c3d 100644 > --- a/drivers/powercap/intel_rapl_msr.c > +++ b/drivers/powercap/intel_rapl_msr.c > @@ -143,6 +143,8 @@ static const struct x86_cpu_id pl4_support_ids[] = { > { X86_VENDOR_INTEL, 6, INTEL_FAM6_ALDERLAKE_N, X86_FEATURE_ANY }, > { X86_VENDOR_INTEL, 6, INTEL_FAM6_RAPTORLAKE, X86_FEATURE_ANY }, > { X86_VENDOR_INTEL, 6, INTEL_FAM6_RAPTORLAKE_P, X86_FEATURE_ANY }, > + { X86_VENDOR_INTEL, 6, INTEL_FAM6_METEORLAKE, X86_FEATURE_ANY }, > + { X86_VENDOR_INTEL, 6, INTEL_FAM6_METEORLAKE_L, X86_FEATURE_ANY }, > {} > }; Sumeet, could you _please_ go take a close look at 'struct x86_cpu_id'? > struct x86_cpu_id { > __u16 vendor; > __u16 family; > __u16 model; > __u16 steppings; > __u16 feature; /* bit index */ > kernel_ulong_t driver_data; > }; You might also want to very carefully count the fields in the structure. Which field is being initialized to X86_FEATURE_ANY? Is it: a. ->feature b. ->steppings c. ->model How could this _possibly_ work, you ask yourself? Well, you lucked out: #define X86_FAMILY_ANY 0 #define X86_MODEL_ANY 0 #define X86_STEPPING_ANY 0 #define X86_FEATURE_ANY 0 so, you actually accidentally *explicitly* specified a 0 for ->steppings *AND* accidentally *implicitly* specified a 0 for ->feature. ... and you did this in at least five separate commits over four years. Why does this matter? Because some hapless maintainer might take your code, copy it, and then s/X86_FEATURE_ANY/X86_FEATURE_FOO/ and then scratch their head for an hour as to why it doesn't work. Could you please fix this up? As penance, you could even fix the _ANY defines so that people can't do this accidentally any longer.
On Thu, May 11, 2023 at 07:55:08AM -0700, Dave Hansen wrote: > Could you please fix this up? As penance, you could even fix the _ANY > defines so that people can't do this accidentally any longer. See the X86_MATCH_INTEL_FAM6_MODEL() for how to do this right. -Tony
> -----Original Message----- > From: Luck, Tony <tony.luck@intel.com> > Sent: Thursday, May 11, 2023 10:15 PM > To: Hansen, Dave <dave.hansen@intel.com> > Cc: Pawnikar, Sumeet R <sumeet.r.pawnikar@intel.com>; rafael@kernel.org; > srinivas.pandruvada@linux.intel.com; Zhang, Rui <rui.zhang@intel.com>; > linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] powercap: RAPL: Add Power Limit4 support for Meteor > Lake SoC > > On Thu, May 11, 2023 at 07:55:08AM -0700, Dave Hansen wrote: > > Could you please fix this up? As penance, you could even fix the _ANY > > defines so that people can't do this accidentally any longer. > > See the X86_MATCH_INTEL_FAM6_MODEL() for how to do this right. > Thanks Dave and Tony for this information. Let me check and submit the fix for this. Regards, Sumeet. > -Tony
diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c index bc6adda58883..a27673706c3d 100644 --- a/drivers/powercap/intel_rapl_msr.c +++ b/drivers/powercap/intel_rapl_msr.c @@ -143,6 +143,8 @@ static const struct x86_cpu_id pl4_support_ids[] = { { X86_VENDOR_INTEL, 6, INTEL_FAM6_ALDERLAKE_N, X86_FEATURE_ANY }, { X86_VENDOR_INTEL, 6, INTEL_FAM6_RAPTORLAKE, X86_FEATURE_ANY }, { X86_VENDOR_INTEL, 6, INTEL_FAM6_RAPTORLAKE_P, X86_FEATURE_ANY }, + { X86_VENDOR_INTEL, 6, INTEL_FAM6_METEORLAKE, X86_FEATURE_ANY }, + { X86_VENDOR_INTEL, 6, INTEL_FAM6_METEORLAKE_L, X86_FEATURE_ANY }, {} };
Add Meteor Lake SoC to the list of processor models for which Power Limit4 is supported by the Intel RAPL driver. Signed-off-by: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com> --- drivers/powercap/intel_rapl_msr.c | 2 ++ 1 file changed, 2 insertions(+)