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

From: Nathan Chancellor <nathan@kernel.org>
To: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Cc: hdegoede@redhat.com, markgross@kernel.org,
	platform-driver-x86@vger.kernel.org, Patil.Reddy@amd.com,
	llvm@lists.linux.dev
Subject: Re: [PATCH v4 03/11] platform/x86/amd/pmf: Add support SPS PMF feature
Date: Thu, 18 Aug 2022 15:47:54 -0700	[thread overview]
Message-ID: <Yv7BmjjLtA3RaKju@thelio-3990X> (raw)
In-Reply-To: <20220802151149.2123699-4-Shyam-sundar.S-k@amd.com>

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