[3/5] thermal/core: Remove unneeded mutex_destroy()

  If the thermal framework fails to initialize, the mutex can be used by
the different functions registering a thermal zone anyway. We should
not destroy the mutexes as other components may use them.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
  

On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> If the thermal framework fails to initialize, the mutex can be used
> by
> the different functions registering a thermal zone anyway.

Hmm, even with no governors and unregistered thermal sysfs class?

IMO, thermal APIs for registering a thermal_zone/cooling_device should
yield early if thermal_init fails.
For other APIs that relies on a valid
thermal_zone_device/thermal_cooling_device pointer, nothing needs to
be changed.

what do you think?

thanks,
rui


>  We should
> not destroy the mutexes as other components may use them.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index fad0c4a07d16..ea78c93277be 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1602,7 +1602,7 @@ static int __init thermal_init(void)
>  
>  	result = thermal_netlink_init();
>  	if (result)
> -		goto error;
> +		return result;
>  
>  	result = thermal_register_governors();
>  	if (result)
> @@ -1623,9 +1623,7 @@ static int __init thermal_init(void)
>  	thermal_unregister_governors();
>  unregister_netlink:
>  	thermal_netlink_exit();
> -error:
> -	mutex_destroy(&thermal_list_lock);
> -	mutex_destroy(&thermal_governor_lock);
> +
>  	return result;
>  }
>  postcore_initcall(thermal_init);
  

On 19/01/2023 08:41, Zhang, Rui wrote:
> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
>> If the thermal framework fails to initialize, the mutex can be used
>> by
>> the different functions registering a thermal zone anyway.
> 
> Hmm, even with no governors and unregistered thermal sysfs class?
> 
> IMO, thermal APIs for registering a thermal_zone/cooling_device should
> yield early if thermal_init fails.
> For other APIs that relies on a valid
> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> be changed.
> 
> what do you think?

I think you are right.

It would be nice if we can check if the thermal class is registered and 
bail out if not. But there is no function to check that AFAICS.

Alternatively we can convert the thermal class static structure to a 
pointer and set it to NULL in case of error in thermal_init() ?
  

On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/01/2023 08:41, Zhang, Rui wrote:
> > On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> >> If the thermal framework fails to initialize, the mutex can be used
> >> by
> >> the different functions registering a thermal zone anyway.
> >
> > Hmm, even with no governors and unregistered thermal sysfs class?
> >
> > IMO, thermal APIs for registering a thermal_zone/cooling_device should
> > yield early if thermal_init fails.
> > For other APIs that relies on a valid
> > thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> > be changed.
> >
> > what do you think?
>
> I think you are right.
>
> It would be nice if we can check if the thermal class is registered and
> bail out if not. But there is no function to check that AFAICS.
>
> Alternatively we can convert the thermal class static structure to a
> pointer and set it to NULL in case of error in thermal_init() ?

It doesn't matter if this is a NULL pointer or a static object that's
clearly marked as unused.
  

On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 19/01/2023 08:41, Zhang, Rui wrote:
>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
>>>> If the thermal framework fails to initialize, the mutex can be used
>>>> by
>>>> the different functions registering a thermal zone anyway.
>>>
>>> Hmm, even with no governors and unregistered thermal sysfs class?
>>>
>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
>>> yield early if thermal_init fails.
>>> For other APIs that relies on a valid
>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
>>> be changed.
>>>
>>> what do you think?
>>
>> I think you are right.
>>
>> It would be nice if we can check if the thermal class is registered and
>> bail out if not. But there is no function to check that AFAICS.
>>
>> Alternatively we can convert the thermal class static structure to a
>> pointer and set it to NULL in case of error in thermal_init() ?
> 
> It doesn't matter if this is a NULL pointer or a static object that's
> clearly marked as unused.

Without introducing another global variable, is it possible to know if 
the class is used or not ?
  

On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 19/01/2023 08:41, Zhang, Rui wrote:
> >>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> >>>> If the thermal framework fails to initialize, the mutex can be used
> >>>> by
> >>>> the different functions registering a thermal zone anyway.
> >>>
> >>> Hmm, even with no governors and unregistered thermal sysfs class?
> >>>
> >>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
> >>> yield early if thermal_init fails.
> >>> For other APIs that relies on a valid
> >>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> >>> be changed.
> >>>
> >>> what do you think?
> >>
> >> I think you are right.
> >>
> >> It would be nice if we can check if the thermal class is registered and
> >> bail out if not. But there is no function to check that AFAICS.
> >>
> >> Alternatively we can convert the thermal class static structure to a
> >> pointer and set it to NULL in case of error in thermal_init() ?
> >
> > It doesn't matter if this is a NULL pointer or a static object that's
> > clearly marked as unused.
>
> Without introducing another global variable, is it possible to know if
> the class is used or not ?

If thermal_class.p is cleared to NULL on class_register() failures in
thermal_init() (unfortunately, the driver core doesn't do that, but
maybe it should - let me cut a patch for that), then it can be used
for that.
  

On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 19/01/2023 08:41, Zhang, Rui wrote:
>>>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
>>>>>> If the thermal framework fails to initialize, the mutex can be used
>>>>>> by
>>>>>> the different functions registering a thermal zone anyway.
>>>>>
>>>>> Hmm, even with no governors and unregistered thermal sysfs class?
>>>>>
>>>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
>>>>> yield early if thermal_init fails.
>>>>> For other APIs that relies on a valid
>>>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
>>>>> be changed.
>>>>>
>>>>> what do you think?
>>>>
>>>> I think you are right.
>>>>
>>>> It would be nice if we can check if the thermal class is registered and
>>>> bail out if not. But there is no function to check that AFAICS.
>>>>
>>>> Alternatively we can convert the thermal class static structure to a
>>>> pointer and set it to NULL in case of error in thermal_init() ?
>>>
>>> It doesn't matter if this is a NULL pointer or a static object that's
>>> clearly marked as unused.
>>
>> Without introducing another global variable, is it possible to know if
>> the class is used or not ?
> 
> If thermal_class.p is cleared to NULL on class_register() failures in
> thermal_init() (unfortunately, the driver core doesn't do that, but
> maybe it should - let me cut a patch for that), then it can be used
> for that.

It should be in class_unregister() too, right ?

And is it possible to add a class_is_registered() ? in order to prevent 
accessing class structure internals ?
  

On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> >>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On 19/01/2023 08:41, Zhang, Rui wrote:
> >>>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> >>>>>> If the thermal framework fails to initialize, the mutex can be used
> >>>>>> by
> >>>>>> the different functions registering a thermal zone anyway.
> >>>>>
> >>>>> Hmm, even with no governors and unregistered thermal sysfs class?
> >>>>>
> >>>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
> >>>>> yield early if thermal_init fails.
> >>>>> For other APIs that relies on a valid
> >>>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> >>>>> be changed.
> >>>>>
> >>>>> what do you think?
> >>>>
> >>>> I think you are right.
> >>>>
> >>>> It would be nice if we can check if the thermal class is registered and
> >>>> bail out if not. But there is no function to check that AFAICS.
> >>>>
> >>>> Alternatively we can convert the thermal class static structure to a
> >>>> pointer and set it to NULL in case of error in thermal_init() ?
> >>>
> >>> It doesn't matter if this is a NULL pointer or a static object that's
> >>> clearly marked as unused.
> >>
> >> Without introducing another global variable, is it possible to know if
> >> the class is used or not ?
> >
> > If thermal_class.p is cleared to NULL on class_register() failures in
> > thermal_init() (unfortunately, the driver core doesn't do that, but
> > maybe it should - let me cut a patch for that), then it can be used
> > for that.
>
> It should be in class_unregister() too, right ?
>
> And is it possible to add a class_is_registered() ? in order to prevent
> accessing class structure internals ?

I suppose so.

And we'd like it to be used some places like
thermal_zone_device_register_with_trips(), wouldn't we?
  

On 19/01/2023 16:05, Rafael J. Wysocki wrote:
> On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
>>> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
>>>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>
>>>>>> On 19/01/2023 08:41, Zhang, Rui wrote:
>>>>>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
>>>>>>>> If the thermal framework fails to initialize, the mutex can be used
>>>>>>>> by
>>>>>>>> the different functions registering a thermal zone anyway.
>>>>>>>
>>>>>>> Hmm, even with no governors and unregistered thermal sysfs class?
>>>>>>>
>>>>>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
>>>>>>> yield early if thermal_init fails.
>>>>>>> For other APIs that relies on a valid
>>>>>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
>>>>>>> be changed.
>>>>>>>
>>>>>>> what do you think?
>>>>>>
>>>>>> I think you are right.
>>>>>>
>>>>>> It would be nice if we can check if the thermal class is registered and
>>>>>> bail out if not. But there is no function to check that AFAICS.
>>>>>>
>>>>>> Alternatively we can convert the thermal class static structure to a
>>>>>> pointer and set it to NULL in case of error in thermal_init() ?
>>>>>
>>>>> It doesn't matter if this is a NULL pointer or a static object that's
>>>>> clearly marked as unused.
>>>>
>>>> Without introducing another global variable, is it possible to know if
>>>> the class is used or not ?
>>>
>>> If thermal_class.p is cleared to NULL on class_register() failures in
>>> thermal_init() (unfortunately, the driver core doesn't do that, but
>>> maybe it should - let me cut a patch for that), then it can be used
>>> for that.
>>
>> It should be in class_unregister() too, right ?
>>
>> And is it possible to add a class_is_registered() ? in order to prevent
>> accessing class structure internals ?
> 
> I suppose so.
> 
> And we'd like it to be used some places like
> thermal_zone_device_register_with_trips(), wouldn't we?

Yes, in thermal_zone_device_register_with_trips() and 
thermal_cooling_device_register().
  

On Thursday, January 19, 2023 5:39:29 PM CET Daniel Lezcano wrote:
> On 19/01/2023 16:05, Rafael J. Wysocki wrote:
> > On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> >>> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> >>>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> >>>>> <daniel.lezcano@linaro.org> wrote:
> >>>>>>
> >>>>>> On 19/01/2023 08:41, Zhang, Rui wrote:
> >>>>>>> On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> >>>>>>>> If the thermal framework fails to initialize, the mutex can be used
> >>>>>>>> by
> >>>>>>>> the different functions registering a thermal zone anyway.
> >>>>>>>
> >>>>>>> Hmm, even with no governors and unregistered thermal sysfs class?
> >>>>>>>
> >>>>>>> IMO, thermal APIs for registering a thermal_zone/cooling_device should
> >>>>>>> yield early if thermal_init fails.
> >>>>>>> For other APIs that relies on a valid
> >>>>>>> thermal_zone_device/thermal_cooling_device pointer, nothing needs to
> >>>>>>> be changed.
> >>>>>>>
> >>>>>>> what do you think?
> >>>>>>
> >>>>>> I think you are right.
> >>>>>>
> >>>>>> It would be nice if we can check if the thermal class is registered and
> >>>>>> bail out if not. But there is no function to check that AFAICS.
> >>>>>>
> >>>>>> Alternatively we can convert the thermal class static structure to a
> >>>>>> pointer and set it to NULL in case of error in thermal_init() ?
> >>>>>
> >>>>> It doesn't matter if this is a NULL pointer or a static object that's
> >>>>> clearly marked as unused.
> >>>>
> >>>> Without introducing another global variable, is it possible to know if
> >>>> the class is used or not ?
> >>>
> >>> If thermal_class.p is cleared to NULL on class_register() failures in
> >>> thermal_init() (unfortunately, the driver core doesn't do that, but
> >>> maybe it should - let me cut a patch for that), then it can be used
> >>> for that.
> >>
> >> It should be in class_unregister() too, right ?
> >>
> >> And is it possible to add a class_is_registered() ? in order to prevent
> >> accessing class structure internals ?
> > 
> > I suppose so.
> > 
> > And we'd like it to be used some places like
> > thermal_zone_device_register_with_trips(), wouldn't we?
> 
> Yes, in thermal_zone_device_register_with_trips() and 
> thermal_cooling_device_register().

Something like the patch below I think, because thermal_cooling_device_register()
is a wrapper around thermal_zone_device_register_with_trips().

It needs to be split into 2 individual patches.

---
 drivers/base/class.c           |   16 +++++++++++-----
 drivers/thermal/thermal_core.c |    3 +++
 include/linux/device/class.h   |    5 +++++
 3 files changed, 19 insertions(+), 5 deletions(-)

Index: linux-pm/include/linux/device/class.h
===================================================================
--- linux-pm.orig/include/linux/device/class.h
+++ linux-pm/include/linux/device/class.h
@@ -82,6 +82,11 @@ struct class_dev_iter {
 	const struct device_type	*type;
 };
 
+static inline bool class_is_registered(struct class *class)
+{
+	return !!class->p;
+}
+
 extern struct kobject *sysfs_dev_block_kobj;
 extern struct kobject *sysfs_dev_char_kobj;
 extern int __must_check __class_register(struct class *class,
Index: linux-pm/drivers/base/class.c
===================================================================
--- linux-pm.orig/drivers/base/class.c
+++ linux-pm/drivers/base/class.c
@@ -53,6 +53,8 @@ static void class_release(struct kobject
 
 	pr_debug("class '%s': release.\n", class->name);
 
+	class->p = NULL;
+
 	if (class->class_release)
 		class->class_release(class);
 	else
@@ -186,17 +188,21 @@ int __class_register(struct class *cls,
 	cls->p = cp;
 
 	error = kset_register(&cp->subsys);
-	if (error) {
-		kfree(cp);
-		return error;
-	}
+	if (error)
+		goto err_out;
+
 	error = class_add_groups(class_get(cls), cls->class_groups);
 	class_put(cls);
 	if (error) {
 		kobject_del(&cp->subsys.kobj);
 		kfree_const(cp->subsys.kobj.name);
-		kfree(cp);
+		goto err_out;
 	}
+	return 0;
+
+err_out:
+	cls->p = NULL;
+	kfree(cp);
 	return error;
 }
 EXPORT_SYMBOL_GPL(__class_register);
Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -1342,6 +1342,9 @@ thermal_zone_device_register_with_trips(
 	if (num_trips > 0 && (!ops->get_trip_type || !ops->get_trip_temp) && !trips)
 		return ERR_PTR(-EINVAL);
 
+	if (!class_is_registered(&thermal_class))
+		return ERR_PTR(-ENODEV);
+
 	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
 	if (!tz)
 		return ERR_PTR(-ENOMEM);
  

On Thu, 2023-01-19 at 18:21 +0100, Rafael J. Wysocki wrote:
> On Thursday, January 19, 2023 5:39:29 PM CET Daniel Lezcano wrote:
> > On 19/01/2023 16:05, Rafael J. Wysocki wrote:
> > > On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
> > > <daniel.lezcano@linaro.org> wrote:
> > > > On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > > > > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > > > > > > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > > > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > > > On 19/01/2023 08:41, Zhang, Rui wrote:
> > > > > > > > > On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano
> > > > > > > > > wrote:
> > > > > > > > > > If the thermal framework fails to initialize, the
> > > > > > > > > > mutex can be used
> > > > > > > > > > by
> > > > > > > > > > the different functions registering a thermal zone
> > > > > > > > > > anyway.
> > > > > > > > > 
> > > > > > > > > Hmm, even with no governors and unregistered thermal
> > > > > > > > > sysfs class?
> > > > > > > > > 
> > > > > > > > > IMO, thermal APIs for registering a
> > > > > > > > > thermal_zone/cooling_device should
> > > > > > > > > yield early if thermal_init fails.
> > > > > > > > > For other APIs that relies on a valid
> > > > > > > > > thermal_zone_device/thermal_cooling_device pointer,
> > > > > > > > > nothing needs to
> > > > > > > > > be changed.
> > > > > > > > > 
> > > > > > > > > what do you think?
> > > > > > > > 
> > > > > > > > I think you are right.
> > > > > > > > 
> > > > > > > > It would be nice if we can check if the thermal class
> > > > > > > > is registered and
> > > > > > > > bail out if not. But there is no function to check that
> > > > > > > > AFAICS.
> > > > > > > > 
> > > > > > > > Alternatively we can convert the thermal class static
> > > > > > > > structure to a
> > > > > > > > pointer and set it to NULL in case of error in
> > > > > > > > thermal_init() ?
> > > > > > > 
> > > > > > > It doesn't matter if this is a NULL pointer or a static
> > > > > > > object that's
> > > > > > > clearly marked as unused.
> > > > > > 
> > > > > > Without introducing another global variable, is it possible
> > > > > > to know if
> > > > > > the class is used or not ?
> > > > > 
> > > > > If thermal_class.p is cleared to NULL on class_register()
> > > > > failures in
> > > > > thermal_init() (unfortunately, the driver core doesn't do
> > > > > that, but
> > > > > maybe it should - let me cut a patch for that), then it can
> > > > > be used
> > > > > for that.
> > > > 
> > > > It should be in class_unregister() too, right ?
> > > > 
> > > > And is it possible to add a class_is_registered() ? in order to
> > > > prevent
> > > > accessing class structure internals ?
> > > 
> > > I suppose so.
> > > 
> > > And we'd like it to be used some places like
> > > thermal_zone_device_register_with_trips(), wouldn't we?
> > 
> > Yes, in thermal_zone_device_register_with_trips() and 
> > thermal_cooling_device_register().
> 
> Something like the patch below I think, because
> thermal_cooling_device_register()
> is a wrapper around thermal_zone_device_register_with_trips().
> 

thermal_zone_device_register() is a wrapper around
thermal_zone_device_register_with_trips(), but
thermal_cooling_device_register() is not. :)

thermal_cooling_device_register() registers a cooling device to thermal
class so the class_is_registered() check is still needed.

thanks,
rui

> It needs to be split into 2 individual patches.
> 
> ---
>  drivers/base/class.c           |   16 +++++++++++-----
>  drivers/thermal/thermal_core.c |    3 +++
>  include/linux/device/class.h   |    5 +++++
>  3 files changed, 19 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/include/linux/device/class.h
> ===================================================================
> --- linux-pm.orig/include/linux/device/class.h
> +++ linux-pm/include/linux/device/class.h
> @@ -82,6 +82,11 @@ struct class_dev_iter {
>  	const struct device_type	*type;
>  };
>  
> +static inline bool class_is_registered(struct class *class)
> +{
> +	return !!class->p;
> +}
> +
>  extern struct kobject *sysfs_dev_block_kobj;
>  extern struct kobject *sysfs_dev_char_kobj;
>  extern int __must_check __class_register(struct class *class,
> Index: linux-pm/drivers/base/class.c
> ===================================================================
> --- linux-pm.orig/drivers/base/class.c
> +++ linux-pm/drivers/base/class.c
> @@ -53,6 +53,8 @@ static void class_release(struct kobject
>  
>  	pr_debug("class '%s': release.\n", class->name);
>  
> +	class->p = NULL;
> +
>  	if (class->class_release)
>  		class->class_release(class);
>  	else
> @@ -186,17 +188,21 @@ int __class_register(struct class *cls,
>  	cls->p = cp;
>  
>  	error = kset_register(&cp->subsys);
> -	if (error) {
> -		kfree(cp);
> -		return error;
> -	}
> +	if (error)
> +		goto err_out;
> +
>  	error = class_add_groups(class_get(cls), cls->class_groups);
>  	class_put(cls);
>  	if (error) {
>  		kobject_del(&cp->subsys.kobj);
>  		kfree_const(cp->subsys.kobj.name);
> -		kfree(cp);
> +		goto err_out;
>  	}
> +	return 0;
> +
> +err_out:
> +	cls->p = NULL;
> +	kfree(cp);
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(__class_register);
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -1342,6 +1342,9 @@ thermal_zone_device_register_with_trips(
>  	if (num_trips > 0 && (!ops->get_trip_type || !ops-
> >get_trip_temp) && !trips)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (!class_is_registered(&thermal_class))
> +		return ERR_PTR(-ENODEV);
> +
>  	tz = kzalloc(sizeof(*tz), GFP_KERNEL);
>  	if (!tz)
>  		return ERR_PTR(-ENOMEM);
> 
> 
>
  

On Fri, Jan 20, 2023 at 3:10 PM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Thu, 2023-01-19 at 18:21 +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 19, 2023 5:39:29 PM CET Daniel Lezcano wrote:
> > > On 19/01/2023 16:05, Rafael J. Wysocki wrote:
> > > > On Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
> > > > <daniel.lezcano@linaro.org> wrote:
> > > > > On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > > > > > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > > On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > > > > > > > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > > > > > > > <daniel.lezcano@linaro.org> wrote:
> > > > > > > > > On 19/01/2023 08:41, Zhang, Rui wrote:
> > > > > > > > > > On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano
> > > > > > > > > > wrote:
> > > > > > > > > > > If the thermal framework fails to initialize, the
> > > > > > > > > > > mutex can be used
> > > > > > > > > > > by
> > > > > > > > > > > the different functions registering a thermal zone
> > > > > > > > > > > anyway.
> > > > > > > > > >
> > > > > > > > > > Hmm, even with no governors and unregistered thermal
> > > > > > > > > > sysfs class?
> > > > > > > > > >
> > > > > > > > > > IMO, thermal APIs for registering a
> > > > > > > > > > thermal_zone/cooling_device should
> > > > > > > > > > yield early if thermal_init fails.
> > > > > > > > > > For other APIs that relies on a valid
> > > > > > > > > > thermal_zone_device/thermal_cooling_device pointer,
> > > > > > > > > > nothing needs to
> > > > > > > > > > be changed.
> > > > > > > > > >
> > > > > > > > > > what do you think?
> > > > > > > > >
> > > > > > > > > I think you are right.
> > > > > > > > >
> > > > > > > > > It would be nice if we can check if the thermal class
> > > > > > > > > is registered and
> > > > > > > > > bail out if not. But there is no function to check that
> > > > > > > > > AFAICS.
> > > > > > > > >
> > > > > > > > > Alternatively we can convert the thermal class static
> > > > > > > > > structure to a
> > > > > > > > > pointer and set it to NULL in case of error in
> > > > > > > > > thermal_init() ?
> > > > > > > >
> > > > > > > > It doesn't matter if this is a NULL pointer or a static
> > > > > > > > object that's
> > > > > > > > clearly marked as unused.
> > > > > > >
> > > > > > > Without introducing another global variable, is it possible
> > > > > > > to know if
> > > > > > > the class is used or not ?
> > > > > >
> > > > > > If thermal_class.p is cleared to NULL on class_register()
> > > > > > failures in
> > > > > > thermal_init() (unfortunately, the driver core doesn't do
> > > > > > that, but
> > > > > > maybe it should - let me cut a patch for that), then it can
> > > > > > be used
> > > > > > for that.
> > > > >
> > > > > It should be in class_unregister() too, right ?
> > > > >
> > > > > And is it possible to add a class_is_registered() ? in order to
> > > > > prevent
> > > > > accessing class structure internals ?
> > > >
> > > > I suppose so.
> > > >
> > > > And we'd like it to be used some places like
> > > > thermal_zone_device_register_with_trips(), wouldn't we?
> > >
> > > Yes, in thermal_zone_device_register_with_trips() and
> > > thermal_cooling_device_register().
> >
> > Something like the patch below I think, because
> > thermal_cooling_device_register()
> > is a wrapper around thermal_zone_device_register_with_trips().
> >
>
> thermal_zone_device_register() is a wrapper around
> thermal_zone_device_register_with_trips(), but
> thermal_cooling_device_register() is not. :)
>
> thermal_cooling_device_register() registers a cooling device to thermal
> class so the class_is_registered() check is still needed.

OK, thanks!