[PATCH 0/2] hwmon: (pmbus/ltc4286) two patches about device matching

[PATCH 0/2] hwmon: (pmbus/ltc4286) two patches about device matching

[Uwe Kleine-König]

Hello,

while working on my quest to improve i2c_device_id I noticed some minor
strange things in the pmbus/ltc4286 driver. The first patch just
documents the first peculiarity found, so the next reader doesn't wonder if
this is done on purpose (and the patch gives the opportunity to notice
if this behaviour is unintended). The second drops the other peculiarity
I identified.

Best regards
Uwe

Uwe Kleine-König (2):
  hwmon: (pmbus/ltc4286) Improve device matching
  hwmon: (pmbus/ltc4286) Drop unused i2c device ids

 drivers/hwmon/pmbus/ltc4286.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

base-commit: 82d01fe6ee52086035b201cfa1410a3b04384257
-- 
2.43.0

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

[Uwe Kleine-König]

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.
+		 */
 		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))
-- 
2.43.0

[PATCH 2/2] hwmon: (pmbus/ltc4286) Drop unused i2c device ids

[Uwe Kleine-König]

The driver doesn't make use of the different numbers assigned to the
different devices. So drop them.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/hwmon/pmbus/ltc4286.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/ltc4286.c b/drivers/hwmon/pmbus/ltc4286.c
index 2e5532300eff..28aa211700fe 100644
--- a/drivers/hwmon/pmbus/ltc4286.c
+++ b/drivers/hwmon/pmbus/ltc4286.c
@@ -58,8 +58,8 @@ static struct pmbus_driver_info ltc4286_info = {
 };
 
 static const struct i2c_device_id ltc4286_id[] = {
-	{ "ltc4286", 0 },
-	{ "ltc4287", 1 },
+	{ "ltc4286", },
+	{ "ltc4287", },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, ltc4286_id);
-- 
2.43.0

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

[Guenter Roeck]

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

Re: [PATCH 2/2] hwmon: (pmbus/ltc4286) Drop unused i2c device ids

[Guenter Roeck]

On Wed, Jul 10, 2024 at 10:35:45AM +0200, Uwe Kleine-König wrote:
> The driver doesn't make use of the different numbers assigned to the
> different devices. So drop them.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Applied.

Thanks,
Guenter

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2390 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Guenter Roeck]

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4175 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]