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