| Message ID | 3324214.44csPzL39Z@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | thermal: Rework binding cooling devices to trip points | expand |
在 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 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 ? [ ... ]
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 ;)
在 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.
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;