| Message ID | cf49bf8b0ba4e50e71e0b31471748b50d7b1a055.1720600141.git.u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | hwmon: (pmbus/ltc4286) two patches about device matching | expand |
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. Guenter
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 :-) 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. Best regards Uwe
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
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
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. + */ if (!strncasecmp(mid->name, block_buffer, strlen(mid->name))) break; - } + if (!mid->name[0]) return dev_err_probe(&client->dev, -ENODEV, - "Unsupported device\n"); + "Unsupported device (reported: \"%*pE\")\n", + ret, block_buffer); if (of_property_read_u32(client->dev.of_node, "shunt-resistor-micro-ohms", &rsense))
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(-)