Re: Regression after recent changes to drivers/thermal/thermal_of.c

[Daniel Lezcano <daniel.lezcano@linexp.org>]

On 26/10/2022 19:06, Rafael J. Wysocki wrote:
> On Wed, Oct 26, 2022 at 5:47 PM Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Tue, Oct 25, 2022 at 4:13 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>
>>> Hi Folks,
>>>
>>> I have this in my dmesg in v6.1-rc1:
>>>
>>> [    3.879229] ab8500-fg ab8500-fg.0: line impedance: 36000 uOhm
>>> [    3.892793] power_supply ab8500_usb: Samsung SDI EB-L1M7FLU battery 1500 mAh
>>> [    3.901663] thermal_sys: Failed to find 'trips' node
>>> [    3.906635] thermal_sys: Failed to find trip points for thermistor id=0
>>> [    3.913427] ntc-thermistor thermistor: unable to register as hwmon device.
>>> [    3.920350] ntc-thermistor: probe of thermistor failed with error -22
>>>
>>> The device tree looks like this
>>> (arch/arm/boot/dts/ste-ux500-samsung-golden.dts):
>>>
>>>          thermal-zones {
>>>                  battery-thermal {
>>>                          /* This zone will be polled by the battery
>>> temperature code */
>>>                          polling-delay = <0>;
>>>                          polling-delay-passive = <0>;
>>>                          thermal-sensors = <&bat_therm>;
>>>                  };
>>>          };
>>>
>>> This is a thermal zone without trip points, which it seems like the new
>>> code does not allow, also the bindings were patched to not allow this,
>>> in commit 8c596324232d22e19f8df59ba03410b9b5b0f3d7
>>> "dt-bindings: thermal: Fix missing required property"
>>> but this broke my systems. The requirement to have trip points also
>>> broke my device trees.
>>>
>>> The reason why I have this is that the thermal zone is not managed
>>> by the OF thermal core, but by the battery charging algorithm which
>>> just retrieves the thermal zone and use it to read the temperature, see
>>> commit 2b0e7ac0841b3906aeecf432567b02af683a596c
>>> "power: supply: ab8500: Integrate thermal zone".
>>>
>>> The code is using
>>> thermal_zone_get_zone_by_name()
>>> thermal_zone_get_temp()
>>> and applying its own policy on the thermal zone in order to not
>>> dulicate code.
>>>
>>> I understand from the code and changes to the bindings that the
>>> authors assume that no zones without trips exist but... well they
>>> exist.
>>>
>>> I understand that the bindings always said that trips are required
>>> but ... thermal zones without trip points make a bit of sense.
>>> It's just a zone without a policy. It can be observed even if it can't
>>> be acted on.
>>>
>>> How do you want to solve this? Can we make trips non-compulsory
>>> again or shall I add dummy trip points to the device trees?
>>>
>>> This:
>>>
>>> diff --git a/arch/arm/boot/dts/ste-ux500-samsung-golden.dts
>>> b/arch/arm/boot/dts/ste-ux500-samsung-golden.dts
>>> index b0dce91aff4b..d00e9e6ebbf7 100644
>>> --- a/arch/arm/boot/dts/ste-ux500-samsung-golden.dts
>>> +++ b/arch/arm/boot/dts/ste-ux500-samsung-golden.dts
>>> @@ -35,6 +35,15 @@ battery-thermal {
>>>                          polling-delay = <0>;
>>>                          polling-delay-passive = <0>;
>>>                          thermal-sensors = <&bat_therm>;
>>> +
>>> +                       trips {
>>> +                               /* Unused trip point to please the framework */
>>> +                               dummy {
>>> +                                       temperature = <700000>;
>>> +                                       hysteresis = <2000>;
>>> +                                       type = "passive";
>>> +                               };
>>> +                       };
>>
>> That's ugly and requiring a DT update breaks the ABI. So the
>> requirement for 'trips' should be reverted. (Well the schema should, I
>> imagine the code change is not just a revert.)
> 
> I agree and the code change is not just a revert AFAICS.
> 
> This is Daniel's work, so I'm still hoping that he'll chime in
> shortly,

Yep, I'm in sick leave ATM, I broke my arm (without wordplay).

I took sometime to read the code, so from my POV we should keep the 
required property patch because the DT was defined that as required 
property. The conversion to yaml obviously spotted the DT not conforming 
with the bindings.

 From an implementation POV, that was not spotted initially because of 
the old OF code design IMO (but I'm not sure).

We can continue registering the thermal with no trip points but then 
still raise a message.

However, a thermal zone without trip point does not really make sense 
IMO. If I'm correct, the ACPI at least defines the critical temperature 
as a non optional object.

Did you consider using hwmon instead of a thermal zone ?

Below a patch (not tested): one hand writing is painful

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index ff4d12ef51bc..51f7dc5b8d18 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -481,8 +481,8 @@ struct thermal_zone_device 
*thermal_of_zone_register(struct device_node *sensor,
  	struct thermal_zone_params *tzp;
  	struct thermal_zone_device_ops *of_ops;
  	struct device_node *np;
-	int delay, pdelay;
-	int ntrips, mask;
+	int delay = 0, pdelay = 0;
+	int ntrips = 0, mask = 0;
  	int ret;

  	of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
@@ -500,8 +500,8 @@ struct thermal_zone_device 
*thermal_of_zone_register(struct device_node *sensor,
  	trips = thermal_of_trips_init(np, &ntrips);
  	if (IS_ERR(trips)) {
  		pr_err("Failed to find trip points for %pOFn id=%d\n", sensor, id);
-		ret = PTR_ERR(trips);
-		goto out_kfree_of_ops;
+		trips = NULL;
+		goto out_register;
  	}

  	ret = thermal_of_monitor_init(np, &delay, &pdelay);
@@ -522,6 +522,7 @@ struct thermal_zone_device 
*thermal_of_zone_register(struct device_node *sensor,

  	mask = GENMASK_ULL((ntrips) - 1, 0);

+out_register:
  	tz = thermal_zone_device_register_with_trips(np->name, trips, ntrips,
  						     mask, data, of_ops, tzp,
  						     pdelay, delay);