| Message ID | 20231002131817.1590966-1-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [UPDATE] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency | expand |
On Mon, 2 Oct 2023, Srinivas Pandruvada wrote: > When the current uncore frequency can't be read, don't create attribute > "current_freq_khz" as any read will fail later. Some user space > applications like turbostat fail to continue with the failure. So, check > error during attribute creation. > > Fixes: 8a54e2253e4c ("platform/x86/intel-uncore-freq: Uncore frequency control via TPMI") Hi, Thanks for the update but that commit id looks bogus, or where the value is used w/o error check?
Hi llPo, On Tue, 2023-10-03 at 16:10 +0300, Ilpo Järvinen wrote: > On Mon, 2 Oct 2023, Srinivas Pandruvada wrote: > > > When the current uncore frequency can't be read, don't create > > attribute > > "current_freq_khz" as any read will fail later. Some user space > > applications like turbostat fail to continue with the failure. So, > > check > > error during attribute creation. > > > > Fixes: 8a54e2253e4c ("platform/x86/intel-uncore-freq: Uncore > > frequency control via TPMI") > > Hi, > > Thanks for the update but that commit id looks bogus, or where the > value > is used w/o error check? commit 8a54e2253e4c25e5b61c9a9bee157bb52da5d432 Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Date: Thu Apr 20 15:05:14 2023 -0700 platform/x86/intel-uncore-freq: Uncore frequency control via TPMI This is the commit exposed the issue. This is not the commit which changed the code in question. I can add also Fixes: dbce412a7733 ("platform/x86/intel-uncore-freq: Split common and enumeration part") But the change even before that as this commit just reorganized code but because of change of folders, that will look like correct commit. Thanks, Srinivas
On Tue, 3 Oct 2023, srinivas pandruvada wrote: > On Tue, 2023-10-03 at 16:10 +0300, Ilpo Järvinen wrote: > > On Mon, 2 Oct 2023, Srinivas Pandruvada wrote: > > > > > When the current uncore frequency can't be read, don't create > > > attribute > > > "current_freq_khz" as any read will fail later. Some user space > > > applications like turbostat fail to continue with the failure. So, > > > check > > > error during attribute creation. > > > > > > Fixes: 8a54e2253e4c ("platform/x86/intel-uncore-freq: Uncore > > > frequency control via TPMI") > > > > Hi, > > > > Thanks for the update but that commit id looks bogus, or where the > > value > > is used w/o error check? > > commit 8a54e2253e4c25e5b61c9a9bee157bb52da5d432 > Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Date: Thu Apr 20 15:05:14 2023 -0700 > > platform/x86/intel-uncore-freq: Uncore frequency control via TPMI > > > This is the commit exposed the issue. This is not the commit which > changed the code in question. > > > I can add also > Fixes: dbce412a7733 ("platform/x86/intel-uncore-freq: Split common and > enumeration part") > > But the change even before that as this commit just reorganized code > but because of change of folders, that will look like correct commit. I never thought dbce412a7733 is being fixed here, it's just a refactor moving code around like you say. But how about 414eef27283a ("platform/x86/intel/uncore-freq: Display uncore current frequency") which actually adds the code line you're now fixing. What was broken before it? All I see is the one call in show_perf_status_freq_khz() but that's checking for errors.
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c index 1152deaa0078..33ab207493e3 100644 --- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c @@ -176,7 +176,7 @@ show_uncore_data(initial_max_freq_khz); static int create_attr_group(struct uncore_data *data, char *name) { - int ret, index = 0; + int ret, freq, index = 0; init_attribute_rw(max_freq_khz); init_attribute_rw(min_freq_khz); @@ -197,7 +197,11 @@ static int create_attr_group(struct uncore_data *data, char *name) data->uncore_attrs[index++] = &data->min_freq_khz_dev_attr.attr; data->uncore_attrs[index++] = &data->initial_min_freq_khz_dev_attr.attr; data->uncore_attrs[index++] = &data->initial_max_freq_khz_dev_attr.attr; - data->uncore_attrs[index++] = &data->current_freq_khz_dev_attr.attr; + + ret = uncore_read_freq(data, &freq); + if (!ret) + data->uncore_attrs[index++] = &data->current_freq_khz_dev_attr.attr; + data->uncore_attrs[index] = NULL; data->uncore_attr_group.name = name;
When the current uncore frequency can't be read, don't create attribute "current_freq_khz" as any read will fail later. Some user space applications like turbostat fail to continue with the failure. So, check error during attribute creation. Fixes: 8a54e2253e4c ("platform/x86/intel-uncore-freq: Uncore frequency control via TPMI") Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- update - Added Fixes tag .../x86/intel/uncore-frequency/uncore-frequency-common.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)