| Message ID | 2819322.BEx9A2HvPv@rjwysocki.net (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | thermal: Rework binding cooling devices to trip points | expand |
On Mon, 2024-08-12 at 15:56 +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Add a new label to thermal_bind_cdev_to_trip() and use it to > eliminate > two redundant !result check from that function, rename a label in > thermal_unbind_cdev_from_trip() to reflect its actual purpose and > adjust some white space in these functions. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: No changes. > > --- > drivers/thermal/thermal_core.c | 32 ++++++++++++++++++------------ > -- > 1 file changed, 18 insertions(+), 14 deletions(-) > > Index: linux-pm/drivers/thermal/thermal_core.c > =================================================================== > --- linux-pm.orig/drivers/thermal/thermal_core.c > +++ linux-pm/drivers/thermal/thermal_core.c > @@ -806,6 +806,7 @@ int thermal_bind_cdev_to_trip(struct the > dev = kzalloc(sizeof(*dev), GFP_KERNEL); > if (!dev) > return -ENOMEM; > + > dev->tz = tz; > dev->cdev = cdev; > dev->trip = trip; > @@ -821,8 +822,7 @@ int thermal_bind_cdev_to_trip(struct the > > dev->id = result; > sprintf(dev->name, "cdev%d", dev->id); > - result = > - sysfs_create_link(&tz->device.kobj, &cdev->device.kobj, > dev->name); > + result = sysfs_create_link(&tz->device.kobj, &cdev- > >device.kobj, dev->name); > if (result) > goto release_ida; > > @@ -849,24 +849,26 @@ int thermal_bind_cdev_to_trip(struct the > > mutex_lock(&tz->lock); > mutex_lock(&cdev->lock); > - list_for_each_entry(pos, &tz->thermal_instances, tz_node) > + > + list_for_each_entry(pos, &tz->thermal_instances, tz_node) { > if (pos->trip == trip && pos->cdev == cdev) { > result = -EEXIST; > - break; > + goto remove_weight_file; Need to unlock tz->lock and cdev->lock before quitting? thanks, rui > } > - if (!result) { > - list_add_tail(&dev->tz_node, &tz->thermal_instances); > - list_add_tail(&dev->cdev_node, &cdev- > >thermal_instances); > - atomic_set(&tz->need_update, 1); > - > - thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV); > } > + > + list_add_tail(&dev->tz_node, &tz->thermal_instances); > + list_add_tail(&dev->cdev_node, &cdev->thermal_instances); > + atomic_set(&tz->need_update, 1); > + > + thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV); > + > mutex_unlock(&cdev->lock); > mutex_unlock(&tz->lock); > > - if (!result) > - return 0; > + return 0; > > +remove_weight_file: > device_remove_file(&tz->device, &dev->weight_attr); > remove_trip_file: > device_remove_file(&tz->device, &dev->attr); > @@ -914,6 +916,7 @@ int thermal_unbind_cdev_from_trip(struct > > mutex_lock(&tz->lock); > mutex_lock(&cdev->lock); > + > list_for_each_entry_safe(pos, next, &tz->thermal_instances, > tz_node) { > if (pos->trip == trip && pos->cdev == cdev) { > list_del(&pos->tz_node); > @@ -923,15 +926,16 @@ int thermal_unbind_cdev_from_trip(struct > > mutex_unlock(&cdev->lock); > mutex_unlock(&tz->lock); > - goto unbind; > + goto free; > } > } > + > mutex_unlock(&cdev->lock); > mutex_unlock(&tz->lock); > > return -ENODEV; > > -unbind: > +free: > device_remove_file(&tz->device, &pos->weight_attr); > device_remove_file(&tz->device, &pos->attr); > sysfs_remove_link(&tz->device.kobj, pos->name); > > >
On Tue, Aug 13, 2024 at 9:39 AM Zhang, Rui <rui.zhang@intel.com> wrote: > > On Mon, 2024-08-12 at 15:56 +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Add a new label to thermal_bind_cdev_to_trip() and use it to > > eliminate > > two redundant !result check from that function, rename a label in > > thermal_unbind_cdev_from_trip() to reflect its actual purpose and > > adjust some white space in these functions. > > > > No intentional functional impact. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > > > v1 -> v2: No changes. > > > > --- > > drivers/thermal/thermal_core.c | 32 ++++++++++++++++++------------ > > -- > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > Index: linux-pm/drivers/thermal/thermal_core.c > > =================================================================== > > --- linux-pm.orig/drivers/thermal/thermal_core.c > > +++ linux-pm/drivers/thermal/thermal_core.c > > @@ -806,6 +806,7 @@ int thermal_bind_cdev_to_trip(struct the > > dev = kzalloc(sizeof(*dev), GFP_KERNEL); > > if (!dev) > > return -ENOMEM; > > + > > dev->tz = tz; > > dev->cdev = cdev; > > dev->trip = trip; > > @@ -821,8 +822,7 @@ int thermal_bind_cdev_to_trip(struct the > > > > dev->id = result; > > sprintf(dev->name, "cdev%d", dev->id); > > - result = > > - sysfs_create_link(&tz->device.kobj, &cdev->device.kobj, > > dev->name); > > + result = sysfs_create_link(&tz->device.kobj, &cdev- > > >device.kobj, dev->name); > > if (result) > > goto release_ida; > > > > @@ -849,24 +849,26 @@ int thermal_bind_cdev_to_trip(struct the > > > > mutex_lock(&tz->lock); > > mutex_lock(&cdev->lock); > > - list_for_each_entry(pos, &tz->thermal_instances, tz_node) > > + > > + list_for_each_entry(pos, &tz->thermal_instances, tz_node) { > > if (pos->trip == trip && pos->cdev == cdev) { > > result = -EEXIST; > > - break; > > + goto remove_weight_file; > > Need to unlock tz->lock and cdev->lock before quitting? Yes, of course. Nice catch, thank you! I'll drop this patch as it's not worth salvaging IMO. > > } > > - if (!result) { > > - list_add_tail(&dev->tz_node, &tz->thermal_instances); > > - list_add_tail(&dev->cdev_node, &cdev- > > >thermal_instances); > > - atomic_set(&tz->need_update, 1); > > - > > - thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV); > > } > > + > > + list_add_tail(&dev->tz_node, &tz->thermal_instances); > > + list_add_tail(&dev->cdev_node, &cdev->thermal_instances); > > + atomic_set(&tz->need_update, 1); > > + > > + thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV); > > + > > mutex_unlock(&cdev->lock); > > mutex_unlock(&tz->lock); > > > > - if (!result) > > - return 0; > > + return 0; > > > > +remove_weight_file: > > device_remove_file(&tz->device, &dev->weight_attr); > > remove_trip_file: > > device_remove_file(&tz->device, &dev->attr); > > @@ -914,6 +916,7 @@ int thermal_unbind_cdev_from_trip(struct > > > > mutex_lock(&tz->lock); > > mutex_lock(&cdev->lock); > > + > > list_for_each_entry_safe(pos, next, &tz->thermal_instances, > > tz_node) { > > if (pos->trip == trip && pos->cdev == cdev) { > > list_del(&pos->tz_node); > > @@ -923,15 +926,16 @@ int thermal_unbind_cdev_from_trip(struct > > > > mutex_unlock(&cdev->lock); > > mutex_unlock(&tz->lock); > > - goto unbind; > > + goto free; > > } > > } > > + > > mutex_unlock(&cdev->lock); > > mutex_unlock(&tz->lock); > > > > return -ENODEV; > > > > -unbind: > > +free: > > device_remove_file(&tz->device, &pos->weight_attr); > > device_remove_file(&tz->device, &pos->attr); > > sysfs_remove_link(&tz->device.kobj, pos->name); > > > > > > >
Index: linux-pm/drivers/thermal/thermal_core.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.c +++ linux-pm/drivers/thermal/thermal_core.c @@ -806,6 +806,7 @@ int thermal_bind_cdev_to_trip(struct the dev = kzalloc(sizeof(*dev), GFP_KERNEL); if (!dev) return -ENOMEM; + dev->tz = tz; dev->cdev = cdev; dev->trip = trip; @@ -821,8 +822,7 @@ int thermal_bind_cdev_to_trip(struct the dev->id = result; sprintf(dev->name, "cdev%d", dev->id); - result = - sysfs_create_link(&tz->device.kobj, &cdev->device.kobj, dev->name); + result = sysfs_create_link(&tz->device.kobj, &cdev->device.kobj, dev->name); if (result) goto release_ida; @@ -849,24 +849,26 @@ int thermal_bind_cdev_to_trip(struct the mutex_lock(&tz->lock); mutex_lock(&cdev->lock); - list_for_each_entry(pos, &tz->thermal_instances, tz_node) + + list_for_each_entry(pos, &tz->thermal_instances, tz_node) { if (pos->trip == trip && pos->cdev == cdev) { result = -EEXIST; - break; + goto remove_weight_file; } - if (!result) { - list_add_tail(&dev->tz_node, &tz->thermal_instances); - list_add_tail(&dev->cdev_node, &cdev->thermal_instances); - atomic_set(&tz->need_update, 1); - - thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV); } + + list_add_tail(&dev->tz_node, &tz->thermal_instances); + list_add_tail(&dev->cdev_node, &cdev->thermal_instances); + atomic_set(&tz->need_update, 1); + + thermal_governor_update_tz(tz, THERMAL_TZ_BIND_CDEV); + mutex_unlock(&cdev->lock); mutex_unlock(&tz->lock); - if (!result) - return 0; + return 0; +remove_weight_file: device_remove_file(&tz->device, &dev->weight_attr); remove_trip_file: device_remove_file(&tz->device, &dev->attr); @@ -914,6 +916,7 @@ int thermal_unbind_cdev_from_trip(struct mutex_lock(&tz->lock); mutex_lock(&cdev->lock); + list_for_each_entry_safe(pos, next, &tz->thermal_instances, tz_node) { if (pos->trip == trip && pos->cdev == cdev) { list_del(&pos->tz_node); @@ -923,15 +926,16 @@ int thermal_unbind_cdev_from_trip(struct mutex_unlock(&cdev->lock); mutex_unlock(&tz->lock); - goto unbind; + goto free; } } + mutex_unlock(&cdev->lock); mutex_unlock(&tz->lock); return -ENODEV; -unbind: +free: device_remove_file(&tz->device, &pos->weight_attr); device_remove_file(&tz->device, &pos->attr); sysfs_remove_link(&tz->device.kobj, pos->name);