Re: [PATCH v1 2/2] thermal: Fail object registration if thermal class is not registered

[Greg Kroah-Hartman]

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@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Fri, Jan 20, 2023 at 08:48:07PM +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > ---
> > >  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?

> Oh well, we may as well just allocate it dynamically.

Allocate what?

confused,

greg k-h