Date:   Wed, 8 Feb 2023 14:28:24 +0000
From:   Wyes Karny <wyes.karny@amd.com>
To:     Arnd Bergmann <arnd@kernel.org>
Cc:     Huang Rui <ray.huang@amd.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Perry Yuan <Perry.Yuan@amd.com>,
        Mario Limonciello <Mario.Limonciello@amd.com>,
        Arnd Bergmann <arnd@arndb.de>, Jinzhou Su <Jinzhou.Su@amd.com>,
        Meng Li <li.meng@amd.com>, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] cpufreq: amd-pstate: avoid uninitialized variable use
Message-ID: <Y+OxiFNRsL39t3tW@beas>
References: <20230207161256.271613-1-arnd@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230207161256.271613-1-arnd@kernel.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?dt2f+XH8KA1hwr1uEsvYLesl7oGa0Yywh6tGGsJcXpaMe5dsWu6TAZobdKNq?=
 =?us-ascii?Q?Ob99GLv7M2IwbLrxQuB4MHmDyD1eQRynpkKarE7Hkn724DJE3aFGMrHHJeTA?=
 =?us-ascii?Q?hrG7bj4RCClPgLu4qCHWgOHelkThv8IuwiSA+FDw4onwAyXTcu6uclURFty9?=
 =?us-ascii?Q?ri9GO9MysrsnUZoBVGU0lDWuSo0HW6I9GVmUBc/wTFCZb93Vne1i0ZM6Ey9p?=
 =?us-ascii?Q?aVRxg+n8XhzuX9p51g51Vfr98+GiPOyo7QeA8FxqXMFxkMLDcsmv4gVK5Rax?=
 =?us-ascii?Q?b271ZTu7OnrfjCpRVqPlSNOOub/MYmazJQXXTmIGxq1q0W4e+dhE4woyJbbL?=
 =?us-ascii?Q?nEDZ1NMIreHgrtm1raei7o2f5W+3bSFTUcEj/CJdo+GaLVpxuvXh1/Ezh22f?=
 =?us-ascii?Q?8k6MGlJ8mSoQLGx6enSfUGjiL38FpVO1ITM0GMBrL65k90CWKrwc6fV6uUWN?=
 =?us-ascii?Q?rzm/WW/+oZHDnE9ju6OZKvJSdWj59sz3H6mLZeZ9H4AXrgSXlT/vEVx/5Xu9?=
 =?us-ascii?Q?1V6aQ+YccOZF74zemu9WTLng2dP97NygYdHjWobDzNsq9BaZ28waS0FnT15l?=
 =?us-ascii?Q?2Iu6+pSo8VEOeq+sheQq0H/dER8imbb8X5hbyuTJlRw041jTOAS8St8Vr2Dl?=
 =?us-ascii?Q?AQH+5iJKbePEc2tztn3hc6TXJaSdrBt2eLQXZKUqAHUD6dFtkF7wK0b3vPA5?=
 =?us-ascii?Q?rRCmbmF07aj7gFm6sMRFFENWaetK4pqbOCskIYfk9TtBk1requmNk3M9f1eG?=
 =?us-ascii?Q?KuIumQZdavsPYmdofUNQ+2qqESrwuH7++Tum76UpRmld2FYBPvI3CHHb9+w3?=
 =?us-ascii?Q?SqjWOuH7RLnWoVbYCVt9zlwEsrGOdjC0pIc28Ep9gYcIWcpxHU8zAYT9Hkmt?=
 =?us-ascii?Q?DZpCr+nX+H167A4BxgI4mv1axMVq8R5Jw+hL/iJ4gfzD/uhM3bnJyO1+LpKO?=
 =?us-ascii?Q?98zZee9+eyYxDM8XHvaxKjCDMSnR4mb7iU9NTjbyIy5t1RY91ghLSQcb2MFw?=
 =?us-ascii?Q?tnaH3C8D+vzUTPRvNCSpmK+2dO1qBILXi4SH6I1iBXO+duJQ68OK3YgFwXzr?=
 =?us-ascii?Q?Hejqyr9FQKvc1xWdi/mS96QQp98ZgDo15215e/bpEuiru+Idh0+0NRCPr04s?=
 =?us-ascii?Q?Jqp7QLS5Xcdypr3pEG2yZbBHp9wwmXCdotX8XAxWWSUhJeGHeq9rHpV5LbJI?=
 =?us-ascii?Q?QTBrEnd7HiNGnYdsHBZ9wXja6Hhg+Ym6tzGsHAKzayeKY9eHyKB5kaXFPcoZ?=
 =?us-ascii?Q?KvB0CNWQn6nthokW6NyRQ8N2niiUsJIfLqYtRB4J3LBx/xQgDYALWNVwsUDR?=
 =?us-ascii?Q?avp+z0YnJdH4k47BBgMJxWEAaBSdV80ENLkJI8oSTNUh6PP8mMH8Tm9HhbWl?=
 =?us-ascii?Q?kS4/pLbHviA6NzWYdCp2TX4HPO0tSUFmdwBdUGM4/Wlx6JNcig9N8LX4C4wJ?=
 =?us-ascii?Q?Itq37xEITKbLOq+oWc0AoxDMp1Ys6b/1TyCFhCvp2bNHBppuaxx/1rSNzFgY?=
 =?us-ascii?Q?zmuQOuLkNPXd/FXRyj4e0cSvpFthgdldxZz/zZqxBkIPsyeO40Wd0DG74iKP?=
 =?us-ascii?Q?HQQaIgMw9RmbnmAhG+wNuk5HkEveisfZ0/2Zm9lV?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 06bfb395-431c-49da-8ff4-08db09e0c405
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB3876.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2023 14:28:37.5329
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: GjdGJ1fxCuUCulATJQpr0XmlLBrh+e/Q61Q2JcUNSkqZ4qh618u0kXMocuRleM1NieWf5C2HnaTaL7QIC6In5Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4103
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 07 Feb 17:12, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The new epp support causes warnings about three separate
> but related bugs:
> 
> 1) failing before allocation should just return an error:
> 
> drivers/cpufreq/amd-pstate.c:951:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>         if (!dev)
>             ^~~~
> drivers/cpufreq/amd-pstate.c:1018:9: note: uninitialized use occurs here
>         return ret;
>                ^~~
> 
> 2) wrong variable to store return code:
> 
> drivers/cpufreq/amd-pstate.c:963:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>         if (rc)
>             ^~
> drivers/cpufreq/amd-pstate.c:1019:9: note: uninitialized use occurs here
>         return ret;
>                ^~~
> drivers/cpufreq/amd-pstate.c:963:2: note: remove the 'if' if its condition is always false
>         if (rc)
>         ^~~~~~~
> 
> 3) calling amd_pstate_set_epp() in cleanup path after determining
> that it should not be called:
> 
> drivers/cpufreq/amd-pstate.c:1055:6: error: variable 'epp' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>         if (cpudata->epp_policy == cpudata->policy)
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/cpufreq/amd-pstate.c:1080:30: note: uninitialized use occurs here
>         amd_pstate_set_epp(cpudata, epp);
>                                     ^~~
> 
> All three are trivial to fix, but most likely there are additional bugs
> in this function when the error handling was not really tested.
> 
> Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/cpufreq/amd-pstate.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 168a28bed6ee..847f5f31396d 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -940,7 +940,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	int min_freq, max_freq, nominal_freq, lowest_nonlinear_freq, ret;
>  	struct amd_cpudata *cpudata;
>  	struct device *dev;
> -	int rc;
>  	u64 value;
>  
>  	/*
> @@ -950,7 +949,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	amd_perf_ctl_reset(policy->cpu);
>  	dev = get_cpu_device(policy->cpu);
>  	if (!dev)
> -		goto free_cpudata1;
> +		return -ENODEV;
>  
>  	cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
>  	if (!cpudata)
> @@ -959,8 +958,8 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>  	cpudata->cpu = policy->cpu;
>  	cpudata->epp_policy = 0;
>  
> -	rc = amd_pstate_init_perf(cpudata);
> -	if (rc)
> +	ret = amd_pstate_init_perf(cpudata);
> +	if (ret)
>  		goto free_cpudata1;
>  
>  	min_freq = amd_get_min_freq(cpudata);
> @@ -1076,9 +1075,9 @@ static void amd_pstate_epp_init(unsigned int cpu)
>  		value |= (u64)epp << 24;
>  	}
>  
> +	amd_pstate_set_epp(cpudata, epp);
>  skip_epp:
>  	WRITE_ONCE(cpudata->cppc_req_cached, value);
> -	amd_pstate_set_epp(cpudata, epp);
>  	cpufreq_cpu_put(policy);
>  }
> 

Tested this on AMD Milan (Zen 3) system.

Tested-by: Wyes Karny <wyes.karny@amd.com>

> -- 
> 2.39.1
>