From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
In order to allow thermal zone creators to specify the writability of
trip point temperature and hysteresis on a per-trip basis, add a flags
field to struct thermal_trip and define flags to represent the desired
trip properties.
Also make thermal_zone_device_register_with_trips() set the
THERMAL_TRIP_FLAG_RW_TEMP flag for all trips covered by the writable
trips mask passed to it and modify the thermal sysfs code to look at
the trip flags instead of using the writable trips mask directly or
checking the presence of the .set_trip_hyst() zone callback.
Additionally, make trip_point_temp_store() and trip_point_hyst_store()
fail with an error code if the trip passed to one of them has
THERMAL_TRIP_FLAG_RW_TEMP or THERMAL_TRIP_FLAG_RW_HYST,
respectively, clear in its flags.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
v1 -> v2:
* Rename trip flags (Stanislaw).
---
drivers/thermal/thermal_core.c | 12 +++++++++++-
drivers/thermal/thermal_core.h | 2 +-
drivers/thermal/thermal_sysfs.c | 28 +++++++++++++++++++---------
include/linux/thermal.h | 7 +++++++
4 files changed, 38 insertions(+), 11 deletions(-)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -64,15 +64,23 @@ enum thermal_notify_event {
* @threshold: trip crossing notification threshold miliCelsius
* @type: trip point type
* @priv: pointer to driver data associated with this trip
+ * @flags: flags representing binary properties of the trip
*/
struct thermal_trip {
int temperature;
int hysteresis;
int threshold;
enum thermal_trip_type type;
+ u8 flags;
void *priv;
};
+#define THERMAL_TRIP_FLAG_RW_TEMP BIT(0)
+#define THERMAL_TRIP_FLAG_RW_HYST BIT(1)
+
+#define THERMAL_TRIP_FLAG_MASK_RW (THERMAL_TRIP_FLAG_RW_TEMP | \
+ THERMAL_TRIP_FLAG_RW_HYST)
+
struct thermal_zone_device_ops {
int (*bind) (struct thermal_zone_device *,
struct thermal_cooling_device *);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1356,13 +1356,23 @@ thermal_zone_device_register_with_trips(
tz->devdata = devdata;
tz->trips = trips;
tz->num_trips = num_trips;
+ if (num_trips > 0) {
+ struct thermal_trip *trip;
+
+ for_each_trip(tz, trip) {
+ if (mask & 1)
+ trip->flags |= THERMAL_TRIP_FLAG_RW_TEMP;
+
+ mask >>= 1;
+ }
+ }
thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
/* sys I/F */
/* Add nodes that are always present via .groups */
- result = thermal_zone_create_device_groups(tz, mask);
+ result = thermal_zone_create_device_groups(tz);
if (result)
goto remove_id;
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -131,7 +131,7 @@ void thermal_zone_trip_updated(struct th
int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
/* sysfs I/F */
-int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
+int thermal_zone_create_device_groups(struct thermal_zone_device *tz);
void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -122,6 +122,11 @@ trip_point_temp_store(struct device *dev
trip = &tz->trips[trip_id];
+ if (!(trip->flags & THERMAL_TRIP_FLAG_RW_TEMP)) {
+ ret = -EPERM;
+ goto unlock;
+ }
+
if (temp != trip->temperature) {
if (tz->ops->set_trip_temp) {
ret = tz->ops->set_trip_temp(tz, trip_id, temp);
@@ -173,6 +178,11 @@ trip_point_hyst_store(struct device *dev
trip = &tz->trips[trip_id];
+ if (!(trip->flags & THERMAL_TRIP_FLAG_RW_HYST)) {
+ ret = -EPERM;
+ goto unlock;
+ }
+
if (hyst != trip->hysteresis) {
if (tz->ops->set_trip_hyst) {
ret = tz->ops->set_trip_hyst(tz, trip_id, hyst);
@@ -392,17 +402,16 @@ static const struct attribute_group *the
/**
* create_trip_attrs() - create attributes for trip points
* @tz: the thermal zone device
- * @mask: Writeable trip point bitmap.
*
* helper function to instantiate sysfs entries for every trip
* point and its properties of a struct thermal_zone_device.
*
* Return: 0 on success, the proper error value otherwise.
*/
-static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
+static int create_trip_attrs(struct thermal_zone_device *tz)
{
+ const struct thermal_trip *trip;
struct attribute **attrs;
- int indx;
/* This function works only for zones with at least one trip */
if (tz->num_trips <= 0)
@@ -437,7 +446,9 @@ static int create_trip_attrs(struct ther
return -ENOMEM;
}
- for (indx = 0; indx < tz->num_trips; indx++) {
+ for_each_trip(tz, trip) {
+ int indx = thermal_zone_trip_id(tz, trip);
+
/* create trip type attribute */
snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
"trip_point_%d_type", indx);
@@ -458,7 +469,7 @@ static int create_trip_attrs(struct ther
tz->trip_temp_attrs[indx].name;
tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
- if (mask & (1 << indx)) {
+ if (trip->flags & THERMAL_TRIP_FLAG_RW_TEMP) {
tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
tz->trip_temp_attrs[indx].attr.store =
trip_point_temp_store;
@@ -473,7 +484,7 @@ static int create_trip_attrs(struct ther
tz->trip_hyst_attrs[indx].name;
tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
- if (tz->ops->set_trip_hyst) {
+ if (trip->flags & THERMAL_TRIP_FLAG_RW_HYST) {
tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
tz->trip_hyst_attrs[indx].attr.store =
trip_point_hyst_store;
@@ -505,8 +516,7 @@ static void destroy_trip_attrs(struct th
kfree(tz->trips_attribute_group.attrs);
}
-int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
- int mask)
+int thermal_zone_create_device_groups(struct thermal_zone_device *tz)
{
const struct attribute_group **groups;
int i, size, result;
@@ -522,7 +532,7 @@ int thermal_zone_create_device_groups(st
groups[i] = thermal_zone_attribute_groups[i];
if (tz->num_trips) {
- result = create_trip_attrs(tz, mask);
+ result = create_trip_attrs(tz);
if (result) {
kfree(groups);On 12/02/2024 19:31, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In order to allow thermal zone creators to specify the writability of
> trip point temperature and hysteresis on a per-trip basis, add a flags
> field to struct thermal_trip and define flags to represent the desired
> trip properties.
>
> Also make thermal_zone_device_register_with_trips() set the
> THERMAL_TRIP_FLAG_RW_TEMP flag for all trips covered by the writable
> trips mask passed to it and modify the thermal sysfs code to look at
> the trip flags instead of using the writable trips mask directly or
> checking the presence of the .set_trip_hyst() zone callback.
>
> Additionally, make trip_point_temp_store() and trip_point_hyst_store()
> fail with an error code if the trip passed to one of them has
> THERMAL_TRIP_FLAG_RW_TEMP or THERMAL_TRIP_FLAG_RW_HYST,
> respectively, clear in its flags.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> v1 -> v2:
> * Rename trip flags (Stanislaw).
>
> ---
> drivers/thermal/thermal_core.c | 12 +++++++++++-
> drivers/thermal/thermal_core.h | 2 +-
> drivers/thermal/thermal_sysfs.c | 28 +++++++++++++++++++---------
> include/linux/thermal.h | 7 +++++++
> 4 files changed, 38 insertions(+), 11 deletions(-)
>
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -64,15 +64,23 @@ enum thermal_notify_event {
> * @threshold: trip crossing notification threshold miliCelsius
> * @type: trip point type
> * @priv: pointer to driver data associated with this trip
> + * @flags: flags representing binary properties of the trip
> */
> struct thermal_trip {
> int temperature;
> int hysteresis;
> int threshold;
> enum thermal_trip_type type;
> + u8 flags;
> void *priv;
> };
>
> +#define THERMAL_TRIP_FLAG_RW_TEMP BIT(0)
> +#define THERMAL_TRIP_FLAG_RW_HYST BIT(1)
> +
> +#define THERMAL_TRIP_FLAG_MASK_RW (THERMAL_TRIP_FLAG_RW_TEMP | \
> + THERMAL_TRIP_FLAG_RW_HYST)
What about THERMAL_TRIP_FLAG_RW instead ?
> struct thermal_zone_device_ops {
> int (*bind) (struct thermal_zone_device *,
> struct thermal_cooling_device *);
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1356,13 +1356,23 @@ thermal_zone_device_register_with_trips(
> tz->devdata = devdata;
> tz->trips = trips;
> tz->num_trips = num_trips;
> + if (num_trips > 0) {
Is this check really necessary? for_each_trip() should exit immediately
if there is no trip points.
> + struct thermal_trip *trip;
> +
> + for_each_trip(tz, trip) {
> + if (mask & 1)
> + trip->flags |= THERMAL_TRIP_FLAG_RW_TEMP;
> +
> + mask >>= 1;
> + }
> + }
>
> thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
> thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
>
> /* sys I/F */
> /* Add nodes that are always present via .groups */
> - result = thermal_zone_create_device_groups(tz, mask);
> + result = thermal_zone_create_device_groups(tz);
> if (result)
> goto remove_id;
>
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -131,7 +131,7 @@ void thermal_zone_trip_updated(struct th
> int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>
> /* sysfs I/F */
> -int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
> +int thermal_zone_create_device_groups(struct thermal_zone_device *tz);
> void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
> void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
> void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -122,6 +122,11 @@ trip_point_temp_store(struct device *dev
>
> trip = &tz->trips[trip_id];
>
> + if (!(trip->flags & THERMAL_TRIP_FLAG_RW_TEMP)) {
> + ret = -EPERM;
> + goto unlock;
> + }
Does it really happen?
If the sysfs file is created with the right permission regarding the
trip->flags then this condition can never be true.
> if (temp != trip->temperature) {
> if (tz->ops->set_trip_temp) {
> ret = tz->ops->set_trip_temp(tz, trip_id, temp);
> @@ -173,6 +178,11 @@ trip_point_hyst_store(struct device *dev
>
> trip = &tz->trips[trip_id];
>
> + if (!(trip->flags & THERMAL_TRIP_FLAG_RW_HYST)) {
> + ret = -EPERM;
> + goto unlock;
> + }
Ditto
> if (hyst != trip->hysteresis) {
> if (tz->ops->set_trip_hyst) {
> ret = tz->ops->set_trip_hyst(tz, trip_id, hyst);
> @@ -392,17 +402,16 @@ static const struct attribute_group *the
> /**
> * create_trip_attrs() - create attributes for trip points
> * @tz: the thermal zone device
> - * @mask: Writeable trip point bitmap.
> *
> * helper function to instantiate sysfs entries for every trip
> * point and its properties of a struct thermal_zone_device.
> *
> * Return: 0 on success, the proper error value otherwise.
> */
> -static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
> +static int create_trip_attrs(struct thermal_zone_device *tz)
> {
> + const struct thermal_trip *trip;
> struct attribute **attrs;
> - int indx;
>
> /* This function works only for zones with at least one trip */
> if (tz->num_trips <= 0)
> @@ -437,7 +446,9 @@ static int create_trip_attrs(struct ther
> return -ENOMEM;
> }
>
> - for (indx = 0; indx < tz->num_trips; indx++) {
> + for_each_trip(tz, trip) {
> + int indx = thermal_zone_trip_id(tz, trip);
> +
> /* create trip type attribute */
> snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
> "trip_point_%d_type", indx);
> @@ -458,7 +469,7 @@ static int create_trip_attrs(struct ther
> tz->trip_temp_attrs[indx].name;
> tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
> tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
> - if (mask & (1 << indx)) {
> + if (trip->flags & THERMAL_TRIP_FLAG_RW_TEMP) {
> tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
> tz->trip_temp_attrs[indx].attr.store =
> trip_point_temp_store;
> @@ -473,7 +484,7 @@ static int create_trip_attrs(struct ther
> tz->trip_hyst_attrs[indx].name;
> tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
> tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
> - if (tz->ops->set_trip_hyst) {
> + if (trip->flags & THERMAL_TRIP_FLAG_RW_HYST) {
> tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
> tz->trip_hyst_attrs[indx].attr.store =
> trip_point_hyst_store;
> @@ -505,8 +516,7 @@ static void destroy_trip_attrs(struct th
> kfree(tz->trips_attribute_group.attrs);
> }
>
> -int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
> - int mask)
> +int thermal_zone_create_device_groups(struct thermal_zone_device *tz)
> {
> const struct attribute_group **groups;
> int i, size, result;
> @@ -522,7 +532,7 @@ int thermal_zone_create_device_groups(st
> groups[i] = thermal_zone_attribute_groups[i];
>
> if (tz->num_trips) {
> - result = create_trip_attrs(tz, mask);
> + result = create_trip_attrs(tz);
> if (result) {
> kfree(groups);
>
>
>
>
--
<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
On Thu, Feb 22, 2024 at 3:36 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 12/02/2024 19:31, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In order to allow thermal zone creators to specify the writability of
> > trip point temperature and hysteresis on a per-trip basis, add a flags
> > field to struct thermal_trip and define flags to represent the desired
> > trip properties.
> >
> > Also make thermal_zone_device_register_with_trips() set the
> > THERMAL_TRIP_FLAG_RW_TEMP flag for all trips covered by the writable
> > trips mask passed to it and modify the thermal sysfs code to look at
> > the trip flags instead of using the writable trips mask directly or
> > checking the presence of the .set_trip_hyst() zone callback.
> >
> > Additionally, make trip_point_temp_store() and trip_point_hyst_store()
> > fail with an error code if the trip passed to one of them has
> > THERMAL_TRIP_FLAG_RW_TEMP or THERMAL_TRIP_FLAG_RW_HYST,
> > respectively, clear in its flags.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > v1 -> v2:
> > * Rename trip flags (Stanislaw).
> >
> > ---
> > drivers/thermal/thermal_core.c | 12 +++++++++++-
> > drivers/thermal/thermal_core.h | 2 +-
> > drivers/thermal/thermal_sysfs.c | 28 +++++++++++++++++++---------
> > include/linux/thermal.h | 7 +++++++
> > 4 files changed, 38 insertions(+), 11 deletions(-)
> >
> > Index: linux-pm/include/linux/thermal.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/thermal.h
> > +++ linux-pm/include/linux/thermal.h
> > @@ -64,15 +64,23 @@ enum thermal_notify_event {
> > * @threshold: trip crossing notification threshold miliCelsius
> > * @type: trip point type
> > * @priv: pointer to driver data associated with this trip
> > + * @flags: flags representing binary properties of the trip
> > */
> > struct thermal_trip {
> > int temperature;
> > int hysteresis;
> > int threshold;
> > enum thermal_trip_type type;
> > + u8 flags;
> > void *priv;
> > };
> >
> > +#define THERMAL_TRIP_FLAG_RW_TEMP BIT(0)
> > +#define THERMAL_TRIP_FLAG_RW_HYST BIT(1)
> > +
> > +#define THERMAL_TRIP_FLAG_MASK_RW (THERMAL_TRIP_FLAG_RW_TEMP | \
> > + THERMAL_TRIP_FLAG_RW_HYST)
>
> What about THERMAL_TRIP_FLAG_RW instead ?
Fine with me.
> > struct thermal_zone_device_ops {
> > int (*bind) (struct thermal_zone_device *,
> > struct thermal_cooling_device *);
> > Index: linux-pm/drivers/thermal/thermal_core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > +++ linux-pm/drivers/thermal/thermal_core.c
> > @@ -1356,13 +1356,23 @@ thermal_zone_device_register_with_trips(
> > tz->devdata = devdata;
> > tz->trips = trips;
> > tz->num_trips = num_trips;
> > + if (num_trips > 0) {
>
> Is this check really necessary?
No, it isn't.
> for_each_trip() should exit immediately if there is no trip points.
Right.
> > + struct thermal_trip *trip;
> > +
> > + for_each_trip(tz, trip) {
> > + if (mask & 1)
> > + trip->flags |= THERMAL_TRIP_FLAG_RW_TEMP;
> > +
> > + mask >>= 1;
> > + }
> > + }
> >
> > thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
> > thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
> >
> > /* sys I/F */
> > /* Add nodes that are always present via .groups */
> > - result = thermal_zone_create_device_groups(tz, mask);
> > + result = thermal_zone_create_device_groups(tz);
> > if (result)
> > goto remove_id;
> >
> > Index: linux-pm/drivers/thermal/thermal_core.h
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.h
> > +++ linux-pm/drivers/thermal/thermal_core.h
> > @@ -131,7 +131,7 @@ void thermal_zone_trip_updated(struct th
> > int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
> >
> > /* sysfs I/F */
> > -int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
> > +int thermal_zone_create_device_groups(struct thermal_zone_device *tz);
> > void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
> > void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
> > void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
> > Index: linux-pm/drivers/thermal/thermal_sysfs.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> > +++ linux-pm/drivers/thermal/thermal_sysfs.c
> > @@ -122,6 +122,11 @@ trip_point_temp_store(struct device *dev
> >
> > trip = &tz->trips[trip_id];
> >
> > + if (!(trip->flags & THERMAL_TRIP_FLAG_RW_TEMP)) {
> > + ret = -EPERM;
> > + goto unlock;
> > + }
>
> Does it really happen?
>
> If the sysfs file is created with the right permission regarding the
> trip->flags then this condition can never be true.
But the permissions can be changed after the file has been created, can't they?
On Thu, Feb 22, 2024 at 4:51 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 22, 2024 at 3:36 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 12/02/2024 19:31, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > In order to allow thermal zone creators to specify the writability of
> > > trip point temperature and hysteresis on a per-trip basis, add a flags
> > > field to struct thermal_trip and define flags to represent the desired
> > > trip properties.
> > >
> > > Also make thermal_zone_device_register_with_trips() set the
> > > THERMAL_TRIP_FLAG_RW_TEMP flag for all trips covered by the writable
> > > trips mask passed to it and modify the thermal sysfs code to look at
> > > the trip flags instead of using the writable trips mask directly or
> > > checking the presence of the .set_trip_hyst() zone callback.
> > >
> > > Additionally, make trip_point_temp_store() and trip_point_hyst_store()
> > > fail with an error code if the trip passed to one of them has
> > > THERMAL_TRIP_FLAG_RW_TEMP or THERMAL_TRIP_FLAG_RW_HYST,
> > > respectively, clear in its flags.
> > >
> > > No intentional functional impact.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >
> > > v1 -> v2:
> > > * Rename trip flags (Stanislaw).
> > >
> > > ---
> > > drivers/thermal/thermal_core.c | 12 +++++++++++-
> > > drivers/thermal/thermal_core.h | 2 +-
> > > drivers/thermal/thermal_sysfs.c | 28 +++++++++++++++++++---------
> > > include/linux/thermal.h | 7 +++++++
> > > 4 files changed, 38 insertions(+), 11 deletions(-)
> > >
> > > Index: linux-pm/include/linux/thermal.h
> > > ===================================================================
> > > --- linux-pm.orig/include/linux/thermal.h
> > > +++ linux-pm/include/linux/thermal.h
> > > @@ -64,15 +64,23 @@ enum thermal_notify_event {
> > > * @threshold: trip crossing notification threshold miliCelsius
> > > * @type: trip point type
> > > * @priv: pointer to driver data associated with this trip
> > > + * @flags: flags representing binary properties of the trip
> > > */
> > > struct thermal_trip {
> > > int temperature;
> > > int hysteresis;
> > > int threshold;
> > > enum thermal_trip_type type;
> > > + u8 flags;
> > > void *priv;
> > > };
> > >
> > > +#define THERMAL_TRIP_FLAG_RW_TEMP BIT(0)
> > > +#define THERMAL_TRIP_FLAG_RW_HYST BIT(1)
> > > +
> > > +#define THERMAL_TRIP_FLAG_MASK_RW (THERMAL_TRIP_FLAG_RW_TEMP | \
> > > + THERMAL_TRIP_FLAG_RW_HYST)
> >
> > What about THERMAL_TRIP_FLAG_RW instead ?
>
> Fine with me.
>
> > > struct thermal_zone_device_ops {
> > > int (*bind) (struct thermal_zone_device *,
> > > struct thermal_cooling_device *);
> > > Index: linux-pm/drivers/thermal/thermal_core.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > > +++ linux-pm/drivers/thermal/thermal_core.c
> > > @@ -1356,13 +1356,23 @@ thermal_zone_device_register_with_trips(
> > > tz->devdata = devdata;
> > > tz->trips = trips;
> > > tz->num_trips = num_trips;
> > > + if (num_trips > 0) {
> >
> > Is this check really necessary?
>
> No, it isn't.
>
> > for_each_trip() should exit immediately if there is no trip points.
>
> Right.
>
> > > + struct thermal_trip *trip;
> > > +
> > > + for_each_trip(tz, trip) {
> > > + if (mask & 1)
> > > + trip->flags |= THERMAL_TRIP_FLAG_RW_TEMP;
> > > +
> > > + mask >>= 1;
> > > + }
> > > + }
> > >
> > > thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
> > > thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
> > >
> > > /* sys I/F */
> > > /* Add nodes that are always present via .groups */
> > > - result = thermal_zone_create_device_groups(tz, mask);
> > > + result = thermal_zone_create_device_groups(tz);
> > > if (result)
> > > goto remove_id;
> > >
> > > Index: linux-pm/drivers/thermal/thermal_core.h
> > > ===================================================================
> > > --- linux-pm.orig/drivers/thermal/thermal_core.h
> > > +++ linux-pm/drivers/thermal/thermal_core.h
> > > @@ -131,7 +131,7 @@ void thermal_zone_trip_updated(struct th
> > > int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
> > >
> > > /* sysfs I/F */
> > > -int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
> > > +int thermal_zone_create_device_groups(struct thermal_zone_device *tz);
> > > void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
> > > void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
> > > void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
> > > Index: linux-pm/drivers/thermal/thermal_sysfs.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> > > +++ linux-pm/drivers/thermal/thermal_sysfs.c
> > > @@ -122,6 +122,11 @@ trip_point_temp_store(struct device *dev
> > >
> > > trip = &tz->trips[trip_id];
> > >
> > > + if (!(trip->flags & THERMAL_TRIP_FLAG_RW_TEMP)) {
> > > + ret = -EPERM;
> > > + goto unlock;
> > > + }
> >
> > Does it really happen?
> >
> > If the sysfs file is created with the right permission regarding the
> > trip->flags then this condition can never be true.
>
> But the permissions can be changed after the file has been created, can't they?
Even so, the .store() callback will not be set then anyway.
I'll send an update of this patch.
On 22/02/2024 15:36, Daniel Lezcano wrote:
> On 12/02/2024 19:31, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> In order to allow thermal zone creators to specify the writability of
>> trip point temperature and hysteresis on a per-trip basis, add a flags
>> field to struct thermal_trip and define flags to represent the desired
>> trip properties.
>>
>> Also make thermal_zone_device_register_with_trips() set the
>> THERMAL_TRIP_FLAG_RW_TEMP flag for all trips covered by the writable
>> trips mask passed to it and modify the thermal sysfs code to look at
>> the trip flags instead of using the writable trips mask directly or
>> checking the presence of the .set_trip_hyst() zone callback.
>>
>> Additionally, make trip_point_temp_store() and trip_point_hyst_store()
>> fail with an error code if the trip passed to one of them has
>> THERMAL_TRIP_FLAG_RW_TEMP or THERMAL_TRIP_FLAG_RW_HYST,
>> respectively, clear in its flags.
>>
>> No intentional functional impact.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>> ===================================================================
>> --- linux-pm.orig/drivers/thermal/thermal_core.c
>> +++ linux-pm/drivers/thermal/thermal_core.c
>> @@ -1356,13 +1356,23 @@ thermal_zone_device_register_with_trips(
>> tz->devdata = devdata;
>> tz->trips = trips;
>> tz->num_trips = num_trips;
[ ... ]
>> + if (num_trips > 0) {
>
> Is this check really necessary? for_each_trip() should exit immediately
> if there is no trip points.
Given that is removed in patch 9/9, please ignore this comment
>> + struct thermal_trip *trip;
>> +
>> + for_each_trip(tz, trip) {
>> + if (mask & 1)
>> + trip->flags |= THERMAL_TRIP_FLAG_RW_TEMP;
>> +
>> + mask >>= 1;
>> + }
>> + }
[ ... ]
--
<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
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
In order to allow thermal zone creators to specify the writability of
trip point temperature and hysteresis on a per-trip basis, add a flags
field to struct thermal_trip and define flags to represent the desired
trip properties.
Also make thermal_zone_device_register_with_trips() set the
THERMAL_TRIP_FLAG_RW_TEMP flag for all trips covered by the writable
trips mask passed to it and modify the thermal sysfs code to look at
the trip flags instead of using the writable trips mask directly or
checking the presence of the .set_trip_hyst() zone callback.
Additionally, make trip_point_temp_store() and trip_point_hyst_store()
fail with an error code if the trip passed to one of them has
THERMAL_TRIP_FLAG_RW_TEMP or THERMAL_TRIP_FLAG_RW_HYST,
respectively, clear in its flags.
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
v2 -> v2.1:
* Don't add redundant checks in 3 places (Daniel).
* Define THERMAL_TRIP_FLAG_RW as the combination of the _RW_ trip flags (Daniel).
v1 -> v2:
* Rename trip flags (Stanislaw).
---
drivers/thermal/thermal_core.c | 9 ++++++++-
drivers/thermal/thermal_core.h | 2 +-
drivers/thermal/thermal_sysfs.c | 18 +++++++++---------
include/linux/thermal.h | 8 ++++++++
4 files changed, 26 insertions(+), 11 deletions(-)
Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -64,15 +64,23 @@ enum thermal_notify_event {
* @threshold: trip crossing notification threshold miliCelsius
* @type: trip point type
* @priv: pointer to driver data associated with this trip
+ * @flags: flags representing binary properties of the trip
*/
struct thermal_trip {
int temperature;
int hysteresis;
int threshold;
enum thermal_trip_type type;
+ u8 flags;
void *priv;
};
+#define THERMAL_TRIP_FLAG_RW_TEMP BIT(0)
+#define THERMAL_TRIP_FLAG_RW_HYST BIT(1)
+
+#define THERMAL_TRIP_FLAG_RW (THERMAL_TRIP_FLAG_RW_TEMP | \
+ THERMAL_TRIP_FLAG_RW_HYST)
+
struct thermal_zone_device_ops {
int (*bind) (struct thermal_zone_device *,
struct thermal_cooling_device *);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1278,6 +1278,7 @@ thermal_zone_device_register_with_trips(
int passive_delay, int polling_delay)
{
struct thermal_zone_device *tz;
+ struct thermal_trip *trip;
int id;
int result;
struct thermal_governor *governor;
@@ -1356,13 +1357,19 @@ thermal_zone_device_register_with_trips(
tz->devdata = devdata;
memcpy(tz->trips, trips, num_trips * sizeof(*trips));
tz->num_trips = num_trips;
+ for_each_trip(tz, trip) {
+ if (mask & 1)
+ trip->flags |= THERMAL_TRIP_FLAG_RW_TEMP;
+
+ mask >>= 1;
+ }
thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
/* sys I/F */
/* Add nodes that are always present via .groups */
- result = thermal_zone_create_device_groups(tz, mask);
+ result = thermal_zone_create_device_groups(tz);
if (result)
goto remove_id;
Index: linux-pm/drivers/thermal/thermal_core.h
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.h
+++ linux-pm/drivers/thermal/thermal_core.h
@@ -131,7 +131,7 @@ void thermal_zone_trip_updated(struct th
int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
/* sysfs I/F */
-int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
+int thermal_zone_create_device_groups(struct thermal_zone_device *tz);
void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -392,17 +392,16 @@ static const struct attribute_group *the
/**
* create_trip_attrs() - create attributes for trip points
* @tz: the thermal zone device
- * @mask: Writeable trip point bitmap.
*
* helper function to instantiate sysfs entries for every trip
* point and its properties of a struct thermal_zone_device.
*
* Return: 0 on success, the proper error value otherwise.
*/
-static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
+static int create_trip_attrs(struct thermal_zone_device *tz)
{
+ const struct thermal_trip *trip;
struct attribute **attrs;
- int indx;
/* This function works only for zones with at least one trip */
if (tz->num_trips <= 0)
@@ -437,7 +436,9 @@ static int create_trip_attrs(struct ther
return -ENOMEM;
}
- for (indx = 0; indx < tz->num_trips; indx++) {
+ for_each_trip(tz, trip) {
+ int indx = thermal_zone_trip_id(tz, trip);
+
/* create trip type attribute */
snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
"trip_point_%d_type", indx);
@@ -458,7 +459,7 @@ static int create_trip_attrs(struct ther
tz->trip_temp_attrs[indx].name;
tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
- if (mask & (1 << indx)) {
+ if (trip->flags & THERMAL_TRIP_FLAG_RW_TEMP) {
tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
tz->trip_temp_attrs[indx].attr.store =
trip_point_temp_store;
@@ -473,7 +474,7 @@ static int create_trip_attrs(struct ther
tz->trip_hyst_attrs[indx].name;
tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
- if (tz->ops.set_trip_hyst) {
+ if (trip->flags & THERMAL_TRIP_FLAG_RW_HYST) {
tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
tz->trip_hyst_attrs[indx].attr.store =
trip_point_hyst_store;
@@ -505,8 +506,7 @@ static void destroy_trip_attrs(struct th
kfree(tz->trips_attribute_group.attrs);
}
-int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
- int mask)
+int thermal_zone_create_device_groups(struct thermal_zone_device *tz)
{
const struct attribute_group **groups;
int i, size, result;
@@ -522,7 +522,7 @@ int thermal_zone_create_device_groups(st
groups[i] = thermal_zone_attribute_groups[i];
if (tz->num_trips) {
- result = create_trip_attrs(tz, mask);
+ result = create_trip_attrs(tz);
if (result) {
kfree(groups);On Thu, Feb 22, 2024 at 8:48 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In order to allow thermal zone creators to specify the writability of
> trip point temperature and hysteresis on a per-trip basis, add a flags
> field to struct thermal_trip and define flags to represent the desired
> trip properties.
>
> Also make thermal_zone_device_register_with_trips() set the
> THERMAL_TRIP_FLAG_RW_TEMP flag for all trips covered by the writable
> trips mask passed to it and modify the thermal sysfs code to look at
> the trip flags instead of using the writable trips mask directly or
> checking the presence of the .set_trip_hyst() zone callback.
>
> Additionally, make trip_point_temp_store() and trip_point_hyst_store()
> fail with an error code if the trip passed to one of them has
> THERMAL_TRIP_FLAG_RW_TEMP or THERMAL_TRIP_FLAG_RW_HYST,
> respectively, clear in its flags.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
AFAICS this patch addresses all of the review comments and because the
rest of the series (depending on it) has been approved, I'm applying
the whole series including this patch.
> ---
>
> v2 -> v2.1:
> * Don't add redundant checks in 3 places (Daniel).
> * Define THERMAL_TRIP_FLAG_RW as the combination of the _RW_ trip flags (Daniel).
>
> v1 -> v2:
> * Rename trip flags (Stanislaw).
>
> ---
> drivers/thermal/thermal_core.c | 9 ++++++++-
> drivers/thermal/thermal_core.h | 2 +-
> drivers/thermal/thermal_sysfs.c | 18 +++++++++---------
> include/linux/thermal.h | 8 ++++++++
> 4 files changed, 26 insertions(+), 11 deletions(-)
>
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -64,15 +64,23 @@ enum thermal_notify_event {
> * @threshold: trip crossing notification threshold miliCelsius
> * @type: trip point type
> * @priv: pointer to driver data associated with this trip
> + * @flags: flags representing binary properties of the trip
> */
> struct thermal_trip {
> int temperature;
> int hysteresis;
> int threshold;
> enum thermal_trip_type type;
> + u8 flags;
> void *priv;
> };
>
> +#define THERMAL_TRIP_FLAG_RW_TEMP BIT(0)
> +#define THERMAL_TRIP_FLAG_RW_HYST BIT(1)
> +
> +#define THERMAL_TRIP_FLAG_RW (THERMAL_TRIP_FLAG_RW_TEMP | \
> + THERMAL_TRIP_FLAG_RW_HYST)
> +
> struct thermal_zone_device_ops {
> int (*bind) (struct thermal_zone_device *,
> struct thermal_cooling_device *);
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1278,6 +1278,7 @@ thermal_zone_device_register_with_trips(
> int passive_delay, int polling_delay)
> {
> struct thermal_zone_device *tz;
> + struct thermal_trip *trip;
> int id;
> int result;
> struct thermal_governor *governor;
> @@ -1356,13 +1357,19 @@ thermal_zone_device_register_with_trips(
> tz->devdata = devdata;
> memcpy(tz->trips, trips, num_trips * sizeof(*trips));
> tz->num_trips = num_trips;
> + for_each_trip(tz, trip) {
> + if (mask & 1)
> + trip->flags |= THERMAL_TRIP_FLAG_RW_TEMP;
> +
> + mask >>= 1;
> + }
>
> thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);
> thermal_set_delay_jiffies(&tz->polling_delay_jiffies, polling_delay);
>
> /* sys I/F */
> /* Add nodes that are always present via .groups */
> - result = thermal_zone_create_device_groups(tz, mask);
> + result = thermal_zone_create_device_groups(tz);
> if (result)
> goto remove_id;
>
> Index: linux-pm/drivers/thermal/thermal_core.h
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.h
> +++ linux-pm/drivers/thermal/thermal_core.h
> @@ -131,7 +131,7 @@ void thermal_zone_trip_updated(struct th
> int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
>
> /* sysfs I/F */
> -int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
> +int thermal_zone_create_device_groups(struct thermal_zone_device *tz);
> void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
> void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
> void thermal_cooling_device_destroy_sysfs(struct thermal_cooling_device *cdev);
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -392,17 +392,16 @@ static const struct attribute_group *the
> /**
> * create_trip_attrs() - create attributes for trip points
> * @tz: the thermal zone device
> - * @mask: Writeable trip point bitmap.
> *
> * helper function to instantiate sysfs entries for every trip
> * point and its properties of a struct thermal_zone_device.
> *
> * Return: 0 on success, the proper error value otherwise.
> */
> -static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
> +static int create_trip_attrs(struct thermal_zone_device *tz)
> {
> + const struct thermal_trip *trip;
> struct attribute **attrs;
> - int indx;
>
> /* This function works only for zones with at least one trip */
> if (tz->num_trips <= 0)
> @@ -437,7 +436,9 @@ static int create_trip_attrs(struct ther
> return -ENOMEM;
> }
>
> - for (indx = 0; indx < tz->num_trips; indx++) {
> + for_each_trip(tz, trip) {
> + int indx = thermal_zone_trip_id(tz, trip);
> +
> /* create trip type attribute */
> snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
> "trip_point_%d_type", indx);
> @@ -458,7 +459,7 @@ static int create_trip_attrs(struct ther
> tz->trip_temp_attrs[indx].name;
> tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
> tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
> - if (mask & (1 << indx)) {
> + if (trip->flags & THERMAL_TRIP_FLAG_RW_TEMP) {
> tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
> tz->trip_temp_attrs[indx].attr.store =
> trip_point_temp_store;
> @@ -473,7 +474,7 @@ static int create_trip_attrs(struct ther
> tz->trip_hyst_attrs[indx].name;
> tz->trip_hyst_attrs[indx].attr.attr.mode = S_IRUGO;
> tz->trip_hyst_attrs[indx].attr.show = trip_point_hyst_show;
> - if (tz->ops.set_trip_hyst) {
> + if (trip->flags & THERMAL_TRIP_FLAG_RW_HYST) {
> tz->trip_hyst_attrs[indx].attr.attr.mode |= S_IWUSR;
> tz->trip_hyst_attrs[indx].attr.store =
> trip_point_hyst_store;
> @@ -505,8 +506,7 @@ static void destroy_trip_attrs(struct th
> kfree(tz->trips_attribute_group.attrs);
> }
>
> -int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
> - int mask)
> +int thermal_zone_create_device_groups(struct thermal_zone_device *tz)
> {
> const struct attribute_group **groups;
> int i, size, result;
> @@ -522,7 +522,7 @@ int thermal_zone_create_device_groups(st
> groups[i] = thermal_zone_attribute_groups[i];
>
> if (tz->num_trips) {
> - result = create_trip_attrs(tz, mask);
> + result = create_trip_attrs(tz);
> if (result) {
> kfree(groups);
>
>
>
>
>
© 2016 - 2026 Red Hat, Inc.