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
