* [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements @ 2021-06-18 21:54 Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 1/4] hwmon: (lm90) Don't override interrupt trigger type Dmitry Osipenko ` (4 more replies) 0 siblings, 5 replies; 23+ messages in thread From: Dmitry Osipenko @ 2021-06-18 21:54 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck; +Cc: linux-kernel, linux-hwmon, linux-tegra Hi, This series makes interrupt usable on NVIDIA Tegra devices, it also switches LM90 driver to use hwmon_notify_event(). Changelog: v3: - No code changes. Added changelog. v2: - Dropped "hwmon: (lm90) Use edge-triggered interrupt" patch and replaced it with "hwmon: (lm90) Don't override interrupt trigger type", as was discussed during review of v1. - Added these new patches: hwmon: (lm90) Use hwmon_notify_event() hwmon: (lm90) Unmask hardware interrupt hwmon: (lm90) Disable interrupt on suspend Dmitry Osipenko (4): hwmon: (lm90) Don't override interrupt trigger type hwmon: (lm90) Use hwmon_notify_event() hwmon: (lm90) Unmask hardware interrupt hwmon: (lm90) Disable interrupt on suspend drivers/hwmon/lm90.c | 79 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 13 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 1/4] hwmon: (lm90) Don't override interrupt trigger type 2021-06-18 21:54 [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Dmitry Osipenko @ 2021-06-18 21:54 ` Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() Dmitry Osipenko ` (3 subsequent siblings) 4 siblings, 0 replies; 23+ messages in thread From: Dmitry Osipenko @ 2021-06-18 21:54 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck; +Cc: linux-kernel, linux-hwmon, linux-tegra The lm90 driver sets interrupt trigger type to level-low. This type is not suitable for sensors like NCT1008 that don't deassert interrupt line until temperature is back to normal, resulting in interrupt storm. The appropriate trigger type should come from OF device description and currently it's overridden by the driver's trigger type. Don't specify the trigger type in the driver code, letting interrupt core to use the device-specific trigger type. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/hwmon/lm90.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index ebbfd5f352c0..2e057fad05b4 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -1908,8 +1908,7 @@ static int lm90_probe(struct i2c_client *client) dev_dbg(dev, "IRQ: %d\n", client->irq); err = devm_request_threaded_irq(dev, client->irq, NULL, lm90_irq_thread, - IRQF_TRIGGER_LOW | IRQF_ONESHOT, - "lm90", client); + IRQF_ONESHOT, "lm90", client); if (err < 0) { dev_err(dev, "cannot request IRQ %d\n", client->irq); return err; -- 2.30.2 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2021-06-18 21:54 [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 1/4] hwmon: (lm90) Don't override interrupt trigger type Dmitry Osipenko @ 2021-06-18 21:54 ` Dmitry Osipenko 2022-02-21 12:01 ` Jon Hunter 2021-06-18 21:54 ` [PATCH v3 3/4] hwmon: (lm90) Unmask hardware interrupt Dmitry Osipenko ` (2 subsequent siblings) 4 siblings, 1 reply; 23+ messages in thread From: Dmitry Osipenko @ 2021-06-18 21:54 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck; +Cc: linux-kernel, linux-hwmon, linux-tegra Use hwmon_notify_event() to notify userspace and thermal core about temperature changes. Suggested-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2021-06-18 21:54 ` [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() Dmitry Osipenko @ 2022-02-21 12:01 ` Jon Hunter 2022-02-21 12:36 ` Dmitry Osipenko 2022-02-21 15:43 ` Guenter Roeck 0 siblings, 2 replies; 23+ messages in thread From: Jon Hunter @ 2022-02-21 12:01 UTC (permalink / raw) To: Dmitry Osipenko, Jean Delvare, Guenter Roeck Cc: linux-kernel, linux-hwmon, linux-tegra 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 <linux@roeck-us.net> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > 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 ]--- ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 12:01 ` Jon Hunter @ 2022-02-21 12:36 ` Dmitry Osipenko 2022-02-21 12:56 ` Jon Hunter 2022-02-21 15:43 ` Guenter Roeck 1 sibling, 1 reply; 23+ messages in thread From: Dmitry Osipenko @ 2022-02-21 12:36 UTC (permalink / raw) To: Jon Hunter, Jean Delvare, Guenter Roeck, Matt Merhar Cc: linux-kernel, linux-hwmon, linux-tegra 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 <linux@roeck-us.net> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> 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 ]--- > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 12:36 ` Dmitry Osipenko @ 2022-02-21 12:56 ` Jon Hunter 2022-02-21 12:59 ` Dmitry Osipenko 0 siblings, 1 reply; 23+ messages in thread From: Jon Hunter @ 2022-02-21 12:56 UTC (permalink / raw) To: Dmitry Osipenko, Jean Delvare, Guenter Roeck, Matt Merhar Cc: linux-kernel, linux-hwmon, linux-tegra 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 <linux@roeck-us.net> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>> --- >>> 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 12:56 ` Jon Hunter @ 2022-02-21 12:59 ` Dmitry Osipenko 2022-02-21 13:50 ` Jon Hunter 2022-02-21 15:25 ` Guenter Roeck 0 siblings, 2 replies; 23+ messages in thread From: Dmitry Osipenko @ 2022-02-21 12:59 UTC (permalink / raw) To: Jon Hunter, Jean Delvare, Guenter Roeck, Matt Merhar Cc: linux-kernel, linux-hwmon, linux-tegra 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 <linux@roeck-us.net> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>>> 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 12:59 ` Dmitry Osipenko @ 2022-02-21 13:50 ` Jon Hunter 2022-02-21 13:59 ` Dmitry Osipenko 2022-02-21 15:25 ` Guenter Roeck 1 sibling, 1 reply; 23+ messages in thread From: Jon Hunter @ 2022-02-21 13:50 UTC (permalink / raw) To: Dmitry Osipenko, Jean Delvare, Guenter Roeck, Matt Merhar Cc: linux-kernel, linux-hwmon, linux-tegra 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 <linux@roeck-us.net> >>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>> --- >>>>> 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 13:50 ` Jon Hunter @ 2022-02-21 13:59 ` Dmitry Osipenko 0 siblings, 0 replies; 23+ messages in thread From: Dmitry Osipenko @ 2022-02-21 13:59 UTC (permalink / raw) To: Jon Hunter, Jean Delvare, Guenter Roeck, Matt Merhar Cc: linux-kernel, linux-hwmon, linux-tegra 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 <linux@roeck-us.net> >>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>>> --- >>>>>> 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) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 12:59 ` Dmitry Osipenko 2022-02-21 13:50 ` Jon Hunter @ 2022-02-21 15:25 ` Guenter Roeck 1 sibling, 0 replies; 23+ messages in thread From: Guenter Roeck @ 2022-02-21 15:25 UTC (permalink / raw) To: Dmitry Osipenko, Jon Hunter, Jean Delvare, Matt Merhar Cc: linux-kernel, linux-hwmon, linux-tegra 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 <linux@roeck-us.net> >>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>> --- >>>>> 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 12:01 ` Jon Hunter 2022-02-21 12:36 ` Dmitry Osipenko @ 2022-02-21 15:43 ` Guenter Roeck 2022-02-21 15:49 ` Jon Hunter 1 sibling, 1 reply; 23+ messages in thread From: Guenter Roeck @ 2022-02-21 15:43 UTC (permalink / raw) To: Jon Hunter, Dmitry Osipenko, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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 <linux@roeck-us.net> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> 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 ]--- > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 15:43 ` Guenter Roeck @ 2022-02-21 15:49 ` Jon Hunter 2022-02-21 16:02 ` Guenter Roeck 0 siblings, 1 reply; 23+ messages in thread From: Jon Hunter @ 2022-02-21 15:49 UTC (permalink / raw) To: Guenter Roeck, Dmitry Osipenko, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 15:49 ` Jon Hunter @ 2022-02-21 16:02 ` Guenter Roeck 2022-02-21 16:13 ` Dmitry Osipenko 2022-02-21 16:16 ` Jon Hunter 0 siblings, 2 replies; 23+ messages in thread From: Guenter Roeck @ 2022-02-21 16:02 UTC (permalink / raw) To: Jon Hunter, Dmitry Osipenko, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 16:02 ` Guenter Roeck @ 2022-02-21 16:13 ` Dmitry Osipenko 2022-02-21 16:16 ` Jon Hunter 1 sibling, 0 replies; 23+ messages in thread From: Dmitry Osipenko @ 2022-02-21 16:13 UTC (permalink / raw) To: Guenter Roeck, Jon Hunter, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 16:02 ` Guenter Roeck 2022-02-21 16:13 ` Dmitry Osipenko @ 2022-02-21 16:16 ` Jon Hunter 2022-02-21 16:20 ` Dmitry Osipenko ` (2 more replies) 1 sibling, 3 replies; 23+ messages in thread From: Jon Hunter @ 2022-02-21 16:16 UTC (permalink / raw) To: Guenter Roeck, Dmitry Osipenko, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 16:16 ` Jon Hunter @ 2022-02-21 16:20 ` Dmitry Osipenko 2022-02-21 16:42 ` Guenter Roeck 2022-02-21 16:22 ` Jon Hunter 2022-02-21 16:23 ` Guenter Roeck 2 siblings, 1 reply; 23+ messages in thread From: Dmitry Osipenko @ 2022-02-21 16:20 UTC (permalink / raw) To: Jon Hunter, Guenter Roeck, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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); } ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 16:20 ` Dmitry Osipenko @ 2022-02-21 16:42 ` Guenter Roeck 0 siblings, 0 replies; 23+ messages in thread From: Guenter Roeck @ 2022-02-21 16:42 UTC (permalink / raw) To: Dmitry Osipenko, Jon Hunter, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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); > } > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 16:16 ` Jon Hunter 2022-02-21 16:20 ` Dmitry Osipenko @ 2022-02-21 16:22 ` Jon Hunter 2022-02-21 18:38 ` Guenter Roeck 2022-02-21 16:23 ` Guenter Roeck 2 siblings, 1 reply; 23+ messages in thread From: Jon Hunter @ 2022-02-21 16:22 UTC (permalink / raw) To: Guenter Roeck, Dmitry Osipenko, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 16:22 ` Jon Hunter @ 2022-02-21 18:38 ` Guenter Roeck 0 siblings, 0 replies; 23+ messages in thread From: Guenter Roeck @ 2022-02-21 18:38 UTC (permalink / raw) To: Jon Hunter, Dmitry Osipenko, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() 2022-02-21 16:16 ` Jon Hunter 2022-02-21 16:20 ` Dmitry Osipenko 2022-02-21 16:22 ` Jon Hunter @ 2022-02-21 16:23 ` Guenter Roeck 2 siblings, 0 replies; 23+ messages in thread From: Guenter Roeck @ 2022-02-21 16:23 UTC (permalink / raw) To: Jon Hunter, Dmitry Osipenko, Jean Delvare Cc: linux-kernel, linux-hwmon, linux-tegra 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 3/4] hwmon: (lm90) Unmask hardware interrupt 2021-06-18 21:54 [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 1/4] hwmon: (lm90) Don't override interrupt trigger type Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() Dmitry Osipenko @ 2021-06-18 21:54 ` Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 4/4] hwmon: (lm90) Disable interrupt on suspend Dmitry Osipenko 2021-06-19 11:10 ` [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Guenter Roeck 4 siblings, 0 replies; 23+ messages in thread From: Dmitry Osipenko @ 2021-06-18 21:54 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck; +Cc: linux-kernel, linux-hwmon, linux-tegra The ALERT interrupt is enabled by default after power-on, but it could be masked by bootloader. For example this is the case on Acer A500 tablet device. Unmask the hardware interrupt if interrupt is provided. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/hwmon/lm90.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index e7b678a40b39..658b486d2f5e 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -1704,6 +1704,13 @@ static int lm90_init_client(struct i2c_client *client, struct lm90_data *data) if (data->kind == max6696) config &= ~0x08; + /* + * Interrupt is enabled by default on reset, but it may be disabled + * by bootloader, unmask it. + */ + if (client->irq) + config &= ~0x80; + config &= 0xBF; /* run */ lm90_update_confreg(data, config); -- 2.30.2 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 4/4] hwmon: (lm90) Disable interrupt on suspend 2021-06-18 21:54 [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Dmitry Osipenko ` (2 preceding siblings ...) 2021-06-18 21:54 ` [PATCH v3 3/4] hwmon: (lm90) Unmask hardware interrupt Dmitry Osipenko @ 2021-06-18 21:54 ` Dmitry Osipenko 2021-06-19 11:10 ` [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Guenter Roeck 4 siblings, 0 replies; 23+ messages in thread From: Dmitry Osipenko @ 2021-06-18 21:54 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck; +Cc: linux-kernel, linux-hwmon, linux-tegra I2C accesses are prohibited and will error out after suspending of the I2C controller, hence we need to ensure that interrupt won't fire on suspend when it's too late. Disable interrupt across suspend/resume. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/hwmon/lm90.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c index 658b486d2f5e..b53f17511b05 100644 --- a/drivers/hwmon/lm90.c +++ b/drivers/hwmon/lm90.c @@ -1973,11 +1973,36 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type, } } +static int __maybe_unused lm90_suspend(struct device *dev) +{ + struct lm90_data *data = dev_get_drvdata(dev); + struct i2c_client *client = data->client; + + if (client->irq) + disable_irq(client->irq); + + return 0; +} + +static int __maybe_unused lm90_resume(struct device *dev) +{ + struct lm90_data *data = dev_get_drvdata(dev); + struct i2c_client *client = data->client; + + if (client->irq) + enable_irq(client->irq); + + return 0; +} + +static SIMPLE_DEV_PM_OPS(lm90_pm_ops, lm90_suspend, lm90_resume); + static struct i2c_driver lm90_driver = { .class = I2C_CLASS_HWMON, .driver = { .name = "lm90", .of_match_table = of_match_ptr(lm90_of_match), + .pm = &lm90_pm_ops, }, .probe_new = lm90_probe, .alert = lm90_alert, -- 2.30.2 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements 2021-06-18 21:54 [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Dmitry Osipenko ` (3 preceding siblings ...) 2021-06-18 21:54 ` [PATCH v3 4/4] hwmon: (lm90) Disable interrupt on suspend Dmitry Osipenko @ 2021-06-19 11:10 ` Guenter Roeck 4 siblings, 0 replies; 23+ messages in thread From: Guenter Roeck @ 2021-06-19 11:10 UTC (permalink / raw) To: Dmitry Osipenko; +Cc: Jean Delvare, linux-kernel, linux-hwmon, linux-tegra On Sat, Jun 19, 2021 at 12:54:51AM +0300, Dmitry Osipenko wrote: > Hi, > > This series makes interrupt usable on NVIDIA Tegra devices, it also > switches LM90 driver to use hwmon_notify_event(). > Series applied. Thanks, Guenter > Changelog: > > v3: - No code changes. Added changelog. > > v2: - Dropped "hwmon: (lm90) Use edge-triggered interrupt" patch > and replaced it with "hwmon: (lm90) Don't override interrupt > trigger type", as was discussed during review of v1. > > - Added these new patches: > > hwmon: (lm90) Use hwmon_notify_event() > hwmon: (lm90) Unmask hardware interrupt > hwmon: (lm90) Disable interrupt on suspend > > Dmitry Osipenko (4): > hwmon: (lm90) Don't override interrupt trigger type > hwmon: (lm90) Use hwmon_notify_event() > hwmon: (lm90) Unmask hardware interrupt > hwmon: (lm90) Disable interrupt on suspend > > drivers/hwmon/lm90.c | 79 ++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 66 insertions(+), 13 deletions(-) > > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-02-21 18:40 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-18 21:54 [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 1/4] hwmon: (lm90) Don't override interrupt trigger type Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 2/4] hwmon: (lm90) Use hwmon_notify_event() Dmitry Osipenko 2022-02-21 12:01 ` Jon Hunter 2022-02-21 12:36 ` Dmitry Osipenko 2022-02-21 12:56 ` Jon Hunter 2022-02-21 12:59 ` Dmitry Osipenko 2022-02-21 13:50 ` Jon Hunter 2022-02-21 13:59 ` Dmitry Osipenko 2022-02-21 15:25 ` Guenter Roeck 2022-02-21 15:43 ` Guenter Roeck 2022-02-21 15:49 ` Jon Hunter 2022-02-21 16:02 ` Guenter Roeck 2022-02-21 16:13 ` Dmitry Osipenko 2022-02-21 16:16 ` Jon Hunter 2022-02-21 16:20 ` Dmitry Osipenko 2022-02-21 16:42 ` Guenter Roeck 2022-02-21 16:22 ` Jon Hunter 2022-02-21 18:38 ` Guenter Roeck 2022-02-21 16:23 ` Guenter Roeck 2021-06-18 21:54 ` [PATCH v3 3/4] hwmon: (lm90) Unmask hardware interrupt Dmitry Osipenko 2021-06-18 21:54 ` [PATCH v3 4/4] hwmon: (lm90) Disable interrupt on suspend Dmitry Osipenko 2021-06-19 11:10 ` [PATCH v3 0/4] HWMON LM90 interrupt fixes and improvements Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.