[PATCH 0/4] thermal: fix locking regressions in linux-next

[PATCH 0/4] thermal: fix locking regressions in linux-next

[Johan Hovold]

This series fixes some of the fallout after the thermal changes that
just landed in linux-next.

Lockdep reported a lock inversion in one of the Qualcomm drivers and a
closer review revealed that the changes had also broken the sysfs
interface for at least three drivers.

Note that a simple revert of the offending patches was not an option as
some of the infrastructure that the old implementation relied on has
also been removed.

Johan


Johan Hovold (4):
  thermal/drivers/qcom: fix set_trip_temp() deadlock
  thermal/drivers/exynos: fix set_trip_temp() deadlock
  thermal/drivers/tegra: fix set_trip_temp() deadlock
  thermal/drivers/qcom: fix lock inversion

 drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 7 ++++++-
 drivers/thermal/samsung/exynos_tmu.c        | 2 +-
 drivers/thermal/tegra/soctherm.c            | 2 +-
 drivers/thermal/thermal_core.c              | 1 +
 include/linux/thermal.h                     | 2 ++
 5 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.37.4

[PATCH 1/4] thermal/drivers/qcom: fix set_trip_temp() deadlock

[Johan Hovold]

The set_trip_temp() callback is used when changing the trip temperature
through sysfs. As it is called with the thermal-zone-device lock held
it must not use thermal_zone_get_trip() directly or it will deadlock.

Fixes: 78c3e2429be8 ("thermal/drivers/qcom: Use generic thermal_zone_get_trip() function")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 2 +-
 drivers/thermal/thermal_core.c              | 1 +
 include/linux/thermal.h                     | 2 ++
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
index 58055a7abaf6..bfaec74f13b2 100644
--- a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
+++ b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
@@ -270,7 +270,7 @@ static int qpnp_tm_set_trip_temp(struct thermal_zone_device *tz, int trip_id, in
 	struct thermal_trip trip;
 	int ret;
 
-	ret = thermal_zone_get_trip(chip->tz_dev, trip_id, &trip);
+	ret = __thermal_zone_get_trip(chip->tz_dev, trip_id, &trip);
 	if (ret)
 		return ret;
 
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index c24c9efcd175..d9a3d9566d73 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1214,6 +1214,7 @@ int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
 
 	return tz->ops->get_trip_type(tz, trip_id, &trip->type);
 }
+EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
 
 int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
 			  struct thermal_trip *trip)
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index e2797f314d99..30353e4b1424 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -334,6 +334,8 @@ static inline void devm_thermal_of_zone_unregister(struct device *dev,
 }
 #endif
 
+int __thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
+			    struct thermal_trip *trip);
 int thermal_zone_get_trip(struct thermal_zone_device *tz, int trip_id,
 			  struct thermal_trip *trip);
 
-- 
2.37.4

[PATCH 2/4] thermal/drivers/exynos: fix set_trip_temp() deadlock

[Johan Hovold]

The set_trip_temp() callback is used when changing the trip temperature
through sysfs. As it is called with the thermal-zone-device lock held
it must not use thermal_zone_get_trip() directly or it will deadlock.

Fixes: 169865e317f2 ("thermal/drivers/exynos: Use generic thermal_zone_get_trip() function")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/thermal/samsung/exynos_tmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 37465af59262..cf9028f80415 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -354,7 +354,7 @@ static void exynos4210_tmu_set_trip_temp(struct exynos_tmu_data *data,
 	struct thermal_trip trip;
 	u8 ref, th_code;
 
-	if (thermal_zone_get_trip(data->tzd, 0, &trip))
+	if (__thermal_zone_get_trip(data->tzd, 0, &trip))
 		return;
 
 	ref = trip.temperature / MCELSIUS;
-- 
2.37.4

Re: [PATCH 2/4] thermal/drivers/exynos: fix set_trip_temp() deadlock

[Johan Hovold]

On Wed, Dec 14, 2022 at 02:16:15PM +0100, Johan Hovold wrote:
> The set_trip_temp() callback is used when changing the trip temperature
> through sysfs. As it is called with the thermal-zone-device lock held
> it must not use thermal_zone_get_trip() directly or it will deadlock.
> 
> Fixes: 169865e317f2 ("thermal/drivers/exynos: Use generic thermal_zone_get_trip() function")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 37465af59262..cf9028f80415 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -354,7 +354,7 @@ static void exynos4210_tmu_set_trip_temp(struct exynos_tmu_data *data,
>  	struct thermal_trip trip;
>  	u8 ref, th_code;
>  
> -	if (thermal_zone_get_trip(data->tzd, 0, &trip))
> +	if (__thermal_zone_get_trip(data->tzd, 0, &trip))
>  		return;
>  
>  	ref = trip.temperature / MCELSIUS;

As Daniel noted, this patch is not correct as this function is not
actually a thermal_zone_device_ops::set_trip_temp callback.

Please drop this one.

Johan

[PATCH 3/4] thermal/drivers/tegra: fix set_trip_temp() deadlock

[Johan Hovold]

The set_trip_temp() callback is used when changing the trip temperature
through sysfs. As it is called with the thermal-zone-device lock held
it must not use thermal_zone_get_trip() directly or it will deadlock.

Fixes: 56d7b397cc29 ("thermal/drivers/tegra: Use generic thermal_zone_get_trip() function")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/thermal/tegra/soctherm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index 4203e74e2f79..220873298d77 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -594,7 +594,7 @@ static int tegra_thermctl_set_trip_temp(struct thermal_zone_device *tz, int trip
 	if (!tz)
 		return -EINVAL;
 
-	ret = thermal_zone_get_trip(tz, trip_id, &trip);
+	ret = __thermal_zone_get_trip(tz, trip_id, &trip);
 	if (ret)
 		return ret;
 
-- 
2.37.4

[PATCH 4/4] thermal/drivers/qcom: fix lock inversion

[Johan Hovold]

The thermal-zone-device lock is held by core when setting trip points
and the driver takes its chip lock in the corresponding callback.

Fetching the thermal trip points using thermal_zone_get_trip() also
involves taking the thermal-zone-device lock, which means that the chip
lock can not be held when doing so.

Drop the chip lock temporarily during probe to avoid the lock inversion
that was detected by lockdep:

  ======================================================
  WARNING: possible circular locking dependency detected
  6.1.0-next-20221213 #122 Not tainted
  ------------------------------------------------------
  systemd-udevd/264 is trying to acquire lock:
  ffff741e444a0920 (&chip->lock){+.+.}-{3:3}, at: qpnp_tm_get_temp+0xb4/0x1b0 [qcom_spmi_temp_alarm]

  but task is already holding lock:
  ffff741e44341618 (&tz->lock){+.+.}-{3:3}, at: thermal_zone_device_update+0x2c/0x70

  which lock already depends on the new lock.

Fixes: 78c3e2429be8 ("thermal/drivers/qcom: Use generic thermal_zone_get_trip() function")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
index bfaec74f13b2..e2429676d0d2 100644
--- a/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
+++ b/drivers/thermal/qcom/qcom-spmi-temp-alarm.c
@@ -348,7 +348,12 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip)
 	if (stage)
 		chip->temp = qpnp_tm_decode_temp(chip, stage);
 
+	mutex_unlock(&chip->lock);
+
 	crit_temp = qpnp_tm_get_critical_trip_temp(chip);
+
+	mutex_lock(&chip->lock);
+
 	ret = qpnp_tm_update_critical_trip_temp(chip, crit_temp);
 	if (ret < 0)
 		goto out;
-- 
2.37.4

Re: [PATCH 0/4] thermal: fix locking regressions in linux-next

[Rafael J. Wysocki]

On Wed, Dec 14, 2022 at 2:18 PM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> This series fixes some of the fallout after the thermal changes that
> just landed in linux-next.
>
> Lockdep reported a lock inversion in one of the Qualcomm drivers and a
> closer review revealed that the changes had also broken the sysfs
> interface for at least three drivers.
>
> Note that a simple revert of the offending patches was not an option as
> some of the infrastructure that the old implementation relied on has
> also been removed.

I've dropped that material from my linux-next branch and Daniel,
please also remove it from your branch that is pulled by linux-next so
that it doesn't show up in there until 6.2-rc1 is out.

It clearly is not ready for merging in its current form.

It is still present in my bleeding-edge branch, though, so please
apply the patches from Johan on top of it and send a new PR to me, so
I can add it back to my linux-next branch once 6.2-rc1 appears.

It would be good to check the code again too for any more similar fallout.

Thanks!

Re: [PATCH 0/4] thermal: fix locking regressions in linux-next

[Daniel Lezcano]

On 14/12/2022 15:02, Rafael J. Wysocki wrote:
> On Wed, Dec 14, 2022 at 2:18 PM Johan Hovold <johan+linaro@kernel.org> wrote:
>>
>> This series fixes some of the fallout after the thermal changes that
>> just landed in linux-next.
>>
>> Lockdep reported a lock inversion in one of the Qualcomm drivers and a
>> closer review revealed that the changes had also broken the sysfs
>> interface for at least three drivers.
>>
>> Note that a simple revert of the offending patches was not an option as
>> some of the infrastructure that the old implementation relied on has
>> also been removed.
> 
> I've dropped that material from my linux-next branch and Daniel,
> please also remove it from your branch that is pulled by linux-next so
> that it doesn't show up in there until 6.2-rc1 is out.
> 
> It clearly is not ready for merging in its current form.

I rebased a linux-next branch without the generic trip points rework.

It can be inverted with the other changes without conflicts.

I've pushed the branch in case you want to have a look. If you think it 
is acceptable in this form, I can send a tagged PR for 6.2-rc1 again.

> It is still present in my bleeding-edge branch, though, so please
> apply the patches from Johan on top of it and send a new PR to me, so
> I can add it back to my linux-next branch once 6.2-rc1 appears.
> 
> It would be good to check the code again too for any more similar fallout.

I've been through already, the exynos fix is not necessary. But anyway, 
I agree we should keep these changes for the next release, it is better.


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 0/4] thermal: fix locking regressions in linux-next

[Johan Hovold]

On Wed, Dec 14, 2022 at 03:37:43PM +0100, Daniel Lezcano wrote:
> On 14/12/2022 15:02, Rafael J. Wysocki wrote:
> > On Wed, Dec 14, 2022 at 2:18 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> >>
> >> This series fixes some of the fallout after the thermal changes that
> >> just landed in linux-next.
> >>
> >> Lockdep reported a lock inversion in one of the Qualcomm drivers and a
> >> closer review revealed that the changes had also broken the sysfs
> >> interface for at least three drivers.

> > It is still present in my bleeding-edge branch, though, so please
> > apply the patches from Johan on top of it and send a new PR to me, so
> > I can add it back to my linux-next branch once 6.2-rc1 appears.
> > 
> > It would be good to check the code again too for any more similar fallout.
> 
> I've been through already, the exynos fix is not necessary.

Right, I failed to notice that tmu_set_trip_temp() was not actually a
thermal_zone_device_ops callback. So that one can be dropped.

Johan

Re: [PATCH 0/4] thermal: fix locking regressions in linux-next

[Rafael J. Wysocki]

On Wed, Dec 14, 2022 at 3:37 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 14/12/2022 15:02, Rafael J. Wysocki wrote:
> > On Wed, Dec 14, 2022 at 2:18 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> >>
> >> This series fixes some of the fallout after the thermal changes that
> >> just landed in linux-next.
> >>
> >> Lockdep reported a lock inversion in one of the Qualcomm drivers and a
> >> closer review revealed that the changes had also broken the sysfs
> >> interface for at least three drivers.
> >>
> >> Note that a simple revert of the offending patches was not an option as
> >> some of the infrastructure that the old implementation relied on has
> >> also been removed.
> >
> > I've dropped that material from my linux-next branch and Daniel,
> > please also remove it from your branch that is pulled by linux-next so
> > that it doesn't show up in there until 6.2-rc1 is out.
> >
> > It clearly is not ready for merging in its current form.
>
> I rebased a linux-next branch without the generic trip points rework.

Thanks!

> It can be inverted with the other changes without conflicts.

Sounds good.

> I've pushed the branch in case you want to have a look. If you think it
> is acceptable in this form, I can send a tagged PR for 6.2-rc1 again.

It looks OK, so please send a PR.

> > It is still present in my bleeding-edge branch, though, so please
> > apply the patches from Johan on top of it and send a new PR to me, so
> > I can add it back to my linux-next branch once 6.2-rc1 appears.
> >
> > It would be good to check the code again too for any more similar fallout.
>
> I've been through already, the exynos fix is not necessary. But anyway,
> I agree we should keep these changes for the next release, it is better.

Awesome, thanks!

Re: [PATCH 0/4] thermal: fix locking regressions in linux-next

[Daniel Lezcano]

Hi Johan,

thanks for your fixes

On 14/12/2022 14:16, Johan Hovold wrote:
> This series fixes some of the fallout after the thermal changes that
> just landed in linux-next.
> 
> Lockdep reported a lock inversion in one of the Qualcomm drivers and a
> closer review revealed that the changes had also broken the sysfs
> interface for at least three drivers.
> 
> Note that a simple revert of the offending patches was not an option as
> some of the infrastructure that the old implementation relied on has
> also been removed.
> 
> Johan
> 
> 
> Johan Hovold (4):
>    thermal/drivers/qcom: fix set_trip_temp() deadlock
>    thermal/drivers/exynos: fix set_trip_temp() deadlock
>    thermal/drivers/tegra: fix set_trip_temp() deadlock
>    thermal/drivers/qcom: fix lock inversion
> 
>   drivers/thermal/qcom/qcom-spmi-temp-alarm.c | 7 ++++++-
>   drivers/thermal/samsung/exynos_tmu.c        | 2 +-
>   drivers/thermal/tegra/soctherm.c            | 2 +-
>   drivers/thermal/thermal_core.c              | 1 +
>   include/linux/thermal.h                     | 2 ++
>   5 files changed, 11 insertions(+), 3 deletions(-)
> 

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog