Re: [PATCH v3 1/8] thermal: core: Add mechanism for connecting trips with driver data

[Daniel Lezcano]

On 03/08/2023 16:15, Rafael J. Wysocki wrote:
> On Thu, Aug 3, 2023 at 3:06 PM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
> > On 02/08/2023 18:48, Rafael J. Wysocki wrote:
> >
> > [ ... ]
> >
> > > > Let me check if I can do something on top of your series to move it in
> > > > the ACPI driver.
> > >
> > > It doesn't need to be on top of my series, so if you have an idea,
> > > please just let me know what it is.
> > >
> > > It can't be entirely in the ACPI driver AFAICS, though, because
> > > trips[i] need to be modified on updates and they belong to the core.
> > > Hence, the driver needs some help from the core to get to them.  It
> > > can be something like "this is my trip tag and please give me the
> > > address of the trip matching it" or similar, but it is needed, because
> > > the driver has to assume that the trip indices used by it initially
> > > may change.
> >
> > May be I'm missing something but driver_ref does not seems to be used
> > except when assigning it, no?
>
> It is used on the other side.  That is, the value assigned to the trip
> field in it is accessed via trip_ref in the driver.
>
> The idea is that the driver puts a pointer to its local struct
> thermal_trip_ref into a struct thermal_trip and the core stores the
> address of that struct thermal_trip in there, which allows the driver
> to access the struct thermal_trip via its local struct
> thermal_trip_ref going forward.
>
> Admittedly, this is somewhat convoluted.
>
> I have an alternative approach in the works, just for illustration
> purposes if nothing else, but I have encountered a problem that I
> would like to ask you about.
>
> Namely, zone disabling is not particularly useful for preventing the
> zone from being used while the trips are updated, because it has side
> effects.  First, it triggers __thermal_zone_device_update() and a
> netlink message every time the mode changes, which can be kind of
> overcome. 

Right

> But second, if the mode is "disabled", it does not actually
> prevent things like __thermal_zone_get_trip() from running and the
> zone lock is the only thing that can be used for that AFAICS.
>
> So by "disabling" a thermal zone, did you mean changing its mode to
> "disabled" or something else?

Yes, that is what I meant.

May be the initial proposal by updating the thermal trips pointer can 
solve that [1]

IMO we can assume the trip point changes are very rare (if any), so 
rebuilding a new trip array and update the thermal zone with the pointer 
may solve the situation.

The routine does a copy of the trips array, so it can reorder it without 
impacting the array passed as a parameter. And it can take the lock.

We just have to constraint the update function to invalidate arrays with 
a number of trip points different from the one initially passed when 
creating the thermal zone.

Alternatively, we can be smarter in the ACPI driver and update the 
corresponding temperature+hysteresis trip point by using the 
thermal_zone_set_trip() function.

[1] 
https://lore.kernel.org/all/20230525140135.3589917-5-daniel.lezcano@xxxxxxxxxx/


--
<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