Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Mon, 23 Jan 2023 21:16:33 +0100 | Subject | Re: [PATCH v1 2/2] thermal: Fail object registration if thermal class is not registered |
| |
On Sat, Jan 21, 2023 at 8:40 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, Jan 20, 2023 at 08:48:07PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If thermal_class is not registered with the driver core, there is no way > > to expose the interfaces used by the thermal control framework, so > > prevent thermal zones and cooling devices from being registered in > > that case by returning an error from object registration functions. > > > > For this purpose, introduce class_is_registered() that checks the > > private pointer of the given class and returns 'false' if it is NULL, > > which means that the class has not been registered, and use it in the > > thermal framework. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/thermal/thermal_core.c | 6 ++++++ > > include/linux/device/class.h | 5 +++++ > > 2 files changed, 11 insertions(+) > > > > 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; > > I really do not like this as it is exposing internals to drivers and > whenever we do that, it gets abused and we have to unwind the mess in a > few years. > > Overall, I'm trying to remove the ->p usage, but that's a longterm goal > of mine (to allow class and bus structures to be in read-only memory), > which isn't your issue here, but it's good to think about why you want > to know this information (more below.) > > > +} > > + > > 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/thermal/thermal_core.c > > =================================================================== > > --- linux-pm.orig/drivers/thermal/thermal_core.c > > +++ linux-pm/drivers/thermal/thermal_core.c > > @@ -880,6 +880,9 @@ __thermal_cooling_device_register(struct > > !ops->set_cur_state) > > return ERR_PTR(-EINVAL); > > > > + if (!class_is_registered(&thermal_class)) > > + return ERR_PTR(-ENODEV); > > If the class isn't registered, then sommething went wrong with the > thermal core code, right? So why isn't the thermal core keeping a local > variable of "class was registered" and relying on the driver core to > know this? > > The number of individual users that should be doing one thing or another > if a class is not registered feels very very slim. How come this code > is being called at all if the thermal class was not registered in the > first place? What would have prevented that from happening? Is it an > ordering issue, or a kernel configuration issue?
It's basically a matter of class_register() returning an error.
Yes, we could use an extra variable for this purpose, but that would be a bit wasteful, because thermal_class will then sit unused and occupy memory in vain.
Oh well, we may as well just allocate it dynamically.
| |