Re: [PATCH 1/2] hwmon: (pmbus/ltc4286) Improve device matching

[Guenter Roeck <linux@roeck-us.net>]

On 7/10/24 08:48, Uwe Kleine-König wrote:
> Hello Guenter,
> 
> On Wed, Jul 10, 2024 at 07:09:28AM -0700, Guenter Roeck wrote:
>> On 7/10/24 01:35, Uwe Kleine-König wrote:
>>> The devices supported by this driver report the model name in their
>>> register space. The way this is evaluated allows longer strings than the
>>> driver's model list. Document this behaviour in a code comment to lessen
>>> the surprise for the next reader.
>>>
>>> Additionally emit the reported model name in case of a mismatch.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
>>> ---
>>>    drivers/hwmon/pmbus/ltc4286.c | 12 +++++++++---
>>>    1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
>>> index 9e7ceeb7e789..2e5532300eff 100644
>>> --- a/drivers/hwmon/pmbus/ltc4286.c
>>> +++ b/drivers/hwmon/pmbus/ltc4286.c
>>> @@ -95,13 +95,19 @@ static int ltc4286_probe(struct i2c_client *client)
>>>    				     "Failed to read manufacturer model\n");
>>>    	}
>>> -	for (mid = ltc4286_id; mid->name[0]; mid++) {
>>> +	for (mid = ltc4286_id; mid->name[0]; mid++)
>>> +		/*
>>> +		 * Note that by limiting the comparison to strlen(mid->name)
>>> +		 * chars, the device reporting "lTc4286chocolade" is accepted,
>>> +		 * too.
>>> +		 */
>>
>> This is misleading; the desired match is LTC4286 and all its variants (LTC4286[A-Z] and
>> whatever else the vendor can come up with), i.e., it is supposed to include all device
>> variants, and ignoring case since it is irrelevant. Referring to the odd string just
>> makes that look unnecessarily bad. I am not going to apply this patch, sorry.
> 
> You're quite an optimist, expecting "whatever the vendor can come up
> with" but nothing bad :-)
> 

"optimist" is relative. A perfectly valid alternative would be to _not_ do any
testing at all. After all, this is not a detect function, this is the probe
function, which should only be called _after_ the chip has been identified.

Since the model number is not used for anything but extra validation, one might
as well argue that the validation is unnecessary and can or should be dropped
to reduce boot time. Of course, given the vagueness of the PMBus specification,
that might result in fatal consequences if the wrong chip is instantiated,
so I think that validation does make sense, and I suggest to add it for all
PMBus drivers. However, one can overdo it (and not all drivers do it).

> Anyhow, what about updating the comment to read:
> 
> 	Note that by limiting the comparison to strlen(mid->name) chars,
> 	matching for devices that report their model with a variant
> 	suffix is supported.
> 
> While looking at the code again, I spotted a (theoretic) bug: Given that
> block_buffer isn't initialized at function entry, it might well contain
> "LTC4286something" (which might even be realistic if the driver just
> probed on a different bus?). Now if i2c_smbus_read_block_data(...
> PMBUS_MFR_MODEL, ...) returned something between 0 and 6, we're looking
> at bytes that didn't come from the block read.
> 

Yes, I would agree that a check ensuring that ret >= 7 would make sense.

Thanks,
Guenter