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