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: <3f1914da-4f94-415e-8c46-8731834e51a2@roeck-us.net>
Date: Wed, 13 Nov 2024 22:51:37 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (core) Avoid ifdef in C source file

On 11/13/24 20:40, Thomas Weißschuh wrote:
> Hi Guenter,
> 
> On 2024-11-12 22:52:36-0800, Guenter Roeck wrote:
>> 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.
> 
> I'm fairly sure that a declaration should be enough, and believe
> to remember seeing such advise somewhere.
> However there is not even a function declaration with !CONFIG_THERMAL.
> So I can add an actual stub for it for v2.
> 
> What do you think?
> 
You mean an extern declaration without the actual function ?
I'd really want to see that documented. It would seem rather unusual.

Besides, there are several other #ifdefs in the same file, so I am not
as much bothered about this as you are.

Thanks,
Guenter