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

[Kant Fan <kant@allwinnertech.com>]

On 14/03/2022 21:41, Lukasz Luba wrote:
> Hi Kant,
> 
> On 3/12/22 04:59, 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.
>>
>> Signed-off-by: Kant Fan <kant@allwinnertech.com>
> 
> Thank you for finding this issue. This was also an issue since the
> beginning of that code. The modified global ops after first registration
> which went through, was also previously there. Thus, we would need two
> different patches for stable kernels.
> 
> For this one, please add the tag:
> Fixes: 615510fe13bd2 ("thermal: devfreq_cooling: remove old power model 
> and use EM")
> 
> This patch would also go via stable tree for kernels v5.11+
> Please read the process how to send a patch which will be merged to the
> stable tree.
> 
> There will be a need to create another patch(es) for stable kernels with
> Fixes: a76caf55e5b35 ("thermal: Add devfreq cooling")
> In those kernels also the global ops is modified and might not support
> properly many cooling devices. It's present in other stable kernels:
> v5.10 and older
> 
>> ---
>>   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);
> 
> The fix looks good.

Hi Lukasz,
Thanks for your advice. According to that, I made two separate patches 
for mainline and the stable trees:
The first patch (patchwork.kernel.org: Message ID: 
20220325073030.91919-1-kant@allwinnertech.com) is for mainline. I added 
the 'fix' tag and 'Cc: stable@vger.kernel.org # 5.13+' to remind which 
stable trees should be back-ported.
The second patch (patchwork.kernel.org: Message ID: 
20220325094436.101419-1-kant@allwinnertech.com) is for stable tree v5.10 
and older. I added an upstream commit ID to indicate where the patch 
comes from. I also added 'Cc: stable@vger.kernel.org # 4.4+' to remind 
which stable trees should be back-ported.
Please check if they are correct. Thank you.

Kant Fan