linux-kernel - Re: [PATCH] i2c: gxp: fix build failure without CONFIG_I2C

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: <c84eee23-1a25-4367-9588-6cfd27a4345f@app.fastmail.com>
Date:   Thu, 13 Apr 2023 22:19:26 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Wolfram Sang" <wsa@...nel.org>, "Arnd Bergmann" <arnd@...nel.org>
Cc:     "Verdun, Jean-Marie" <verdun@....com>,
        "Hawkins, Nick" <nick.hawkins@....com>,
        "Joel Stanley" <joel@....id.au>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: gxp: fix build failure without CONFIG_I2C_SLAVE

On Thu, Apr 13, 2023, at 21:27, Wolfram Sang wrote:
>> The gxp_i2c_slave_irq_handler() is hidden in an #ifdef, but the
>> caller uses an IS_ENABLED() check:
>> 
>> drivers/i2c/busses/i2c-gxp.c: In function 'gxp_i2c_irq_handler':
>> drivers/i2c/busses/i2c-gxp.c:467:29: error: implicit declaration of function 'gxp_i2c_slave_irq_handler'; did you mean 'gxp_i2c_irq_handler'? [-Werror=implicit-function-declaration]
>> 
>> It has to consistently use one method or the other to avoid warnings,
>> so move to IS_ENABLED() here for readability and build coverage, and
>> move the #ifdef in linux/i2c.h to allow building it as dead code.
>
> Can't we have a solution which modifies this driver only (maybe by
> defining an empty irq handler for the non-IS_ENABLED part?)? Doesn't
> feel good to touch i2c.h only because of this...

The idea was to avoid the problem for the next driver as well. At the
moment, there are #ifdef checks like this one in three more drivers,
and I suspect we could clean them all up the same way.

>> -#if IS_ENABLED(CONFIG_I2C_SLAVE)
>>  enum i2c_slave_event {
>>  	I2C_SLAVE_READ_REQUESTED,
>>  	I2C_SLAVE_WRITE_REQUESTED,
>> @@ -396,9 +395,10 @@ enum i2c_slave_event {
>>  
>>  int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
>>  int i2c_slave_unregister(struct i2c_client *client);
>
> ... especially with moving these two prototypes out of the protected
> block. The functions themselves are also protected by the same symbol
> via the Makefile. I'd rather get a build error right away than a linker
> error later if a driver misses to select I2C_SLAVE. Or do I miss
> something?

I usually prefer having greater build coverage by allowing symbols
to be referenced by dead code that gets eliminated during the compile
stage. It helps find issues in the unused code paths at compile
time, and tends to be easier to read. than a group of #ifdef checks.

The only time I would put a declaration in an #ifdef is when
there is an #else path with an empty inline function.

     Arnd