Re: [PATCH v4 03/11] platform/x86/amd/pmf: Add support SPS PMF feature

Re: [PATCH v4 03/11] platform/x86/amd/pmf: Add support SPS PMF feature

[Nathan Chancellor]

Hi Shyam,

On Tue, Aug 02, 2022 at 08:41:41PM +0530, Shyam Sundar S K wrote:
> SPS (a.k.a. Static Power Slider) gives a feel of Windows performance
> power slider for the Linux users, where the user selects a certain
> mode (like "balanced", "low-power" or "performance") and the thermals
> associated with each selected mode gets applied from the silicon
> side via the mailboxes defined through PMFW.
> 
> PMF driver hooks to platform_profile by reading the PMF ACPI fn9 to
> see if the support is being advertised by ACPI interface.
> 
> If supported, the PMF driver reacts to platform_profile selection choices
> made by the user and adjust the system thermal behavior.
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

<snip>

> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> new file mode 100644
> index 000000000000..ef4df3fd774b
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/sps.c

<snip>

> +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
> +{
> +	u8 mode;
> +
> +	switch (pmf->current_profile) {
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		mode = POWER_MODE_PERFORMANCE;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		mode = POWER_MODE_BALANCED_POWER;
> +		break;
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		mode = POWER_MODE_POWER_SAVER;
> +		break;
> +	default:
> +		dev_err(pmf->dev, "Unknown Platform Profile.\n");
> +		break;
> +	}
> +
> +	return mode;
> +}

This patch is now in -next as commit 4c71ae414474
("platform/x86/amd/pmf: Add support SPS PMF feature"), where it causes
the following clang warning:

  drivers/platform/x86/amd/pmf/sps.c:96:2: error: variable 'mode' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized]
          default:
          ^~~~~~~
  drivers/platform/x86/amd/pmf/sps.c:101:9: note: uninitialized use occurs here
          return mode;
                 ^~~~
  drivers/platform/x86/amd/pmf/sps.c:84:9: note: initialize the variable 'mode' to silence this warning
          u8 mode;
                 ^
                  = '\0'
  1 error generated.

As far as I can tell, the default case cannot actually happen due to the
advertising of choices in amd_pmf_init_sps() and the check against those
choices in platform_profile_store() but it would be good to avoid this
warning, especially given that it is fatal with CONFIG_WERROR.

I do not mind sending a patch for this but I am a little unclear what
the best fix would be. Removing the default case would cause -Wswitch
warnings because current_profile is an enum (plus it would make finding
invalid profiles harder if there was ever a change in the core). Perhaps
changing the return type to be an int, returning an error code in the
default case, then updating the call sites to check for an error? I am
open to other suggestions (or if you want to sent a fix yourself, just
consider this a report).

Cheers,
Nathan

Re: [PATCH v4 03/11] platform/x86/amd/pmf: Add support SPS PMF feature

[Shyam Sundar S K]

Hi Nathan,

Thanks for bringing this up.

On 8/19/2022 4:17 AM, Nathan Chancellor wrote:
> Hi Shyam,
> 
> On Tue, Aug 02, 2022 at 08:41:41PM +0530, Shyam Sundar S K wrote:
>> SPS (a.k.a. Static Power Slider) gives a feel of Windows performance
>> power slider for the Linux users, where the user selects a certain
>> mode (like "balanced", "low-power" or "performance") and the thermals
>> associated with each selected mode gets applied from the silicon
>> side via the mailboxes defined through PMFW.
>>
>> PMF driver hooks to platform_profile by reading the PMF ACPI fn9 to
>> see if the support is being advertised by ACPI interface.
>>
>> If supported, the PMF driver reacts to platform_profile selection choices
>> made by the user and adjust the system thermal behavior.
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> 
> <snip>
> 
>> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
>> new file mode 100644
>> index 000000000000..ef4df3fd774b
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/pmf/sps.c
> 
> <snip>
> 
>> +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
>> +{
>> +	u8 mode;
>> +
>> +	switch (pmf->current_profile) {
>> +	case PLATFORM_PROFILE_PERFORMANCE:
>> +		mode = POWER_MODE_PERFORMANCE;
>> +		break;
>> +	case PLATFORM_PROFILE_BALANCED:
>> +		mode = POWER_MODE_BALANCED_POWER;
>> +		break;
>> +	case PLATFORM_PROFILE_LOW_POWER:
>> +		mode = POWER_MODE_POWER_SAVER;
>> +		break;
>> +	default:
>> +		dev_err(pmf->dev, "Unknown Platform Profile.\n");
>> +		break;
>> +	}
>> +
>> +	return mode;
>> +}
> 
> This patch is now in -next as commit 4c71ae414474
> ("platform/x86/amd/pmf: Add support SPS PMF feature"), where it causes
> the following clang warning:
> 
>   drivers/platform/x86/amd/pmf/sps.c:96:2: error: variable 'mode' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized]
>           default:
>           ^~~~~~~
>   drivers/platform/x86/amd/pmf/sps.c:101:9: note: uninitialized use occurs here
>           return mode;
>                  ^~~~
>   drivers/platform/x86/amd/pmf/sps.c:84:9: note: initialize the variable 'mode' to silence this warning
>           u8 mode;
>                  ^
>                   = '\0'
>   1 error generated.
> 
> As far as I can tell, the default case cannot actually happen due to the
> advertising of choices in amd_pmf_init_sps() and the check against those
> choices in platform_profile_store() but it would be good to avoid this
> warning, especially given that it is fatal with CONFIG_WERROR.
> 
> I do not mind sending a patch for this but I am a little unclear what
> the best fix would be. Removing the default case would cause -Wswitch
> warnings because current_profile is an enum (plus it would make finding
> invalid profiles harder if there was ever a change in the core). Perhaps
> changing the return type to be an int, returning an error code in the
> default case, then updating the call sites to check for an error? I am
> open to other suggestions (or if you want to sent a fix yourself, just
> consider this a report).

yes, you are right. We can just change the return an error code like the
other driver implementing the platform_profile. Like below.

diff --git a/drivers/platform/x86/amd/pmf/pmf.h
b/drivers/platform/x86/amd/pmf/pmf.h
index 7613ed2ef6e3..172610f93bd1 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -303,7 +303,7 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev);
 int amd_pmf_get_power_source(void);

 /* SPS Layer */
-u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
+int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
 void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
                           struct amd_pmf_static_slider_granular *table);
 int amd_pmf_init_sps(struct amd_pmf_dev *dev);
diff --git a/drivers/platform/x86/amd/pmf/sps.c
b/drivers/platform/x86/amd/pmf/sps.c
index 8923e29cc6ca..dba7e36962dc 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -79,9 +79,9 @@ static int amd_pmf_profile_get(struct
platform_profile_handler *pprof,
        return 0;
 }

-u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
+int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
 {
-       u8 mode;
+       int mode;

        switch (pmf->current_profile) {
        case PLATFORM_PROFILE_PERFORMANCE:
@@ -95,7 +95,7 @@ u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
                break;
        default:
                dev_err(pmf->dev, "Unknown Platform Profile.\n");
-               break;
+               return -EOPNOTSUPP;
        }

        return mode;
@@ -105,10 +105,13 @@ static int amd_pmf_profile_set(struct
platform_profile_handler *pprof,
                               enum platform_profile_option profile)
 {
        struct amd_pmf_dev *pmf = container_of(pprof, struct
amd_pmf_dev, pprof);
-       u8 mode;
+       int mode;

        pmf->current_profile = profile;
        mode = amd_pmf_get_pprof_modes(pmf);
+       if (mode < 0)
+               return mode;
+
        amd_pmf_update_slider(pmf, SLIDER_OP_SET, mode, NULL);
        return 0;
 }


Thanks,
Shyam

> 
> Cheers,
> Nathan
> 

Re: [PATCH v4 03/11] platform/x86/amd/pmf: Add support SPS PMF feature

[Hans de Goede]

Hi Shyam,

On 8/19/22 10:58, Shyam Sundar S K wrote:
> Hi Nathan,
> 
> Thanks for bringing this up.
> 
> On 8/19/2022 4:17 AM, Nathan Chancellor wrote:
>> Hi Shyam,
>>
>> On Tue, Aug 02, 2022 at 08:41:41PM +0530, Shyam Sundar S K wrote:
>>> SPS (a.k.a. Static Power Slider) gives a feel of Windows performance
>>> power slider for the Linux users, where the user selects a certain
>>> mode (like "balanced", "low-power" or "performance") and the thermals
>>> associated with each selected mode gets applied from the silicon
>>> side via the mailboxes defined through PMFW.
>>>
>>> PMF driver hooks to platform_profile by reading the PMF ACPI fn9 to
>>> see if the support is being advertised by ACPI interface.
>>>
>>> If supported, the PMF driver reacts to platform_profile selection choices
>>> made by the user and adjust the system thermal behavior.
>>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>
>> <snip>
>>
>>> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
>>> new file mode 100644
>>> index 000000000000..ef4df3fd774b
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/amd/pmf/sps.c
>>
>> <snip>
>>
>>> +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
>>> +{
>>> +	u8 mode;
>>> +
>>> +	switch (pmf->current_profile) {
>>> +	case PLATFORM_PROFILE_PERFORMANCE:
>>> +		mode = POWER_MODE_PERFORMANCE;
>>> +		break;
>>> +	case PLATFORM_PROFILE_BALANCED:
>>> +		mode = POWER_MODE_BALANCED_POWER;
>>> +		break;
>>> +	case PLATFORM_PROFILE_LOW_POWER:
>>> +		mode = POWER_MODE_POWER_SAVER;
>>> +		break;
>>> +	default:
>>> +		dev_err(pmf->dev, "Unknown Platform Profile.\n");
>>> +		break;
>>> +	}
>>> +
>>> +	return mode;
>>> +}
>>
>> This patch is now in -next as commit 4c71ae414474
>> ("platform/x86/amd/pmf: Add support SPS PMF feature"), where it causes
>> the following clang warning:
>>
>>   drivers/platform/x86/amd/pmf/sps.c:96:2: error: variable 'mode' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized]
>>           default:
>>           ^~~~~~~
>>   drivers/platform/x86/amd/pmf/sps.c:101:9: note: uninitialized use occurs here
>>           return mode;
>>                  ^~~~
>>   drivers/platform/x86/amd/pmf/sps.c:84:9: note: initialize the variable 'mode' to silence this warning
>>           u8 mode;
>>                  ^
>>                   = '\0'
>>   1 error generated.
>>
>> As far as I can tell, the default case cannot actually happen due to the
>> advertising of choices in amd_pmf_init_sps() and the check against those
>> choices in platform_profile_store() but it would be good to avoid this
>> warning, especially given that it is fatal with CONFIG_WERROR.
>>
>> I do not mind sending a patch for this but I am a little unclear what
>> the best fix would be. Removing the default case would cause -Wswitch
>> warnings because current_profile is an enum (plus it would make finding
>> invalid profiles harder if there was ever a change in the core). Perhaps
>> changing the return type to be an int, returning an error code in the
>> default case, then updating the call sites to check for an error? I am
>> open to other suggestions (or if you want to sent a fix yourself, just
>> consider this a report).
> 
> yes, you are right. We can just change the return an error code like the
> other driver implementing the platform_profile. Like below.

Yes returning  -EOPNOTSUPP is the right thing to do in the
default case.

Can you turn this into a proper patch/commit and submit
it please?

Regards,

Hans


> 
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
> b/drivers/platform/x86/amd/pmf/pmf.h
> index 7613ed2ef6e3..172610f93bd1 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -303,7 +303,7 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev);
>  int amd_pmf_get_power_source(void);
> 
>  /* SPS Layer */
> -u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
> +int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
>  void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
>                            struct amd_pmf_static_slider_granular *table);
>  int amd_pmf_init_sps(struct amd_pmf_dev *dev);
> diff --git a/drivers/platform/x86/amd/pmf/sps.c
> b/drivers/platform/x86/amd/pmf/sps.c
> index 8923e29cc6ca..dba7e36962dc 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -79,9 +79,9 @@ static int amd_pmf_profile_get(struct
> platform_profile_handler *pprof,
>         return 0;
>  }
> 
> -u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
> +int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
>  {
> -       u8 mode;
> +       int mode;
> 
>         switch (pmf->current_profile) {
>         case PLATFORM_PROFILE_PERFORMANCE:
> @@ -95,7 +95,7 @@ u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
>                 break;
>         default:
>                 dev_err(pmf->dev, "Unknown Platform Profile.\n");
> -               break;
> +               return -EOPNOTSUPP;
>         }
> 
>         return mode;
> @@ -105,10 +105,13 @@ static int amd_pmf_profile_set(struct
> platform_profile_handler *pprof,
>                                enum platform_profile_option profile)
>  {
>         struct amd_pmf_dev *pmf = container_of(pprof, struct
> amd_pmf_dev, pprof);
> -       u8 mode;
> +       int mode;
> 
>         pmf->current_profile = profile;
>         mode = amd_pmf_get_pprof_modes(pmf);
> +       if (mode < 0)
> +               return mode;
> +
>         amd_pmf_update_slider(pmf, SLIDER_OP_SET, mode, NULL);
>         return 0;
>  }
> 
> 
> Thanks,
> Shyam
> 
>>
>> Cheers,
>> Nathan
>>
>