[PATCH] hwmon: lm75: Use zero lm75_type for lm75

[PATCH] hwmon: lm75: Use zero lm75_type for lm75

[Matwey V. Kornilov]

There is a logical flaw in lm75_probe() function introduced in

    e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")

Note, that of_device_get_match_data() returns NULL when no match
found. This is the case when OF node exists but has unknown
compatible line, while the module is still loaded via i2c
detection.

arch/powerpc/boot/dts/fsl/p2041rdb.dts:

    lm75b@48 {
    	compatible = "nxp,lm75a";
    	reg = <0x48>;
    };

In this case, the sensor is mistakenly considered as ADT75 variant.
The simplest way to handle this issue is to make the LM75 code
zero.

Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/hwmon/lm75.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index e447febd121a..3aa7f9454f57 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -25,12 +25,12 @@
  */
 
 enum lm75_type {		/* keep sorted in alphabetical order */
+	lm75 = 0,		/* except of lm75 which is default fallback */
 	adt75,
 	ds1775,
 	ds75,
 	ds7505,
 	g751,
-	lm75,
 	lm75a,
 	lm75b,
 	max6625,
-- 
2.26.2

Re: [PATCH] hwmon: lm75: Use zero lm75_type for lm75

[Guenter Roeck]

On 1/30/21 2:10 AM, Matwey V. Kornilov wrote:
> There is a logical flaw in lm75_probe() function introduced in
> 
>     e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
> 
> Note, that of_device_get_match_data() returns NULL when no match
> found. This is the case when OF node exists but has unknown
> compatible line, while the module is still loaded via i2c
> detection.
> 
> arch/powerpc/boot/dts/fsl/p2041rdb.dts:
> 
>     lm75b@48 {
>     	compatible = "nxp,lm75a";
>     	reg = <0x48>;
>     };
> 
> In this case, the sensor is mistakenly considered as ADT75 variant.
> The simplest way to handle this issue is to make the LM75 code
> zero.
> 

This doesn't really solve the problem since it would match _all_
non-existing entries with lm75 (instead of whatever is intended).
That doesn't matter for lm75a, but it would matter if someone
would enter, say, "bla,adt75".

On a side note, "nxp,lm75a" (nor "nxp,lm75", for that matter) is not a
documented compatible string for this driver. If anything, we would
need a means to explicitly reject such undefined compatible strings.
One option might be to define the first entry in enum lm75_type
explicitly as invalid, check for it and reject it if returned.

Guenter

> Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
>  drivers/hwmon/lm75.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index e447febd121a..3aa7f9454f57 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -25,12 +25,12 @@
>   */
>  
>  enum lm75_type {		/* keep sorted in alphabetical order */
> +	lm75 = 0,		/* except of lm75 which is default fallback */
>  	adt75,
>  	ds1775,
>  	ds75,
>  	ds7505,
>  	g751,
> -	lm75,
>  	lm75a,
>  	lm75b,
>  	max6625,
> 

Re: [PATCH] hwmon: lm75: Use zero lm75_type for lm75

[Guenter Roeck]

On 1/30/21 7:43 AM, Matwey V. Kornilov wrote:
> 
> 
> сб, 30 янв. 2021 г. в 18:31, Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>>:
>>
>> On 1/30/21 2:10 AM, Matwey V. Kornilov wrote:
>> > There is a logical flaw in lm75_probe() function introduced in
>> >
>> >     e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
>> >
>> > Note, that of_device_get_match_data() returns NULL when no match
>> > found. This is the case when OF node exists but has unknown
>> > compatible line, while the module is still loaded via i2c
>> > detection.
>> >
>> > arch/powerpc/boot/dts/fsl/p2041rdb.dts:
>> >
>> >     lm75b@48 {
>> >       compatible = "nxp,lm75a";
>> >       reg = <0x48>;
>> >     };
>> >
>> > In this case, the sensor is mistakenly considered as ADT75 variant.
>> > The simplest way to handle this issue is to make the LM75 code
>> > zero.
>> >
>>
>> This doesn't really solve the problem since it would match _all_
>> non-existing entries with lm75 (instead of whatever is intended).
> 
> Just exactly how it happened before e97a45f1b460
> 
>> That doesn't matter for lm75a, but it would matter if someone
>> would enter, say, "bla,adt75".
>>
>> On a side note, "nxp,lm75a" (nor "nxp,lm75", for that matter) is not a
>> documented compatible string for this driver. If anything, we would
>> need a means to explicitly reject such undefined compatible strings.
>> One option might be to define the first entry in enum lm75_type
>> explicitly as invalid, check for it and reject it if returned.
> 
> It is fine for me. I am afraid that this will break some dts files in the tree.
> The following compatible strings missed in the driver are currently in use:
> 
> ti,lm75
> nxp,lm75
> nxp,lm75a
> 
> I suppose these boards currently rely on the i2c detection path.
> 

Correct. But relying on a bug doesn't improve the situation.
The above compatible strings need to be documented (and properly
implemented), or removed. And we really need a better means to handle
NULL returns from of_device_get_match_data(). Maybe we should use
of_match_device() instead and check for a NULL return.

Guenter

[PATCH v2] hwmon: lm75: Handle broken device nodes gracefully

[Matwey V. Kornilov]

There is a logical flaw in lm75_probe() function introduced in

    commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")

Note, that of_device_get_match_data() returns NULL when no match
found. This is the case when OF node exists but has unknown
compatible line, while the module is still loaded via i2c
detection.

arch/powerpc/boot/dts/fsl/p2041rdb.dts:

    lm75b@48 {
    	compatible = "nxp,lm75a";
    	reg = <0x48>;
    };

In this case, the sensor is mistakenly considered as ADT75 variant.

Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 drivers/hwmon/lm75.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index e447febd121a..130ad5042107 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -561,9 +561,15 @@ static int lm75_probe(struct i2c_client *client)
 	int status, err;
 	enum lm75_type kind;
 
-	if (client->dev.of_node)
-		kind = (enum lm75_type)of_device_get_match_data(&client->dev);
-	else
+	if (dev->of_node) {
+		const struct of_device_id *match =
+			of_match_device(dev->driver->of_match_table, dev);
+
+		if (!match)
+			return -ENODEV;
+
+		kind = (enum lm75_type)(match->data);
+	} else
 		kind = i2c_match_id(lm75_ids, client)->driver_data;
 
 	if (!i2c_check_functionality(client->adapter,
-- 
2.26.2

Re: [PATCH v2] hwmon: lm75: Handle broken device nodes gracefully

[Guenter Roeck]

On 2/2/21 6:21 AM, Matwey V. Kornilov wrote:
> There is a logical flaw in lm75_probe() function introduced in
> 
>     commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
> 
> Note, that of_device_get_match_data() returns NULL when no match
> found. This is the case when OF node exists but has unknown

is found.

> compatible line, while the module is still loaded via i2c
> detection.
> 
> arch/powerpc/boot/dts/fsl/p2041rdb.dts:
> 
>     lm75b@48 {
>     	compatible = "nxp,lm75a";
>     	reg = <0x48>;
>     };
> 
> In this case, the sensor is mistakenly considered as ADT75 variant.
> 
> Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
> ---
>  drivers/hwmon/lm75.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index e447febd121a..130ad5042107 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -561,9 +561,15 @@ static int lm75_probe(struct i2c_client *client)
>  	int status, err;
>  	enum lm75_type kind;
>  
> -	if (client->dev.of_node)
> -		kind = (enum lm75_type)of_device_get_match_data(&client->dev);
> -	else
> +	if (dev->of_node) {
> +		const struct of_device_id *match =
> +			of_match_device(dev->driver->of_match_table, dev);
> +
> +		if (!match)
> +			return -ENODEV;
> +
> +		kind = (enum lm75_type)(match->data);
> +	} else
>  		kind = i2c_match_id(lm75_ids, client)->driver_data;
>  

else needs to be in { } to match if.

Guenter

>  	if (!i2c_check_functionality(client->adapter,
> 

[PATCH v3] hwmon: lm75: Handle broken device nodes gracefully

[Matwey V. Kornilov]

There is a logical flaw in lm75_probe() function introduced in

    commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")

Note, that of_device_get_match_data() returns NULL when no match
is found. This is the case when OF node exists but has unknown
compatible line, while the module is still loaded via i2c
detection.

arch/powerpc/boot/dts/fsl/p2041rdb.dts:

    lm75b@48 {
    	compatible = "nxp,lm75a";
    	reg = <0x48>;
    };

In this case, the sensor is mistakenly considered as ADT75 variant.

Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
 Changes since v2:
 * fixed typo in the message
 * fixed brackets

 drivers/hwmon/lm75.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index e447febd121a..53882c334a0d 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -561,10 +561,17 @@ static int lm75_probe(struct i2c_client *client)
 	int status, err;
 	enum lm75_type kind;
 
-	if (client->dev.of_node)
-		kind = (enum lm75_type)of_device_get_match_data(&client->dev);
-	else
+	if (dev->of_node) {
+		const struct of_device_id *match =
+			of_match_device(dev->driver->of_match_table, dev);
+
+		if (!match)
+			return -ENODEV;
+
+		kind = (enum lm75_type)(match->data);
+	} else {
 		kind = i2c_match_id(lm75_ids, client)->driver_data;
+	}
 
 	if (!i2c_check_functionality(client->adapter,
 			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
-- 
2.26.2

Re: [PATCH v3] hwmon: lm75: Handle broken device nodes gracefully

[Guenter Roeck]

On 2/2/21 10:37 AM, Matwey V. Kornilov wrote:
> There is a logical flaw in lm75_probe() function introduced in
> 
>     commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
> 
> Note, that of_device_get_match_data() returns NULL when no match
> is found. This is the case when OF node exists but has unknown
> compatible line, while the module is still loaded via i2c
> detection.
> 
> arch/powerpc/boot/dts/fsl/p2041rdb.dts:
> 
>     lm75b@48 {
>     	compatible = "nxp,lm75a";
>     	reg = <0x48>;
>     };
> 
> In this case, the sensor is mistakenly considered as ADT75 variant.
> 
> Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>

Looks good, but we'll also need a solution for the existing devicetree
nodes since they are being used. The best solution would probably
be to document and add "nxp,lm75a" to the list of devicetree nodes.
The same applies to other used but not documented compatible strings
(I think you sent a list earlier if I recall correctly).

Sorry that I didn't bring this up earlier, but I am concerned that
if we don't do this, this patch might be considered a regression.

Thanks,
Guenter

> ---
>  Changes since v2:
>  * fixed typo in the message
>  * fixed brackets
> 
>  drivers/hwmon/lm75.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index e447febd121a..53882c334a0d 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -561,10 +561,17 @@ static int lm75_probe(struct i2c_client *client)
>  	int status, err;
>  	enum lm75_type kind;
>  
> -	if (client->dev.of_node)
> -		kind = (enum lm75_type)of_device_get_match_data(&client->dev);
> -	else
> +	if (dev->of_node) {
> +		const struct of_device_id *match =
> +			of_match_device(dev->driver->of_match_table, dev);
> +
> +		if (!match)
> +			return -ENODEV;
> +
> +		kind = (enum lm75_type)(match->data);
> +	} else {
>  		kind = i2c_match_id(lm75_ids, client)->driver_data;
> +	}
>  
>  	if (!i2c_check_functionality(client->adapter,
>  			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
> 

Re: [PATCH v3] hwmon: lm75: Handle broken device nodes gracefully

[Guenter Roeck]

On 2/2/21 11:31 AM, Matwey V. Kornilov wrote:
> 
> 
> вт, 2 февр. 2021 г. в 22:29, Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>>:
>>
>> On 2/2/21 10:37 AM, Matwey V. Kornilov wrote:
>> > There is a logical flaw in lm75_probe() function introduced in
>> >
>> >     commit e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
>> >
>> > Note, that of_device_get_match_data() returns NULL when no match
>> > is found. This is the case when OF node exists but has unknown
>> > compatible line, while the module is still loaded via i2c
>> > detection.
>> >
>> > arch/powerpc/boot/dts/fsl/p2041rdb.dts:
>> >
>> >     lm75b@48 {
>> >       compatible = "nxp,lm75a";
>> >       reg = <0x48>;
>> >     };
>> >
>> > In this case, the sensor is mistakenly considered as ADT75 variant.
>> >
>> > Fixes: e97a45f1b460 ("hwmon: (lm75) Add OF device ID table")
>> > Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru <mailto:matwey@sai.msu.ru>>
>>
>> Looks good, but we'll also need a solution for the existing devicetree
>> nodes since they are being used. The best solution would probably
>> be to document and add "nxp,lm75a" to the list of devicetree nodes.
>> The same applies to other used but not documented compatible strings
>> (I think you sent a list earlier if I recall correctly).
>>
>> Sorry that I didn't bring this up earlier, but I am concerned that
>> if we don't do this, this patch might be considered a regression.
> 
> 
> What if I just concatenate this patch with the other patch series where "nxp,lm75a" compatible string is introduced?
>  

It just has to be handled in one series, best with the compatible string(s)
introduced first and then this patch as last patch of the series.

Thanks,
Guenter

>>
>>
>> Thanks,
>> Guenter
>>
>> > ---
>> >  Changes since v2:
>> >  * fixed typo in the message
>> >  * fixed brackets
>> >
>> >  drivers/hwmon/lm75.c | 13 ++++++++++---
>> >  1 file changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
>> > index e447febd121a..53882c334a0d 100644
>> > --- a/drivers/hwmon/lm75.c
>> > +++ b/drivers/hwmon/lm75.c
>> > @@ -561,10 +561,17 @@ static int lm75_probe(struct i2c_client *client)
>> >       int status, err;
>> >       enum lm75_type kind;
>> >
>> > -     if (client->dev.of_node)
>> > -             kind = (enum lm75_type)of_device_get_match_data(&client->dev);
>> > -     else
>> > +     if (dev->of_node) {
>> > +             const struct of_device_id *match =
>> > +                     of_match_device(dev->driver->of_match_table, dev);
>> > +
>> > +             if (!match)
>> > +                     return -ENODEV;
>> > +
>> > +             kind = (enum lm75_type)(match->data);
>> > +     } else {
>> >               kind = i2c_match_id(lm75_ids, client)->driver_data;
>> > +     }
>> >
>> >       if (!i2c_check_functionality(client->adapter,
>> >                       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
>> >
>>
> 
> 
> --
> With best regards,
> Matwey V. Kornilov