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 <[email protected]>
---
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);
--
2.34.1
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() ?
--
<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, Jan 19, 2023 at 10:30 AM Daniel Lezcano
<[email protected]> 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
> <[email protected]> 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 ?
--
<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 19/01/2023 14:24, Rafael J. Wysocki wrote:
> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
>>> <[email protected]> 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 ?
--
<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, Jan 19, 2023 at 1:48 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > <[email protected]> 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 Thu, Jan 19, 2023 at 3:13 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> >>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> >>> <[email protected]> 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
> <[email protected]> wrote:
>>
>> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
>>> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
>>> <[email protected]> wrote:
>>>>
>>>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
>>>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
>>>>> <[email protected]> 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().
--
<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 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
> > <[email protected]> wrote:
> >>
> >> On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> >>> On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> >>>>> On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> >>>>> <[email protected]> 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 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 <[email protected]>
> ---
> 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 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
> > > <[email protected]> wrote:
> > > > On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > > > > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > > > > <[email protected]> wrote:
> > > > > > On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > > > > > > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > > > > > > <[email protected]> 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 <[email protected]> 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
> > > > <[email protected]> wrote:
> > > > > On 19/01/2023 14:24, Rafael J. Wysocki wrote:
> > > > > > On Thu, Jan 19, 2023 at 1:48 PM Daniel Lezcano
> > > > > > <[email protected]> wrote:
> > > > > > > On 19/01/2023 13:11, Rafael J. Wysocki wrote:
> > > > > > > > On Thu, Jan 19, 2023 at 10:30 AM Daniel Lezcano
> > > > > > > > <[email protected]> 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!