From: Dave Hansen <dave.hansen@intel.com>
To: Sumeet Pawnikar <sumeet.r.pawnikar@intel.com>,
rafael@kernel.org, srinivas.pandruvada@linux.intel.com,
rui.zhang@intel.com, linux-pm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] powercap: RAPL: Add Power Limit4 support for Meteor Lake SoC
Date: Thu, 11 May 2023 07:55:08 -0700 [thread overview]
Message-ID: <28ead36b-2d9e-1a36-6f4e-04684e420260@intel.com> (raw)
In-Reply-To: <20230215123249.4473-1-sumeet.r.pawnikar@intel.com>
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.
next prev parent reply other threads:[~2023-05-11 14:58 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-15 12:32 [PATCH] powercap: RAPL: Add Power Limit4 support for Meteor Lake SoC Sumeet Pawnikar
2023-02-23 19:09 ` Rafael J. Wysocki
2023-05-11 14:55 ` Dave Hansen [this message]
2023-05-11 16:45 ` Tony Luck
2023-05-18 17:33 ` Pawnikar, Sumeet R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=28ead36b-2d9e-1a36-6f4e-04684e420260@intel.com \
--to=dave.hansen@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rui.zhang@intel.com \
--cc=srinivas.pandruvada@linux.intel.com \
--cc=sumeet.r.pawnikar@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).