| Message ID | 20220325073030.91919-1-kant@allwinnertech.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] thermal: devfreq_cooling: use local ops instead of global ops | expand |
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. Thanks!
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. > > Thanks!
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
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);
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(-)