linux-kernel - Re: [PATCH 00/31] Remove use of i2c_match

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: <fcafe904-383c-49c0-b576-81cbcde045c5@roeck-us.net>
Date: Mon, 8 Apr 2024 04:49:43 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Andrew Davis <afd@...com>
Cc: Jean Delvare <jdelvare@...e.com>, Juerg Haefliger <juergh@...ton.me>,
	Riku Voipio <riku.voipio@....fi>, linux-hwmon@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/31] Remove use of i2c_match_id in HWMON

On Wed, Apr 03, 2024 at 05:06:43PM -0500, Andrew Davis wrote:
> On 4/3/24 4:30 PM, Guenter Roeck wrote:
> > On Wed, Apr 03, 2024 at 03:36:02PM -0500, Andrew Davis wrote:
> > > Hello all,
> > > 
> > > Goal here is to remove the i2c_match_id() function from all drivers.
> > > Using i2c_get_match_data() can simplify code and has some other
> > > benefits described in the patches.
> > > 
> > 
> > The return value from i2c_match_id() is typically an integer (chip ID)
> > starting with 0. Previously it has been claimed that this would be
> > unacceptable for i2c_get_match_data(), and chip IDs were changed to start
> > with 1. Commit ac0c26bae662 ("hwmon: (lm25066) Use i2c_get_match_data()")
> > is an example. Either this series is wrong, or the previous claim that
> > chip IDs (i.e., the content of .driver_data or .data) must not be 0 was
> > wrong. Which one is it ? I find it very confusing that the chip type for
> > some drivers now starts with 1 and for others with 0. Given that, I am not
> > inclined to accept this series unless it is explained in detail why the
> > chip type enum in, for example, drivers/hwmon/pmbus/lm25066.c has to start
> > with one but is ok to start with 0 for all drivers affected by this
> > series. Quite frankly, even if there is some kind of explanation, I am not
> > sure if I am going to accept it because future driver developers won't
> > know if they have to start chip types with 0 or 1.
> > 
> 
> i2c_get_match_data() has no issue with returning 0 when the driver_data
> for the match is also 0 (as it will be when the chip type is 0 here).
> 
> The confusion might be that returning 0 is also considered a failure code.
> This is a problem in general with returning errors in-band with data, and
> that is nothing new as i2c_match_id() does the same thing.
> 
> Actually, i2c_match_id() is worse as most of these drivers take the result
> from that and immediately dereference it. Meaning if i2c_match_id() ever did
> failed to find a match, they would crash before this series. Luckily i2c_match_id()
> can't fail to find a match as far as I can tell, and so for the same reason
> neither can i2c_get_match_data(), which means if 0 is returned it is always
> because the chip ID was actually 0.
> 
> At some point we should switch all the *_get_match_data() functions to
> return an error code and put the match if found as a argument pointer.
> Forcing everyone to changing the chip type to avoid 0 as done in
> ac0c26bae662 is the wrong way to fix an issue like that.
> 

That doesn't really answer my question. It does not explain why it was
necessary to change the chip ID base for other drivers from 0 to 1,
but not for the drivers in this series. I fail to see the difference,
and I have to assume that others looking into the code will have the
same problem.

Guenter