| Message ID | 20210130101038.26331-1-matwey@sai.msu.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: lm75: Use zero lm75_type for lm75 | expand |
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, >
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
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,
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(-)