| Message ID | 5762433.DvuYhMxLoT@kreacher (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | thermal: Store trips table and ops in thermal_zone_device | expand |
On Mon, Feb 05, 2024 at 10:14:31PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The current code requires thermal zone creators to pass a pointer to a > writable trips table to thermal_zone_device_register_with_trips() and > that trips table is then used by the thermal core going forward. > > Consequently, the callers of thermal_zone_device_register_with_trips() > are required to hold on to the trips table passed to it until the given > thermal zone is unregistered, at which point the trips table can be > freed, but at the same time they are not allowed to access the cells in > that table directly. This is both error prone and confusing. > > To address it, turn the trips table pointer in struct thermal_zone_device > into a flex array (counted by its num_trips field), allocate it during > thermal zone device allocation and copy the contents of the trips table > supplied by the zone creator (which can be const now) into it. > > This allows the callers of thermal_zone_device_register_with_trips() to > drop their trip tables right after the zone registration. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > --- > drivers/thermal/thermal_core.c | 16 +++++++++------- > include/linux/thermal.h | 10 +++++----- > 2 files changed, 14 insertions(+), 12 deletions(-) > > Index: linux-pm/include/linux/thermal.h > =================================================================== > --- linux-pm.orig/include/linux/thermal.h > +++ linux-pm/include/linux/thermal.h > @@ -130,7 +130,6 @@ struct thermal_cooling_device { > * @trip_hyst_attrs: attributes for trip points for sysfs: trip hysteresis > * @mode: current mode of this thermal zone > * @devdata: private pointer for device private data > - * @trips: an array of struct thermal_trip > * @num_trips: number of trip points the thermal zone supports > * @passive_delay_jiffies: number of jiffies to wait between polls when > * performing passive cooling. > @@ -160,6 +159,7 @@ struct thermal_cooling_device { > * @poll_queue: delayed work for polling > * @notify_event: Last notification event > * @suspended: thermal zone suspend indicator > + * @trips: array of struct thermal_trip objects > */ > struct thermal_zone_device { > int id; > @@ -172,7 +172,6 @@ struct thermal_zone_device { > struct thermal_attr *trip_hyst_attrs; > enum thermal_device_mode mode; > void *devdata; > - struct thermal_trip *trips; > int num_trips; > unsigned long passive_delay_jiffies; > unsigned long polling_delay_jiffies; > @@ -193,10 +192,11 @@ struct thermal_zone_device { > struct list_head node; > struct delayed_work poll_queue; > enum thermal_notify_event notify_event; > + bool suspended; > #ifdef CONFIG_THERMAL_DEBUGFS > struct thermal_debugfs *debugfs; > #endif > - bool suspended; > + struct thermal_trip trips[] __counted_by(num_trips); > }; > > /** > @@ -315,7 +315,7 @@ int thermal_zone_get_crit_temp(struct th > #ifdef CONFIG_THERMAL > struct thermal_zone_device *thermal_zone_device_register_with_trips( > const char *type, > - struct thermal_trip *trips, > + const struct thermal_trip *trips, > int num_trips, int mask, > void *devdata, > struct thermal_zone_device_ops *ops, > @@ -375,7 +375,7 @@ void thermal_zone_device_critical(struct > #else > static inline struct thermal_zone_device *thermal_zone_device_register_with_trips( > const char *type, > - struct thermal_trip *trips, > + const struct thermal_trip *trips, > int num_trips, int mask, > void *devdata, > struct thermal_zone_device_ops *ops, > Index: linux-pm/drivers/thermal/thermal_core.c > =================================================================== > --- linux-pm.orig/drivers/thermal/thermal_core.c > +++ linux-pm/drivers/thermal/thermal_core.c > @@ -1272,10 +1272,13 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_ > * IS_ERR*() helpers. > */ > struct thermal_zone_device * > -thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int num_trips, int mask, > - void *devdata, struct thermal_zone_device_ops *ops, > - const struct thermal_zone_params *tzp, int passive_delay, > - int polling_delay) > +thermal_zone_device_register_with_trips(const char *type, > + const struct thermal_trip *trips, > + int num_trips, int mask, > + void *devdata, > + struct thermal_zone_device_ops *ops, > + const struct thermal_zone_params *tzp, > + int passive_delay, int polling_delay) > { > struct thermal_zone_device *tz; > int id; > @@ -1322,7 +1325,7 @@ thermal_zone_device_register_with_trips( > if (!thermal_class) > return ERR_PTR(-ENODEV); > > - tz = kzalloc(sizeof(*tz), GFP_KERNEL); > + tz = kzalloc(struct_size(tz, trips, num_trips), GFP_KERNEL); > if (!tz) > return ERR_PTR(-ENOMEM); > > @@ -1344,7 +1347,6 @@ thermal_zone_device_register_with_trips( > result = id; > goto free_tzp; > } > - > tz->id = id; > strscpy(tz->type, type, sizeof(tz->type)); > > @@ -1354,7 +1356,7 @@ thermal_zone_device_register_with_trips( > tz->ops = ops; > tz->device.class = thermal_class; > tz->devdata = devdata; > - tz->trips = trips; > + memcpy(tz->trips, trips, num_trips * sizeof(trips[0])); > tz->num_trips = num_trips; > > thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay); > > > >
On Thu, Feb 8, 2024 at 9:22 AM Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> wrote: > > On Mon, Feb 05, 2024 at 10:14:31PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The current code requires thermal zone creators to pass a pointer to a > > writable trips table to thermal_zone_device_register_with_trips() and > > that trips table is then used by the thermal core going forward. > > > > Consequently, the callers of thermal_zone_device_register_with_trips() > > are required to hold on to the trips table passed to it until the given > > thermal zone is unregistered, at which point the trips table can be > > freed, but at the same time they are not allowed to access the cells in > > that table directly. This is both error prone and confusing. > > > > To address it, turn the trips table pointer in struct thermal_zone_device > > into a flex array (counted by its num_trips field), allocate it during > > thermal zone device allocation and copy the contents of the trips table > > supplied by the zone creator (which can be const now) into it. > > > > This allows the callers of thermal_zone_device_register_with_trips() to > > drop their trip tables right after the zone registration. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> Thanks a lot for all of the reviews, much appreciated, especially regarding the Intel drivers changes. Unfortunately, this patch series, and the first half of it in particular, is somewhat premature, because a couple of thermal drivers do unexpected things to their trip point tables and they need to be modified to stop accessing the trip tables directly before the core can start using internal copies of them. I'm going to save the tags for the future, however.
Index: linux-pm/include/linux/thermal.h =================================================================== --- linux-pm.orig/include/linux/thermal.h +++ linux-pm/include/linux/thermal.h @@ -130,7 +130,6 @@ struct thermal_cooling_device { * @trip_hyst_attrs: attributes for trip points for sysfs: trip hysteresis * @mode: current mode of this thermal zone * @devdata: private pointer for device private data - * @trips: an array of struct thermal_trip * @num_trips: number of trip points the thermal zone supports * @passive_delay_jiffies: number of jiffies to wait between polls when * performing passive cooling. @@ -160,6 +159,7 @@ struct thermal_cooling_device { * @poll_queue: delayed work for polling * @notify_event: Last notification event * @suspended: thermal zone suspend indicator + * @trips: array of struct thermal_trip objects */ struct thermal_zone_device { int id; @@ -172,7 +172,6 @@ struct thermal_zone_device { struct thermal_attr *trip_hyst_attrs; enum thermal_device_mode mode; void *devdata; - struct thermal_trip *trips; int num_trips; unsigned long passive_delay_jiffies; unsigned long polling_delay_jiffies; @@ -193,10 +192,11 @@ struct thermal_zone_device { struct list_head node; struct delayed_work poll_queue; enum thermal_notify_event notify_event; + bool suspended; #ifdef CONFIG_THERMAL_DEBUGFS struct thermal_debugfs *debugfs; #endif - bool suspended; + struct thermal_trip trips[] __counted_by(num_trips); }; /** @@ -315,7 +315,7 @@ int thermal_zone_get_crit_temp(struct th #ifdef CONFIG_THERMAL struct thermal_zone_device *thermal_zone_device_register_with_trips( const char *type, - struct thermal_trip *trips, + const struct thermal_trip *trips, int num_trips, int mask, void *devdata, struct thermal_zone_device_ops *ops, @@ -375,7 +375,7 @@ void thermal_zone_device_critical(struct #else static inline struct thermal_zone_device *thermal_zone_device_register_with_trips( const char *type, - struct thermal_trip *trips, + const struct thermal_trip *trips, int num_trips, int mask, void *devdata, struct thermal_zone_device_ops *ops, Index: linux-pm/drivers/thermal/thermal_core.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.c +++ linux-pm/drivers/thermal/thermal_core.c @@ -1272,10 +1272,13 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_ * IS_ERR*() helpers. */ struct thermal_zone_device * -thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int num_trips, int mask, - void *devdata, struct thermal_zone_device_ops *ops, - const struct thermal_zone_params *tzp, int passive_delay, - int polling_delay) +thermal_zone_device_register_with_trips(const char *type, + const struct thermal_trip *trips, + int num_trips, int mask, + void *devdata, + struct thermal_zone_device_ops *ops, + const struct thermal_zone_params *tzp, + int passive_delay, int polling_delay) { struct thermal_zone_device *tz; int id; @@ -1322,7 +1325,7 @@ thermal_zone_device_register_with_trips( if (!thermal_class) return ERR_PTR(-ENODEV); - tz = kzalloc(sizeof(*tz), GFP_KERNEL); + tz = kzalloc(struct_size(tz, trips, num_trips), GFP_KERNEL); if (!tz) return ERR_PTR(-ENOMEM); @@ -1344,7 +1347,6 @@ thermal_zone_device_register_with_trips( result = id; goto free_tzp; } - tz->id = id; strscpy(tz->type, type, sizeof(tz->type)); @@ -1354,7 +1356,7 @@ thermal_zone_device_register_with_trips( tz->ops = ops; tz->device.class = thermal_class; tz->devdata = devdata; - tz->trips = trips; + memcpy(tz->trips, trips, num_trips * sizeof(trips[0])); tz->num_trips = num_trips; thermal_set_delay_jiffies(&tz->passive_delay_jiffies, passive_delay);