diff mbox series

[v3,02/14] thermal: core: Rearrange checks in thermal_bind_cdev_to_trip()

Message ID 3324214.44csPzL39Z@rjwysocki.net (mailing list archive)
State Mainlined, archived
Headers show
Series thermal: Rework binding cooling devices to trip points | expand

Commit Message

Rafael J. Wysocki Aug. 19, 2024, 3:51 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is not necessary to look up the thermal zone and the cooling device
in the respective global lists to check whether or not they are
registered.  It is sufficient to check whether or not their respective
list nodes are empty for this purpose.

Use the above observation to simplify thermal_bind_cdev_to_trip().  In
addition, eliminate an unnecessary ternary operator from it.

Moreover, add lockdep_assert_held() for thermal_list_lock to it because
that lock must be held by its callers when it is running.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v3: No changes

---
 drivers/thermal/thermal_core.c |   16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

Comments

Zhang Rui Aug. 20, 2024, 7:05 a.m. UTC | #1
On Mon, 2024-08-19 at 17:51 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is not necessary to look up the thermal zone and the cooling
> device
> in the respective global lists to check whether or not they are
> registered.  It is sufficient to check whether or not their
> respective
> list nodes are empty for this purpose.
> 
> Use the above observation to simplify thermal_bind_cdev_to_trip(). 
> In
> addition, eliminate an unnecessary ternary operator from it.
> 
> Moreover, add lockdep_assert_held() for thermal_list_lock to it
> because
> that lock must be held by its callers when it is running.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

> ---
> 
> v1 -> v3: No changes
> 
> ---
>  drivers/thermal/thermal_core.c |   16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -781,25 +781,17 @@ int thermal_bind_cdev_to_trip(struct the
>  {
>         struct thermal_instance *dev;
>         struct thermal_instance *pos;
> -       struct thermal_zone_device *pos1;
> -       struct thermal_cooling_device *pos2;
>         bool upper_no_limit;
>         int result;
>  
> -       list_for_each_entry(pos1, &thermal_tz_list, node) {
> -               if (pos1 == tz)
> -                       break;
> -       }
> -       list_for_each_entry(pos2, &thermal_cdev_list, node) {
> -               if (pos2 == cdev)
> -                       break;
> -       }
> +       lockdep_assert_held(&thermal_list_lock);
>  
> -       if (tz != pos1 || cdev != pos2)
> +       if (list_empty(&tz->node) || list_empty(&cdev->node))
>                 return -EINVAL;
>  
>         /* lower default 0, upper default max_state */
> -       lower = lower == THERMAL_NO_LIMIT ? 0 : lower;
> +       if (lower == THERMAL_NO_LIMIT)
> +               lower = 0;
>  
>         if (upper == THERMAL_NO_LIMIT) {
>                 upper = cdev->max_state;
> 
> 
>
Daniel Lezcano Aug. 21, 2024, 7:59 a.m. UTC | #2
On 19/08/2024 17:51, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is not necessary to look up the thermal zone and the cooling device
> in the respective global lists to check whether or not they are
> registered.  It is sufficient to check whether or not their respective
> list nodes are empty for this purpose.
> 
> Use the above observation to simplify thermal_bind_cdev_to_trip().  In
> addition, eliminate an unnecessary ternary operator from it.
> 
> Moreover, add lockdep_assert_held() for thermal_list_lock to it because
> that lock must be held by its callers when it is running.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Good catch

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
lihuisong (C) Aug. 21, 2024, 8:49 a.m. UTC | #3
在 2024/8/19 23:51, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It is not necessary to look up the thermal zone and the cooling device
> in the respective global lists to check whether or not they are
> registered.  It is sufficient to check whether or not their respective
> list nodes are empty for this purpose.
>
> Use the above observation to simplify thermal_bind_cdev_to_trip().  In
> addition, eliminate an unnecessary ternary operator from it.
>
> Moreover, add lockdep_assert_held() for thermal_list_lock to it because
> that lock must be held by its callers when it is running.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> v1 -> v3: No changes
>
> ---
>   drivers/thermal/thermal_core.c |   16 ++++------------
>   1 file changed, 4 insertions(+), 12 deletions(-)
>
> Index: linux-pm/drivers/thermal/thermal_core.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_core.c
> +++ linux-pm/drivers/thermal/thermal_core.c
> @@ -781,25 +781,17 @@ int thermal_bind_cdev_to_trip(struct the
>   {
>   	struct thermal_instance *dev;
>   	struct thermal_instance *pos;
> -	struct thermal_zone_device *pos1;
> -	struct thermal_cooling_device *pos2;
>   	bool upper_no_limit;
>   	int result;
>   
> -	list_for_each_entry(pos1, &thermal_tz_list, node) {
> -		if (pos1 == tz)
> -			break;
> -	}
> -	list_for_each_entry(pos2, &thermal_cdev_list, node) {
> -		if (pos2 == cdev)
> -			break;
> -	}
> +	lockdep_assert_held(&thermal_list_lock);
>   
> -	if (tz != pos1 || cdev != pos2)
> +	if (list_empty(&tz->node) || list_empty(&cdev->node))
The old verification is ensure that tz and cdev already add to 
thermal_tz_list and thermal_cdev_list,respectively.
Namely, tz and cdev are definitely registered and intialized.
The check is ok for all untizalized thermal_zone_device and cooling device.
But the new verification doesn't seem to do that.
>   		return -EINVAL;
>   
>   	/* lower default 0, upper default max_state */
> -	lower = lower == THERMAL_NO_LIMIT ? 0 : lower;
> +	if (lower == THERMAL_NO_LIMIT)
> +		lower = 0;
>   
>   	if (upper == THERMAL_NO_LIMIT) {
>   		upper = cdev->max_state;
>
>
>
>
> .
Daniel Lezcano Aug. 21, 2024, 9:28 a.m. UTC | #4
On 21/08/2024 10:49, lihuisong (C) wrote:

[ ... ]

>> -    list_for_each_entry(pos2, &thermal_cdev_list, node) {
>> -        if (pos2 == cdev)
>> -            break;
>> -    }
>> +    lockdep_assert_held(&thermal_list_lock);
>> -    if (tz != pos1 || cdev != pos2)
>> +    if (list_empty(&tz->node) || list_empty(&cdev->node))
> The old verification is ensure that tz and cdev already add to 
> thermal_tz_list and thermal_cdev_list,respectively.
> Namely, tz and cdev are definitely registered and intialized.
> The check is ok for all untizalized thermal_zone_device and cooling device.
> But the new verification doesn't seem to do that.

If the tz or the cdev are registered then their "->node" is not empty 
because they are linked with the thermal_list and cdev_list

So either way is browsing the lists to find the tz/cdev or just check 
"->node" is not empty. The latter the faster.

Did I misunderstood your comment ?

[ ... ]
lihuisong (C) Aug. 21, 2024, 9:44 a.m. UTC | #5
在 2024/8/21 17:28, Daniel Lezcano 写道:
> On 21/08/2024 10:49, lihuisong (C) wrote:
>
> [ ... ]
>
>>> -    list_for_each_entry(pos2, &thermal_cdev_list, node) {
>>> -        if (pos2 == cdev)
>>> -            break;
>>> -    }
>>> +    lockdep_assert_held(&thermal_list_lock);
>>> -    if (tz != pos1 || cdev != pos2)
>>> +    if (list_empty(&tz->node) || list_empty(&cdev->node))
>> The old verification is ensure that tz and cdev already add to 
>> thermal_tz_list and thermal_cdev_list,respectively.
>> Namely, tz and cdev are definitely registered and intialized.
>> The check is ok for all untizalized thermal_zone_device and cooling 
>> device.
>> But the new verification doesn't seem to do that.
>
> If the tz or the cdev are registered then their "->node" is not empty 
> because they are linked with the thermal_list and cdev_list
>
> So either way is browsing the lists to find the tz/cdev or just check 
> "->node" is not empty. The latter the faster.
Assume that tz/cdev isn't intiazlized and registered to thermal_tz_list 
or thermal_cdev_list. And then directly call this interface.
>
> Did I misunderstood your comment ?
>
> [ ... ]
>
Daniel Lezcano Aug. 21, 2024, 10:49 a.m. UTC | #6
On 21/08/2024 11:44, lihuisong (C) wrote:
> 
> 在 2024/8/21 17:28, Daniel Lezcano 写道:
>> On 21/08/2024 10:49, lihuisong (C) wrote:
>>
>> [ ... ]
>>
>>>> -    list_for_each_entry(pos2, &thermal_cdev_list, node) {
>>>> -        if (pos2 == cdev)
>>>> -            break;
>>>> -    }
>>>> +    lockdep_assert_held(&thermal_list_lock);
>>>> -    if (tz != pos1 || cdev != pos2)
>>>> +    if (list_empty(&tz->node) || list_empty(&cdev->node))
>>> The old verification is ensure that tz and cdev already add to 
>>> thermal_tz_list and thermal_cdev_list,respectively.
>>> Namely, tz and cdev are definitely registered and intialized.
>>> The check is ok for all untizalized thermal_zone_device and cooling 
>>> device.
>>> But the new verification doesn't seem to do that.
>>
>> If the tz or the cdev are registered then their "->node" is not empty 
>> because they are linked with the thermal_list and cdev_list
>>
>> So either way is browsing the lists to find the tz/cdev or just check 
>> "->node" is not empty. The latter the faster.
> Assume that tz/cdev isn't intiazlized and registered to thermal_tz_list 
> or thermal_cdev_list. And then directly call this interface.

Then there is a bug in the internal code because the 
thermal_zone_device_register*() and cooling_device_device_register() 
allocate and initialize those structures.

The caller of the function is supposed to use the API provided by the 
thermal framework. It is not possible to plan every stupid things a 
driver can do. In this particular case, very likely the kernel will 
crash immediately which is a sufficient test for me and coercive enough 
to have the API user to put its code in question ;)
Rafael J. Wysocki Aug. 21, 2024, 10:51 a.m. UTC | #7
On Wed, Aug 21, 2024 at 12:43 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2024/8/19 23:51, Rafael J. Wysocki 写道:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > It is not necessary to look up the thermal zone and the cooling device
> > in the respective global lists to check whether or not they are
> > registered.  It is sufficient to check whether or not their respective
> > list nodes are empty for this purpose.
> >
> > Use the above observation to simplify thermal_bind_cdev_to_trip().  In
> > addition, eliminate an unnecessary ternary operator from it.
> >
> > Moreover, add lockdep_assert_held() for thermal_list_lock to it because
> > that lock must be held by its callers when it is running.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > v1 -> v3: No changes
> >
> > ---
> >   drivers/thermal/thermal_core.c |   16 ++++------------
> >   1 file changed, 4 insertions(+), 12 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_core.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_core.c
> > +++ linux-pm/drivers/thermal/thermal_core.c
> > @@ -781,25 +781,17 @@ int thermal_bind_cdev_to_trip(struct the
> >   {
> >       struct thermal_instance *dev;
> >       struct thermal_instance *pos;
> > -     struct thermal_zone_device *pos1;
> > -     struct thermal_cooling_device *pos2;
> >       bool upper_no_limit;
> >       int result;
> >
> > -     list_for_each_entry(pos1, &thermal_tz_list, node) {
> > -             if (pos1 == tz)
> > -                     break;
> > -     }
> > -     list_for_each_entry(pos2, &thermal_cdev_list, node) {
> > -             if (pos2 == cdev)
> > -                     break;
> > -     }
> > +     lockdep_assert_held(&thermal_list_lock);
> >
> > -     if (tz != pos1 || cdev != pos2)
> > +     if (list_empty(&tz->node) || list_empty(&cdev->node))
> The old verification is ensure that tz and cdev already add to
> thermal_tz_list and thermal_cdev_list,respectively.
> Namely, tz and cdev are definitely registered and intialized.
> The check is ok for all untizalized thermal_zone_device and cooling device.
> But the new verification doesn't seem to do that.

It doesn't need to do it and after this series it is only called from
thermal_zone_device_register_with_trips() and
__thermal_cooling_device_register() via thermal_zone_cdev_binding()
after both the cdev and the tz have been added to the list, under
thermal_list_lock.

I guess I can send a patch to remove the check altogether now.
Rafael J. Wysocki Aug. 21, 2024, 11:12 a.m. UTC | #8
On Wed, Aug 21, 2024 at 1:11 PM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2024/8/21 17:28, Daniel Lezcano 写道:
> > On 21/08/2024 10:49, lihuisong (C) wrote:
> >
> > [ ... ]
> >
> >>> -    list_for_each_entry(pos2, &thermal_cdev_list, node) {
> >>> -        if (pos2 == cdev)
> >>> -            break;
> >>> -    }
> >>> +    lockdep_assert_held(&thermal_list_lock);
> >>> -    if (tz != pos1 || cdev != pos2)
> >>> +    if (list_empty(&tz->node) || list_empty(&cdev->node))
> >> The old verification is ensure that tz and cdev already add to
> >> thermal_tz_list and thermal_cdev_list,respectively.
> >> Namely, tz and cdev are definitely registered and intialized.
> >> The check is ok for all untizalized thermal_zone_device and cooling
> >> device.
> >> But the new verification doesn't seem to do that.
> >
> > If the tz or the cdev are registered then their "->node" is not empty
> > because they are linked with the thermal_list and cdev_list
> >
> > So either way is browsing the lists to find the tz/cdev or just check
> > "->node" is not empty. The latter the faster.
> Assume that tz/cdev isn't intiazlized and registered to thermal_tz_list
> or thermal_cdev_list. And then directly call this interface.

Who does this?
lihuisong (C) Aug. 21, 2024, 11:22 a.m. UTC | #9
在 2024/8/21 18:49, Daniel Lezcano 写道:
> On 21/08/2024 11:44, lihuisong (C) wrote:
>>
>> 在 2024/8/21 17:28, Daniel Lezcano 写道:
>>> On 21/08/2024 10:49, lihuisong (C) wrote:
>>>
>>> [ ... ]
>>>
>>>>> -    list_for_each_entry(pos2, &thermal_cdev_list, node) {
>>>>> -        if (pos2 == cdev)
>>>>> -            break;
>>>>> -    }
>>>>> +    lockdep_assert_held(&thermal_list_lock);
>>>>> -    if (tz != pos1 || cdev != pos2)
>>>>> +    if (list_empty(&tz->node) || list_empty(&cdev->node))
>>>> The old verification is ensure that tz and cdev already add to 
>>>> thermal_tz_list and thermal_cdev_list,respectively.
>>>> Namely, tz and cdev are definitely registered and intialized.
>>>> The check is ok for all untizalized thermal_zone_device and cooling 
>>>> device.
>>>> But the new verification doesn't seem to do that.
>>>
>>> If the tz or the cdev are registered then their "->node" is not 
>>> empty because they are linked with the thermal_list and cdev_list
>>>
>>> So either way is browsing the lists to find the tz/cdev or just 
>>> check "->node" is not empty. The latter the faster.
>> Assume that tz/cdev isn't intiazlized and registered to 
>> thermal_tz_list or thermal_cdev_list. And then directly call this 
>> interface.
>
> Then there is a bug in the internal code because the 
> thermal_zone_device_register*() and cooling_device_device_register() 
> allocate and initialize those structures.
>
> The caller of the function is supposed to use the API provided by the 
> thermal framework. It is not possible to plan every stupid things a 
> driver can do. In this particular case, very likely the kernel will 
> crash immediately which is a sufficient test for me and coercive 
> enough to have the API user to put its code in question ;)

A good point. Agree.
diff mbox series

Patch

Index: linux-pm/drivers/thermal/thermal_core.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_core.c
+++ linux-pm/drivers/thermal/thermal_core.c
@@ -781,25 +781,17 @@  int thermal_bind_cdev_to_trip(struct the
 {
 	struct thermal_instance *dev;
 	struct thermal_instance *pos;
-	struct thermal_zone_device *pos1;
-	struct thermal_cooling_device *pos2;
 	bool upper_no_limit;
 	int result;
 
-	list_for_each_entry(pos1, &thermal_tz_list, node) {
-		if (pos1 == tz)
-			break;
-	}
-	list_for_each_entry(pos2, &thermal_cdev_list, node) {
-		if (pos2 == cdev)
-			break;
-	}
+	lockdep_assert_held(&thermal_list_lock);
 
-	if (tz != pos1 || cdev != pos2)
+	if (list_empty(&tz->node) || list_empty(&cdev->node))
 		return -EINVAL;
 
 	/* lower default 0, upper default max_state */
-	lower = lower == THERMAL_NO_LIMIT ? 0 : lower;
+	if (lower == THERMAL_NO_LIMIT)
+		lower = 0;
 
 	if (upper == THERMAL_NO_LIMIT) {
 		upper = cdev->max_state;