[PATCH] platform/x86: hp-bioscfg: Use more common code in hp_init_bios_package_attribute()

[PATCH] platform/x86: hp-bioscfg: Use more common code in hp_init_bios_package_attribute()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 18 Jun 2026 21:42:27 +0200

Use an existing label once more so that a bit of common code can be better
reused at the end of this function implementation.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 27fd6cd21529..819313a4425a 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -692,8 +692,7 @@ static int hp_init_bios_package_attribute(enum hp_wmi_data_type attr_type,
 	if (ret) {
 		pr_debug("Failed to populate integer package data. Error [0%0x]\n",
 			 ret);
-		kfree(str_value);
-		return ret;
+		goto pack_attr_exit;
 	}
 
 	if (!str_value || !str_value[0]) {
-- 
2.54.0

Re: [PATCH] platform/x86: hp-bioscfg: Use more common code in hp_init_bios_package_attribute()

[Ilpo Järvinen]

On Thu, 18 Jun 2026, Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 18 Jun 2026 21:42:27 +0200
> 
> Use an existing label once more so that a bit of common code can be better
> reused at the end of this function implementation.
> 
> This issue was detected by using the Coccinelle software.

This patch leaves me quite unimpressed of Coccinelle's abilities.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 27fd6cd21529..819313a4425a 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -692,8 +692,7 @@ static int hp_init_bios_package_attribute(enum hp_wmi_data_type attr_type,
>  	if (ret) {
>  		pr_debug("Failed to populate integer package data. Error [0%0x]\n",
>  			 ret);
> -		kfree(str_value);
> -		return ret;
> +		goto pack_attr_exit;

If a call fails, it's expected to handle cleanup itself --- which is 
exactly what hp_convert_hexstr_to_str() appears to be doing (by not 
writing into *str until it's committed to returning 0). So why is 
this kfree() necessary in the first place?!?

-- 
 i.

Re: [PATCH] platform/x86: hp-bioscfg: Use more common code in hp_init_bios_package_attribute()

[Markus Elfring]

>> Use an existing label once more so that a bit of common code can be better
>> reused at the end of this function implementation.
>>
>> This issue was detected by using the Coccinelle software.
> 
> This patch leaves me quite unimpressed of Coccinelle's abilities.

The intention of this development tool is not to impress you with a possible
source code transformation.


…
>> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
>> @@ -692,8 +692,7 @@ static int hp_init_bios_package_attribute(enum hp_wmi_data_type attr_type,
>>  	if (ret) {
>>  		pr_debug("Failed to populate integer package data. Error [0%0x]\n",
>>  			 ret);
>> -		kfree(str_value);
>> -		return ret;
>> +		goto pack_attr_exit;

Another source code search approach pointed implementation details out
for further development considerations.


> If a call fails, it's expected to handle cleanup itself --- which is 
> exactly what hp_convert_hexstr_to_str() appears to be doing (by not 
> writing into *str until it's committed to returning 0). So why is 
> this kfree() necessary in the first place?!?
Would any contributors like to adjust background information accordingly?

Regards,
Markus

Re: [PATCH] platform/x86: hp-bioscfg: Use more common code in hp_init_bios_package_attribute()

[Ilpo Järvinen]

On Wed, 1 Jul 2026, Markus Elfring wrote:

> >> Use an existing label once more so that a bit of common code can be better
> >> reused at the end of this function implementation.
> >>
> >> This issue was detected by using the Coccinelle software.
> > 
> > This patch leaves me quite unimpressed of Coccinelle's abilities.
> 
> The intention of this development tool is not to impress you with a possible
> source code transformation.

That's exactly in the heart of the problem.

I'm not interested in seeing patches that focus on "source code 
transformation" but patches that actually improve things. I don't want to 
be the person who has to differentiate those out of all "source code 
transformations" coccinelle can be automated to do. The person doing that 
differentiation should be the submitter (in this case, you), which clearly 
didn't happen here.

You stated: "this issue" so what exactly is the issue coccinelle found 
for you? Is it "issue" at all? I don't think it is. Instead, there seems 
to be another issue with this code but that was not found by neither 
coccinelle nor you. The former doesn't surprise me because understanding 
code is not within coccinelle's capabilities.

I suppose you'll once again deflect and suggest lets write another 
non-intelligent transformation for coccinelle (which will have its own 
false positives like this particular source code transformation did have). 
But that would entirely miss my point, which is:

You should really look through and understand all the code related to the 
change suggested by coccinelle before considering the change for 
submission.

That being said, you can write just as many coccinelle transformations you 
want and more, but please only send those results from them which actually 
improve things to the kernel development lists. In the other cases, figure 
out what is the correct way to address the root issue which is not 
necessarily the one that matches that suggested by coccinelle. If you want 
to write more transformations for them, again up to you, but then you'll 
have another set of false positives to weed out.

> >> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> >> @@ -692,8 +692,7 @@ static int hp_init_bios_package_attribute(enum hp_wmi_data_type attr_type,
> >>  	if (ret) {
> >>  		pr_debug("Failed to populate integer package data. Error [0%0x]\n",
> >>  			 ret);
> >> -		kfree(str_value);
> >> -		return ret;
> >> +		goto pack_attr_exit;
> 
> Another source code search approach pointed implementation details out
> for further development considerations.
> 
> 
> > If a call fails, it's expected to handle cleanup itself --- which is 
> > exactly what hp_convert_hexstr_to_str() appears to be doing (by not 
> > writing into *str until it's committed to returning 0). So why is 
> > this kfree() necessary in the first place?!?
>
> Would any contributors like to adjust background information accordingly?

For the other two points, your intent has totally eluded me so I cannot 
comment on those point. Please rephrase.

-- 
 i.