Messages in this thread | | | Date | Wed, 13 Apr 2022 16:41:26 +0100 | Subject | Re: [PATCH v2] thermal: devfreq_cooling: use local ops instead of global ops | From | Lukasz Luba <> |
| |
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@arm.com> 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@vger.kernel.org # 5.13+ >>>> Signed-off-by: Kant Fan <kant@allwinnertech.com> >>>> --- >>>> 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@arm.com> >> >> 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
| |