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

[Hans de Goede <hdegoede@redhat.com>]

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