Messages in this thread | | | From | "Zhang, Rui" <> | Subject | Re: [PATCH 3/5] thermal/core: Remove unneeded mutex_destroy() | Date | Fri, 20 Jan 2023 14:09:36 +0000 |
| |
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); > > >
| |