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@xxxxxxxxxx>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>
> <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
>
