Re: [PATCH v2] thermal: devfreq_cooling: use local ops instead of global ops

[Lukasz Luba <lukasz.luba@xxxxxxx>]

On 4/13/22 16:06, Lukasz Luba wrote:
> On 4/13/22 15:58, Rafael J. Wysocki wrote:
> > On Fri, Mar 25, 2022 at 10:02 AM Lukasz Luba <lukasz.luba@xxxxxxx> wrote:
> > > Hi Kant,
> > >
> > > On 3/25/22 07:30, Kant Fan wrote:
> > > > Fix access illegal address problem in following condition:
> > > > There are muti devfreq cooling devices in system, some of them has
> > > > em model but other does not, energy model ops such as state2power will
> > > > append to global devfreq_cooling_ops when the cooling device with
> > > > em model register. It makes the cooling device without em model
> > > > also use devfreq_cooling_ops after appending when register later by
> > > > of_devfreq_cooling_register_power() or of_devfreq_cooling_register().
> > > >
> > > > IPA governor regards the cooling devices without em model as a power 
> > > > actor
> > > > because they also have energy model ops, and will access illegal 
> > > > address
> > > > at dfc->em_pd when execute cdev->ops->get_requested_power,
> > > > cdev->ops->state2power or cdev->ops->power2state.
> > > >
> > > > Fixes: 615510fe13bd2 ("thermal: devfreq_cooling: remove old power 
> > > > model and use EM")
> > > > Cc: stable@xxxxxxxxxxxxxxx # 5.13+
> > > > Signed-off-by: Kant Fan <kant@xxxxxxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/thermal/devfreq_cooling.c | 25 ++++++++++++++++++-------
> > > >    1 file changed, 18 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/thermal/devfreq_cooling.c 
> > > > b/drivers/thermal/devfreq_cooling.c
> > > > index 4310cb342a9f..d38a80adec73 100644
> > > > --- a/drivers/thermal/devfreq_cooling.c
> > > > +++ b/drivers/thermal/devfreq_cooling.c
> > > > @@ -358,21 +358,28 @@ of_devfreq_cooling_register_power(struct 
> > > > device_node *np, struct devfreq *df,
> > > >        struct thermal_cooling_device *cdev;
> > > >        struct device *dev = df->dev.parent;
> > > >        struct devfreq_cooling_device *dfc;
> > > > +     struct thermal_cooling_device_ops *ops;
> > > >        char *name;
> > > >        int err, num_opps;
> > > >
> > > > -     dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
> > > > -     if (!dfc)
> > > > +     ops = kmemdup(&devfreq_cooling_ops, sizeof(*ops), GFP_KERNEL);
> > > > +     if (!ops)
> > > >                return ERR_PTR(-ENOMEM);
> > > >
> > > > +     dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
> > > > +     if (!dfc) {
> > > > +             err = -ENOMEM;
> > > > +             goto free_ops;
> > > > +     }
> > > > +
> > > >        dfc->devfreq = df;
> > > >
> > > >        dfc->em_pd = em_pd_get(dev);
> > > >        if (dfc->em_pd) {
> > > > -             devfreq_cooling_ops.get_requested_power =
> > > > +             ops->get_requested_power =
> > > >                        devfreq_cooling_get_requested_power;
> > > > -             devfreq_cooling_ops.state2power = 
> > > > devfreq_cooling_state2power;
> > > > -             devfreq_cooling_ops.power2state = 
> > > > devfreq_cooling_power2state;
> > > > +             ops->state2power = devfreq_cooling_state2power;
> > > > +             ops->power2state = devfreq_cooling_power2state;
> > > >
> > > >                dfc->power_ops = dfc_power;
> > > >
> > > > @@ -407,8 +414,7 @@ of_devfreq_cooling_register_power(struct 
> > > > device_node *np, struct devfreq *df,
> > > >        if (!name)
> > > >                goto remove_qos_req;
> > > >
> > > > -     cdev = thermal_of_cooling_device_register(np, name, dfc,
> > > > -                                               &devfreq_cooling_ops);
> > > > +     cdev = thermal_of_cooling_device_register(np, name, dfc, ops);
> > > >        kfree(name);
> > > >
> > > >        if (IS_ERR(cdev)) {
> > > > @@ -429,6 +435,8 @@ of_devfreq_cooling_register_power(struct 
> > > > device_node *np, struct devfreq *df,
> > > >        kfree(dfc->freq_table);
> > > >    free_dfc:
> > > >        kfree(dfc);
> > > > +free_ops:
> > > > +     kfree(ops);
> > > >
> > > >        return ERR_PTR(err);
> > > >    }
> > > > @@ -510,11 +518,13 @@ EXPORT_SYMBOL_GPL(devfreq_cooling_em_register);
> > > >    void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
> > > >    {
> > > >        struct devfreq_cooling_device *dfc;
> > > > +     const struct thermal_cooling_device_ops *ops;
> > > >        struct device *dev;
> > > >
> > > >        if (IS_ERR_OR_NULL(cdev))
> > > >                return;
> > > >
> > > > +     ops = cdev->ops;
> > > >        dfc = cdev->devdata;
> > > >        dev = dfc->devfreq->dev.parent;
> > > >
> > > > @@ -525,5 +535,6 @@ void devfreq_cooling_unregister(struct 
> > > > thermal_cooling_device *cdev)
> > > >
> > > >        kfree(dfc->freq_table);
> > > >        kfree(dfc);
> > > > +     kfree(ops);
> > > >    }
> > > >    EXPORT_SYMBOL_GPL(devfreq_cooling_unregister);
> > >
> > >
> > > Thank you for updating it, LGTM
> > >
> > > Reviewed-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> >
> > Applied as 5.19 material.
> >
> > Lukasz, this had a conflict with your EM series, please double check
> > if my resolution in the bleeding-edge branch is correct.
>
> OK, I'll let you know after I fetch and build that branch.

I've read the code and confirm you've do this correctly.
I've also built that branch with ENERGY_MODEL and DEVFREQ_COOLING
configs set - no issues observed.
Later this week I would use it for some other development
so I will test it as well.

Thank you for solving this!

Regards,
Lukasz