mbox series

[v2,0/9] thermal: Writable trip points handling rework

Message ID 6017196.lOV4Wx5bFT@kreacher
Headers
Series thermal: Writable trip points handling rework |

Message

Rafael J. Wysocki Feb. 12, 2024, 6:25 p.m. UTC 
  Hi Everyone,

This is an update of

https://lore.kernel.org/linux-pm/3232442.5fSG56mABF@kreacher/

fixing a few bugs and renaming the new trip point flags introduced by it.

The original description of the patch series is still applicable:

"The purpose of this patch series is to allow thermal zone creators
 to specify which properties (temperature or hysteresis) of which
 trip points can be set from user space via sysfs on a per-trip basis
 instead of passing writable trips masks to the thermal zone registration
 function which is both cumbersome and error prone and it doesn't even
 allow to request different treatment of different trip properties.

 The writable trip masks used today only affect trip temperatures (that is, if
 a trip point is in a writable trips mask, its temperature can be set via
 sysfs) and they only take effect if the CONFIG_THERMAL_WRITABLE_TRIPS kernel
 configuration option is set, which appears to be assumed by at least some
 of the drivers using writable trips masks.  Some other drivers using them
 simply select CONFIG_THERMAL_WRITABLE_TRIPS which pretty much defeats its
 purpose (and imx even sets this option in its defconfig).

 For this reasons, patch [1/9] removes CONFIG_THERMAL_WRITABLE_TRIPS and makes
 the writable trips masks always work.

 Moreover, trip hysteresis, which is not affected either by the writable trips
 masks or by CONFIG_THERMAL_WRITABLE_TRIPS, can only be set via sysfs if the
 .set_trip_hyst() operation is provided by the given thermal zone, but currently
 this thermal zone operation is used by no one, so effectively trip hysteresis
 cannot be set via sysfs at all.  This is not a problem for the majority of
 drivers that want trip temperatures to be set via sysfs, because they also
 don't want trip hysteresis to be changed for any trips (at least as far as I
 can say), but there are use cases in which it is desirable to be able to
 update trip hysteresis as well as trip temperature (for example see
 https://lore.kernel.org/linux-pm/20240106191502.29126-1-quic_manafm@quicinc.com/).
 Those use cases are not addressed here directly, but after this series
 addressing them should be relatively straightforward.

 Namely, patch [2/9] adds flags to struct thermal_trip and defines two of them
 to indicate whether or not setting the temperature or hysteresis of the given
 trip via sysfs is allowed.  If a writable trips mask is passed to
 thermal_zone_device_register_with_trips(), is it is used to set the
 "writable temperature" flag for the trips covered by it and that flag is
 then consulted by the thermal sysfs code.  The "writable hysteresis" trip
 flag is also taken into account by the thermal sysfs code, but it is not set
 automatically in any case.

 Patch [3/9] is based on the observation that the .set_trip_hyst() thermal zone
 operation is never used - it simply drops that callback from struct
 thermal_zone_device_ops and adjusts the code checking its presence.

 Patches [4-8/9] update drivers using writable trips masks to set the new
 "writable temperature" flag directly instead and some of them are simplified
 a bit as a result.  After these patches, all of the callers of
 thermal_zone_device_register_with_trips() pass a zero writable trips mask
 to it, so patch [9/9] drops that mask from the functions argument list and
 adjusts all of its callers accordingly.

 After all of the changes in this series, allowing the hysteresis value to be
 set via sysfs for a given trip is a matter of setting its "writable
 hysteresis" flag (and analogously for trip temperature)."

Thanks!

Comments

Rafael J. Wysocki Feb. 15, 2024, 5:56 p.m. UTC | #1 
On Mon, Feb 12, 2024 at 7:44 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> This is an update of
>
> https://lore.kernel.org/linux-pm/3232442.5fSG56mABF@kreacher/
>
> fixing a few bugs and renaming the new trip point flags introduced by it.
>
> The original description of the patch series is still applicable:
>
> "The purpose of this patch series is to allow thermal zone creators
>  to specify which properties (temperature or hysteresis) of which
>  trip points can be set from user space via sysfs on a per-trip basis
>  instead of passing writable trips masks to the thermal zone registration
>  function which is both cumbersome and error prone and it doesn't even
>  allow to request different treatment of different trip properties.
>
>  The writable trip masks used today only affect trip temperatures (that is, if
>  a trip point is in a writable trips mask, its temperature can be set via
>  sysfs) and they only take effect if the CONFIG_THERMAL_WRITABLE_TRIPS kernel
>  configuration option is set, which appears to be assumed by at least some
>  of the drivers using writable trips masks.  Some other drivers using them
>  simply select CONFIG_THERMAL_WRITABLE_TRIPS which pretty much defeats its
>  purpose (and imx even sets this option in its defconfig).
>
>  For this reasons, patch [1/9] removes CONFIG_THERMAL_WRITABLE_TRIPS and makes
>  the writable trips masks always work.
>
>  Moreover, trip hysteresis, which is not affected either by the writable trips
>  masks or by CONFIG_THERMAL_WRITABLE_TRIPS, can only be set via sysfs if the
>  .set_trip_hyst() operation is provided by the given thermal zone, but currently
>  this thermal zone operation is used by no one, so effectively trip hysteresis
>  cannot be set via sysfs at all.  This is not a problem for the majority of
>  drivers that want trip temperatures to be set via sysfs, because they also
>  don't want trip hysteresis to be changed for any trips (at least as far as I
>  can say), but there are use cases in which it is desirable to be able to
>  update trip hysteresis as well as trip temperature (for example see
>  https://lore.kernel.org/linux-pm/20240106191502.29126-1-quic_manafm@quicinc.com/).
>  Those use cases are not addressed here directly, but after this series
>  addressing them should be relatively straightforward.
>
>  Namely, patch [2/9] adds flags to struct thermal_trip and defines two of them
>  to indicate whether or not setting the temperature or hysteresis of the given
>  trip via sysfs is allowed.  If a writable trips mask is passed to
>  thermal_zone_device_register_with_trips(), is it is used to set the
>  "writable temperature" flag for the trips covered by it and that flag is
>  then consulted by the thermal sysfs code.  The "writable hysteresis" trip
>  flag is also taken into account by the thermal sysfs code, but it is not set
>  automatically in any case.
>
>  Patch [3/9] is based on the observation that the .set_trip_hyst() thermal zone
>  operation is never used - it simply drops that callback from struct
>  thermal_zone_device_ops and adjusts the code checking its presence.
>
>  Patches [4-8/9] update drivers using writable trips masks to set the new
>  "writable temperature" flag directly instead and some of them are simplified
>  a bit as a result.  After these patches, all of the callers of
>  thermal_zone_device_register_with_trips() pass a zero writable trips mask
>  to it, so patch [9/9] drops that mask from the functions argument list and
>  adjusts all of its callers accordingly.
>
>  After all of the changes in this series, allowing the hysteresis value to be
>  set via sysfs for a given trip is a matter of setting its "writable
>  hysteresis" flag (and analogously for trip temperature)."

By the lack of comments I gather that this series is not controversial.

It unlocks further development and it should be run through linux-next
for a couple of weeks before the merge window, so reviews are welcome.

Thanks!
Daniel Lezcano Feb. 22, 2024, 3:32 p.m. UTC | #2 
On 12/02/2024 19:39, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is now possible to flag trip points with THERMAL_TRIP_FLAG_RW_TEMP
> to allow their temperature to be set from user space via sysfs instead
> of using a nonzero writable trips mask during thermal zone registration,
> so make the imx thermal code do that.
> 
> No intentional functional impact.
> 
> Note that this change is requisite for dropping the mask argument from
> thermal_zone_device_register_with_trips() going forward.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---


Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>