Re: [PATCH v2 2/7] thermal/core: Encapsulate tz->device field

[Daniel Lezcano]

On 12/04/2023 20:56, Rafael J. Wysocki wrote:
> On Tue, Apr 11, 2023 at 10:20 PM Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx> wrote:
> > On 11/04/2023 20:19, Rafael J. Wysocki wrote:
> > > On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
> > > <daniel.lezcano@xxxxxxxxxx> wrote:
> > > > There are still some drivers needing to play with the thermal zone
> > > > device internals. That is not the best but until we can figure out if
> > > > the information is really needed, let's encapsulate the field used in
> > > > the thermal zone device structure, so we can move forward relocating
> > > > the thermal zone device structure definition in the thermal framework
> > > > private headers.
> > >
> > > I'm not really sure why this is needed, so please explain.
> >
> > Some drivers are accessing tz->device, that implies they have the
> > knowledge of the thermal_zone_device structure but we want to
> > self-encapsulate this structure and reduce the scope of the structure to
> > the thermal core only.
> >
> > The ACPI and the Menlon drivers are the drivers accessing tz->device.
> >
> > By adding this wrapper, these drivers do no longer need the thermal zone
> > device structure definition.
>
> So you want to move the definition of struct thermal_zone_device from
> include/linux/thermal.h into a local header in drivers/thermal/ and
> make it entirely local to the thermal core IIUC.
>
> Which would be forcing the callers of
> thermal_zone_device_register_with_trips() (and friends) to use
> pointers to a data type that's not completely defined (from their
> perspective), but they would still have access to the trips array
> passed to that function.
>
> That doesn't sound particularly consistent and what's the purpose of doing it?
The first thing is prevent drivers tampering with the thermal core 
structure internals, so moving the structures from 
include/linux/thermal.h to drivers/thermal/thermal_core.h is one step to 
this direction.

As you point it, drivers can still do things with the trip arrays passed 
as parameter. For that the idea is to do the same as:

https://lore.kernel.org/all/20230404075138.2914680-3-daniel.lezcano@xxxxxxxxxx/

So the array is an initialization data and it will stay private to the 
thermal core code.

In order to update the trip points, we add a 
thermal_zone_device_trips_update() function which does a kmemdup, 
pointer switch, kfree of the previous trips and a notification.

I addressed that in response to your ACPI series questions:

https://lore.kernel.org/all/38a5a6e0-2af8-8365-b20e-8494a4efcb0c@xxxxxxxxxx/

Does it make sense with this additional information ?


> > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > > > ---
> > > >    drivers/thermal/thermal_core.c | 6 ++++++
> > > >    include/linux/thermal.h        | 1 +
> > > >    2 files changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > > > index c5025aca22ee..842f678c1c3e 100644
> > > > --- a/drivers/thermal/thermal_core.c
> > > > +++ b/drivers/thermal/thermal_core.c
> > > > @@ -1398,6 +1398,12 @@ int thermal_zone_device_id(struct thermal_zone_device *tzd)
> > > >    }
> > > >    EXPORT_SYMBOL_GPL(thermal_zone_device_id);
> > > >
> > > > +struct device *thermal_zone_device(struct thermal_zone_device *tzd)
> > > > +{
> > > > +       return &tzd->device;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(thermal_zone_device);
> > > > +
> > > >    /**
> > > >     * thermal_zone_device_unregister - removes the registered thermal zone device
> > > >     * @tz: the thermal zone device to remove
> > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > > > index 82ddb32f9876..87837094d549 100644
> > > > --- a/include/linux/thermal.h
> > > > +++ b/include/linux/thermal.h
> > > > @@ -313,6 +313,7 @@ thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int
> > > >    void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
> > > >    const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
> > > >    int thermal_zone_device_id(struct thermal_zone_device *tzd);
> > > > +struct device *thermal_zone_device(struct thermal_zone_device *tzd);
> > > >
> > > >    int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
> > > >                                        struct thermal_cooling_device *,
> > > > --
> > > > 2.34.1
> >
> > --
> > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> >
> > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > <http://twitter.com/#!/linaroorg> Twitter |
> > <http://www.linaro.org/linaro-blog/> Blog

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog