| Message ID | cover.1720600141.git.u.kleine-koenig@baylibre.com |
|---|---|
| Headers | show |
| Series | hwmon: (pmbus/ltc4286) two patches about device matching | expand |
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@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). +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