platform/x86: alienware-wmi-wmax: Fix uninitialized variable due to bad error handling

[PATCH] platform/x86: alienware-wmi-wmax: Fix uninitialized variable due to bad error handling

Posted by Kurt Borja 9 months, 3 weeks ago

wmax_thermal_information() may also return -ENOMSG, which would leave
`id` uninitialized in thermal_profile_probe.

Reorder and modify logic to catch all errors.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/Z_-KVqNbD9ygvE2X@stanley.mountain
Fixes: 27e9e6339896 ("platform/x86: alienware-wmi: Refactor thermal control methods")
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
Hi all,

@Ilpo: This will definitely conflict with the for-next branch when
merging.

Also, the fixes tag references a commit from before the split (same
series though), but ofc this fix is meant to be applied on top of it
(fixes branch). Is this ok or would it be better to change the fixes
tag to the "split" commit?
---
 drivers/platform/x86/dell/alienware-wmi-wmax.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
index 3d3014b5adf046c94c1ebf39a0e28a92622b40d6..b8e71f06fdde347573bff5c27a9ba53a0efcacae 100644
--- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
+++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
@@ -607,12 +607,10 @@ static int thermal_profile_probe(void *drvdata, unsigned long *choices)
 	for (u32 i = 0; i < sys_desc[3]; i++) {
 		ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_LIST_IDS,
 					       i + first_mode, &out_data);
-
-		if (ret == -EIO)
-			return ret;
-
 		if (ret == -EBADRQC)
 			break;
+		if (ret)
+			return ret;
 
 		if (!is_wmax_thermal_code(out_data))
 			continue;

---
base-commit: fcf27a6a926fd9eeba39e9c3fde43c9298fe284e
change-id: 20250416-smatch-fix-d1191e2514f5

Best regards,
-- 
 ~ Kurt

Re: [PATCH] platform/x86: alienware-wmi-wmax: Fix uninitialized variable due to bad error handling

Posted by Ilpo Järvinen 9 months, 3 weeks ago

On Wed, 16 Apr 2025 13:50:23 -0300, Kurt Borja wrote:

> wmax_thermal_information() may also return -ENOMSG, which would leave
> `id` uninitialized in thermal_profile_probe.
> 
> Reorder and modify logic to catch all errors.
> 
> 

Thank you for your contribution, it has been applied to my local
review-ilpo-fixes branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-fixes branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86: alienware-wmi-wmax: Fix uninitialized variable due to bad error handling
      commit: 4a8e04e2bdcb98d513e97b039899bda03b07bcf2

--
 i.

Re: [PATCH] platform/x86: alienware-wmi-wmax: Fix uninitialized variable due to bad error handling

Posted by Ilpo Järvinen 9 months, 3 weeks ago

On Wed, 16 Apr 2025, Kurt Borja wrote:

> wmax_thermal_information() may also return -ENOMSG, which would leave
> `id` uninitialized in thermal_profile_probe.
> 
> Reorder and modify logic to catch all errors.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/Z_-KVqNbD9ygvE2X@stanley.mountain
> Fixes: 27e9e6339896 ("platform/x86: alienware-wmi: Refactor thermal control methods")
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
> Hi all,
> 
> @Ilpo: This will definitely conflict with the for-next branch when
> merging.

Okay, thanks for the heads up (I'll eventually merge fixes into for-next
once I merge this fix).

> Also, the fixes tag references a commit from before the split (same
> series though), but ofc this fix is meant to be applied on top of it
> (fixes branch). Is this ok or would it be better to change the fixes
> tag to the "split" commit?

Pointing to the correct commit is preferred.

It doesn't look very likely that the series would be "split" such that 
only a part of it appears in a specific stable kernel so the difference 
shouldn't matter anyway.

In general, stable people would just send you a notification if something 
cannot be backported to some stable version due to a conflict, and they'd 
depend on you to submit the amended backport anyway so it's not much extra 
"work" for them if something ends up conflicting. (And I don't think your 
inbox would be exactly filling from stable notifications unlike mine --- 
one of those joys of being a subsystem maintainer).

-- 
 i.

> ---
>  drivers/platform/x86/dell/alienware-wmi-wmax.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
> index 3d3014b5adf046c94c1ebf39a0e28a92622b40d6..b8e71f06fdde347573bff5c27a9ba53a0efcacae 100644
> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
> @@ -607,12 +607,10 @@ static int thermal_profile_probe(void *drvdata, unsigned long *choices)
>  	for (u32 i = 0; i < sys_desc[3]; i++) {
>  		ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_LIST_IDS,
>  					       i + first_mode, &out_data);
> -
> -		if (ret == -EIO)
> -			return ret;
> -
>  		if (ret == -EBADRQC)
>  			break;
> +		if (ret)
> +			return ret;
>  
>  		if (!is_wmax_thermal_code(out_data))
>  			continue;
> 
> ---
> base-commit: fcf27a6a926fd9eeba39e9c3fde43c9298fe284e
> change-id: 20250416-smatch-fix-d1191e2514f5
> 
> Best regards,
>

Re: [PATCH] platform/x86: alienware-wmi-wmax: Fix uninitialized variable due to bad error handling

Posted by Kurt Borja 9 months, 3 weeks ago

On Thu Apr 17, 2025 at 7:57 AM -03, Ilpo Järvinen wrote:
> On Wed, 16 Apr 2025, Kurt Borja wrote:
>
>> wmax_thermal_information() may also return -ENOMSG, which would leave
>> `id` uninitialized in thermal_profile_probe.
>> 
>> Reorder and modify logic to catch all errors.
>> 
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/r/Z_-KVqNbD9ygvE2X@stanley.mountain
>> Fixes: 27e9e6339896 ("platform/x86: alienware-wmi: Refactor thermal control methods")
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>> Hi all,
>> 
>> @Ilpo: This will definitely conflict with the for-next branch when
>> merging.
>
> Okay, thanks for the heads up (I'll eventually merge fixes into for-next
> once I merge this fix).
>
>> Also, the fixes tag references a commit from before the split (same
>> series though), but ofc this fix is meant to be applied on top of it
>> (fixes branch). Is this ok or would it be better to change the fixes
>> tag to the "split" commit?
>
> Pointing to the correct commit is preferred.
>
> It doesn't look very likely that the series would be "split" such that 
> only a part of it appears in a specific stable kernel so the difference 
> shouldn't matter anyway.

Yeah, this is what I thought too.

>
> In general, stable people would just send you a notification if something 
> cannot be backported to some stable version due to a conflict, and they'd 
> depend on you to submit the amended backport anyway so it's not much extra 
> "work" for them if something ends up conflicting. (And I don't think your 
> inbox would be exactly filling from stable notifications unlike mine --- 
> one of those joys of being a subsystem maintainer).

Guess I'm still lucky :)

Thanks for the explanation. I'm going to stop worrying so much about
stable haha

-- 
 ~ Kurt