linux-kernel - Re: [PATCH] platform/x86: system76: Reducing redundant conditional judgments in system76

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <079a7474-0255-f486-9847-97d67c2e44d2@linux.intel.com>
Date: Tue, 20 Aug 2024 14:06:40 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Xi Huang <xuiagnh@...il.com>
cc: jeremy@...tem76.com, productdev@...tem76.com, 
    Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: system76: Reducing redundant conditional
 judgments in system76_add()

On Tue, 20 Aug 2024, Xi Huang wrote:

> In case of an error, goto jumps to the “error” label,
> where the if (data->has_open_ec) check is redundant in most cases.
> Since the conditions for most goto statements have already
> been satisfied by if (data->has_open_ec),the code has been modified to
> improve execution speed.

And why would the error rollback path need to be improved in execution 
speed? In any case, this change is going to below the noise level when it 
comes to a measurable impact.

I'm sorry but I don't think this change is useful.

-- 
 i.

> Signed-off-by: Xi Huang <xuiagnh@...il.com>
> ---
>  drivers/platform/x86/system76_acpi.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index 3da753b3d..05b4bf18f 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -757,33 +757,34 @@ static int system76_add(struct acpi_device *acpi_dev)
>  
>  	err = input_register_device(data->input);
>  	if (err)
> -		goto error;
> +		if (data->has_open_ec)
> +			goto free_error;
> +		else
> +			return err;
>  
>  	if (data->has_open_ec) {
>  		err = system76_get_object(data, "NFAN", &data->nfan);
>  		if (err)
> -			goto error;
> +			goto free_error;
>  
>  		err = system76_get_object(data, "NTMP", &data->ntmp);
>  		if (err)
> -			goto error;
> +			goto free_error;
>  
>  		data->therm = devm_hwmon_device_register_with_info(&acpi_dev->dev,
>  			"system76_acpi", data, &thermal_chip_info, NULL);
>  		err = PTR_ERR_OR_ZERO(data->therm);
>  		if (err)
> -			goto error;
> +			goto free_error;
>  
>  		system76_battery_init();
>  	}
>  
>  	return 0;
>  
> -error:
> -	if (data->has_open_ec) {
> -		kfree(data->ntmp);
> -		kfree(data->nfan);
> -	}
> +free_error:
> +	kfree(data->ntmp);
> +	kfree(data->nfan);
>  	return err;
>  }
>  
>