Messages in this thread | | | Date | Thu, 11 May 2023 07:55:08 -0700 | Subject | Re: [PATCH] powercap: RAPL: Add Power Limit4 support for Meteor Lake SoC | From | Dave Hansen <> |
| |
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.
| |