linux-kernel - Re: [PATCH 1/2] i2c: smbus: pass write disabling bit to spd instantiating function

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] [day] [month] [year] [list]

Message-ID: <axsc6nipjntdtcgxdmlyz3awlcwawqu6k4uxjmdlx3zgzfkjhj@zlffk4sx5uaw>
Date: Thu, 24 Apr 2025 15:58:17 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: "Yo-Jung (Leo) Lin" <leo.lin@...onical.com>
Cc: Jean Delvare <jdelvare@...e.com>, 
	Wolfram Sang <wsa+renesas@...g-engineering.com>, Guenter Roeck <linux@...ck-us.net>, 
	"Chia-Lin Kao (AceLan)" <acelan.kao@...onical.com>, linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] i2c: smbus: pass write disabling bit to spd
 instantiating function

Hi,

> @@ -44,9 +44,9 @@ static inline void i2c_free_slave_host_notify_device(struct i2c_client *client)
>  #endif
>  
>  #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI)
> -void i2c_register_spd(struct i2c_adapter *adap);
> +void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled);
>  #else
> -static inline void i2c_register_spd(struct i2c_adapter *adap) { }
> +static inline void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled) { }
>  #endif

Please don't add random bool flags to function parameters,
especially in library functions. They add confusion and make the
code harder to understand. I even dislike them when they're used
inside drivers.

I'd much prefer something like this:

   static void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
   {
   	...
   }

   void i2c_register_spd_write_disable(struct i2c_adapter *adap)
   {
   	i2c_register_spd(adap, true);
   }

   void i2c_register_spd_write_enable(struct i2c_adapter *adap)
   {
   	i2c_register_spd(adap, false);
   }

This way, we gain readability at the cost of a bit of redundancy,
a trade-off I'm more than happy with.

Of course, this is just my preference (even if I'm pretty strong
about it, especially when it comes to libraries). Wolfram is free
to override me on this.

Andi