Messages in this thread Patch in this message | | | From | "Rafael J. Wysocki" <> | Subject | Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy() | Date | Thu, 19 Jan 2023 18:21:00 +0100 |
| |
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);
| |