linux-kernel - Re: [PATCH] hwmon: (core) Avoid ifdef in C source file

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: <041a52c7-ac0b-4a78-8b39-4fc4ac4d2fd2@roeck-us.net>
Date: Tue, 12 Nov 2024 22:52:36 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Thomas Weißschuh <linux@...ssschuh.net>,
 Jean Delvare <jdelvare@...e.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (core) Avoid ifdef in C source file

On 11/12/24 20:39, Thomas Weißschuh wrote:
> Using an #ifdef in a C source files to have different definitions
> of the same symbol makes the code harder to read and understand.
> Furthermore it makes it harder to test compilation of the different
> branches.
> 
> Replace the ifdeffery with IS_ENABLED() which is just a normal
> conditional.
> The resulting binary is still the same as before as the compiler
> optimizes away all the unused code and definitions.
> 
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> ---
> This confused me a bit while looking at the implementation of
> HWMON_C_REGISTER_TZ.
> ---
>   drivers/hwmon/hwmon.c | 21 ++++++---------------
>   1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 9c35c4d0369d7aad7ea61ccd25f4f63fc98b9e02..86fb674c85d3f54d475be014c3fd3dd74c815c57 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -147,11 +147,6 @@ static DEFINE_IDA(hwmon_ida);
>   
>   /* Thermal zone handling */
>   
> -/*
> - * The complex conditional is necessary to avoid a cyclic dependency
> - * between hwmon and thermal_sys modules.
> - */
> -#ifdef CONFIG_THERMAL_OF
>   static int hwmon_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
>   {
>   	struct hwmon_thermal_data *tdata = thermal_zone_device_priv(tz);
> @@ -257,6 +252,9 @@ static int hwmon_thermal_register_sensors(struct device *dev)
>   	void *drvdata = dev_get_drvdata(dev);
>   	int i;
>   
> +	if (!IS_ENABLED(CONFIG_THERMAL_OF))
> +		return 0;
> +
>   	for (i = 1; info[i]; i++) {
>   		int j;
>   
> @@ -285,6 +283,9 @@ static void hwmon_thermal_notify(struct device *dev, int index)
>   	struct hwmon_device *hwdev = to_hwmon_device(dev);
>   	struct hwmon_thermal_data *tzdata;
>   
> +	if (!IS_ENABLED(CONFIG_THERMAL_OF))
> +		return;
> +
>   	list_for_each_entry(tzdata, &hwdev->tzdata, node) {
>   		if (tzdata->index == index) {
>   			thermal_zone_device_update(tzdata->tzd,

There is no dummy function for thermal_zone_device_update().
I really don't want to trust the compiler/linker to remove that code
unless someone points me to a document explaining that it is guaranteed
to not cause any problems.

Guenter

> @@ -293,16 +294,6 @@ static void hwmon_thermal_notify(struct device *dev, int index)
>   	}
>   }
>   
> -#else
> -static int hwmon_thermal_register_sensors(struct device *dev)
> -{
> -	return 0;
> -}
> -
> -static void hwmon_thermal_notify(struct device *dev, int index) { }
> -
> -#endif /* IS_REACHABLE(CONFIG_THERMAL) && ... */
> -
>   static int hwmon_attr_base(enum hwmon_sensor_types type)
>   {
>   	if (type == hwmon_in || type == hwmon_intrusion)
> 
> ---
> base-commit: 3022e9d00ebec31ed435ae0844e3f235dba998a9
> change-id: 20241113-hwmon-thermal-2d2da581c276
> 
> Best regards,