Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1706630 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3 v2] thermal: core: Fix some error handling code in 'thermal_zone_device_register()' Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2017-08-08 16:50 +0200
    [PATCH 1/3 v2] thermal: core: Add some new helper functions to free resources Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2017-08-08 16:50 +0200
      Re: [PATCH 1/3 v2] thermal: core: Add some new helper functions to  free resources Zhang Rui <rui.zhang@intel.com> - 2017-08-11 05:30 +0200
        Re: [PATCH 1/3 v2] thermal: core: Add some new helper functions to  free resources Zhang Rui <rui.zhang@intel.com> - 2017-08-11 05:40 +0200
      Re: [PATCH 1/3 v2] thermal: core: Add some new helper functions to  free resources walter harms <wharms@bfs.de> - 2017-08-11 09:40 +0200
        Re: [PATCH 1/3 v2] thermal: core: Add some new helper functions to  free resources Zhang Rui <rui.zhang@intel.com> - 2017-08-11 10:30 +0200
    [PATCH 2/3 v2] thermal: core: Use the new 'thermal_zone_destroy_device_groups()' helper function Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2017-08-08 16:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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,

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web