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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Guenter,

On Wed, Jul 10, 2024 at 12:16:55PM -0700, Guenter Roeck wrote:
> On 7/10/24 08:48, Uwe Kleine-König wrote:
> > 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@xxxxxxxxxxxx>
> > > > ---
> > > >    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).

+1 for a generic check in generic code.

One could also argue that it's an error if the device was declared to be
a ltc4286 but reports LTC4287A in its PMBUS_MFR_MODEL register.
So something like:

	mid = i2c_client_get_device_id(client);

would make sense, too. (There is a corner case that the driver is not
bound via the entries in the driver's .id_table, not sure how relevant
this is.)

> > 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.

alternatively do

	block_buffer[ret] = '\0';

before the comparison.

To be honest, patch #2 was my focus and I don't have a pmbus device. So
I'll drop this topic and let you (or someone else) handle the action
items arising from this discussion.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux