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

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

[Kant Fan]

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

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

[Lukasz Luba]

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.

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

[Kant Fan]

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

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

[Lukasz Luba]

On 3/25/22 10:43, Kant Fan wrote:
> 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.

Thank you for sending them. Let me have a look.

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

[Kant Fan]

On 20/04/2022 18:32, Lukasz Luba wrote:
> Hi Kant,
> 
> On 4/19/22 16:49, Kant Fan wrote:
>> On 29/03/2022 14:59, Lukasz Luba wrote:
>>>
>>>
>>> On 3/25/22 09:44, Kant Fan wrote:
>>>> commit 7b62935828266658714f81d4e9176edad808dc70 upstream.
>>>>
>>>> Fix access illegal address problem in following condition:
>>>> There are muti devfreq cooling devices in system, some of them register
>>>> with dfc_power but other does not, power model ops such as 
>>>> state2power will
>>>> append to global devfreq_cooling_ops when the cooling device with
>>>> dfc_power register. It makes the cooling device without dfc_power
>>>> 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 dfc_power as a 
>>>> power actor
>>>> because they also have power model ops, and will access illegal 
>>>> address at
>>>> dfc->power_ops when execute cdev->ops->get_requested_power or
>>>> cdev->ops->power2state. As the calltrace below shows:
>>>>
>>>> Unable to handle kernel NULL pointer dereference at virtual address
>>>> 00000008
>>>> ...
>>>> calltrace:
>>>> [<c06e5488>] devfreq_cooling_power2state+0x24/0x184
>>>> [<c06df420>] power_actor_set_power+0x54/0xa8
>>>> [<c06e3774>] power_allocator_throttle+0x770/0x97c
>>>> [<c06dd120>] handle_thermal_trip+0x1b4/0x26c
>>>> [<c06ddb48>] thermal_zone_device_update+0x154/0x208
>>>> [<c014159c>] process_one_work+0x1ec/0x36c
>>>> [<c0141c58>] worker_thread+0x204/0x2ec
>>>> [<c0146788>] kthread+0x140/0x154
>>>> [<c01010e8>] ret_from_fork+0x14/0x2c
>>>>
>>>> Fixes: a76caf55e5b35 ("thermal: Add devfreq cooling")
>>>> Cc: stable@vger.kernel.org # 4.4+
>>>> Signed-off-by: Kant Fan <kant@allwinnertech.com>
>>>> ---
>>>>   drivers/thermal/devfreq_cooling.c | 25 ++++++++++++++++++-------
>>>>   1 file changed, 18 insertions(+), 7 deletions(-)
>>>>
>>>
>>> Looks good. So this patch should be applied for all stable
>>> kernels starting from v4.4 to v5.12 (the v5.13 and later need
>>> other patch).
>>>
>>> Next time you might use in the subject something like:
>>> [PATCH 4.4] thermal: devfreq_cooling: use local ops instead of global 
>>> ops
>>> It would be better distinguished from your other patch with the
>>> same subject, which was for mainline and v5.13+
>>
>> Hi Lukasz,
>> Thank you for the guidance. I want to know if I'm understanding you in 
>> a right way. Could you confirm the following information?
>>
>> 1. The stable patches
>> After the patch is merged into mainline later, I'll submit the 
>> following patches individually for v4.4 ~ v5.12:
> 
> Correct, after it gets mainline you can point to that commit hash and
> process with those patches. I don't now which of those older stable
> kernels are still maintained, since some of them have longer support
> and the rest had shorter and might already ended. You can check the
> end of life for those 'Longterm' here [1]. AFAICS the 4.4 is not in that
> table, so you can start from 4.9, should be OK.
> So the list of needed patches would be for those stable kernels:
> 4.9, 4.14, 4.19, 5.4, 5.10
> I can see that last release for 5.11.x was in May 2021, so it's probably
> ended, similar for 5.12.x (Jul 2021). That's why I suggested that list
> for the long support kernels.
> 

Hi Lukasz,
Thanks for figuring it out. I'll check the stable versions carefully.

>>
>> [PATCH 4.4] thermal: devfreq_cooling: use local ops instead of global ops
>> [PATCH 4.5] thermal: devfreq_cooling: use local ops instead of global ops
>> ...
>> [PATCH 5.12] thermal: devfreq_cooling: use local ops instead of global 
>> ops
>>
>> And also the following patches individually for v5.13+ :
> 
> For this, you probably don't have to. You have added 'v5.13+' in the
> original patch v2, so it will be picked correctly. It should apply
> on those stable kernels w/o issues. If there will be, stable kernel
> engineers will ping us.
> 
>> [PATCH 5.13] thermal: devfreq_cooling: use local ops instead of global 
>> ops
>> [PATCH 5.14] thermal: devfreq_cooling: use local ops instead of global 
>> ops
>> ...
>> [PATCH 5.17] thermal: devfreq_cooling: use local ops instead of global 
>> ops
>>
>> 2. The mainline patch
>> I saw your mail with Rafael, seems there are conflicts... I wonder if 
>> there's anything wrong with my patch, or anything I can help?
>>
> 
> Thank you for offering help. Rafael solved that correctly, so it doesn't
> need any more work.
> 
> Thank you for doing that work!
> 
> Regards,
> Lukasz
> 
> [1] https://www.kernel.org/category/releases.html

No problem. I'll submit the stable patches after the mainline patch is 
merged.

-- 
Best Regards,
Kant Fan

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

[Lukasz Luba]

Hi Kant,

On 4/19/22 16:49, Kant Fan wrote:
> On 29/03/2022 14:59, Lukasz Luba wrote:
>>
>>
>> On 3/25/22 09:44, Kant Fan wrote:
>>> commit 7b62935828266658714f81d4e9176edad808dc70 upstream.
>>>
>>> Fix access illegal address problem in following condition:
>>> There are muti devfreq cooling devices in system, some of them register
>>> with dfc_power but other does not, power model ops such as 
>>> state2power will
>>> append to global devfreq_cooling_ops when the cooling device with
>>> dfc_power register. It makes the cooling device without dfc_power
>>> 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 dfc_power as a power 
>>> actor
>>> because they also have power model ops, and will access illegal 
>>> address at
>>> dfc->power_ops when execute cdev->ops->get_requested_power or
>>> cdev->ops->power2state. As the calltrace below shows:
>>>
>>> Unable to handle kernel NULL pointer dereference at virtual address
>>> 00000008
>>> ...
>>> calltrace:
>>> [<c06e5488>] devfreq_cooling_power2state+0x24/0x184
>>> [<c06df420>] power_actor_set_power+0x54/0xa8
>>> [<c06e3774>] power_allocator_throttle+0x770/0x97c
>>> [<c06dd120>] handle_thermal_trip+0x1b4/0x26c
>>> [<c06ddb48>] thermal_zone_device_update+0x154/0x208
>>> [<c014159c>] process_one_work+0x1ec/0x36c
>>> [<c0141c58>] worker_thread+0x204/0x2ec
>>> [<c0146788>] kthread+0x140/0x154
>>> [<c01010e8>] ret_from_fork+0x14/0x2c
>>>
>>> Fixes: a76caf55e5b35 ("thermal: Add devfreq cooling")
>>> Cc: stable@vger.kernel.org # 4.4+
>>> Signed-off-by: Kant Fan <kant@allwinnertech.com>
>>> ---
>>>   drivers/thermal/devfreq_cooling.c | 25 ++++++++++++++++++-------
>>>   1 file changed, 18 insertions(+), 7 deletions(-)
>>>
>>
>> Looks good. So this patch should be applied for all stable
>> kernels starting from v4.4 to v5.12 (the v5.13 and later need
>> other patch).
>>
>> Next time you might use in the subject something like:
>> [PATCH 4.4] thermal: devfreq_cooling: use local ops instead of global ops
>> It would be better distinguished from your other patch with the
>> same subject, which was for mainline and v5.13+
> 
> Hi Lukasz,
> Thank you for the guidance. I want to know if I'm understanding you in a 
> right way. Could you confirm the following information?
> 
> 1. The stable patches
> After the patch is merged into mainline later, I'll submit the following 
> patches individually for v4.4 ~ v5.12:

Correct, after it gets mainline you can point to that commit hash and
process with those patches. I don't now which of those older stable
kernels are still maintained, since some of them have longer support
and the rest had shorter and might already ended. You can check the
end of life for those 'Longterm' here [1]. AFAICS the 4.4 is not in that
table, so you can start from 4.9, should be OK.
So the list of needed patches would be for those stable kernels:
4.9, 4.14, 4.19, 5.4, 5.10
I can see that last release for 5.11.x was in May 2021, so it's probably
ended, similar for 5.12.x (Jul 2021). That's why I suggested that list
for the long support kernels.

> 
> [PATCH 4.4] thermal: devfreq_cooling: use local ops instead of global ops
> [PATCH 4.5] thermal: devfreq_cooling: use local ops instead of global ops
> ...
> [PATCH 5.12] thermal: devfreq_cooling: use local ops instead of global ops
> 
> And also the following patches individually for v5.13+ :

For this, you probably don't have to. You have added 'v5.13+' in the
original patch v2, so it will be picked correctly. It should apply
on those stable kernels w/o issues. If there will be, stable kernel
engineers will ping us.

> [PATCH 5.13] thermal: devfreq_cooling: use local ops instead of global ops
> [PATCH 5.14] thermal: devfreq_cooling: use local ops instead of global ops
> ...
> [PATCH 5.17] thermal: devfreq_cooling: use local ops instead of global ops
> 
> 2. The mainline patch
> I saw your mail with Rafael, seems there are conflicts... I wonder if 
> there's anything wrong with my patch, or anything I can help?
> 

Thank you for offering help. Rafael solved that correctly, so it doesn't
need any more work.

Thank you for doing that work!

Regards,
Lukasz

[1] https://www.kernel.org/category/releases.html

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

[Kant Fan]

On 29/03/2022 14:59, Lukasz Luba wrote:
> 
> 
> On 3/25/22 09:44, Kant Fan wrote:
>> commit 7b62935828266658714f81d4e9176edad808dc70 upstream.
>>
>> Fix access illegal address problem in following condition:
>> There are muti devfreq cooling devices in system, some of them register
>> with dfc_power but other does not, power model ops such as state2power 
>> will
>> append to global devfreq_cooling_ops when the cooling device with
>> dfc_power register. It makes the cooling device without dfc_power
>> 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 dfc_power as a power 
>> actor
>> because they also have power model ops, and will access illegal 
>> address at
>> dfc->power_ops when execute cdev->ops->get_requested_power or
>> cdev->ops->power2state. As the calltrace below shows:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address
>> 00000008
>> ...
>> calltrace:
>> [<c06e5488>] devfreq_cooling_power2state+0x24/0x184
>> [<c06df420>] power_actor_set_power+0x54/0xa8
>> [<c06e3774>] power_allocator_throttle+0x770/0x97c
>> [<c06dd120>] handle_thermal_trip+0x1b4/0x26c
>> [<c06ddb48>] thermal_zone_device_update+0x154/0x208
>> [<c014159c>] process_one_work+0x1ec/0x36c
>> [<c0141c58>] worker_thread+0x204/0x2ec
>> [<c0146788>] kthread+0x140/0x154
>> [<c01010e8>] ret_from_fork+0x14/0x2c
>>
>> Fixes: a76caf55e5b35 ("thermal: Add devfreq cooling")
>> Cc: stable@vger.kernel.org # 4.4+
>> Signed-off-by: Kant Fan <kant@allwinnertech.com>
>> ---
>>   drivers/thermal/devfreq_cooling.c | 25 ++++++++++++++++++-------
>>   1 file changed, 18 insertions(+), 7 deletions(-)
>>
> 
> Looks good. So this patch should be applied for all stable
> kernels starting from v4.4 to v5.12 (the v5.13 and later need
> other patch).
> 
> Next time you might use in the subject something like:
> [PATCH 4.4] thermal: devfreq_cooling: use local ops instead of global ops
> It would be better distinguished from your other patch with the
> same subject, which was for mainline and v5.13+

Hi Lukasz,
Thank you for the guidance. I want to know if I'm understanding you in a 
right way. Could you confirm the following information?

1. The stable patches
After the patch is merged into mainline later, I'll submit the following 
patches individually for v4.4 ~ v5.12:

[PATCH 4.4] thermal: devfreq_cooling: use local ops instead of global ops
[PATCH 4.5] thermal: devfreq_cooling: use local ops instead of global ops
...
[PATCH 5.12] thermal: devfreq_cooling: use local ops instead of global ops

And also the following patches individually for v5.13+ :
[PATCH 5.13] thermal: devfreq_cooling: use local ops instead of global ops
[PATCH 5.14] thermal: devfreq_cooling: use local ops instead of global ops
...
[PATCH 5.17] thermal: devfreq_cooling: use local ops instead of global ops

2. The mainline patch
I saw your mail with Rafael, seems there are conflicts... I wonder if 
there's anything wrong with my patch, or anything I can help?

-- 
Best Regards,
Kant Fan

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

[Lukasz Luba]

On 3/29/22 12:20, Greg KH wrote:
> On Fri, Mar 25, 2022 at 05:44:36PM +0800, Kant Fan wrote:
>> commit 7b62935828266658714f81d4e9176edad808dc70 upstream.
> 
> I do not see this commit in Linus's tree :(
> 
> confused,
> 
> greg k-h

My apologies Greg, I missed that.

We need to first get the patch [1] into upstream.
Kant would resend this patch after that happen.

[1] 
https://lore.kernel.org/linux-pm/20220325073030.91919-1-kant@allwinnertech.com/

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

[Greg KH]

On Fri, Mar 25, 2022 at 05:44:36PM +0800, Kant Fan wrote:
> commit 7b62935828266658714f81d4e9176edad808dc70 upstream.

I do not see this commit in Linus's tree :(

confused,

greg k-h

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

[Lukasz Luba]

On 3/25/22 09:44, Kant Fan wrote:
> commit 7b62935828266658714f81d4e9176edad808dc70 upstream.
> 
> Fix access illegal address problem in following condition:
> There are muti devfreq cooling devices in system, some of them register
> with dfc_power but other does not, power model ops such as state2power will
> append to global devfreq_cooling_ops when the cooling device with
> dfc_power register. It makes the cooling device without dfc_power
> 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 dfc_power as a power actor
> because they also have power model ops, and will access illegal address at
> dfc->power_ops when execute cdev->ops->get_requested_power or
> cdev->ops->power2state. As the calltrace below shows:
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 00000008
> ...
> calltrace:
> [<c06e5488>] devfreq_cooling_power2state+0x24/0x184
> [<c06df420>] power_actor_set_power+0x54/0xa8
> [<c06e3774>] power_allocator_throttle+0x770/0x97c
> [<c06dd120>] handle_thermal_trip+0x1b4/0x26c
> [<c06ddb48>] thermal_zone_device_update+0x154/0x208
> [<c014159c>] process_one_work+0x1ec/0x36c
> [<c0141c58>] worker_thread+0x204/0x2ec
> [<c0146788>] kthread+0x140/0x154
> [<c01010e8>] ret_from_fork+0x14/0x2c
> 
> Fixes: a76caf55e5b35 ("thermal: Add devfreq cooling")
> Cc: stable@vger.kernel.org # 4.4+
> Signed-off-by: Kant Fan <kant@allwinnertech.com>
> ---
>   drivers/thermal/devfreq_cooling.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
> 

Looks good. So this patch should be applied for all stable
kernels starting from v4.4 to v5.12 (the v5.13 and later need
other patch).

Next time you might use in the subject something like:
[PATCH 4.4] thermal: devfreq_cooling: use local ops instead of global ops
It would be better distinguished from your other patch with the
same subject, which was for mainline and v5.13+

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

[Kant Fan]

commit 7b62935828266658714f81d4e9176edad808dc70 upstream.

Fix access illegal address problem in following condition:
There are muti devfreq cooling devices in system, some of them register
with dfc_power but other does not, power model ops such as state2power will
append to global devfreq_cooling_ops when the cooling device with
dfc_power register. It makes the cooling device without dfc_power
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 dfc_power as a power actor
because they also have power model ops, and will access illegal address at
dfc->power_ops when execute cdev->ops->get_requested_power or
cdev->ops->power2state. As the calltrace below shows:

Unable to handle kernel NULL pointer dereference at virtual address
00000008
...
calltrace:
[<c06e5488>] devfreq_cooling_power2state+0x24/0x184
[<c06df420>] power_actor_set_power+0x54/0xa8
[<c06e3774>] power_allocator_throttle+0x770/0x97c
[<c06dd120>] handle_thermal_trip+0x1b4/0x26c
[<c06ddb48>] thermal_zone_device_update+0x154/0x208
[<c014159c>] process_one_work+0x1ec/0x36c
[<c0141c58>] worker_thread+0x204/0x2ec
[<c0146788>] kthread+0x140/0x154
[<c01010e8>] ret_from_fork+0x14/0x2c

Fixes: a76caf55e5b35 ("thermal: Add devfreq cooling")
Cc: stable@vger.kernel.org # 4.4+
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 dfab49a67252..d36f70513e6a 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -462,22 +462,29 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
 {
 	struct thermal_cooling_device *cdev;
 	struct devfreq_cooling_device *dfc;
+	struct thermal_cooling_device_ops *ops;
 	char dev_name[THERMAL_NAME_LENGTH];
 	int err;
 
-	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;
 
 	if (dfc_power) {
 		dfc->power_ops = dfc_power;
 
-		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;
 	}
 
 	err = devfreq_cooling_gen_tables(dfc);
@@ -497,8 +504,7 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
 
 	snprintf(dev_name, sizeof(dev_name), "thermal-devfreq-%d", dfc->id);
 
-	cdev = thermal_of_cooling_device_register(np, dev_name, dfc,
-						  &devfreq_cooling_ops);
+	cdev = thermal_of_cooling_device_register(np, dev_name, dfc, ops);
 	if (IS_ERR(cdev)) {
 		err = PTR_ERR(cdev);
 		dev_err(df->dev.parent,
@@ -522,6 +528,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);
 }
@@ -557,10 +565,12 @@ EXPORT_SYMBOL_GPL(devfreq_cooling_register);
 void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
 {
 	struct devfreq_cooling_device *dfc;
+	const struct thermal_cooling_device_ops *ops;
 
 	if (!cdev)
 		return;
 
+	ops = cdev->ops;
 	dfc = cdev->devdata;
 
 	thermal_cooling_device_unregister(dfc->cdev);
@@ -570,5 +580,6 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
 	kfree(dfc->freq_table);
 
 	kfree(dfc);
+	kfree(ops);
 }
 EXPORT_SYMBOL_GPL(devfreq_cooling_unregister);
-- 
2.29.0