Use hwmon_notify_event() to notify userspace and thermal core about
temperature changes.
Suggested-by: Guenter Roeck <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/hwmon/lm90.c | 44 +++++++++++++++++++++++++++++++++-----------
1 file changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 2e057fad05b4..e7b678a40b39 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
struct lm90_data {
struct i2c_client *client;
+ struct device *hwmon_dev;
u32 channel_config[4];
struct hwmon_channel_info temp_info;
const struct hwmon_channel_info *info[3];
@@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM)) ||
(st2 & MAX6696_STATUS2_LOT2))
- dev_warn(&client->dev,
- "temp%d out of range, please check!\n", 1);
+ dev_dbg(&client->dev,
+ "temp%d out of range, please check!\n", 1);
if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM)) ||
(st2 & MAX6696_STATUS2_ROT2))
- dev_warn(&client->dev,
- "temp%d out of range, please check!\n", 2);
+ dev_dbg(&client->dev,
+ "temp%d out of range, please check!\n", 2);
if (st & LM90_STATUS_ROPEN)
- dev_warn(&client->dev,
- "temp%d diode open, please check!\n", 2);
+ dev_dbg(&client->dev,
+ "temp%d diode open, please check!\n", 2);
if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
- dev_warn(&client->dev,
- "temp%d out of range, please check!\n", 3);
+ dev_dbg(&client->dev,
+ "temp%d out of range, please check!\n", 3);
if (st2 & MAX6696_STATUS2_R2OPEN)
- dev_warn(&client->dev,
- "temp%d diode open, please check!\n", 3);
+ dev_dbg(&client->dev,
+ "temp%d diode open, please check!\n", 3);
+
+ if (st & LM90_STATUS_LLOW)
+ hwmon_notify_event(data->hwmon_dev, hwmon_temp,
+ hwmon_temp_min, 0);
+ if (st & LM90_STATUS_RLOW)
+ hwmon_notify_event(data->hwmon_dev, hwmon_temp,
+ hwmon_temp_min, 1);
+ if (st2 & MAX6696_STATUS2_R2LOW)
+ hwmon_notify_event(data->hwmon_dev, hwmon_temp,
+ hwmon_temp_min, 2);
+ if (st & LM90_STATUS_LHIGH)
+ hwmon_notify_event(data->hwmon_dev, hwmon_temp,
+ hwmon_temp_max, 0);
+ if (st & LM90_STATUS_RHIGH)
+ hwmon_notify_event(data->hwmon_dev, hwmon_temp,
+ hwmon_temp_max, 1);
+ if (st2 & MAX6696_STATUS2_R2HIGH)
+ hwmon_notify_event(data->hwmon_dev, hwmon_temp,
+ hwmon_temp_max, 2);
return true;
}
@@ -1904,6 +1924,8 @@ static int lm90_probe(struct i2c_client *client)
if (IS_ERR(hwmon_dev))
return PTR_ERR(hwmon_dev);
+ data->hwmon_dev = hwmon_dev;
+
if (client->irq) {
dev_dbg(dev, "IRQ: %d\n", client->irq);
err = devm_request_threaded_irq(dev, client->irq,
@@ -1940,7 +1962,7 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
lm90_update_confreg(data, data->config | 0x80);
}
} else {
- dev_info(&client->dev, "Everything OK\n");
+ dev_dbg(&client->dev, "Everything OK\n");
}
}
--
2.30.2
Hi Dmitry,
On 18/06/2021 22:54, Dmitry Osipenko wrote:
> Use hwmon_notify_event() to notify userspace and thermal core about
> temperature changes.
>
> Suggested-by: Guenter Roeck <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/hwmon/lm90.c | 44 +++++++++++++++++++++++++++++++++-----------
> 1 file changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 2e057fad05b4..e7b678a40b39 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
>
> struct lm90_data {
> struct i2c_client *client;
> + struct device *hwmon_dev;
> u32 channel_config[4];
> struct hwmon_channel_info temp_info;
> const struct hwmon_channel_info *info[3];
> @@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
>
> if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM)) ||
> (st2 & MAX6696_STATUS2_LOT2))
> - dev_warn(&client->dev,
> - "temp%d out of range, please check!\n", 1);
> + dev_dbg(&client->dev,
> + "temp%d out of range, please check!\n", 1);
> if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM)) ||
> (st2 & MAX6696_STATUS2_ROT2))
> - dev_warn(&client->dev,
> - "temp%d out of range, please check!\n", 2);
> + dev_dbg(&client->dev,
> + "temp%d out of range, please check!\n", 2);
> if (st & LM90_STATUS_ROPEN)
> - dev_warn(&client->dev,
> - "temp%d diode open, please check!\n", 2);
> + dev_dbg(&client->dev,
> + "temp%d diode open, please check!\n", 2);
> if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
> MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
> - dev_warn(&client->dev,
> - "temp%d out of range, please check!\n", 3);
> + dev_dbg(&client->dev,
> + "temp%d out of range, please check!\n", 3);
> if (st2 & MAX6696_STATUS2_R2OPEN)
> - dev_warn(&client->dev,
> - "temp%d diode open, please check!\n", 3);
> + dev_dbg(&client->dev,
> + "temp%d diode open, please check!\n", 3);
> +
> + if (st & LM90_STATUS_LLOW)
> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> + hwmon_temp_min, 0);
> + if (st & LM90_STATUS_RLOW)
> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> + hwmon_temp_min, 1);
> + if (st2 & MAX6696_STATUS2_R2LOW)
> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> + hwmon_temp_min, 2);
> + if (st & LM90_STATUS_LHIGH)
> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> + hwmon_temp_max, 0);
> + if (st & LM90_STATUS_RHIGH)
> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> + hwmon_temp_max, 1);
> + if (st2 & MAX6696_STATUS2_R2HIGH)
> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
> + hwmon_temp_max, 2);
We observed a random null pointer deference crash somewhere in the
thermal core (crash log below is not very helpful) when calling
mutex_lock(). It looks like we get an interrupt when this crash
happens.
Looking at the lm90 driver, per the above, I now see we are calling
hwmon_notify_event() from the lm90 interrupt handler. Looking at
hwmon_notify_event() I see that ...
hwmon_notify_event()
--> hwmon_thermal_notify()
--> thermal_zone_device_update()
--> update_temperature()
--> mutex_lock()
So although I don't completely understand the crash, it does seem
that we should not be calling hwmon_notify_event() from the
interrupt handler.
BTW I have not reproduced this myself yet, so I have just been
reviewing the code to try and understand this.
Jon
[ 7465.595066] Unable to handle kernel NULL pointer dereference at virtual address 00000000000003cd
[ 7465.596619] Mem abort info:
[ 7465.597854] ESR = 0x96000021
[ 7465.599097] EC = 0x25: DABT (current EL), IL = 32 bits
[ 7465.600338] SET = 0, FnV = 0
[ 7465.601526] EA = 0, S1PTW = 0
[ 7465.602705] FSC = 0x21: alignment fault
[ 7465.603885] Data abort info:
[ 7465.605017] ISV = 0, ISS = 0x00000021
[ 7465.606171] CM = 0, WnR = 0
[ 7465.607301] user pgtable: 64k pages, 48-bit VAs, pgdp=00000001041f1800
[ 7465.608490] [00000000000003cd] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
[ 7465.609814] Internal error: Oops: 96000021 [#1] PREEMPT SMP
[ 7465.610991] Modules linked in: bridge stp llc snd_soc_tegra210_admaif snd_soc_tegra_pcm snd_soc_tegra210_mixer snd_soc_tegra210_mvc snd_soc_tegra210_i2s snd_soc_tegra210_dmic sn
d_soc_tegra210_adx snd_soc_tegra210_sfc snd_soc_tegra210_amx snd_soc_tegra210_ahub tegra210_adma snd_soc_rt5659 snd_soc_rl6231 pwm_tegra tegra_aconnect snd_hda_codec_hdmi rfkill snd_hda_tegra snd_hda_codec at24 phy_tegra194_p2u snd_hda_core lm90 snd_soc_tegra_audio_graph_card tegra_bpmp_thermal snd_soc_audio_graph_card snd_soc_simple_card_utils pwm_fan crct10dif_ce pcie_tegra194 cec drm_kms_helper drm ip_tables x_tables ipv6
[ 7465.632232] CPU: 2 PID: 433 Comm: irq/140-lm90 Tainted: G O 5.16.0-tegra-g9d109504d83a #1
[ 7465.636285] Hardware name: Unknown NVIDIA Jetson AGX Xavier Developer Kit/NVIDIA Jetson AGX Xavier Developer Kit, BIOS v1.1.2-901d3c52 02/07/2022
[ 7465.650457] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 7465.656210] pc : mutex_lock+0x18/0x60
[ 7465.660134] lr : thermal_zone_device_update+0x40/0x2e0
[ 7465.665117] sp : ffff800014c4fc60
[ 7465.668781] x29: ffff800014c4fc60 x28: ffff365ee3f6e000 x27: ffffdde218426790
[ 7465.675882] x26: ffff365ee3f6e000 x25: 0000000000000000 x24: ffff365ee3f6e000
[ 7465.683485] x23: ffffdde218426870 x22: ffff365ee3f6e000 x21: 00000000000003cd
[ 7465.690816] x20: ffff365ee8bf3308 x19: ffffffffffffffed x18: 0000000000000000
[ 7465.697982] x17: ffffdde21842689c x16: ffffdde1cb7a0b7c x15: 0000000000000040
[ 7465.705320] x14: ffffdde21a4889a0 x13: 0000000000000228 x12: 0000000000000000
[ 7465.712493] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
[ 7465.719580] x8 : 0000000001120000 x7 : 0000000000000001 x6 : 0000000000000000
[ 7465.727099] x5 : 0068000878e20f07 x4 : 0000000000000000 x3 : 00000000000003cd
[ 7465.734348] x2 : ffff365ee3f6e000 x1 : 0000000000000000 x0 : 00000000000003cd
[ 7465.741347] Call trace:
[ 7465.744207] mutex_lock+0x18/0x60
[ 7465.747427] hwmon_notify_event+0xfc/0x110
[ 7465.751358] 0xffffdde1cb7a0a90
[ 7465.754574] 0xffffdde1cb7a0b7c
[ 7465.757705] irq_thread_fn+0x2c/0xa0
[ 7465.760937] irq_thread+0x134/0x240
[ 7465.764850] kthread+0x178/0x190
[ 7465.768083] ret_from_fork+0x10/0x20
[ 7465.771748] Code: d503201f d503201f d2800001 aa0103e4 (c8e47c02)
[ 7465.777865] ---[ end trace f0b3723991411538 ]---
On 2/21/22 08:16, Jon Hunter wrote:
>
> On 21/02/2022 16:02, Guenter Roeck wrote:
>
> ...
>
>>> The platform I see this on does use device-tree and it does have a node for the ti,tmp451 device which uses the lm90 device. This platform uses the device-tree source arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451 node is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
>>>
>>
>> Interesting. It appears that the call to devm_thermal_zone_of_sensor_register()
>> in the hwmon core nevertheless returns -ENODEV which is not handled properly
>> in the hwmon core. I can see a number of reasons for this to happen:
>> - there is no devicetree node for the lm90 device
>> - there is no thermal-zones devicetree node
>> - there is no thermal zone entry in the thermal-zones node which matches
>> the sensor
>
>
> So we definitely have the node for the lm90 device and a thermal-zones node, but I do not see a thermal-sensor node. Maybe this is what we are missing?
>
Correct.
Guenter
On 21/02/2022 12:36, Dmitry Osipenko wrote:
> 21.02.2022 15:01, Jon Hunter пишет:
>> Hi Dmitry,
>>
>> On 18/06/2021 22:54, Dmitry Osipenko wrote:
>>> Use hwmon_notify_event() to notify userspace and thermal core about
>>> temperature changes.
>>>
>>> Suggested-by: Guenter Roeck <[email protected]>
>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>> ---
>>> drivers/hwmon/lm90.c | 44 +++++++++++++++++++++++++++++++++-----------
>>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>> index 2e057fad05b4..e7b678a40b39 100644
>>> --- a/drivers/hwmon/lm90.c
>>> +++ b/drivers/hwmon/lm90.c
>>> @@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
>>> struct lm90_data {
>>> struct i2c_client *client;
>>> + struct device *hwmon_dev;
>>> u32 channel_config[4];
>>> struct hwmon_channel_info temp_info;
>>> const struct hwmon_channel_info *info[3];
>>> @@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client
>>> *client, u16 *status)
>>> if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH |
>>> LM90_STATUS_LTHRM)) ||
>>> (st2 & MAX6696_STATUS2_LOT2))
>>> - dev_warn(&client->dev,
>>> - "temp%d out of range, please check!\n", 1);
>>> + dev_dbg(&client->dev,
>>> + "temp%d out of range, please check!\n", 1);
>>> if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH |
>>> LM90_STATUS_RTHRM)) ||
>>> (st2 & MAX6696_STATUS2_ROT2))
>>> - dev_warn(&client->dev,
>>> - "temp%d out of range, please check!\n", 2);
>>> + dev_dbg(&client->dev,
>>> + "temp%d out of range, please check!\n", 2);
>>> if (st & LM90_STATUS_ROPEN)
>>> - dev_warn(&client->dev,
>>> - "temp%d diode open, please check!\n", 2);
>>> + dev_dbg(&client->dev,
>>> + "temp%d diode open, please check!\n", 2);
>>> if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
>>> MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
>>> - dev_warn(&client->dev,
>>> - "temp%d out of range, please check!\n", 3);
>>> + dev_dbg(&client->dev,
>>> + "temp%d out of range, please check!\n", 3);
>>> if (st2 & MAX6696_STATUS2_R2OPEN)
>>> - dev_warn(&client->dev,
>>> - "temp%d diode open, please check!\n", 3);
>>> + dev_dbg(&client->dev,
>>> + "temp%d diode open, please check!\n", 3);
>>> +
>>> + if (st & LM90_STATUS_LLOW)
>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> + hwmon_temp_min, 0);
>>> + if (st & LM90_STATUS_RLOW)
>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> + hwmon_temp_min, 1);
>>> + if (st2 & MAX6696_STATUS2_R2LOW)
>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> + hwmon_temp_min, 2);
>>> + if (st & LM90_STATUS_LHIGH)
>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> + hwmon_temp_max, 0);
>>> + if (st & LM90_STATUS_RHIGH)
>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> + hwmon_temp_max, 1);
>>> + if (st2 & MAX6696_STATUS2_R2HIGH)
>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>> + hwmon_temp_max, 2);
>>
>>
>> We observed a random null pointer deference crash somewhere in the
>> thermal core (crash log below is not very helpful) when calling
>> mutex_lock(). It looks like we get an interrupt when this crash
>> happens.
>>
>> Looking at the lm90 driver, per the above, I now see we are calling
>> hwmon_notify_event() from the lm90 interrupt handler. Looking at
>> hwmon_notify_event() I see that ...
>>
>> hwmon_notify_event()
>> --> hwmon_thermal_notify()
>> --> thermal_zone_device_update()
>> --> update_temperature()
>> --> mutex_lock()
>>
>> So although I don't completely understand the crash, it does seem
>> that we should not be calling hwmon_notify_event() from the
>> interrupt handler.
>>
>> BTW I have not reproduced this myself yet, so I have just been
>> reviewing the code to try and understand this.
>
> Matt Merhar was experiencing a similar issue on T30 Ouya, but I never
> managed to reproduce it on Nexus 7 and Acer A500 tablets, and couldn't
> spot any problem in the code. IIRC, it was a NULL dereference of another
> pointer within that code.
OK. From looking at the above I don't think we can call
hwmon_notify_event() from an interrupt handler because this is going to
try and request a mutex. So we need to fix that.
Jon
--
nvpublic
On 21/02/2022 16:02, Guenter Roeck wrote:
...
>> The platform I see this on does use device-tree and it does have a
>> node for the ti,tmp451 device which uses the lm90 device. This
>> platform uses the device-tree source
>> arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451 node
>> is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
>>
>
> Interesting. It appears that the call to
> devm_thermal_zone_of_sensor_register()
> in the hwmon core nevertheless returns -ENODEV which is not handled
> properly
> in the hwmon core. I can see a number of reasons for this to happen:
> - there is no devicetree node for the lm90 device
> - there is no thermal-zones devicetree node
> - there is no thermal zone entry in the thermal-zones node which matches
> the sensor
So we definitely have the node for the lm90 device and a thermal-zones
node, but I do not see a thermal-sensor node. Maybe this is what we are
missing?
Jon
--
nvpublic
On 2/21/22 04:59, Dmitry Osipenko wrote:
> 21.02.2022 15:56, Jon Hunter пишет:
>>
>> On 21/02/2022 12:36, Dmitry Osipenko wrote:
>>> 21.02.2022 15:01, Jon Hunter пишет:
>>>> Hi Dmitry,
>>>>
>>>> On 18/06/2021 22:54, Dmitry Osipenko wrote:
>>>>> Use hwmon_notify_event() to notify userspace and thermal core about
>>>>> temperature changes.
>>>>>
>>>>> Suggested-by: Guenter Roeck <[email protected]>
>>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>>> ---
>>>>> drivers/hwmon/lm90.c | 44
>>>>> +++++++++++++++++++++++++++++++++-----------
>>>>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>>>> index 2e057fad05b4..e7b678a40b39 100644
>>>>> --- a/drivers/hwmon/lm90.c
>>>>> +++ b/drivers/hwmon/lm90.c
>>>>> @@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
>>>>> struct lm90_data {
>>>>> struct i2c_client *client;
>>>>> + struct device *hwmon_dev;
>>>>> u32 channel_config[4];
>>>>> struct hwmon_channel_info temp_info;
>>>>> const struct hwmon_channel_info *info[3];
>>>>> @@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client
>>>>> *client, u16 *status)
>>>>> if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH |
>>>>> LM90_STATUS_LTHRM)) ||
>>>>> (st2 & MAX6696_STATUS2_LOT2))
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d out of range, please check!\n", 1);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d out of range, please check!\n", 1);
>>>>> if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH |
>>>>> LM90_STATUS_RTHRM)) ||
>>>>> (st2 & MAX6696_STATUS2_ROT2))
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d out of range, please check!\n", 2);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d out of range, please check!\n", 2);
>>>>> if (st & LM90_STATUS_ROPEN)
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d diode open, please check!\n", 2);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d diode open, please check!\n", 2);
>>>>> if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
>>>>> MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d out of range, please check!\n", 3);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d out of range, please check!\n", 3);
>>>>> if (st2 & MAX6696_STATUS2_R2OPEN)
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d diode open, please check!\n", 3);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d diode open, please check!\n", 3);
>>>>> +
>>>>> + if (st & LM90_STATUS_LLOW)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_min, 0);
>>>>> + if (st & LM90_STATUS_RLOW)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_min, 1);
>>>>> + if (st2 & MAX6696_STATUS2_R2LOW)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_min, 2);
>>>>> + if (st & LM90_STATUS_LHIGH)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_max, 0);
>>>>> + if (st & LM90_STATUS_RHIGH)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_max, 1);
>>>>> + if (st2 & MAX6696_STATUS2_R2HIGH)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_max, 2);
>>>>
>>>>
>>>> We observed a random null pointer deference crash somewhere in the
>>>> thermal core (crash log below is not very helpful) when calling
>>>> mutex_lock(). It looks like we get an interrupt when this crash
>>>> happens.
>>>>
>>>> Looking at the lm90 driver, per the above, I now see we are calling
>>>> hwmon_notify_event() from the lm90 interrupt handler. Looking at
>>>> hwmon_notify_event() I see that ...
>>>>
>>>> hwmon_notify_event()
>>>> --> hwmon_thermal_notify()
>>>> --> thermal_zone_device_update()
>>>> --> update_temperature()
>>>> --> mutex_lock()
>>>>
>>>> So although I don't completely understand the crash, it does seem
>>>> that we should not be calling hwmon_notify_event() from the
>>>> interrupt handler.
>>>>
>>>> BTW I have not reproduced this myself yet, so I have just been
>>>> reviewing the code to try and understand this.
>>>
>>> Matt Merhar was experiencing a similar issue on T30 Ouya, but I never
>>> managed to reproduce it on Nexus 7 and Acer A500 tablets, and couldn't
>>> spot any problem in the code. IIRC, it was a NULL dereference of another
>>> pointer within that code.
>>
>>
>> OK. From looking at the above I don't think we can call
>> hwmon_notify_event() from an interrupt handler because this is going to
>> try and request a mutex. So we need to fix that.
>
> The interrupt is threaded, so it can take a mutex.
Exactly. The problem is elsewhere.
Guenter
21.02.2022 19:16, Jon Hunter пишет:
>
> On 21/02/2022 16:02, Guenter Roeck wrote:
>
> ...
>
>>> The platform I see this on does use device-tree and it does have a
>>> node for the ti,tmp451 device which uses the lm90 device. This
>>> platform uses the device-tree source
>>> arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451
>>> node is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
>>>
>>
>> Interesting. It appears that the call to
>> devm_thermal_zone_of_sensor_register()
>> in the hwmon core nevertheless returns -ENODEV which is not handled
>> properly
>> in the hwmon core. I can see a number of reasons for this to happen:
>> - there is no devicetree node for the lm90 device
>> - there is no thermal-zones devicetree node
>> - there is no thermal zone entry in the thermal-zones node which matches
>> the sensor
>
>
> So we definitely have the node for the lm90 device and a thermal-zones
> node, but I do not see a thermal-sensor node. Maybe this is what we are
> missing?
Could you please try this:
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 5915fedee69b..48f80bc99fe6 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -233,8 +233,12 @@ static int hwmon_thermal_add_sensor(struct device
*dev, int index)
* If CONFIG_THERMAL_OF is disabled, this returns -ENODEV,
* so ignore that error but forward any other error.
*/
- if (IS_ERR(tzd) && (PTR_ERR(tzd) != -ENODEV))
- return PTR_ERR(tzd);
+ if (IS_ERR(tzd)) {
+ if (PTR_ERR(tzd) != -ENODEV)
+ return PTR_ERR(tzd);
+
+ tzd = NULL;
+ }
err = devm_add_action(dev, hwmon_thermal_remove_sensor, &tdata->node);
if (err)
@@ -283,7 +287,7 @@ static void hwmon_thermal_notify(struct device *dev,
int index)
struct hwmon_thermal_data *tzdata;
list_for_each_entry(tzdata, &hwdev->tzdata, node) {
- if (tzdata->index == index) {
+ if (tzdata->index == index && tzdata->tzd) {
thermal_zone_device_update(tzdata->tzd,
THERMAL_EVENT_UNSPECIFIED);
}
On 2/21/22 08:20, Dmitry Osipenko wrote:
> 21.02.2022 19:16, Jon Hunter пишет:
>>
>> On 21/02/2022 16:02, Guenter Roeck wrote:
>>
>> ...
>>
>>>> The platform I see this on does use device-tree and it does have a
>>>> node for the ti,tmp451 device which uses the lm90 device. This
>>>> platform uses the device-tree source
>>>> arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451
>>>> node is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
>>>>
>>>
>>> Interesting. It appears that the call to
>>> devm_thermal_zone_of_sensor_register()
>>> in the hwmon core nevertheless returns -ENODEV which is not handled
>>> properly
>>> in the hwmon core. I can see a number of reasons for this to happen:
>>> - there is no devicetree node for the lm90 device
>>> - there is no thermal-zones devicetree node
>>> - there is no thermal zone entry in the thermal-zones node which matches
>>> the sensor
>>
>>
>> So we definitely have the node for the lm90 device and a thermal-zones
>> node, but I do not see a thermal-sensor node. Maybe this is what we are
>> missing?
>
> Could you please try this:
>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 5915fedee69b..48f80bc99fe6 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -233,8 +233,12 @@ static int hwmon_thermal_add_sensor(struct device
> *dev, int index)
> * If CONFIG_THERMAL_OF is disabled, this returns -ENODEV,
> * so ignore that error but forward any other error.
> */
> - if (IS_ERR(tzd) && (PTR_ERR(tzd) != -ENODEV))
> - return PTR_ERR(tzd);
> + if (IS_ERR(tzd)) {
> + if (PTR_ERR(tzd) != -ENODEV)
> + return PTR_ERR(tzd);
> +
> + tzd = NULL;
That should just bail out. I'll send a patch in a minute.
Guenter
> + }
>
> err = devm_add_action(dev, hwmon_thermal_remove_sensor, &tdata->node);
> if (err)
> @@ -283,7 +287,7 @@ static void hwmon_thermal_notify(struct device *dev,
> int index)
> struct hwmon_thermal_data *tzdata;
>
> list_for_each_entry(tzdata, &hwdev->tzdata, node) {
> - if (tzdata->index == index) {
> + if (tzdata->index == index && tzdata->tzd) {
> thermal_zone_device_update(tzdata->tzd,
> THERMAL_EVENT_UNSPECIFIED);
> }
>
21.02.2022 16:50, Jon Hunter пишет:
>
> On 21/02/2022 12:59, Dmitry Osipenko wrote:
>> 21.02.2022 15:56, Jon Hunter пишет:
>>>
>>> On 21/02/2022 12:36, Dmitry Osipenko wrote:
>>>> 21.02.2022 15:01, Jon Hunter пишет:
>>>>> Hi Dmitry,
>>>>>
>>>>> On 18/06/2021 22:54, Dmitry Osipenko wrote:
>>>>>> Use hwmon_notify_event() to notify userspace and thermal core about
>>>>>> temperature changes.
>>>>>>
>>>>>> Suggested-by: Guenter Roeck <[email protected]>
>>>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>>>> ---
>>>>>> drivers/hwmon/lm90.c | 44
>>>>>> +++++++++++++++++++++++++++++++++-----------
>>>>>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>>>>> index 2e057fad05b4..e7b678a40b39 100644
>>>>>> --- a/drivers/hwmon/lm90.c
>>>>>> +++ b/drivers/hwmon/lm90.c
>>>>>> @@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
>>>>>> struct lm90_data {
>>>>>> struct i2c_client *client;
>>>>>> + struct device *hwmon_dev;
>>>>>> u32 channel_config[4];
>>>>>> struct hwmon_channel_info temp_info;
>>>>>> const struct hwmon_channel_info *info[3];
>>>>>> @@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client
>>>>>> *client, u16 *status)
>>>>>> if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH |
>>>>>> LM90_STATUS_LTHRM)) ||
>>>>>> (st2 & MAX6696_STATUS2_LOT2))
>>>>>> - dev_warn(&client->dev,
>>>>>> - "temp%d out of range, please check!\n", 1);
>>>>>> + dev_dbg(&client->dev,
>>>>>> + "temp%d out of range, please check!\n", 1);
>>>>>> if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH |
>>>>>> LM90_STATUS_RTHRM)) ||
>>>>>> (st2 & MAX6696_STATUS2_ROT2))
>>>>>> - dev_warn(&client->dev,
>>>>>> - "temp%d out of range, please check!\n", 2);
>>>>>> + dev_dbg(&client->dev,
>>>>>> + "temp%d out of range, please check!\n", 2);
>>>>>> if (st & LM90_STATUS_ROPEN)
>>>>>> - dev_warn(&client->dev,
>>>>>> - "temp%d diode open, please check!\n", 2);
>>>>>> + dev_dbg(&client->dev,
>>>>>> + "temp%d diode open, please check!\n", 2);
>>>>>> if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
>>>>>> MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
>>>>>> - dev_warn(&client->dev,
>>>>>> - "temp%d out of range, please check!\n", 3);
>>>>>> + dev_dbg(&client->dev,
>>>>>> + "temp%d out of range, please check!\n", 3);
>>>>>> if (st2 & MAX6696_STATUS2_R2OPEN)
>>>>>> - dev_warn(&client->dev,
>>>>>> - "temp%d diode open, please check!\n", 3);
>>>>>> + dev_dbg(&client->dev,
>>>>>> + "temp%d diode open, please check!\n", 3);
>>>>>> +
>>>>>> + if (st & LM90_STATUS_LLOW)
>>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> + hwmon_temp_min, 0);
>>>>>> + if (st & LM90_STATUS_RLOW)
>>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> + hwmon_temp_min, 1);
>>>>>> + if (st2 & MAX6696_STATUS2_R2LOW)
>>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> + hwmon_temp_min, 2);
>>>>>> + if (st & LM90_STATUS_LHIGH)
>>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> + hwmon_temp_max, 0);
>>>>>> + if (st & LM90_STATUS_RHIGH)
>>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> + hwmon_temp_max, 1);
>>>>>> + if (st2 & MAX6696_STATUS2_R2HIGH)
>>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>>> + hwmon_temp_max, 2);
>>>>>
>>>>>
>>>>> We observed a random null pointer deference crash somewhere in the
>>>>> thermal core (crash log below is not very helpful) when calling
>>>>> mutex_lock(). It looks like we get an interrupt when this crash
>>>>> happens.
>>>>>
>>>>> Looking at the lm90 driver, per the above, I now see we are calling
>>>>> hwmon_notify_event() from the lm90 interrupt handler. Looking at
>>>>> hwmon_notify_event() I see that ...
>>>>>
>>>>> hwmon_notify_event()
>>>>> --> hwmon_thermal_notify()
>>>>> --> thermal_zone_device_update()
>>>>> --> update_temperature()
>>>>> --> mutex_lock()
>>>>>
>>>>> So although I don't completely understand the crash, it does seem
>>>>> that we should not be calling hwmon_notify_event() from the
>>>>> interrupt handler.
>>>>>
>>>>> BTW I have not reproduced this myself yet, so I have just been
>>>>> reviewing the code to try and understand this.
>>>>
>>>> Matt Merhar was experiencing a similar issue on T30 Ouya, but I never
>>>> managed to reproduce it on Nexus 7 and Acer A500 tablets, and couldn't
>>>> spot any problem in the code. IIRC, it was a NULL dereference of
>>>> another
>>>> pointer within that code.
>>>
>>>
>>> OK. From looking at the above I don't think we can call
>>> hwmon_notify_event() from an interrupt handler because this is going to
>>> try and request a mutex. So we need to fix that.
>>
>> The interrupt is threaded, so it can take a mutex.
>
>
> Ah yes, I clearly overlooked that detail.
>
> Good news is that I have been able to reproduce this on Jetson Xavier by
> ...
>
> $ echo 40000 | sudo tee
> /sys/devices/platform/bpmp/bpmp\:i2c/i2c-0/0-004c/hwmon/hwmon0/temp1_max
> 40000
> [ 105.890995] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000000003cd
> [ 105.900105] Mem abort info:
> [ 105.903328] ESR = 0x96000021
> [ 105.906673] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 105.912407] SET = 0, FnV = 0
> [ 105.915751] EA = 0, S1PTW = 0
> [ 105.919230] FSC = 0x21: alignment fault
> [ 105.923698] Data abort info:
> [ 105.926853] ISV = 0, ISS = 0x00000021
> [ 105.931139] CM = 0, WnR = 0
> [ 105.934420] user pgtable: 64k pages, 48-bit VAs, pgdp=0000000101f6b600
> [ 105.941230] [00000000000003cd] pgd=0000000000000000,
> p4d=0000000000000000, pud=0000000000000000
> [ 105.950864] Internal error: Oops: 96000021 [#1] PREEMPT SMP
> [ 105.956608] Modules linked in: btrfs blake2b_generic libcrc32c xor
> xor_neon raid6_pq zstd_compress rfkill snd_soc_tegra210_mixer
> snd_soc_tegra210_adx snd_soc_tegra210_dmic snd_soc_tegra210_mvc
> snd_soc_tegra210_amx snd_soc_tegra210_sfc snd_soc_tegra210_admaif
> snd_soc_tegra_pcm snd_soc_tegra210_i2s snd_soc_tegra210_ahub uvcvideo
> videobuf2_vmalloc tegra210_adma videobuf2_memops videobuf2_v4l2 cec
> videobuf2_common drm_kms_
> helper videodev mc snd_soc_rt5659 snd_soc_rl6231 pwm_tegra
> tegra_aconnect snd_hda_codec_hdmi lm90 tegra_bpmp_thermal snd_hda_tegra
> snd_soc_tegra_audio_graph_card snd_hda_codec snd_hda_core
> phy_tegra194_p2u snd_soc_
> audio_graph_card at24 snd_soc_simple_card_utils pwm_fan pcie_tegra194
> crct10dif_ce drm ip_tables x_tables ipv6
> [ 106.032497] CPU: 3 PID: 296 Comm: irq/126-lm90 Tainted: G
> O 5.16.0-tegra-291805-gf905a41db850 #3
> [ 106.042869] Hardware name: Unknown NVIDIA Jetson AGX Xavier Developer
> Kit/NVIDIA Jetson AGX Xavier Developer Kit, BIOS v1.1.2-901d3c52ed23
> 02/14/2022
> [ 106.056392] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [ 106.063903] pc : mutex_lock+0x18/0x60
> [ 106.067750] lr : thermal_zone_device_update+0x40/0x2e0
> [ 106.073161] sp : ffff80001494fc60
> [ 106.076553] x29: ffff80001494fc60 x28: ffff59bb27801c00 x27:
> ffffa4295d826790
> [ 106.084052] x26: ffff59bb27801c00 x25: 0000000000000000 x24:
> ffff59bb27801c00
> [ 106.091541] x23: ffffa4295d826870 x22: ffff59bb27801c00 x21:
> 00000000000003cd
> [ 106.098905] x20: ffff59bb28078f08 x19: ffffffffffffffed x18:
> 0000000000000000
> [ 106.106387] x17: ffffa4295d82689c x16: ffffa4292d400b7c x15:
> 0000000000000040
> [ 106.113766] x14: ffffa4295f8889a0 x13: 0000000000000228 x12:
> 0000000000000000
> [ 106.121294] x11: 0000000000000000 x10: 0000000000000000 x9 :
> 0000000000000000
> [ 106.128793] x8 : 0000000000c2f000 x7 : 0000000000000001 x6 :
> 0000000000000000
> [ 106.136133] x5 : 006800047a8e0f07 x4 : 0000000000000000 x3 :
> 00000000000003cd
> [ 106.143473] x2 : ffff59bb27801c00 x1 : 0000000000000000 x0 :
> 00000000000003cd
> [ 106.150813] Call trace:
> [ 106.153333] mutex_lock+0x18/0x60
> [ 106.156804] hwmon_notify_event+0xfc/0x110
> [ 106.161164] 0xffffa4292d400a74
> [ 106.164417] 0xffffa4292d400b7c
> [ 106.167659] irq_thread_fn+0x2c/0xa0
> [ 106.171359] irq_thread+0x134/0x240
> [ 106.174971] kthread+0x178/0x190
> [ 106.178469] ret_from_fork+0x10/0x20
> [ 106.182187] Code: d503201f d503201f d2800001 aa0103e4 (c8e47c02)
> [ 106.188550] ---[ end trace 62bf0e0b37a16815 ]---
> [ 106.193261] Kernel panic - not syncing: Oops: Fatal exception
> [ 106.199106] SMP: stopping secondary CPUs
> [ 106.203401] Kernel Offset: 0x24294d740000 from 0xffff800010000000
> [ 106.209584] PHYS_OFFSET: 0xffffa645e0000000
> [ 106.214011] CPU features: 0x0,40000843,06400846
> [ 106.218651] Memory Limit: none
> [ 106.221822] ---[ end Kernel panic - not syncing: Oops: Fatal
> exception ]---
>
>
> I am wondering if this is some sort of race condition in the thermal
> shutdown path. I would be interested to know if you see the same.
Indeed, it feels like a race condition. I tried to reproduce again on
A500 using next-20220217 and nothing bad happens.
# sensors
nct1008-i2c-2-4c
Adapter: 7000d000.i2c
temp1: +31.0°C (low = -64.0°C, high = +30.0°C) ALARM (HIGH)
(crit = +120.0°C, hyst = +110.0°C)
temp2: +37.8°C (low = -64.0°C, high = +30.0°C) ALARM (HIGH)
(crit = +115.0°C, hyst = +105.0°C)
On 2/21/22 08:22, Jon Hunter wrote:
>
> On 21/02/2022 16:16, Jon Hunter wrote:
>>
>> On 21/02/2022 16:02, Guenter Roeck wrote:
>>
>> ...
>>
>>>> The platform I see this on does use device-tree and it does have a node for the ti,tmp451 device which uses the lm90 device. This platform uses the device-tree source arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451 node is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
>>>>
>>>
>>> Interesting. It appears that the call to devm_thermal_zone_of_sensor_register()
>>> in the hwmon core nevertheless returns -ENODEV which is not handled properly
>>> in the hwmon core. I can see a number of reasons for this to happen:
>>> - there is no devicetree node for the lm90 device
>>> - there is no thermal-zones devicetree node
>>> - there is no thermal zone entry in the thermal-zones node which matches
>>> the sensor
>>
>>
>> So we definitely have the node for the lm90 device and a thermal-zones node, but I do not see a thermal-sensor node. Maybe this is what we are missing?
>
> Actually, that is not true. We do have thermal-sensor nodes in arch/arm64/boot/dts/nvidia/tegra194.dtsi.
>
There is probably a zone to sensor id mismatch. hwmon sends the sensor index
as sensor_id to the thermal subsystem. Those sensor IDs would be 0, 1, and
possibly 2 for the lm90 driver. Assuming this should match the thermal-sensors
values in arch/arm64/boot/dts/nvidia/tegra194.dtsi, those start with 2,
so there would be a likely mismatch. Also, all those dtsi entries match
against pbmp/thermal, not against the lm90 sensor(s).
Thanks,
Guenter
21.02.2022 15:56, Jon Hunter пишет:
>
> On 21/02/2022 12:36, Dmitry Osipenko wrote:
>> 21.02.2022 15:01, Jon Hunter пишет:
>>> Hi Dmitry,
>>>
>>> On 18/06/2021 22:54, Dmitry Osipenko wrote:
>>>> Use hwmon_notify_event() to notify userspace and thermal core about
>>>> temperature changes.
>>>>
>>>> Suggested-by: Guenter Roeck <[email protected]>
>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>> ---
>>>> drivers/hwmon/lm90.c | 44
>>>> +++++++++++++++++++++++++++++++++-----------
>>>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>>> index 2e057fad05b4..e7b678a40b39 100644
>>>> --- a/drivers/hwmon/lm90.c
>>>> +++ b/drivers/hwmon/lm90.c
>>>> @@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
>>>> struct lm90_data {
>>>> struct i2c_client *client;
>>>> + struct device *hwmon_dev;
>>>> u32 channel_config[4];
>>>> struct hwmon_channel_info temp_info;
>>>> const struct hwmon_channel_info *info[3];
>>>> @@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client
>>>> *client, u16 *status)
>>>> if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH |
>>>> LM90_STATUS_LTHRM)) ||
>>>> (st2 & MAX6696_STATUS2_LOT2))
>>>> - dev_warn(&client->dev,
>>>> - "temp%d out of range, please check!\n", 1);
>>>> + dev_dbg(&client->dev,
>>>> + "temp%d out of range, please check!\n", 1);
>>>> if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH |
>>>> LM90_STATUS_RTHRM)) ||
>>>> (st2 & MAX6696_STATUS2_ROT2))
>>>> - dev_warn(&client->dev,
>>>> - "temp%d out of range, please check!\n", 2);
>>>> + dev_dbg(&client->dev,
>>>> + "temp%d out of range, please check!\n", 2);
>>>> if (st & LM90_STATUS_ROPEN)
>>>> - dev_warn(&client->dev,
>>>> - "temp%d diode open, please check!\n", 2);
>>>> + dev_dbg(&client->dev,
>>>> + "temp%d diode open, please check!\n", 2);
>>>> if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
>>>> MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
>>>> - dev_warn(&client->dev,
>>>> - "temp%d out of range, please check!\n", 3);
>>>> + dev_dbg(&client->dev,
>>>> + "temp%d out of range, please check!\n", 3);
>>>> if (st2 & MAX6696_STATUS2_R2OPEN)
>>>> - dev_warn(&client->dev,
>>>> - "temp%d diode open, please check!\n", 3);
>>>> + dev_dbg(&client->dev,
>>>> + "temp%d diode open, please check!\n", 3);
>>>> +
>>>> + if (st & LM90_STATUS_LLOW)
>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> + hwmon_temp_min, 0);
>>>> + if (st & LM90_STATUS_RLOW)
>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> + hwmon_temp_min, 1);
>>>> + if (st2 & MAX6696_STATUS2_R2LOW)
>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> + hwmon_temp_min, 2);
>>>> + if (st & LM90_STATUS_LHIGH)
>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> + hwmon_temp_max, 0);
>>>> + if (st & LM90_STATUS_RHIGH)
>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> + hwmon_temp_max, 1);
>>>> + if (st2 & MAX6696_STATUS2_R2HIGH)
>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>> + hwmon_temp_max, 2);
>>>
>>>
>>> We observed a random null pointer deference crash somewhere in the
>>> thermal core (crash log below is not very helpful) when calling
>>> mutex_lock(). It looks like we get an interrupt when this crash
>>> happens.
>>>
>>> Looking at the lm90 driver, per the above, I now see we are calling
>>> hwmon_notify_event() from the lm90 interrupt handler. Looking at
>>> hwmon_notify_event() I see that ...
>>>
>>> hwmon_notify_event()
>>> --> hwmon_thermal_notify()
>>> --> thermal_zone_device_update()
>>> --> update_temperature()
>>> --> mutex_lock()
>>>
>>> So although I don't completely understand the crash, it does seem
>>> that we should not be calling hwmon_notify_event() from the
>>> interrupt handler.
>>>
>>> BTW I have not reproduced this myself yet, so I have just been
>>> reviewing the code to try and understand this.
>>
>> Matt Merhar was experiencing a similar issue on T30 Ouya, but I never
>> managed to reproduce it on Nexus 7 and Acer A500 tablets, and couldn't
>> spot any problem in the code. IIRC, it was a NULL dereference of another
>> pointer within that code.
>
>
> OK. From looking at the above I don't think we can call
> hwmon_notify_event() from an interrupt handler because this is going to
> try and request a mutex. So we need to fix that.
The interrupt is threaded, so it can take a mutex.
On 2/21/22 04:01, Jon Hunter wrote:
> Hi Dmitry,
>
> On 18/06/2021 22:54, Dmitry Osipenko wrote:
>> Use hwmon_notify_event() to notify userspace and thermal core about
>> temperature changes.
>>
>> Suggested-by: Guenter Roeck <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/hwmon/lm90.c | 44 +++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 2e057fad05b4..e7b678a40b39 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
>> struct lm90_data {
>> struct i2c_client *client;
>> + struct device *hwmon_dev;
>> u32 channel_config[4];
>> struct hwmon_channel_info temp_info;
>> const struct hwmon_channel_info *info[3];
>> @@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client *client, u16 *status)
>> if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH | LM90_STATUS_LTHRM)) ||
>> (st2 & MAX6696_STATUS2_LOT2))
>> - dev_warn(&client->dev,
>> - "temp%d out of range, please check!\n", 1);
>> + dev_dbg(&client->dev,
>> + "temp%d out of range, please check!\n", 1);
>> if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH | LM90_STATUS_RTHRM)) ||
>> (st2 & MAX6696_STATUS2_ROT2))
>> - dev_warn(&client->dev,
>> - "temp%d out of range, please check!\n", 2);
>> + dev_dbg(&client->dev,
>> + "temp%d out of range, please check!\n", 2);
>> if (st & LM90_STATUS_ROPEN)
>> - dev_warn(&client->dev,
>> - "temp%d diode open, please check!\n", 2);
>> + dev_dbg(&client->dev,
>> + "temp%d diode open, please check!\n", 2);
>> if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
>> MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
>> - dev_warn(&client->dev,
>> - "temp%d out of range, please check!\n", 3);
>> + dev_dbg(&client->dev,
>> + "temp%d out of range, please check!\n", 3);
>> if (st2 & MAX6696_STATUS2_R2OPEN)
>> - dev_warn(&client->dev,
>> - "temp%d diode open, please check!\n", 3);
>> + dev_dbg(&client->dev,
>> + "temp%d diode open, please check!\n", 3);
>> +
>> + if (st & LM90_STATUS_LLOW)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_min, 0);
>> + if (st & LM90_STATUS_RLOW)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_min, 1);
>> + if (st2 & MAX6696_STATUS2_R2LOW)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_min, 2);
>> + if (st & LM90_STATUS_LHIGH)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_max, 0);
>> + if (st & LM90_STATUS_RHIGH)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_max, 1);
>> + if (st2 & MAX6696_STATUS2_R2HIGH)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_max, 2);
>
>
> We observed a random null pointer deference crash somewhere in the
> thermal core (crash log below is not very helpful) when calling
> mutex_lock(). It looks like we get an interrupt when this crash
> happens.
>
> Looking at the lm90 driver, per the above, I now see we are calling
> hwmon_notify_event() from the lm90 interrupt handler. Looking at
> hwmon_notify_event() I see that ...
>
> hwmon_notify_event()
> --> hwmon_thermal_notify()
> --> thermal_zone_device_update()
> --> update_temperature()
> --> mutex_lock()
>
> So although I don't completely understand the crash, it does seem
> that we should not be calling hwmon_notify_event() from the
> interrupt handler.
>
As mentioned separately, this is not the problem.
I think the problem may be that this is not a devicetree system
(or the lm90 devide does not have a devicetree node), but thermal
notification currently only works in such systems because the hwmon
subsystem uses the devicetree registration method. At the same time,
CONFIG_THERMAL_OF is obviously enabled. Unfortunately, the hwmon code
does not bail out in that situation due to another bug.
I'll revert the related patches. This will have to be sorted out
separately in a later kernel release.
Thanks,
Guenter
> BTW I have not reproduced this myself yet, so I have just been
> reviewing the code to try and understand this.
>
> Jon
>
> [ 7465.595066] Unable to handle kernel NULL pointer dereference at virtual address 00000000000003cd
> [ 7465.596619] Mem abort info:
> [ 7465.597854] ESR = 0x96000021
> [ 7465.599097] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 7465.600338] SET = 0, FnV = 0
> [ 7465.601526] EA = 0, S1PTW = 0
> [ 7465.602705] FSC = 0x21: alignment fault
> [ 7465.603885] Data abort info:
> [ 7465.605017] ISV = 0, ISS = 0x00000021
> [ 7465.606171] CM = 0, WnR = 0
> [ 7465.607301] user pgtable: 64k pages, 48-bit VAs, pgdp=00000001041f1800
> [ 7465.608490] [00000000000003cd] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
> [ 7465.609814] Internal error: Oops: 96000021 [#1] PREEMPT SMP
> [ 7465.610991] Modules linked in: bridge stp llc snd_soc_tegra210_admaif snd_soc_tegra_pcm snd_soc_tegra210_mixer snd_soc_tegra210_mvc snd_soc_tegra210_i2s snd_soc_tegra210_dmic sn
> d_soc_tegra210_adx snd_soc_tegra210_sfc snd_soc_tegra210_amx snd_soc_tegra210_ahub tegra210_adma snd_soc_rt5659 snd_soc_rl6231 pwm_tegra tegra_aconnect snd_hda_codec_hdmi rfkill snd_hda_tegra snd_hda_codec at24 phy_tegra194_p2u snd_hda_core lm90 snd_soc_tegra_audio_graph_card tegra_bpmp_thermal snd_soc_audio_graph_card snd_soc_simple_card_utils pwm_fan crct10dif_ce pcie_tegra194 cec drm_kms_helper drm ip_tables x_tables ipv6
> [ 7465.632232] CPU: 2 PID: 433 Comm: irq/140-lm90 Tainted: G O 5.16.0-tegra-g9d109504d83a #1
> [ 7465.636285] Hardware name: Unknown NVIDIA Jetson AGX Xavier Developer Kit/NVIDIA Jetson AGX Xavier Developer Kit, BIOS v1.1.2-901d3c52 02/07/2022
> [ 7465.650457] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 7465.656210] pc : mutex_lock+0x18/0x60
> [ 7465.660134] lr : thermal_zone_device_update+0x40/0x2e0
> [ 7465.665117] sp : ffff800014c4fc60
> [ 7465.668781] x29: ffff800014c4fc60 x28: ffff365ee3f6e000 x27: ffffdde218426790
> [ 7465.675882] x26: ffff365ee3f6e000 x25: 0000000000000000 x24: ffff365ee3f6e000
> [ 7465.683485] x23: ffffdde218426870 x22: ffff365ee3f6e000 x21: 00000000000003cd
> [ 7465.690816] x20: ffff365ee8bf3308 x19: ffffffffffffffed x18: 0000000000000000
> [ 7465.697982] x17: ffffdde21842689c x16: ffffdde1cb7a0b7c x15: 0000000000000040
> [ 7465.705320] x14: ffffdde21a4889a0 x13: 0000000000000228 x12: 0000000000000000
> [ 7465.712493] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
> [ 7465.719580] x8 : 0000000001120000 x7 : 0000000000000001 x6 : 0000000000000000
> [ 7465.727099] x5 : 0068000878e20f07 x4 : 0000000000000000 x3 : 00000000000003cd
> [ 7465.734348] x2 : ffff365ee3f6e000 x1 : 0000000000000000 x0 : 00000000000003cd
> [ 7465.741347] Call trace:
> [ 7465.744207] mutex_lock+0x18/0x60
> [ 7465.747427] hwmon_notify_event+0xfc/0x110
> [ 7465.751358] 0xffffdde1cb7a0a90
> [ 7465.754574] 0xffffdde1cb7a0b7c
> [ 7465.757705] irq_thread_fn+0x2c/0xa0
> [ 7465.760937] irq_thread+0x134/0x240
> [ 7465.764850] kthread+0x178/0x190
> [ 7465.768083] ret_from_fork+0x10/0x20
> [ 7465.771748] Code: d503201f d503201f d2800001 aa0103e4 (c8e47c02)
> [ 7465.777865] ---[ end trace f0b3723991411538 ]---
>
21.02.2022 19:02, Guenter Roeck пишет:
> On 2/21/22 07:49, Jon Hunter wrote:
>>
>> On 21/02/2022 15:43, Guenter Roeck wrote:
>>
>> ...
>>
>>>> We observed a random null pointer deference crash somewhere in the
>>>> thermal core (crash log below is not very helpful) when calling
>>>> mutex_lock(). It looks like we get an interrupt when this crash
>>>> happens.
>>>>
>>>> Looking at the lm90 driver, per the above, I now see we are calling
>>>> hwmon_notify_event() from the lm90 interrupt handler. Looking at
>>>> hwmon_notify_event() I see that ...
>>>>
>>>> hwmon_notify_event()
>>>> --> hwmon_thermal_notify()
>>>> --> thermal_zone_device_update()
>>>> --> update_temperature()
>>>> --> mutex_lock()
>>>>
>>>> So although I don't completely understand the crash, it does seem
>>>> that we should not be calling hwmon_notify_event() from the
>>>> interrupt handler.
>>>>
>>> As mentioned separately, this is not the problem.
>>
>> Yes I can see that now.
>>
>>> I think the problem may be that this is not a devicetree system
>>> (or the lm90 devide does not have a devicetree node), but thermal
>>> notification currently only works in such systems because the hwmon
>>> subsystem uses the devicetree registration method. At the same time,
>>> CONFIG_THERMAL_OF is obviously enabled. Unfortunately, the hwmon code
>>> does not bail out in that situation due to another bug.
>>
>> The platform I see this on does use device-tree and it does have a
>> node for the ti,tmp451 device which uses the lm90 device. This
>> platform uses the device-tree source
>> arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451 node
>> is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
>>
>
> Interesting. It appears that the call to
> devm_thermal_zone_of_sensor_register()
> in the hwmon core nevertheless returns -ENODEV which is not handled
> properly
> in the hwmon core. I can see a number of reasons for this to happen:
> - there is no devicetree node for the lm90 device
> - there is no thermal-zones devicetree node
> - there is no thermal zone entry in the thermal-zones node which matches
> the sensor
>
> We'll have to revert the lm90 changes until this is sorted out.
Oh, yeah. Seems there is a problem there and tzd pointer could be
-ENODEV. But it's a hwmon core problem, which apparently existed for a
long time, not the lm90 problem.
On 21/02/2022 15:43, Guenter Roeck wrote:
...
>> We observed a random null pointer deference crash somewhere in the
>> thermal core (crash log below is not very helpful) when calling
>> mutex_lock(). It looks like we get an interrupt when this crash
>> happens.
>>
>> Looking at the lm90 driver, per the above, I now see we are calling
>> hwmon_notify_event() from the lm90 interrupt handler. Looking at
>> hwmon_notify_event() I see that ...
>>
>> hwmon_notify_event()
>> --> hwmon_thermal_notify()
>> --> thermal_zone_device_update()
>> --> update_temperature()
>> --> mutex_lock()
>>
>> So although I don't completely understand the crash, it does seem
>> that we should not be calling hwmon_notify_event() from the
>> interrupt handler.
>>
> As mentioned separately, this is not the problem.
Yes I can see that now.
> I think the problem may be that this is not a devicetree system
> (or the lm90 devide does not have a devicetree node), but thermal
> notification currently only works in such systems because the hwmon
> subsystem uses the devicetree registration method. At the same time,
> CONFIG_THERMAL_OF is obviously enabled. Unfortunately, the hwmon code
> does not bail out in that situation due to another bug.
The platform I see this on does use device-tree and it does have a node
for the ti,tmp451 device which uses the lm90 device. This platform uses
the device-tree source
arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451 node
is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
Cheers
Jon
--
nvpublic
21.02.2022 15:01, Jon Hunter пишет:
> Hi Dmitry,
>
> On 18/06/2021 22:54, Dmitry Osipenko wrote:
>> Use hwmon_notify_event() to notify userspace and thermal core about
>> temperature changes.
>>
>> Suggested-by: Guenter Roeck <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/hwmon/lm90.c | 44 +++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>> index 2e057fad05b4..e7b678a40b39 100644
>> --- a/drivers/hwmon/lm90.c
>> +++ b/drivers/hwmon/lm90.c
>> @@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
>> struct lm90_data {
>> struct i2c_client *client;
>> + struct device *hwmon_dev;
>> u32 channel_config[4];
>> struct hwmon_channel_info temp_info;
>> const struct hwmon_channel_info *info[3];
>> @@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client
>> *client, u16 *status)
>> if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH |
>> LM90_STATUS_LTHRM)) ||
>> (st2 & MAX6696_STATUS2_LOT2))
>> - dev_warn(&client->dev,
>> - "temp%d out of range, please check!\n", 1);
>> + dev_dbg(&client->dev,
>> + "temp%d out of range, please check!\n", 1);
>> if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH |
>> LM90_STATUS_RTHRM)) ||
>> (st2 & MAX6696_STATUS2_ROT2))
>> - dev_warn(&client->dev,
>> - "temp%d out of range, please check!\n", 2);
>> + dev_dbg(&client->dev,
>> + "temp%d out of range, please check!\n", 2);
>> if (st & LM90_STATUS_ROPEN)
>> - dev_warn(&client->dev,
>> - "temp%d diode open, please check!\n", 2);
>> + dev_dbg(&client->dev,
>> + "temp%d diode open, please check!\n", 2);
>> if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
>> MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
>> - dev_warn(&client->dev,
>> - "temp%d out of range, please check!\n", 3);
>> + dev_dbg(&client->dev,
>> + "temp%d out of range, please check!\n", 3);
>> if (st2 & MAX6696_STATUS2_R2OPEN)
>> - dev_warn(&client->dev,
>> - "temp%d diode open, please check!\n", 3);
>> + dev_dbg(&client->dev,
>> + "temp%d diode open, please check!\n", 3);
>> +
>> + if (st & LM90_STATUS_LLOW)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_min, 0);
>> + if (st & LM90_STATUS_RLOW)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_min, 1);
>> + if (st2 & MAX6696_STATUS2_R2LOW)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_min, 2);
>> + if (st & LM90_STATUS_LHIGH)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_max, 0);
>> + if (st & LM90_STATUS_RHIGH)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_max, 1);
>> + if (st2 & MAX6696_STATUS2_R2HIGH)
>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>> + hwmon_temp_max, 2);
>
>
> We observed a random null pointer deference crash somewhere in the
> thermal core (crash log below is not very helpful) when calling
> mutex_lock(). It looks like we get an interrupt when this crash
> happens.
>
> Looking at the lm90 driver, per the above, I now see we are calling
> hwmon_notify_event() from the lm90 interrupt handler. Looking at
> hwmon_notify_event() I see that ...
>
> hwmon_notify_event()
> --> hwmon_thermal_notify()
> --> thermal_zone_device_update()
> --> update_temperature()
> --> mutex_lock()
>
> So although I don't completely understand the crash, it does seem
> that we should not be calling hwmon_notify_event() from the
> interrupt handler.
>
> BTW I have not reproduced this myself yet, so I have just been
> reviewing the code to try and understand this.
Matt Merhar was experiencing a similar issue on T30 Ouya, but I never
managed to reproduce it on Nexus 7 and Acer A500 tablets, and couldn't
spot any problem in the code. IIRC, it was a NULL dereference of another
pointer within that code.
We tried to add couple debug printks and the problem disappears when you
try to look at it, which is very suspicious.
>
> [ 7465.595066] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000000003cd
> [ 7465.596619] Mem abort info:
> [ 7465.597854] ESR = 0x96000021
> [ 7465.599097] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 7465.600338] SET = 0, FnV = 0
> [ 7465.601526] EA = 0, S1PTW = 0
> [ 7465.602705] FSC = 0x21: alignment fault
> [ 7465.603885] Data abort info:
> [ 7465.605017] ISV = 0, ISS = 0x00000021
> [ 7465.606171] CM = 0, WnR = 0
> [ 7465.607301] user pgtable: 64k pages, 48-bit VAs, pgdp=00000001041f1800
> [ 7465.608490] [00000000000003cd] pgd=0000000000000000,
> p4d=0000000000000000, pud=0000000000000000
> [ 7465.609814] Internal error: Oops: 96000021 [#1] PREEMPT SMP
> [ 7465.610991] Modules linked in: bridge stp llc snd_soc_tegra210_admaif
> snd_soc_tegra_pcm snd_soc_tegra210_mixer snd_soc_tegra210_mvc
> snd_soc_tegra210_i2s snd_soc_tegra210_dmic sn
> d_soc_tegra210_adx snd_soc_tegra210_sfc snd_soc_tegra210_amx
> snd_soc_tegra210_ahub tegra210_adma snd_soc_rt5659 snd_soc_rl6231
> pwm_tegra tegra_aconnect snd_hda_codec_hdmi rfkill snd_hda_tegra
> snd_hda_codec at24 phy_tegra194_p2u snd_hda_core lm90
> snd_soc_tegra_audio_graph_card tegra_bpmp_thermal
> snd_soc_audio_graph_card snd_soc_simple_card_utils pwm_fan crct10dif_ce
> pcie_tegra194 cec drm_kms_helper drm ip_tables x_tables ipv6
> [ 7465.632232] CPU: 2 PID: 433 Comm: irq/140-lm90 Tainted: G
> O 5.16.0-tegra-g9d109504d83a #1
> [ 7465.636285] Hardware name: Unknown NVIDIA Jetson AGX Xavier Developer
> Kit/NVIDIA Jetson AGX Xavier Developer Kit, BIOS v1.1.2-901d3c52 02/07/2022
> [ 7465.650457] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [ 7465.656210] pc : mutex_lock+0x18/0x60
> [ 7465.660134] lr : thermal_zone_device_update+0x40/0x2e0
> [ 7465.665117] sp : ffff800014c4fc60
> [ 7465.668781] x29: ffff800014c4fc60 x28: ffff365ee3f6e000 x27:
> ffffdde218426790
> [ 7465.675882] x26: ffff365ee3f6e000 x25: 0000000000000000 x24:
> ffff365ee3f6e000
> [ 7465.683485] x23: ffffdde218426870 x22: ffff365ee3f6e000 x21:
> 00000000000003cd
> [ 7465.690816] x20: ffff365ee8bf3308 x19: ffffffffffffffed x18:
> 0000000000000000
> [ 7465.697982] x17: ffffdde21842689c x16: ffffdde1cb7a0b7c x15:
> 0000000000000040
> [ 7465.705320] x14: ffffdde21a4889a0 x13: 0000000000000228 x12:
> 0000000000000000
> [ 7465.712493] x11: 0000000000000000 x10: 0000000000000000 x9 :
> 0000000000000000
> [ 7465.719580] x8 : 0000000001120000 x7 : 0000000000000001 x6 :
> 0000000000000000
> [ 7465.727099] x5 : 0068000878e20f07 x4 : 0000000000000000 x3 :
> 00000000000003cd
> [ 7465.734348] x2 : ffff365ee3f6e000 x1 : 0000000000000000 x0 :
> 00000000000003cd
> [ 7465.741347] Call trace:
> [ 7465.744207] mutex_lock+0x18/0x60
> [ 7465.747427] hwmon_notify_event+0xfc/0x110
> [ 7465.751358] 0xffffdde1cb7a0a90
> [ 7465.754574] 0xffffdde1cb7a0b7c
> [ 7465.757705] irq_thread_fn+0x2c/0xa0
> [ 7465.760937] irq_thread+0x134/0x240
> [ 7465.764850] kthread+0x178/0x190
> [ 7465.768083] ret_from_fork+0x10/0x20
> [ 7465.771748] Code: d503201f d503201f d2800001 aa0103e4 (c8e47c02)
> [ 7465.777865] ---[ end trace f0b3723991411538 ]---
>
On 21/02/2022 12:59, Dmitry Osipenko wrote:
> 21.02.2022 15:56, Jon Hunter пишет:
>>
>> On 21/02/2022 12:36, Dmitry Osipenko wrote:
>>> 21.02.2022 15:01, Jon Hunter пишет:
>>>> Hi Dmitry,
>>>>
>>>> On 18/06/2021 22:54, Dmitry Osipenko wrote:
>>>>> Use hwmon_notify_event() to notify userspace and thermal core about
>>>>> temperature changes.
>>>>>
>>>>> Suggested-by: Guenter Roeck <[email protected]>
>>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>>> ---
>>>>> drivers/hwmon/lm90.c | 44
>>>>> +++++++++++++++++++++++++++++++++-----------
>>>>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
>>>>> index 2e057fad05b4..e7b678a40b39 100644
>>>>> --- a/drivers/hwmon/lm90.c
>>>>> +++ b/drivers/hwmon/lm90.c
>>>>> @@ -465,6 +465,7 @@ enum lm90_temp11_reg_index {
>>>>> struct lm90_data {
>>>>> struct i2c_client *client;
>>>>> + struct device *hwmon_dev;
>>>>> u32 channel_config[4];
>>>>> struct hwmon_channel_info temp_info;
>>>>> const struct hwmon_channel_info *info[3];
>>>>> @@ -1731,22 +1732,41 @@ static bool lm90_is_tripped(struct i2c_client
>>>>> *client, u16 *status)
>>>>> if ((st & (LM90_STATUS_LLOW | LM90_STATUS_LHIGH |
>>>>> LM90_STATUS_LTHRM)) ||
>>>>> (st2 & MAX6696_STATUS2_LOT2))
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d out of range, please check!\n", 1);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d out of range, please check!\n", 1);
>>>>> if ((st & (LM90_STATUS_RLOW | LM90_STATUS_RHIGH |
>>>>> LM90_STATUS_RTHRM)) ||
>>>>> (st2 & MAX6696_STATUS2_ROT2))
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d out of range, please check!\n", 2);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d out of range, please check!\n", 2);
>>>>> if (st & LM90_STATUS_ROPEN)
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d diode open, please check!\n", 2);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d diode open, please check!\n", 2);
>>>>> if (st2 & (MAX6696_STATUS2_R2LOW | MAX6696_STATUS2_R2HIGH |
>>>>> MAX6696_STATUS2_R2THRM | MAX6696_STATUS2_R2OT2))
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d out of range, please check!\n", 3);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d out of range, please check!\n", 3);
>>>>> if (st2 & MAX6696_STATUS2_R2OPEN)
>>>>> - dev_warn(&client->dev,
>>>>> - "temp%d diode open, please check!\n", 3);
>>>>> + dev_dbg(&client->dev,
>>>>> + "temp%d diode open, please check!\n", 3);
>>>>> +
>>>>> + if (st & LM90_STATUS_LLOW)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_min, 0);
>>>>> + if (st & LM90_STATUS_RLOW)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_min, 1);
>>>>> + if (st2 & MAX6696_STATUS2_R2LOW)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_min, 2);
>>>>> + if (st & LM90_STATUS_LHIGH)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_max, 0);
>>>>> + if (st & LM90_STATUS_RHIGH)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_max, 1);
>>>>> + if (st2 & MAX6696_STATUS2_R2HIGH)
>>>>> + hwmon_notify_event(data->hwmon_dev, hwmon_temp,
>>>>> + hwmon_temp_max, 2);
>>>>
>>>>
>>>> We observed a random null pointer deference crash somewhere in the
>>>> thermal core (crash log below is not very helpful) when calling
>>>> mutex_lock(). It looks like we get an interrupt when this crash
>>>> happens.
>>>>
>>>> Looking at the lm90 driver, per the above, I now see we are calling
>>>> hwmon_notify_event() from the lm90 interrupt handler. Looking at
>>>> hwmon_notify_event() I see that ...
>>>>
>>>> hwmon_notify_event()
>>>> --> hwmon_thermal_notify()
>>>> --> thermal_zone_device_update()
>>>> --> update_temperature()
>>>> --> mutex_lock()
>>>>
>>>> So although I don't completely understand the crash, it does seem
>>>> that we should not be calling hwmon_notify_event() from the
>>>> interrupt handler.
>>>>
>>>> BTW I have not reproduced this myself yet, so I have just been
>>>> reviewing the code to try and understand this.
>>>
>>> Matt Merhar was experiencing a similar issue on T30 Ouya, but I never
>>> managed to reproduce it on Nexus 7 and Acer A500 tablets, and couldn't
>>> spot any problem in the code. IIRC, it was a NULL dereference of another
>>> pointer within that code.
>>
>>
>> OK. From looking at the above I don't think we can call
>> hwmon_notify_event() from an interrupt handler because this is going to
>> try and request a mutex. So we need to fix that.
>
> The interrupt is threaded, so it can take a mutex.
Ah yes, I clearly overlooked that detail.
Good news is that I have been able to reproduce this on Jetson Xavier by ...
$ echo 40000 | sudo tee /sys/devices/platform/bpmp/bpmp\:i2c/i2c-0/0-004c/hwmon/hwmon0/temp1_max
40000
[ 105.890995] Unable to handle kernel NULL pointer dereference at virtual address 00000000000003cd
[ 105.900105] Mem abort info:
[ 105.903328] ESR = 0x96000021
[ 105.906673] EC = 0x25: DABT (current EL), IL = 32 bits
[ 105.912407] SET = 0, FnV = 0
[ 105.915751] EA = 0, S1PTW = 0
[ 105.919230] FSC = 0x21: alignment fault
[ 105.923698] Data abort info:
[ 105.926853] ISV = 0, ISS = 0x00000021
[ 105.931139] CM = 0, WnR = 0
[ 105.934420] user pgtable: 64k pages, 48-bit VAs, pgdp=0000000101f6b600
[ 105.941230] [00000000000003cd] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
[ 105.950864] Internal error: Oops: 96000021 [#1] PREEMPT SMP
[ 105.956608] Modules linked in: btrfs blake2b_generic libcrc32c xor xor_neon raid6_pq zstd_compress rfkill snd_soc_tegra210_mixer snd_soc_tegra210_adx snd_soc_tegra210_dmic snd_soc_tegra210_mvc snd_soc_tegra210_amx snd_soc_tegra210_sfc snd_soc_tegra210_admaif snd_soc_tegra_pcm snd_soc_tegra210_i2s snd_soc_tegra210_ahub uvcvideo videobuf2_vmalloc tegra210_adma videobuf2_memops videobuf2_v4l2 cec videobuf2_common drm_kms_
helper videodev mc snd_soc_rt5659 snd_soc_rl6231 pwm_tegra tegra_aconnect snd_hda_codec_hdmi lm90 tegra_bpmp_thermal snd_hda_tegra snd_soc_tegra_audio_graph_card snd_hda_codec snd_hda_core phy_tegra194_p2u snd_soc_
audio_graph_card at24 snd_soc_simple_card_utils pwm_fan pcie_tegra194 crct10dif_ce drm ip_tables x_tables ipv6
[ 106.032497] CPU: 3 PID: 296 Comm: irq/126-lm90 Tainted: G O 5.16.0-tegra-291805-gf905a41db850 #3
[ 106.042869] Hardware name: Unknown NVIDIA Jetson AGX Xavier Developer Kit/NVIDIA Jetson AGX Xavier Developer Kit, BIOS v1.1.2-901d3c52ed23 02/14/2022
[ 106.056392] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 106.063903] pc : mutex_lock+0x18/0x60
[ 106.067750] lr : thermal_zone_device_update+0x40/0x2e0
[ 106.073161] sp : ffff80001494fc60
[ 106.076553] x29: ffff80001494fc60 x28: ffff59bb27801c00 x27: ffffa4295d826790
[ 106.084052] x26: ffff59bb27801c00 x25: 0000000000000000 x24: ffff59bb27801c00
[ 106.091541] x23: ffffa4295d826870 x22: ffff59bb27801c00 x21: 00000000000003cd
[ 106.098905] x20: ffff59bb28078f08 x19: ffffffffffffffed x18: 0000000000000000
[ 106.106387] x17: ffffa4295d82689c x16: ffffa4292d400b7c x15: 0000000000000040
[ 106.113766] x14: ffffa4295f8889a0 x13: 0000000000000228 x12: 0000000000000000
[ 106.121294] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
[ 106.128793] x8 : 0000000000c2f000 x7 : 0000000000000001 x6 : 0000000000000000
[ 106.136133] x5 : 006800047a8e0f07 x4 : 0000000000000000 x3 : 00000000000003cd
[ 106.143473] x2 : ffff59bb27801c00 x1 : 0000000000000000 x0 : 00000000000003cd
[ 106.150813] Call trace:
[ 106.153333] mutex_lock+0x18/0x60
[ 106.156804] hwmon_notify_event+0xfc/0x110
[ 106.161164] 0xffffa4292d400a74
[ 106.164417] 0xffffa4292d400b7c
[ 106.167659] irq_thread_fn+0x2c/0xa0
[ 106.171359] irq_thread+0x134/0x240
[ 106.174971] kthread+0x178/0x190
[ 106.178469] ret_from_fork+0x10/0x20
[ 106.182187] Code: d503201f d503201f d2800001 aa0103e4 (c8e47c02)
[ 106.188550] ---[ end trace 62bf0e0b37a16815 ]---
[ 106.193261] Kernel panic - not syncing: Oops: Fatal exception
[ 106.199106] SMP: stopping secondary CPUs
[ 106.203401] Kernel Offset: 0x24294d740000 from 0xffff800010000000
[ 106.209584] PHYS_OFFSET: 0xffffa645e0000000
[ 106.214011] CPU features: 0x0,40000843,06400846
[ 106.218651] Memory Limit: none
[ 106.221822] ---[ end Kernel panic - not syncing: Oops: Fatal exception ]---
I am wondering if this is some sort of race condition in the thermal
shutdown path. I would be interested to know if you see the same.
Cheers
Jon
--
nvpublic
On 21/02/2022 16:16, Jon Hunter wrote:
>
> On 21/02/2022 16:02, Guenter Roeck wrote:
>
> ...
>
>>> The platform I see this on does use device-tree and it does have a
>>> node for the ti,tmp451 device which uses the lm90 device. This
>>> platform uses the device-tree source
>>> arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451
>>> node is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
>>>
>>
>> Interesting. It appears that the call to
>> devm_thermal_zone_of_sensor_register()
>> in the hwmon core nevertheless returns -ENODEV which is not handled
>> properly
>> in the hwmon core. I can see a number of reasons for this to happen:
>> - there is no devicetree node for the lm90 device
>> - there is no thermal-zones devicetree node
>> - there is no thermal zone entry in the thermal-zones node which matches
>> the sensor
>
>
> So we definitely have the node for the lm90 device and a thermal-zones
> node, but I do not see a thermal-sensor node. Maybe this is what we are
> missing?
Actually, that is not true. We do have thermal-sensor nodes in
arch/arm64/boot/dts/nvidia/tegra194.dtsi.
Jon
--
nvpublic
On 2/21/22 07:49, Jon Hunter wrote:
>
> On 21/02/2022 15:43, Guenter Roeck wrote:
>
> ...
>
>>> We observed a random null pointer deference crash somewhere in the
>>> thermal core (crash log below is not very helpful) when calling
>>> mutex_lock(). It looks like we get an interrupt when this crash
>>> happens.
>>>
>>> Looking at the lm90 driver, per the above, I now see we are calling
>>> hwmon_notify_event() from the lm90 interrupt handler. Looking at
>>> hwmon_notify_event() I see that ...
>>>
>>> hwmon_notify_event()
>>> --> hwmon_thermal_notify()
>>> --> thermal_zone_device_update()
>>> --> update_temperature()
>>> --> mutex_lock()
>>>
>>> So although I don't completely understand the crash, it does seem
>>> that we should not be calling hwmon_notify_event() from the
>>> interrupt handler.
>>>
>> As mentioned separately, this is not the problem.
>
> Yes I can see that now.
>
>> I think the problem may be that this is not a devicetree system
>> (or the lm90 devide does not have a devicetree node), but thermal
>> notification currently only works in such systems because the hwmon
>> subsystem uses the devicetree registration method. At the same time,
>> CONFIG_THERMAL_OF is obviously enabled. Unfortunately, the hwmon code
>> does not bail out in that situation due to another bug.
>
> The platform I see this on does use device-tree and it does have a node for the ti,tmp451 device which uses the lm90 device. This platform uses the device-tree source arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts and the tmp451 node is in arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi.
>
Interesting. It appears that the call to devm_thermal_zone_of_sensor_register()
in the hwmon core nevertheless returns -ENODEV which is not handled properly
in the hwmon core. I can see a number of reasons for this to happen:
- there is no devicetree node for the lm90 device
- there is no thermal-zones devicetree node
- there is no thermal zone entry in the thermal-zones node which matches
the sensor
We'll have to revert the lm90 changes until this is sorted out.
Guenter