| Message ID | 20201208153046.297456-1-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Zhang Rui |
| Headers | show |
| Series | thermal/core: Make 'forced_passive' as obsolete candidate | expand |
On 08/12/2020 16:30, Daniel Lezcano wrote: > The passive file in sysfs forces the usage of a passive trip point set > by the userspace when a broken BIOS does not provide the mitigation > temperature for such thermal zone. The hardware evolved a lot since > 2008 as a good thermal management is no longer an option. > > Linux on the other side also provides now a way to load fixed ACPI > table via the option ACPI_TABLE_UPGRADE, so additionnal trip point > could be added there. > > Set the option obsolete and plan to remove it, so the corresponding > code can be removed from the core code and allow more cleanups the > thermal framework deserves. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- Is there any concern about this change ? > Documentation/ABI/obsolete/sysfs-thermal-passive | 13 +++++++++++++ > drivers/thermal/thermal_sysfs.c | 2 ++ > 2 files changed, 15 insertions(+) > create mode 100644 Documentation/ABI/obsolete/sysfs-thermal-passive > > diff --git a/Documentation/ABI/obsolete/sysfs-thermal-passive b/Documentation/ABI/obsolete/sysfs-thermal-passive > new file mode 100644 > index 000000000000..2510724cc165 > --- /dev/null > +++ b/Documentation/ABI/obsolete/sysfs-thermal-passive > @@ -0,0 +1,13 @@ > +What: /sys/class/thermal/thermal_zone*/passive > +Date: December 2008 > +KernelVersion: 2.6.28 > +Contact: Daniel Lezcano <daniel.lezcano@linaro.org> > +Description: > + > + The passive file in sysfs forces the usage of a passive trip point > + set by the userspace when a broken BIOS does not provide the > + mitigation temperature for such thermal zone. However, the Linux > + kernel evolved a lot since 2008 as well as the hardware and it is > + able to manage correctly the thermal envelope. It does also provide > + a way to load fixed ACPI table via the option ACPI_TABLE_UPGRADE, so > + additionnal trip point could be added there. > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > index 0866e949339b..578099b520b1 100644 > --- a/drivers/thermal/thermal_sysfs.c > +++ b/drivers/thermal/thermal_sysfs.c > @@ -232,6 +232,8 @@ passive_store(struct device *dev, struct device_attribute *attr, > if (state && state < 1000) > return -EINVAL; > > + pr_warn("%s: Consider the 'passive' option obsolete\n", tz->type); > + > if (state && !tz->forced_passive) { > if (!tz->passive_delay) > tz->passive_delay = 1000; >
On Fri, Dec 11, 2020 at 02:17:55PM +0100, Daniel Lezcano wrote: > On 08/12/2020 16:30, Daniel Lezcano wrote: > > The passive file in sysfs forces the usage of a passive trip point set > > by the userspace when a broken BIOS does not provide the mitigation > > temperature for such thermal zone. The hardware evolved a lot since > > 2008 as a good thermal management is no longer an option. > > > > Linux on the other side also provides now a way to load fixed ACPI > > table via the option ACPI_TABLE_UPGRADE, so additionnal trip point > > could be added there. > > > > Set the option obsolete and plan to remove it, so the corresponding > > code can be removed from the core code and allow more cleanups the > > thermal framework deserves. > > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > --- > > Is there any concern about this change ? Yes - what's the reason to do so? The code isn't specific to ACPI, so being able to override ACPI tables doesn't seem to justify it.
On 12/12/2020 04:50, Matthew Garrett wrote: > On Fri, Dec 11, 2020 at 02:17:55PM +0100, Daniel Lezcano wrote: >> On 08/12/2020 16:30, Daniel Lezcano wrote: >>> The passive file in sysfs forces the usage of a passive trip point set >>> by the userspace when a broken BIOS does not provide the mitigation >>> temperature for such thermal zone. The hardware evolved a lot since >>> 2008 as a good thermal management is no longer an option. >>> >>> Linux on the other side also provides now a way to load fixed ACPI >>> table via the option ACPI_TABLE_UPGRADE, so additionnal trip point >>> could be added there. >>> >>> Set the option obsolete and plan to remove it, so the corresponding >>> code can be removed from the core code and allow more cleanups the >>> thermal framework deserves. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> --- >> >> Is there any concern about this change ? > > Yes - what's the reason to do so? I'm cleaning up the thermal core code, so questioning every old ABI. > The code isn't specific to ACPI, > so being able to override ACPI tables doesn't seem to justify it. I agree, the code is no specific to ACPI. What non-ACPI architecture, without device tree or platform data would need the 'passive' option today ?
On Sat, Dec 12, 2020 at 10:11:31AM +0100, Daniel Lezcano wrote: > On 12/12/2020 04:50, Matthew Garrett wrote: > > Yes - what's the reason to do so? > > I'm cleaning up the thermal core code, so questioning every old ABI. > > > The code isn't specific to ACPI, > > so being able to override ACPI tables doesn't seem to justify it. > > I agree, the code is no specific to ACPI. > > What non-ACPI architecture, without device tree or platform data would > need the 'passive' option today ? Anything that provides a trip point that has no active notifications and doesn't provide any information that tells the kernel to poll it.
On 12/12/2020 21:08, Matthew Garrett wrote: > On Sat, Dec 12, 2020 at 10:11:31AM +0100, Daniel Lezcano wrote: >> On 12/12/2020 04:50, Matthew Garrett wrote: >>> Yes - what's the reason to do so? >> >> I'm cleaning up the thermal core code, so questioning every old ABI. >> >>> The code isn't specific to ACPI, >>> so being able to override ACPI tables doesn't seem to justify it. >> >> I agree, the code is no specific to ACPI. >> >> What non-ACPI architecture, without device tree or platform data would >> need the 'passive' option today ? > > 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 commit 0c01ebbfd3caf1 may be responsible of this. If my analysis is correct, this 'feature' is broken since years, more than 8 years to be exact and nobody complained. If I'm right, we can remove this feature directly.
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? > 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).
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 ?
diff --git a/Documentation/ABI/obsolete/sysfs-thermal-passive b/Documentation/ABI/obsolete/sysfs-thermal-passive new file mode 100644 index 000000000000..2510724cc165 --- /dev/null +++ b/Documentation/ABI/obsolete/sysfs-thermal-passive @@ -0,0 +1,13 @@ +What: /sys/class/thermal/thermal_zone*/passive +Date: December 2008 +KernelVersion: 2.6.28 +Contact: Daniel Lezcano <daniel.lezcano@linaro.org> +Description: + + The passive file in sysfs forces the usage of a passive trip point + set by the userspace when a broken BIOS does not provide the + mitigation temperature for such thermal zone. However, the Linux + kernel evolved a lot since 2008 as well as the hardware and it is + able to manage correctly the thermal envelope. It does also provide + a way to load fixed ACPI table via the option ACPI_TABLE_UPGRADE, so + additionnal trip point could be added there. diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c index 0866e949339b..578099b520b1 100644 --- a/drivers/thermal/thermal_sysfs.c +++ b/drivers/thermal/thermal_sysfs.c @@ -232,6 +232,8 @@ passive_store(struct device *dev, struct device_attribute *attr, if (state && state < 1000) return -EINVAL; + pr_warn("%s: Consider the 'passive' option obsolete\n", tz->type); + if (state && !tz->forced_passive) { if (!tz->passive_delay) tz->passive_delay = 1000;
The passive file in sysfs forces the usage of a passive trip point set by the userspace when a broken BIOS does not provide the mitigation temperature for such thermal zone. The hardware evolved a lot since 2008 as a good thermal management is no longer an option. Linux on the other side also provides now a way to load fixed ACPI table via the option ACPI_TABLE_UPGRADE, so additionnal trip point could be added there. Set the option obsolete and plan to remove it, so the corresponding code can be removed from the core code and allow more cleanups the thermal framework deserves. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- Documentation/ABI/obsolete/sysfs-thermal-passive | 13 +++++++++++++ drivers/thermal/thermal_sysfs.c | 2 ++ 2 files changed, 15 insertions(+) create mode 100644 Documentation/ABI/obsolete/sysfs-thermal-passive