drivers/thermal/thermal_core.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
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))
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;
在 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;
>
>
>
>
> .
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.
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 ?
[ ... ]
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
在 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 ?
>
> [ ... ]
>
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?
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 ;)
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
在 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.
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> -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
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;
>
>
>
© 2016 - 2026 Red Hat, Inc.