[PATCH 0/3 v2] thermal: core: Fix some error handling code in 'thermal_zone_device_register()'

[PATCH 0/3 v2] thermal: core: Fix some error handling code in 'thermal_zone_device_register()'

[Christophe JAILLET]

These patches fix some issue in the error handling path in
'thermal_zone_device_register()'

The first patch adds some new helper function in order to ease the resource
management.
Maybe using devm_ variant could also be a better choise to free manage
these resources. As it would be a bigger change, I've not looked at it at
all at the moment.
Also, note that this patch triggers a checkpatch warning about missing
identifier name in function definition. I've left it as is to keep the
style of the .h file.

The 2nd patch makes use of the new helper function.

The 3rd patch reorders the error handling path of
'thermal_zone_device_register()' in order to avoid the leaks.

*** These patches have been compiled tested only. ***

I also unsure about the right place for 'ida_simple_remove' in patch 3/3.


v1 -> v2:
    - add some helper functions in order to release some resources more
      easily
    - split the error handling path into 2. One before a successful call to
      'device_register()', and one after


Christophe JAILLET (3):
  thermal: core: Add some new helper functions to free resources
  thermal: core: Use the new 'thermal_zone_destroy_device_groups()'
    helper function
  thermal: core: Fix resources release in error paths in
    thermal_zone_device_register()

 drivers/thermal/thermal_core.c  | 29 ++++++++++++++---------------
 drivers/thermal/thermal_core.h  |  1 +
 drivers/thermal/thermal_sysfs.c | 29 +++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH 1/3 v2] thermal: core: Add some new helper functions to free resources

[Christophe JAILLET]

In order to easily free resources allocated by
'thermal_zone_create_device_groups()' we need 2 new helper functions.

The first one undoes 'thermal_zone_create_device_groups()'.
The 2nd one undoes 'create_trip_attrs()', which is a function called by
'thermal_zone_create_device_groups()'.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
These functions will be used in patch 2/3 in order to simplify
'thermal_release()'
I've tried to implement it as close a possible as the way the resources have
been allocated.
However, in 'thermal_release()', the code is simplier without some
additionnal 'if'.
No sure if we should prefer consistancy with allocation or simplicity of code.
---
 drivers/thermal/thermal_core.h  |  1 +
 drivers/thermal/thermal_sysfs.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index 2412b3759e16..27e3b1df7360 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -71,6 +71,7 @@ int thermal_build_list_of_policies(char *buf);
 
 /* sysfs I/F */
 int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
+void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
 void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
 /* used only at binding time */
 ssize_t
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index a694de907a26..eb95d64b9446 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -605,6 +605,24 @@ static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
 	return 0;
 }
 
+/**
+ * destroy_trip_attrs() - create attributes for trip points
+ * @tz:		the thermal zone device
+ *
+ * helper function to free resources alocated by create_trip_attrs()
+ */
+static void destroy_trip_attrs(struct thermal_zone_device *tz)
+{
+	if (!tz)
+		return;
+
+	kfree(tz->trip_type_attrs);
+	kfree(tz->trip_temp_attrs);
+	if (tz->ops->get_trip_hyst)
+		kfree(tz->trip_hyst_attrs);
+	kfree(tz->trips_attribute_group.attrs);
+}
+
 int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
 				      int mask)
 {
@@ -637,6 +655,17 @@ int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
 	return 0;
 }
 
+void thermal_zone_destroy_device_groups(struct thermal_zone_device *tz)
+{
+	if (!tz)
+		return;
+
+	if (tz->trips)
+		destroy_trip_attrs(tz);
+
+	kfree(tz->device.groups);
+}
+
 /* sys I/F for cooling device */
 static ssize_t
 thermal_cooling_device_type_show(struct device *dev,
-- 
2.11.0

[PATCH 2/3 v2] thermal: core: Use the new 'thermal_zone_destroy_device_groups()' helper function

[Christophe JAILLET]

Simplify code by using the new 'thermal_zone_destroy_device_groups()'
helper function.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/thermal/thermal_core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 5a51c740e372..2523901325d0 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -836,11 +836,7 @@ static void thermal_release(struct device *dev)
 	if (!strncmp(dev_name(dev), "thermal_zone",
 		     sizeof("thermal_zone") - 1)) {
 		tz = to_thermal_zone(dev);
-		kfree(tz->trip_type_attrs);
-		kfree(tz->trip_temp_attrs);
-		kfree(tz->trip_hyst_attrs);
-		kfree(tz->trips_attribute_group.attrs);
-		kfree(tz->device.groups);
+		thermal_zone_destroy_device_groups(tz);
 		kfree(tz);
 	} else if (!strncmp(dev_name(dev), "cooling_device",
 			    sizeof("cooling_device") - 1)) {
-- 
2.11.0

[PATCH 3/3 v2] thermal: core: Fix resources release in error paths in thermal_zone_device_register()

[Christophe JAILLET]

Reorder error handling code in order to fix some resources leaks in some
cases:
   - 'tz' would leak if 'thermal_zone_create_device_groups()' fails
   - memory allocated by 'thermal_zone_create_device_groups()' would leak
     if 'device_register()' fails

With this patch, we now have 2 error handling paths: one before
'device_register()', and one after it.
This is needed because some resources are released in 'thermal_release()'.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Should 'ida_simple_remove()' be folded somehow into 'thermal_release()'?
---
 drivers/thermal/thermal_core.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 2523901325d0..dfb4b5570c5a 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1209,10 +1209,8 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	ida_init(&tz->ida);
 	mutex_init(&tz->lock);
 	result = ida_simple_get(&thermal_tz_ida, 0, 0, GFP_KERNEL);
-	if (result < 0) {
-		kfree(tz);
-		return ERR_PTR(result);
-	}
+	if (result < 0)
+		goto free_tz;
 
 	tz->id = result;
 	strlcpy(tz->type, type, sizeof(tz->type));
@@ -1228,18 +1226,15 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	/* Add nodes that are always present via .groups */
 	result = thermal_zone_create_device_groups(tz, mask);
 	if (result)
-		goto unregister;
+		goto remove_id;
 
 	/* A new thermal zone needs to be updated anyway. */
 	atomic_set(&tz->need_update, 1);
 
 	dev_set_name(&tz->device, "thermal_zone%d", tz->id);
 	result = device_register(&tz->device);
-	if (result) {
-		ida_simple_remove(&thermal_tz_ida, tz->id);
-		kfree(tz);
-		return ERR_PTR(result);
-	}
+	if (result)
+		goto remove_device_groups;
 
 	for (count = 0; count < trips; count++) {
 		if (tz->ops->get_trip_type(tz, count, &trip_type))
@@ -1293,6 +1288,14 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	ida_simple_remove(&thermal_tz_ida, tz->id);
 	device_unregister(&tz->device);
 	return ERR_PTR(result);
+
+remove_device_groups:
+	thermal_zone_destroy_device_groups(tz);
+remove_id:
+	ida_simple_remove(&thermal_tz_ida, tz->id);
+free_tz:
+	kfree(tz);
+	return ERR_PTR(result);
 }
 EXPORT_SYMBOL_GPL(thermal_zone_device_register);
 
-- 
2.11.0

Re: [PATCH 1/3 v2] thermal: core: Add some new helper functions to free resources

[Zhang Rui]

On Tue, 2017-08-08 at 16:39 +0200, Christophe JAILLET wrote:
> In order to easily free resources allocated by
> 'thermal_zone_create_device_groups()' we need 2 new helper functions.
> 
> The first one undoes 'thermal_zone_create_device_groups()'.
> The 2nd one undoes 'create_trip_attrs()', which is a function called
> by
> 'thermal_zone_create_device_groups()'.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> These functions will be used in patch 2/3 in order to simplify
> 'thermal_release()'
> I've tried to implement it as close a possible as the way the
> resources have
> been allocated.
> However, in 'thermal_release()', the code is simplier without some
> additionnal 'if'.
> No sure if we should prefer consistancy with allocation or simplicity
> of code.
> ---
>  drivers/thermal/thermal_core.h  |  1 +
>  drivers/thermal/thermal_sysfs.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.h
> b/drivers/thermal/thermal_core.h
> index 2412b3759e16..27e3b1df7360 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -71,6 +71,7 @@ int thermal_build_list_of_policies(char *buf);
>  
>  /* sysfs I/F */
>  int thermal_zone_create_device_groups(struct thermal_zone_device *,
> int);
> +void thermal_zone_destroy_device_groups(struct thermal_zone_device
> *);
>  void thermal_cooling_device_setup_sysfs(struct
> thermal_cooling_device *);
>  /* used only at binding time */
>  ssize_t
> diff --git a/drivers/thermal/thermal_sysfs.c
> b/drivers/thermal/thermal_sysfs.c
> index a694de907a26..eb95d64b9446 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -605,6 +605,24 @@ static int create_trip_attrs(struct
> thermal_zone_device *tz, int mask)
>  	return 0;
>  }
>  
> +/**
> + * destroy_trip_attrs() - create attributes for trip points

s/create/destroy

> + * @tz:		the thermal zone device
> + *
> + * helper function to free resources alocated by create_trip_attrs()

s/alocated/allocated

thanks,
rui
> + */
> +static void destroy_trip_attrs(struct thermal_zone_device *tz)
> +{
> +	if (!tz)
> +		return;
> +
> +	kfree(tz->trip_type_attrs);
> +	kfree(tz->trip_temp_attrs);
> +	if (tz->ops->get_trip_hyst)
> +		kfree(tz->trip_hyst_attrs);
> +	kfree(tz->trips_attribute_group.attrs);
> +}
> +
>  int thermal_zone_create_device_groups(struct thermal_zone_device
> *tz,
>  				      int mask)
>  {
> @@ -637,6 +655,17 @@ int thermal_zone_create_device_groups(struct
> thermal_zone_device *tz,
>  	return 0;
>  }
>  
> +void thermal_zone_destroy_device_groups(struct thermal_zone_device
> *tz)
> +{
> +	if (!tz)
> +		return;
> +
> +	if (tz->trips)
> +		destroy_trip_attrs(tz);
> +
> +	kfree(tz->device.groups);
> +}
> +
>  /* sys I/F for cooling device */
>  static ssize_t
>  thermal_cooling_device_type_show(struct device *dev,

Re: [PATCH 1/3 v2] thermal: core: Add some new helper functions to free resources

[Zhang Rui]

On Fri, 2017-08-11 at 11:23 +0800, Zhang Rui wrote:
> On Tue, 2017-08-08 at 16:39 +0200, Christophe JAILLET wrote:
> > 
> > In order to easily free resources allocated by
> > 'thermal_zone_create_device_groups()' we need 2 new helper
> > functions.
> > 
> > The first one undoes 'thermal_zone_create_device_groups()'.
> > The 2nd one undoes 'create_trip_attrs()', which is a function
> > called
> > by
> > 'thermal_zone_create_device_groups()'.
> > 
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > These functions will be used in patch 2/3 in order to simplify
> > 'thermal_release()'
> > I've tried to implement it as close a possible as the way the
> > resources have
> > been allocated.
> > However, in 'thermal_release()', the code is simplier without some
> > additionnal 'if'.
> > No sure if we should prefer consistancy with allocation or
> > simplicity
> > of code.
> > ---
> >  drivers/thermal/thermal_core.h  |  1 +
> >  drivers/thermal/thermal_sysfs.c | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h
> > index 2412b3759e16..27e3b1df7360 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -71,6 +71,7 @@ int thermal_build_list_of_policies(char *buf);
> >  
> >  /* sysfs I/F */
> >  int thermal_zone_create_device_groups(struct thermal_zone_device
> > *,
> > int);
> > +void thermal_zone_destroy_device_groups(struct thermal_zone_device
> > *);
> >  void thermal_cooling_device_setup_sysfs(struct
> > thermal_cooling_device *);
> >  /* used only at binding time */
> >  ssize_t
> > diff --git a/drivers/thermal/thermal_sysfs.c
> > b/drivers/thermal/thermal_sysfs.c
> > index a694de907a26..eb95d64b9446 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -605,6 +605,24 @@ static int create_trip_attrs(struct
> > thermal_zone_device *tz, int mask)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * destroy_trip_attrs() - create attributes for trip points
> s/create/destroy
> 
> > 
> > + * @tz:		the thermal zone device
> > + *
> > + * helper function to free resources alocated by
> > create_trip_attrs()
> s/alocated/allocated
> 
as I have only two minor comments for this patch set, I will apply it
directly, with these two typos fixed.

thanks,
rui
> thanks,
> rui
> > 
> > + */
> > +static void destroy_trip_attrs(struct thermal_zone_device *tz)
> > +{
> > +	if (!tz)
> > +		return;
> > +
> > +	kfree(tz->trip_type_attrs);
> > +	kfree(tz->trip_temp_attrs);
> > +	if (tz->ops->get_trip_hyst)
> > +		kfree(tz->trip_hyst_attrs);
> > +	kfree(tz->trips_attribute_group.attrs);
> > +}
> > +
> >  int thermal_zone_create_device_groups(struct thermal_zone_device
> > *tz,
> >  				      int mask)
> >  {
> > @@ -637,6 +655,17 @@ int thermal_zone_create_device_groups(struct
> > thermal_zone_device *tz,
> >  	return 0;
> >  }
> >  
> > +void thermal_zone_destroy_device_groups(struct thermal_zone_device
> > *tz)
> > +{
> > +	if (!tz)
> > +		return;
> > +
> > +	if (tz->trips)
> > +		destroy_trip_attrs(tz);
> > +
> > +	kfree(tz->device.groups);
> > +}
> > +
> >  /* sys I/F for cooling device */
> >  static ssize_t
> >  thermal_cooling_device_type_show(struct device *dev,

Re: [PATCH 1/3 v2] thermal: core: Add some new helper functions to free resources

[walter harms]

Am 08.08.2017 16:39, schrieb Christophe JAILLET:
> In order to easily free resources allocated by
> 'thermal_zone_create_device_groups()' we need 2 new helper functions.
> 
> The first one undoes 'thermal_zone_create_device_groups()'.
> The 2nd one undoes 'create_trip_attrs()', which is a function called by
> 'thermal_zone_create_device_groups()'.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> These functions will be used in patch 2/3 in order to simplify
> 'thermal_release()'
> I've tried to implement it as close a possible as the way the resources have
> been allocated.
> However, in 'thermal_release()', the code is simplier without some
> additionnal 'if'.
> No sure if we should prefer consistancy with allocation or simplicity of code.
> ---
>  drivers/thermal/thermal_core.h  |  1 +
>  drivers/thermal/thermal_sysfs.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index 2412b3759e16..27e3b1df7360 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -71,6 +71,7 @@ int thermal_build_list_of_policies(char *buf);
>  
>  /* sysfs I/F */
>  int thermal_zone_create_device_groups(struct thermal_zone_device *, int);
> +void thermal_zone_destroy_device_groups(struct thermal_zone_device *);
>  void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *);
>  /* used only at binding time */
>  ssize_t
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index a694de907a26..eb95d64b9446 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -605,6 +605,24 @@ static int create_trip_attrs(struct thermal_zone_device *tz, int mask)
>  	return 0;
>  }
>  
> +/**
> + * destroy_trip_attrs() - create attributes for trip points
> + * @tz:		the thermal zone device
> + *
> + * helper function to free resources alocated by create_trip_attrs()
> + */
> +static void destroy_trip_attrs(struct thermal_zone_device *tz)
> +{
> +	if (!tz)
> +		return;
> +
> +	kfree(tz->trip_type_attrs);
> +	kfree(tz->trip_temp_attrs);
> +	if (tz->ops->get_trip_hyst)
> +		kfree(tz->trip_hyst_attrs);
> +	kfree(tz->trips_attribute_group.attrs);
> +}
> +
>  int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
>  				      int mask)
>  {
> @@ -637,6 +655,17 @@ int thermal_zone_create_device_groups(struct thermal_zone_device *tz,
>  	return 0;
>  }
>  
> +void thermal_zone_destroy_device_groups(struct thermal_zone_device *tz)
> +{
> +	if (!tz)
> +		return;
> +
> +	if (tz->trips)
> +		destroy_trip_attrs(tz);

why the check for tz->trips ?
destroy_trip_attrs does not access tz->trips->

re,
 wh

> +
> +	kfree(tz->device.groups);
> +}
> +
>  /* sys I/F for cooling device */
>  static ssize_t
>  thermal_cooling_device_type_show(struct device *dev,

Re: [PATCH 1/3 v2] thermal: core: Add some new helper functions to free resources

[Zhang Rui]

On Fri, 2017-08-11 at 09:30 +0200, walter harms wrote:
> 
> Am 08.08.2017 16:39, schrieb Christophe JAILLET:
> > 
> > In order to easily free resources allocated by
> > 'thermal_zone_create_device_groups()' we need 2 new helper
> > functions.
> > 
> > The first one undoes 'thermal_zone_create_device_groups()'.
> > The 2nd one undoes 'create_trip_attrs()', which is a function
> > called by
> > 'thermal_zone_create_device_groups()'.
> > 
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > These functions will be used in patch 2/3 in order to simplify
> > 'thermal_release()'
> > I've tried to implement it as close a possible as the way the
> > resources have
> > been allocated.
> > However, in 'thermal_release()', the code is simplier without some
> > additionnal 'if'.
> > No sure if we should prefer consistancy with allocation or
> > simplicity of code.
> > ---
> >  drivers/thermal/thermal_core.h  |  1 +
> >  drivers/thermal/thermal_sysfs.c | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+)
> > 
> > diff --git a/drivers/thermal/thermal_core.h
> > b/drivers/thermal/thermal_core.h
> > index 2412b3759e16..27e3b1df7360 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -71,6 +71,7 @@ int thermal_build_list_of_policies(char *buf);
> >  
> >  /* sysfs I/F */
> >  int thermal_zone_create_device_groups(struct thermal_zone_device
> > *, int);
> > +void thermal_zone_destroy_device_groups(struct thermal_zone_device
> > *);
> >  void thermal_cooling_device_setup_sysfs(struct
> > thermal_cooling_device *);
> >  /* used only at binding time */
> >  ssize_t
> > diff --git a/drivers/thermal/thermal_sysfs.c
> > b/drivers/thermal/thermal_sysfs.c
> > index a694de907a26..eb95d64b9446 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -605,6 +605,24 @@ static int create_trip_attrs(struct
> > thermal_zone_device *tz, int mask)
> >  	return 0;
> >  }
> >  
> > +/**
> > + * destroy_trip_attrs() - create attributes for trip points
> > + * @tz:		the thermal zone device
> > + *
> > + * helper function to free resources alocated by
> > create_trip_attrs()
> > + */
> > +static void(struct thermal_zone_device *tz)
> > +{
> > +	if (!tz)
> > +		return;
> > +
> > +	kfree(tz->trip_type_attrs);
> > +	kfree(tz->trip_temp_attrs);
> > +	if (tz->ops->get_trip_hyst)
> > +		kfree(tz->trip_hyst_attrs);
> > +	kfree(tz->trips_attribute_group.attrs);
> > +}
> > +
> >  int thermal_zone_create_device_groups(struct thermal_zone_device
> > *tz,
> >  				      int mask)
> >  {
> > @@ -637,6 +655,17 @@ int thermal_zone_create_device_groups(struct
> > thermal_zone_device *tz,
> >  	return 0;
> >  }
> >  
> > +void thermal_zone_destroy_device_groups(struct thermal_zone_device
> > *tz)
> > +{
> > +	if (!tz)
> > +		return;
> > +
> > +	if (tz->trips)
> > +		destroy_trip_attrs(tz);
> why the check for tz->trips ?
> destroy_trip_attrs does not access tz->trips->
> 
tz->trips is an integer represents the number of trip points.

We add this check because there is not any trips attributes if we don't
have any trip points.
It is true that the code also works if we don't have this check as
destroy_trip_attrs() would be no-op when tz->trips equals 0.
But I won't say this check is wrong.

thanks,
rui
> re,
>  wh
> 
> > 
> > +
> > +	kfree(tz->device.groups);
> > +}
> > +
> >  /* sys I/F for cooling device */
> >  static ssize_t
> >  thermal_cooling_device_type_show(struct device *dev,