Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Tue, 24 Jan 2023 14:57:55 +0100 | Subject | Re: [PATCH v1 2/2] thermal: Fail object registration if thermal class is not registered |
| |
On Tue, Jan 24, 2023 at 7:03 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Jan 23, 2023 at 09:16:33PM +0100, Rafael J. Wysocki wrote: > > 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. > > Ok, so not a real problem then :) > > > 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. > > How would it retain memory if class_register() failed?
The point was that we might use the existing (but not registered) class object to "flag" the fact that the class could not be used without adding extra variables.
> > Oh well, we may as well just allocate it dynamically. > > Allocate what?
Well, that was a bit terse, sorry.
This patch implements what I meant: https://patchwork.kernel.org/project/linux-pm/patch/5660360.DvuYhMxLoT@kreacher/
| |