From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The current code requires thermal zone creators to pass pointers to
writable ops structures to thermal_zone_device_register_with_trips()
which needs to modify the target struct thermal_zone_device_ops object
if the "critical" operation in it is NULL.

Moreover, the callers of thermal_zone_device_register_with_trips() are
required to hold on to the struct thermal_zone_device_ops object passed
to it until the given thermal zone is unregistered.

Both of these requirements are quite inconvenient, so modify struct
thermal_zone_device to contain struct thermal_zone_device_ops as field and
make thermal_zone_device_register_with_trips() copy the contents of the
struct thermal_zone_device_ops passed to it via a pointer (which can be
const now) to that field.

Also adjust the code using thermal zone ops accordingly and modify
thermal_of_zone_register() to use a local ops variable during
thermal zone registration so ops do not need to be freed in
thermal_of_zone_unregister() any more.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---

v1 -> v2:
   * Rebase.
   * Add thermal_of modifications.
   * Add R-by from Stanislaw (still applicable IMV).

---
 drivers/thermal/thermal_core.c    |   40 +++++++++++++++++++-------------------
 drivers/thermal/thermal_helpers.c |   10 ++++-----
 drivers/thermal/thermal_hwmon.c   |    4 +--
 drivers/thermal/thermal_of.c      |   26 +++++++-----------------
 drivers/thermal/thermal_sysfs.c   |    8 +++----
 drivers/thermal/thermal_trip.c    |    4 +--
 include/linux/thermal.h           |    6 ++---
 7 files changed, 44 insertions(+), 54 deletions(-)

Index: linux-pm/include/linux/thermal.h
===================================================================
--- linux-pm.orig/include/linux/thermal.h
+++ linux-pm/include/linux/thermal.h
@@ -189,7 +189,7 @@ struct thermal_zone_device {
 	int prev_low_trip;
 	int prev_high_trip;
 	atomic_t need_update;
-	struct thermal_zone_device_ops *ops;
+	struct thermal_zone_device_ops ops;
 	struct thermal_zone_params *tzp;
 	struct thermal_governor *governor;
 	void *governor_data;
@@ -324,14 +324,14 @@ struct thermal_zone_device *thermal_zone
 					const char *type,
 					const struct thermal_trip *trips,
 					int num_trips, void *devdata,
-					struct thermal_zone_device_ops *ops,
+					const struct thermal_zone_device_ops *ops,
 					const struct thermal_zone_params *tzp,
 					int passive_delay, int polling_delay);
 
 struct thermal_zone_device *thermal_tripless_zone_device_register(
 					const char *type,
 					void *devdata,
-					struct thermal_zone_device_ops *ops,
+					const struct thermal_zone_device_ops *ops,
 					const struct thermal_zone_params *tzp);
 
 void thermal_zone_device_unregister(struct thermal_zone_device *tz);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -356,9 +356,9 @@ static void handle_critical_trips(struct
 	trace_thermal_zone_trip(tz, thermal_zone_trip_id(tz, trip), trip->type);
 
 	if (trip->type == THERMAL_TRIP_CRITICAL)
-		tz->ops->critical(tz);
-	else if (tz->ops->hot)
-		tz->ops->hot(tz);
+		tz->ops.critical(tz);
+	else if (tz->ops.hot)
+		tz->ops.hot(tz);
 }
 
 static void handle_thermal_trip(struct thermal_zone_device *tz,
@@ -493,8 +493,8 @@ static int thermal_zone_device_set_mode(
 		return ret;
 	}
 
-	if (tz->ops->change_mode)
-		ret = tz->ops->change_mode(tz, mode);
+	if (tz->ops.change_mode)
+		ret = tz->ops.change_mode(tz, mode);
 
 	if (!ret)
 		tz->mode = mode;
@@ -867,8 +867,8 @@ static void bind_cdev(struct thermal_coo
 	struct thermal_zone_device *pos = NULL;
 
 	list_for_each_entry(pos, &thermal_tz_list, node) {
-		if (pos->ops->bind) {
-			ret = pos->ops->bind(pos, cdev);
+		if (pos->ops.bind) {
+			ret = pos->ops.bind(pos, cdev);
 			if (ret)
 				print_bind_err_msg(pos, cdev, ret);
 		}
@@ -1184,8 +1184,8 @@ void thermal_cooling_device_unregister(s
 
 	/* Unbind all thermal zones associated with 'this' cdev */
 	list_for_each_entry(tz, &thermal_tz_list, node) {
-		if (tz->ops->unbind)
-			tz->ops->unbind(tz, cdev);
+		if (tz->ops.unbind)
+			tz->ops.unbind(tz, cdev);
 	}
 
 	mutex_unlock(&thermal_list_lock);
@@ -1199,13 +1199,13 @@ static void bind_tz(struct thermal_zone_
 	int ret;
 	struct thermal_cooling_device *pos = NULL;
 
-	if (!tz->ops->bind)
+	if (!tz->ops.bind)
 		return;
 
 	mutex_lock(&thermal_list_lock);
 
 	list_for_each_entry(pos, &thermal_cdev_list, node) {
-		ret = tz->ops->bind(tz, pos);
+		ret = tz->ops.bind(tz, pos);
 		if (ret)
 			print_bind_err_msg(tz, pos, ret);
 	}
@@ -1224,8 +1224,8 @@ int thermal_zone_get_crit_temp(struct th
 {
 	int i, ret = -EINVAL;
 
-	if (tz->ops->get_crit_temp)
-		return tz->ops->get_crit_temp(tz, temp);
+	if (tz->ops.get_crit_temp)
+		return tz->ops.get_crit_temp(tz, temp);
 
 	mutex_lock(&tz->lock);
 
@@ -1271,7 +1271,7 @@ struct thermal_zone_device *
 thermal_zone_device_register_with_trips(const char *type,
 					const struct thermal_trip *trips,
 					int num_trips, void *devdata,
-					struct thermal_zone_device_ops *ops,
+					const struct thermal_zone_device_ops *ops,
 					const struct thermal_zone_params *tzp,
 					int passive_delay, int polling_delay)
 {
@@ -1333,10 +1333,10 @@ thermal_zone_device_register_with_trips(
 	tz->id = id;
 	strscpy(tz->type, type, sizeof(tz->type));
 
-	if (!ops->critical)
-		ops->critical = thermal_zone_device_critical;
+	tz->ops = *ops;
+	if (!tz->ops.critical)
+		tz->ops.critical = thermal_zone_device_critical;
 
-	tz->ops = ops;
 	tz->device.class = thermal_class;
 	tz->devdata = devdata;
 	memcpy(tz->trips, trips, num_trips * sizeof(*trips));
@@ -1422,7 +1422,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_device_re
 struct thermal_zone_device *thermal_tripless_zone_device_register(
 					const char *type,
 					void *devdata,
-					struct thermal_zone_device_ops *ops,
+					const struct thermal_zone_device_ops *ops,
 					const struct thermal_zone_params *tzp)
 {
 	return thermal_zone_device_register_with_trips(type, NULL, 0, devdata,
@@ -1484,8 +1484,8 @@ void thermal_zone_device_unregister(stru
 
 	/* Unbind all cdevs associated with 'this' thermal zone */
 	list_for_each_entry(cdev, &thermal_cdev_list, node)
-		if (tz->ops->unbind)
-			tz->ops->unbind(tz, cdev);
+		if (tz->ops.unbind)
+			tz->ops.unbind(tz, cdev);
 
 	mutex_unlock(&thermal_list_lock);
 
Index: linux-pm/drivers/thermal/thermal_sysfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_sysfs.c
+++ linux-pm/drivers/thermal/thermal_sysfs.c
@@ -128,8 +128,8 @@ trip_point_temp_store(struct device *dev
 	}
 
 	if (temp != trip->temperature) {
-		if (tz->ops->set_trip_temp) {
-			ret = tz->ops->set_trip_temp(tz, trip_id, temp);
+		if (tz->ops.set_trip_temp) {
+			ret = tz->ops.set_trip_temp(tz, trip_id, temp);
 			if (ret)
 				goto unlock;
 		}
@@ -254,10 +254,10 @@ emul_temp_store(struct device *dev, stru
 
 	mutex_lock(&tz->lock);
 
-	if (!tz->ops->set_emul_temp)
+	if (!tz->ops.set_emul_temp)
 		tz->emul_temperature = temperature;
 	else
-		ret = tz->ops->set_emul_temp(tz, temperature);
+		ret = tz->ops.set_emul_temp(tz, temperature);
 
 	if (!ret)
 		__thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
Index: linux-pm/drivers/thermal/thermal_helpers.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_helpers.c
+++ linux-pm/drivers/thermal/thermal_helpers.c
@@ -26,8 +26,8 @@ int get_tz_trend(struct thermal_zone_dev
 {
 	enum thermal_trend trend;
 
-	if (tz->emul_temperature || !tz->ops->get_trend ||
-	    tz->ops->get_trend(tz, trip, &trend)) {
+	if (tz->emul_temperature || !tz->ops.get_trend ||
+	    tz->ops.get_trend(tz, trip, &trend)) {
 		if (tz->temperature > tz->last_temperature)
 			trend = THERMAL_TREND_RAISING;
 		else if (tz->temperature < tz->last_temperature)
@@ -75,7 +75,7 @@ EXPORT_SYMBOL(get_thermal_instance);
  * temperature and fill @temp.
  *
  * Both tz and tz->ops must be valid pointers when calling this function,
- * and the tz->ops->get_temp callback must be provided.
+ * and the tz->ops.get_temp callback must be provided.
  * The function must be called under tz->lock.
  *
  * Return: On success returns 0, an error code otherwise
@@ -88,7 +88,7 @@ int __thermal_zone_get_temp(struct therm
 
 	lockdep_assert_held(&tz->lock);
 
-	ret = tz->ops->get_temp(tz, temp);
+	ret = tz->ops.get_temp(tz, temp);
 
 	if (IS_ENABLED(CONFIG_THERMAL_EMULATION) && tz->emul_temperature) {
 		for_each_trip(tz, trip) {
@@ -132,7 +132,7 @@ int thermal_zone_get_temp(struct thermal
 
 	mutex_lock(&tz->lock);
 
-	if (!tz->ops->get_temp) {
+	if (!tz->ops.get_temp) {
 		ret = -EINVAL;
 		goto unlock;
 	}
Index: linux-pm/drivers/thermal/thermal_trip.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_trip.c
+++ linux-pm/drivers/thermal/thermal_trip.c
@@ -70,7 +70,7 @@ void __thermal_zone_set_trips(struct the
 
 	lockdep_assert_held(&tz->lock);
 
-	if (!tz->ops->set_trips)
+	if (!tz->ops.set_trips)
 		return;
 
 	for_each_trip(tz, trip) {
@@ -114,7 +114,7 @@ void __thermal_zone_set_trips(struct the
 	 * Set a temperature window. When this window is left the driver
 	 * must inform the thermal core via thermal_zone_device_update.
 	 */
-	ret = tz->ops->set_trips(tz, low, high);
+	ret = tz->ops.set_trips(tz, low, high);
 	if (ret)
 		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
 }
Index: linux-pm/drivers/thermal/thermal_hwmon.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_hwmon.c
+++ linux-pm/drivers/thermal/thermal_hwmon.c
@@ -80,7 +80,7 @@ temp_crit_show(struct device *dev, struc
 
 	mutex_lock(&tz->lock);
 
-	ret = tz->ops->get_crit_temp(tz, &temperature);
+	ret = tz->ops.get_crit_temp(tz, &temperature);
 
 	mutex_unlock(&tz->lock);
 
@@ -132,7 +132,7 @@ thermal_hwmon_lookup_temp(const struct t
 static bool thermal_zone_crit_temp_valid(struct thermal_zone_device *tz)
 {
 	int temp;
-	return tz->ops->get_crit_temp && !tz->ops->get_crit_temp(tz, &temp);
+	return tz->ops.get_crit_temp && !tz->ops.get_crit_temp(tz, &temp);
 }
 
 int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
Index: linux-pm/drivers/thermal/thermal_of.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_of.c
+++ linux-pm/drivers/thermal/thermal_of.c
@@ -441,12 +441,10 @@ static int thermal_of_unbind(struct ther
 static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
 {
 	struct thermal_trip *trips = tz->trips;
-	struct thermal_zone_device_ops *ops = tz->ops;
 
 	thermal_zone_device_disable(tz);
 	thermal_zone_device_unregister(tz);
 	kfree(trips);
-	kfree(ops);
 }
 
 /**
@@ -472,33 +470,27 @@ static void thermal_of_zone_unregister(s
 static struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
 							    const struct thermal_zone_device_ops *ops)
 {
+	struct thermal_zone_device_ops of_ops = *ops;
 	struct thermal_zone_device *tz;
 	struct thermal_trip *trips;
 	struct thermal_zone_params tzp = {};
-	struct thermal_zone_device_ops *of_ops;
 	struct device_node *np;
 	const char *action;
 	int delay, pdelay;
 	int ntrips;
 	int ret;
 
-	of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
-	if (!of_ops)
-		return ERR_PTR(-ENOMEM);
-
 	np = of_thermal_zone_find(sensor, id);
 	if (IS_ERR(np)) {
 		if (PTR_ERR(np) != -ENODEV)
 			pr_err("Failed to find thermal zone for %pOFn id=%d\n", sensor, id);
-		ret = PTR_ERR(np);
-		goto out_kfree_of_ops;
+		return ERR_CAST(np);
 	}
 
 	trips = thermal_of_trips_init(np, &ntrips);
 	if (IS_ERR(trips)) {
 		pr_err("Failed to find trip points for %pOFn id=%d\n", sensor, id);
-		ret = PTR_ERR(trips);
-		goto out_kfree_of_ops;
+		return ERR_CAST(trips);
 	}
 
 	ret = thermal_of_monitor_init(np, &delay, &pdelay);
@@ -509,16 +501,16 @@ static struct thermal_zone_device *therm
 
 	thermal_of_parameters_init(np, &tzp);
 
-	of_ops->bind = thermal_of_bind;
-	of_ops->unbind = thermal_of_unbind;
+	of_ops.bind = thermal_of_bind;
+	of_ops.unbind = thermal_of_unbind;
 
 	ret = of_property_read_string(np, "critical-action", &action);
 	if (!ret)
-		if (!of_ops->critical && !strcasecmp(action, "reboot"))
-			of_ops->critical = thermal_zone_device_critical_reboot;
+		if (!of_ops.critical && !strcasecmp(action, "reboot"))
+			of_ops.critical = thermal_zone_device_critical_reboot;
 
 	tz = thermal_zone_device_register_with_trips(np->name, trips, ntrips,
-						     data, of_ops, &tzp,
+						     data, &of_ops, &tzp,
 						     pdelay, delay);
 	if (IS_ERR(tz)) {
 		ret = PTR_ERR(tz);
@@ -538,8 +530,6 @@ static struct thermal_zone_device *therm
 
 out_kfree_trips:
 	kfree(trips);
-out_kfree_of_ops:
-	kfree(of_ops);
 
 	return ERR_PTR(ret);
 }

On 14/02/2024 13:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The current code requires thermal zone creators to pass pointers to
> writable ops structures to thermal_zone_device_register_with_trips()
> which needs to modify the target struct thermal_zone_device_ops object
> if the "critical" operation in it is NULL.
> 
> Moreover, the callers of thermal_zone_device_register_with_trips() are
> required to hold on to the struct thermal_zone_device_ops object passed
> to it until the given thermal zone is unregistered.
> 
> Both of these requirements are quite inconvenient, so modify struct
> thermal_zone_device to contain struct thermal_zone_device_ops as field and
> make thermal_zone_device_register_with_trips() copy the contents of the
> struct thermal_zone_device_ops passed to it via a pointer (which can be
> const now) to that field.
> 
> Also adjust the code using thermal zone ops accordingly and modify
> thermal_of_zone_register() to use a local ops variable during
> thermal zone registration so ops do not need to be freed in
> thermal_of_zone_unregister() any more.

[ ... ]

>   static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
>   {
>   	struct thermal_trip *trips = tz->trips;
> -	struct thermal_zone_device_ops *ops = tz->ops;
>   
>   	thermal_zone_device_disable(tz);
>   	thermal_zone_device_unregister(tz);
>   	kfree(trips);

Not related to the current patch but with patch 1/6. Freeing the trip 
points here will lead to a double free given it is already freed from 
thermal_zone_device_unregister() after the changes introduces in patch 
1, right ?


-- 
<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 11:47 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 14/02/2024 13:45, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The current code requires thermal zone creators to pass pointers to
> > writable ops structures to thermal_zone_device_register_with_trips()
> > which needs to modify the target struct thermal_zone_device_ops object
> > if the "critical" operation in it is NULL.
> >
> > Moreover, the callers of thermal_zone_device_register_with_trips() are
> > required to hold on to the struct thermal_zone_device_ops object passed
> > to it until the given thermal zone is unregistered.
> >
> > Both of these requirements are quite inconvenient, so modify struct
> > thermal_zone_device to contain struct thermal_zone_device_ops as field and
> > make thermal_zone_device_register_with_trips() copy the contents of the
> > struct thermal_zone_device_ops passed to it via a pointer (which can be
> > const now) to that field.
> >
> > Also adjust the code using thermal zone ops accordingly and modify
> > thermal_of_zone_register() to use a local ops variable during
> > thermal zone registration so ops do not need to be freed in
> > thermal_of_zone_unregister() any more.
>
> [ ... ]
>
> >   static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
> >   {
> >       struct thermal_trip *trips = tz->trips;
> > -     struct thermal_zone_device_ops *ops = tz->ops;
> >
> >       thermal_zone_device_disable(tz);
> >       thermal_zone_device_unregister(tz);
> >       kfree(trips);
>
> Not related to the current patch but with patch 1/6. Freeing the trip
> points here will lead to a double free given it is already freed from
> thermal_zone_device_unregister() after the changes introduces in patch
> 1, right ?

No, patch [1/6] doesn't free the caller-supplied ops, just copies them
into the local instance.  Attempting to free a static ops would not be
a good idea, for example.

BTW, thanks for all of the reviews, but this series is not applicable
without the one at

https://lore.kernel.org/linux-pm/6017196.lOV4Wx5bFT@kreacher/

On Thu, Feb 22, 2024 at 11:52 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 22, 2024 at 11:47 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 14/02/2024 13:45, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > The current code requires thermal zone creators to pass pointers to
> > > writable ops structures to thermal_zone_device_register_with_trips()
> > > which needs to modify the target struct thermal_zone_device_ops object
> > > if the "critical" operation in it is NULL.
> > >
> > > Moreover, the callers of thermal_zone_device_register_with_trips() are
> > > required to hold on to the struct thermal_zone_device_ops object passed
> > > to it until the given thermal zone is unregistered.
> > >
> > > Both of these requirements are quite inconvenient, so modify struct
> > > thermal_zone_device to contain struct thermal_zone_device_ops as field and
> > > make thermal_zone_device_register_with_trips() copy the contents of the
> > > struct thermal_zone_device_ops passed to it via a pointer (which can be
> > > const now) to that field.
> > >
> > > Also adjust the code using thermal zone ops accordingly and modify
> > > thermal_of_zone_register() to use a local ops variable during
> > > thermal zone registration so ops do not need to be freed in
> > > thermal_of_zone_unregister() any more.
> >
> > [ ... ]
> >
> > >   static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
> > >   {
> > >       struct thermal_trip *trips = tz->trips;
> > > -     struct thermal_zone_device_ops *ops = tz->ops;
> > >
> > >       thermal_zone_device_disable(tz);
> > >       thermal_zone_device_unregister(tz);
> > >       kfree(trips);
> >
> > Not related to the current patch but with patch 1/6. Freeing the trip
> > points here will lead to a double free given it is already freed from
> > thermal_zone_device_unregister() after the changes introduces in patch
> > 1, right ?
>
> No, patch [1/6] doesn't free the caller-supplied ops, just copies them
> into the local instance.  Attempting to free a static ops would not be
> a good idea, for example.
>
> BTW, thanks for all of the reviews, but this series is not applicable
> without the one at
>
> https://lore.kernel.org/linux-pm/6017196.lOV4Wx5bFT@kreacher/

As it turns out, this really is not a big deal, because the rebase is
trivial for everything except for the Intel patches, but for those two
I have the v1 versions that apply just fine without the other series
and have been reviewed.

So I can apply this series before the above one and rebase the latter
(I'd rather not send a new version of it though, so it can be reviewed
as is).

So never mind.

On 22/02/2024 11:52, Rafael J. Wysocki wrote:
> On Thu, Feb 22, 2024 at 11:47 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 14/02/2024 13:45, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> The current code requires thermal zone creators to pass pointers to
>>> writable ops structures to thermal_zone_device_register_with_trips()
>>> which needs to modify the target struct thermal_zone_device_ops object
>>> if the "critical" operation in it is NULL.
>>>
>>> Moreover, the callers of thermal_zone_device_register_with_trips() are
>>> required to hold on to the struct thermal_zone_device_ops object passed
>>> to it until the given thermal zone is unregistered.
>>>
>>> Both of these requirements are quite inconvenient, so modify struct
>>> thermal_zone_device to contain struct thermal_zone_device_ops as field and
>>> make thermal_zone_device_register_with_trips() copy the contents of the
>>> struct thermal_zone_device_ops passed to it via a pointer (which can be
>>> const now) to that field.
>>>
>>> Also adjust the code using thermal zone ops accordingly and modify
>>> thermal_of_zone_register() to use a local ops variable during
>>> thermal zone registration so ops do not need to be freed in
>>> thermal_of_zone_unregister() any more.
>>
>> [ ... ]
>>
>>>    static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
>>>    {
>>>        struct thermal_trip *trips = tz->trips;
>>> -     struct thermal_zone_device_ops *ops = tz->ops;
>>>
>>>        thermal_zone_device_disable(tz);
>>>        thermal_zone_device_unregister(tz);
>>>        kfree(trips);
>>
>> Not related to the current patch but with patch 1/6. Freeing the trip
>> points here will lead to a double free given it is already freed from
>> thermal_zone_device_unregister() after the changes introduces in patch
>> 1, right ?
> 
> No, patch [1/6] doesn't free the caller-supplied ops, just copies them
> into the local instance.  Attempting to free a static ops would not be
> a good idea, for example.

I'm referring to the trip points not the ops.

The patch 1 does:

tz = kzalloc(struct_size(tz, trips, num_trips), GFP_KERNEL);

Then the last line of thermal_zone_device_unregister() does:

kfree(tz);

That includes the trip points in the flexible array.

Now in thermal_of_zone_unregister(), we do:

trips = tz->trips;
thermal_zone_device_unregister(tz);
kfree(trips);

Hence double kfree, right?



> BTW, thanks for all of the reviews, but this series is not applicable
> without the one at
> 
> https://lore.kernel.org/linux-pm/6017196.lOV4Wx5bFT@kreacher/



-- 
<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 11:58 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 22/02/2024 11:52, Rafael J. Wysocki wrote:
> > On Thu, Feb 22, 2024 at 11:47 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 14/02/2024 13:45, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> The current code requires thermal zone creators to pass pointers to
> >>> writable ops structures to thermal_zone_device_register_with_trips()
> >>> which needs to modify the target struct thermal_zone_device_ops object
> >>> if the "critical" operation in it is NULL.
> >>>
> >>> Moreover, the callers of thermal_zone_device_register_with_trips() are
> >>> required to hold on to the struct thermal_zone_device_ops object passed
> >>> to it until the given thermal zone is unregistered.
> >>>
> >>> Both of these requirements are quite inconvenient, so modify struct
> >>> thermal_zone_device to contain struct thermal_zone_device_ops as field and
> >>> make thermal_zone_device_register_with_trips() copy the contents of the
> >>> struct thermal_zone_device_ops passed to it via a pointer (which can be
> >>> const now) to that field.
> >>>
> >>> Also adjust the code using thermal zone ops accordingly and modify
> >>> thermal_of_zone_register() to use a local ops variable during
> >>> thermal zone registration so ops do not need to be freed in
> >>> thermal_of_zone_unregister() any more.
> >>
> >> [ ... ]
> >>
> >>>    static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
> >>>    {
> >>>        struct thermal_trip *trips = tz->trips;
> >>> -     struct thermal_zone_device_ops *ops = tz->ops;
> >>>
> >>>        thermal_zone_device_disable(tz);
> >>>        thermal_zone_device_unregister(tz);
> >>>        kfree(trips);
> >>
> >> Not related to the current patch but with patch 1/6. Freeing the trip
> >> points here will lead to a double free given it is already freed from
> >> thermal_zone_device_unregister() after the changes introduces in patch
> >> 1, right ?
> >
> > No, patch [1/6] doesn't free the caller-supplied ops, just copies them
> > into the local instance.  Attempting to free a static ops would not be
> > a good idea, for example.
>
> I'm referring to the trip points not the ops.

Ah, sorry for the confusion.

> The patch 1 does:
>
> tz = kzalloc(struct_size(tz, trips, num_trips), GFP_KERNEL);
>
> Then the last line of thermal_zone_device_unregister() does:
>
> kfree(tz);
>
> That includes the trip points in the flexible array.

Right.

> Now in thermal_of_zone_unregister(), we do:
>
> trips = tz->trips;

I missed this.

> thermal_zone_device_unregister(tz);
> kfree(trips);
>
> Hence double kfree, right?

Indeed, so patch [1/6] is missing a thermal_of change to stop freeing
trips separately.  Let me send an update of just that patch.

On 14/02/2024 13:45, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The current code requires thermal zone creators to pass pointers to
> writable ops structures to thermal_zone_device_register_with_trips()
> which needs to modify the target struct thermal_zone_device_ops object
> if the "critical" operation in it is NULL.
> 
> Moreover, the callers of thermal_zone_device_register_with_trips() are
> required to hold on to the struct thermal_zone_device_ops object passed
> to it until the given thermal zone is unregistered.
> 
> Both of these requirements are quite inconvenient, so modify struct
> thermal_zone_device to contain struct thermal_zone_device_ops as field and
> make thermal_zone_device_register_with_trips() copy the contents of the
> struct thermal_zone_device_ops passed to it via a pointer (which can be
> const now) to that field.
> 
> Also adjust the code using thermal zone ops accordingly and modify
> thermal_of_zone_register() to use a local ops variable during
> thermal zone registration so ops do not need to be freed in
> thermal_of_zone_unregister() any more.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---

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


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