Re: [PATCH] thermal/core: update cooling device during thermal zone unregistration

[Zhang, Rui]

On Tue, 2023-03-21 at 20:43 +0100, Rafael J. Wysocki wrote:
> On Tue, Mar 21, 2023 at 4:46 PM Zhang Rui <rui.zhang@xxxxxxxxx>
> wrote:
> > When unregistering a thermal zone device, update the cooling device
> > in
> > case the cooling device is activated by the current thermal zone.
> 
> I think that all cooling devices bound to the thermal zone being
> removed need to be updated at this point?  Which is what the patch
> really does IIUC.

yes, that is what I want to say.

> 
> It also avoids unbinding cooling devices that have not been bound to
> that zone.
> 
The thermal zone device driver' .unbind() callback should be able to
handle this. But still, this is a valid improvement.

> > This fixes a problem that the frequency of ACPI processors are
> > still
> > limited after unloading ACPI thermal driver while ACPI passive
> > cooling
> > is activated.
> > 
> 
> Cc: stable@vger ?

yeah, will add it.
> 
> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> > ---
> >  drivers/thermal/thermal_core.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/thermal/thermal_core.c
> > b/drivers/thermal/thermal_core.c
> > index cfd4c1afeae7..9f120e3c9bf0 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -1477,6 +1477,7 @@ void thermal_zone_device_unregister(struct
> > thermal_zone_device *tz)
> >         const struct thermal_zone_params *tzp;
> >         struct thermal_cooling_device *cdev;
> >         struct thermal_zone_device *pos = NULL;
> > +       struct thermal_instance *ti;
> > 
> >         if (!tz)
> >                 return;
> > @@ -1497,9 +1498,22 @@ void thermal_zone_device_unregister(struct
> > thermal_zone_device *tz)
> > 
> 
> I would rearrange the code as follows.
> 
> >         /* Unbind all cdevs associated with 'this' thermal zone */
> >         list_for_each_entry(cdev, &thermal_cdev_list, node) {
>                     struct thermal_instance *ti;
> 
> > +               mutex_lock(&tz->lock);
> > +               list_for_each_entry(ti, &tz->thermal_instances,
> > tz_node) {
> 
>                             if (ti->cdev == cdev) {
>                                     mutex_unlock(&tz->lock);
>                                     goto unbind;
>                             }
>                     }
>                     /* The cooling device is not used by this thermal
> zone. */
>                     mutex_unlock(&tz->lock);
>                     continue;
> 
> unbind:
> 
> >                 if (tz->ops->unbind) {
> >                         tz->ops->unbind(tz, cdev);
> 
>                             /*
>                              * The thermal instance for current
> thermal zone has been
>                              * removed, so update the cooling device
> in case it has been
>                              * activated by the thermal zone device
> going away.
>                              */
>                             mutex_lock(&cdev->lock);
>                             __thermal_cdev_update(cdev);
>                             mutex_unlock(&cdev->lock);
> 
>                             continue;
> >                 }

IMO, updating the cooling device is required, no matter the cooling
device is bound by .bind() callback or by static thermal_bind_params
structure.

Actually, I think struct thermal_bind_params is obsoleted and nobody is
using it now. I will write a cleanup patch to remove it after this one.

thanks,
rui
> > 
> >                 if (!tzp || !tzp->tbp)