diff mbox series

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

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

Commit Message

Kant Fan March 25, 2022, 7:30 a.m. UTC
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(-)

Comments

Rafael J. Wysocki April 13, 2022, 2:58 p.m. UTC | #1
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!
Lukasz Luba April 13, 2022, 3:06 p.m. UTC | #2
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!
Lukasz Luba April 13, 2022, 3:41 p.m. UTC | #3
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 mbox series

Patch

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