Re: [PATCH] thermal/core: Make 'forced_passive' as obsolete candidate

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Matthew Garrett <mjg59@codon.org.uk>
Cc: rui.zhang@intel.com, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org, amitk@kernel.org,
	Matthew Garrett <mjg59@srcf.ucam.org>
Subject: Re: [PATCH] thermal/core: Make 'forced_passive' as obsolete candidate
Date: Sun, 13 Dec 2020 12:02:13 +0100	[thread overview]
Message-ID: <a2e3c6ac-b87d-9dec-bf1f-60814739b2fe@linaro.org> (raw)
In-Reply-To: <20201213011105.GA21385@codon.org.uk>

On 13/12/2020 02:11, Matthew Garrett wrote:
> On Sun, Dec 13, 2020 at 12:39:26AM +0100, Daniel Lezcano wrote:
>> On 12/12/2020 21:08, Matthew Garrett wrote:
>>> Anything that provides a trip point that has no active notifications and
>>> doesn't provide any information that tells the kernel to poll it.
>>
>> I'm not able to create a setup as you describe working correctly with
>> the forced passive trip point.
>>
>> The forced passive trip can not be detected as there is no comparison
>> with the defined temperature in the thermal_zone_device_update() function.
> 
> The logic seems to be in the step_wise thermal governor. I'm not sure
> why it would be used in thermal_zone_device_update() - the entire point
> is that we don't get updates from the device?

The thermal_zone_device_update() loops the trip points:

        for (count = 0; count < tz->trips; count++)
                handle_thermal_trip(tz, count);

As the 'forced_passive' is not in this loop (because it was moved in the
step_wise governor), the temperature crossing is never detected and the
'forced_passive' logic in the governor is never called.

That is something I realized when answering to your comment.

>> If my analysis is correct, this 'feature' is broken since years, more
>> than 8 years to be exact and nobody complained.
> 
> I've no problem with it being removed if there are no users, but in that
> case the justification should be rewritten - ACPI table updates aren't a
> complete replacement for the functionality offered (and can't be used if
> the lockdown LSM is being used in any case).

Yes, I understand your point. Given it is not working since years, I
think we can just drop the feature and change the reason of the removal
in the log, instead of ACPI table updates, just say it is no longer used.

Does it sound fine ?

-- 
<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