| Message ID | 13583081.uLZWGnKmhe@rjwysocki.net |
|---|---|
| State | New |
| Headers | show |
| Series | thermal: gov_bang_bang: Prevent cooling devices from getting stuck in the "on" state | expand |
On 13.08.24 16:25, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Instead of clearing the "updated" flag for each cooling device > affected by the trip point crossing in bang_bang_control() and > walking all thermal instances to run thermal_cdev_update() for all > of the affected cooling devices, call __thermal_cdev_update() > directly for each of them. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Acked-by: Peter Kästle <peter@piie.net> > --- > drivers/thermal/gov_bang_bang.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > Index: linux-pm/drivers/thermal/gov_bang_bang.c > =================================================================== > --- linux-pm.orig/drivers/thermal/gov_bang_bang.c > +++ linux-pm/drivers/thermal/gov_bang_bang.c > @@ -71,12 +71,9 @@ static void bang_bang_control(struct the > dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target); > > mutex_lock(&instance->cdev->lock); > - instance->cdev->updated = false; /* cdev needs update */ > + __thermal_cdev_update(instance->cdev); > mutex_unlock(&instance->cdev->lock); > } > - > - list_for_each_entry(instance, &tz->thermal_instances, tz_node) > - thermal_cdev_update(instance->cdev); > } > > static struct thermal_governor thermal_gov_bang_bang = { > > >
On Tue, 2024-08-13 at 16:25 +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Instead of clearing the "updated" flag for each cooling device > affected by the trip point crossing in bang_bang_control() and > walking all thermal instances to run thermal_cdev_update() for all > of the affected cooling devices, call __thermal_cdev_update() > directly for each of them. with this change, we may invoke thermal_cdev_set_cur_state() for multiple times instead of one, in one bang_bang_control() run. So this effectively changes the notifications and statistics. If this is not a problem, maybe better to mention this change in the changelog? thanks, rui > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/thermal/gov_bang_bang.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > Index: linux-pm/drivers/thermal/gov_bang_bang.c > =================================================================== > --- linux-pm.orig/drivers/thermal/gov_bang_bang.c > +++ linux-pm/drivers/thermal/gov_bang_bang.c > @@ -71,12 +71,9 @@ static void bang_bang_control(struct the > dev_dbg(&instance->cdev->device, "target=%ld\n", > instance->target); > > mutex_lock(&instance->cdev->lock); > - instance->cdev->updated = false; /* cdev needs update > */ > + __thermal_cdev_update(instance->cdev); > mutex_unlock(&instance->cdev->lock); > } > - > - list_for_each_entry(instance, &tz->thermal_instances, > tz_node) > - thermal_cdev_update(instance->cdev); > } > > static struct thermal_governor thermal_gov_bang_bang = { > > >
On Wed, Aug 14, 2024 at 8:18 AM Zhang, Rui <rui.zhang@intel.com> wrote: > > On Tue, 2024-08-13 at 16:25 +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Instead of clearing the "updated" flag for each cooling device > > affected by the trip point crossing in bang_bang_control() and > > walking all thermal instances to run thermal_cdev_update() for all > > of the affected cooling devices, call __thermal_cdev_update() > > directly for each of them. > > with this change, we may invoke thermal_cdev_set_cur_state() for > multiple times instead of one, in one bang_bang_control() run. No, this cannot happen AFAICS because one cooling device cannot be bound to the same trip point more than once. Since bang_bang_control() only checks thermal instances for one trip point, all cooling devices pointed to by them are guaranteed to be different. > So this effectively changes the notifications and statistics. > > If this is not a problem, maybe better to mention this change in the > changelog? > > thanks, > rui > > > > No intentional functional impact. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/thermal/gov_bang_bang.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > Index: linux-pm/drivers/thermal/gov_bang_bang.c > > =================================================================== > > --- linux-pm.orig/drivers/thermal/gov_bang_bang.c > > +++ linux-pm/drivers/thermal/gov_bang_bang.c > > @@ -71,12 +71,9 @@ static void bang_bang_control(struct the > > dev_dbg(&instance->cdev->device, "target=%ld\n", > > instance->target); > > > > mutex_lock(&instance->cdev->lock); > > - instance->cdev->updated = false; /* cdev needs update > > */ > > + __thermal_cdev_update(instance->cdev); > > mutex_unlock(&instance->cdev->lock); > > } > > - > > - list_for_each_entry(instance, &tz->thermal_instances, > > tz_node) > > - thermal_cdev_update(instance->cdev); > > } > > > > static struct thermal_governor thermal_gov_bang_bang = { > > > > > > >
Index: linux-pm/drivers/thermal/gov_bang_bang.c =================================================================== --- linux-pm.orig/drivers/thermal/gov_bang_bang.c +++ linux-pm/drivers/thermal/gov_bang_bang.c @@ -71,12 +71,9 @@ static void bang_bang_control(struct the dev_dbg(&instance->cdev->device, "target=%ld\n", instance->target); mutex_lock(&instance->cdev->lock); - instance->cdev->updated = false; /* cdev needs update */ + __thermal_cdev_update(instance->cdev); mutex_unlock(&instance->cdev->lock); } - - list_for_each_entry(instance, &tz->thermal_instances, tz_node) - thermal_cdev_update(instance->cdev); } static struct thermal_governor thermal_gov_bang_bang = {