| Message ID | 20210202183737.28747-1-matwey@sai.msu.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] hwmon: lm75: Handle broken device nodes gracefully | expand |
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)) >
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
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))
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(-)