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

[Rafael J. Wysocki]

On Tue, Jan 24, 2023 at 7:03 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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@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?

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/