[UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[Srinivas Pandruvada]

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(-)

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;
-- 
2.40.1

Re: [UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[Ilpo Järvinen]

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?

-- 
 i.


> 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(-)
> 
> 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;
> 

Re: [UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[srinivas pandruvada]

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

Re: [UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

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.

-- 
 i.

[UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[Srinivas Pandruvada]

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: 414eef27283a ("platform/x86/intel/uncore-freq: Display uncore current frequency")
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
Added fixes tag which added current_freq_khz. But after this
tag there is reorg of code, so need manual backport for some versions.
I will separately submit to stable trees after merge.

 .../x86/intel/uncore-frequency/uncore-frequency-common.c  | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

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;
-- 
2.41.0

Re: [UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[Hans de Goede]

Hi Srinivas,

On 10/4/23 20:19, 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: 414eef27283a ("platform/x86/intel/uncore-freq: Display uncore current frequency")
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> Added fixes tag which added current_freq_khz. But after this
> tag there is reorg of code, so need manual backport for some versions.
> I will separately submit to stable trees after merge.

Can you for future updated patches please use the ususal [PATCH v2],
[PATCH v3], etc. prefix ?

Also please document the changes per version after the cutline, e.g. :

```
---
Changes in v2:
- Added fixes tag which added current_freq_khz. Note after this
  tag there is reorg of code, so need manual backport for some versions.
  I will separately submit to stable trees after merge.
```

Regards,

Hans




> 
>  .../x86/intel/uncore-frequency/uncore-frequency-common.c  | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> 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;

Re: [UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[Andy Shevchenko]

On Thu, Oct 05, 2023 at 09:03:20AM +0200, Hans de Goede wrote:
> On 10/4/23 20:19, Srinivas Pandruvada wrote:

…

> Can you for future updated patches please use the ususal [PATCH v2],
> [PATCH v3], etc. prefix ?

Side note: the `git format-patch ...` has -vX option, where X is a version.

-- 
With Best Regards,
Andy Shevchenko

Re: [UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[srinivas pandruvada]

Hi Hans,


On Thu, 2023-10-05 at 09:03 +0200, Hans de Goede wrote:
> Hi Srinivas,
> 
> On 10/4/23 20:19, 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: 414eef27283a ("platform/x86/intel/uncore-freq: Display
> > uncore current frequency")
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> > ---
> > Added fixes tag which added current_freq_khz. But after this
> > tag there is reorg of code, so need manual backport for some
> > versions.
> > I will separately submit to stable trees after merge.
> 
> Can you for future updated patches please use the ususal [PATCH v2],
> [PATCH v3], etc. prefix ?
> 
Sorry about this. I usually do with code change. But will keep in mind.

Thanks,
Srinivas


> Also please document the changes per version after the cutline, e.g.
> :
> 
> ```
> ---
> Changes in v2:
> - Added fixes tag which added current_freq_khz. Note after this
>   tag there is reorg of code, so need manual backport for some
> versions.
>   I will separately submit to stable trees after merge.
> ```
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > 
> >  .../x86/intel/uncore-frequency/uncore-frequency-common.c  | 8
> > ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > 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;
> 

Re: [UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

On Thu, 5 Oct 2023, srinivas pandruvada wrote:
> On Thu, 2023-10-05 at 09:03 +0200, Hans de Goede wrote:
> > Hi Srinivas,
> > 
> > On 10/4/23 20:19, 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: 414eef27283a ("platform/x86/intel/uncore-freq: Display
> > > uncore current frequency")
> > > Signed-off-by: Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com>
> > > ---
> > > Added fixes tag which added current_freq_khz. But after this
> > > tag there is reorg of code, so need manual backport for some
> > > versions.
> > > I will separately submit to stable trees after merge.
> > 
> > Can you for future updated patches please use the ususal [PATCH v2],
> > [PATCH v3], etc. prefix ?
> > 
> Sorry about this. I usually do with code change. But will keep in mind.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [UPDATE][PATCH] platform/x86/intel-uncore-freq: Conditionally create attribute for read frequency

[Hans de Goede]

Hi,

On 10/4/23 20:19, 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: 414eef27283a ("platform/x86/intel/uncore-freq: Display uncore current frequency")
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> Added fixes tag which added current_freq_khz. But after this
> tag there is reorg of code, so need manual backport for some versions.
> I will separately submit to stable trees after merge.

Thank you for your patch/series, I've applied this patch
(series) to the pdx86 fixes branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes

Note it will show up in the pdx86 fixes branch once I've pushed
my local branch there, which might take a while.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans




> 
>  .../x86/intel/uncore-frequency/uncore-frequency-common.c  | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> 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;