| Message ID | 20200409151515.6607-1-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Zhang Rui |
| Headers | show |
| Series | [1/2] thermal: core: Move thermal_cdev_update next to updated=false | expand |
Hi, Daniel, On Thu, 2020-04-09 at 17:15 +0200, Daniel Lezcano wrote: > The call to the thermal_cdev_update() function is done after browsing > the thermal instances which sets the updated flag by browsing them > again. > > Instead of doing this, let's move the call right after setting the > cooling device 'updated' flag as it is done in the other governors. The reason we do this in two steps is that we want to avoid redundant cooling device state changes. Further more, I think it is better to move the thermal_cdev_update out of .throllte() callback, to thermal_zone_device_update(). So that we do not need to update the cooling device for each trip point. is there any specific reason we need to do thermal_cdev_update() for every potential change? thanks, rui > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/thermal/gov_bang_bang.c | 10 +--------- > drivers/thermal/step_wise.c | 10 +--------- > 2 files changed, 2 insertions(+), 18 deletions(-) > > diff --git a/drivers/thermal/gov_bang_bang.c > b/drivers/thermal/gov_bang_bang.c > index 991a1c54296d..c292a69845bb 100644 > --- a/drivers/thermal/gov_bang_bang.c > +++ b/drivers/thermal/gov_bang_bang.c > @@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct > thermal_zone_device *tz, int trip) > mutex_lock(&instance->cdev->lock); > instance->cdev->updated = false; /* cdev needs update > */ > mutex_unlock(&instance->cdev->lock); > + thermal_cdev_update(instance->cdev); > } > > mutex_unlock(&tz->lock); > @@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct > thermal_zone_device *tz, int trip) > */ > static int bang_bang_control(struct thermal_zone_device *tz, int > trip) > { > - struct thermal_instance *instance; > - > thermal_zone_trip_update(tz, trip); > > - mutex_lock(&tz->lock); > - > - list_for_each_entry(instance, &tz->thermal_instances, tz_node) > - thermal_cdev_update(instance->cdev); > - > - mutex_unlock(&tz->lock); > - > return 0; > } > > diff --git a/drivers/thermal/step_wise.c > b/drivers/thermal/step_wise.c > index 2ae7198d3067..298eedac0293 100644 > --- a/drivers/thermal/step_wise.c > +++ b/drivers/thermal/step_wise.c > @@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct > thermal_zone_device *tz, int trip) > mutex_lock(&instance->cdev->lock); > instance->cdev->updated = false; /* cdev needs update > */ > mutex_unlock(&instance->cdev->lock); > + thermal_cdev_update(instance->cdev); > } > > mutex_unlock(&tz->lock); > @@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct > thermal_zone_device *tz, int trip) > */ > static int step_wise_throttle(struct thermal_zone_device *tz, int > trip) > { > - struct thermal_instance *instance; > - > thermal_zone_trip_update(tz, trip); > > if (tz->forced_passive) > thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE); > > - mutex_lock(&tz->lock); > - > - list_for_each_entry(instance, &tz->thermal_instances, tz_node) > - thermal_cdev_update(instance->cdev); > - > - mutex_unlock(&tz->lock); > - > return 0; > } >
On 10/04/2020 12:14, Zhang Rui wrote: > Hi, Daniel, > > On Thu, 2020-04-09 at 17:15 +0200, Daniel Lezcano wrote: >> The call to the thermal_cdev_update() function is done after browsing >> the thermal instances which sets the updated flag by browsing them >> again. >> >> Instead of doing this, let's move the call right after setting the >> cooling device 'updated' flag as it is done in the other governors. > > The reason we do this in two steps is that we want to avoid redundant > cooling device state changes. > > Further more, I think it is better to move the thermal_cdev_update out > of .throllte() callback, to thermal_zone_device_update(). So that we do > not need to update the cooling device for each trip point. > > is there any specific reason we need to do thermal_cdev_update() for > every potential change? I agree we can go further and move the cooling device update in the thermal_zone_device_update() by letting the throttle callback let us know an update is needed with the return value. Makes sense to provide more changes on top of those two patches ? >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/thermal/gov_bang_bang.c | 10 +--------- >> drivers/thermal/step_wise.c | 10 +--------- >> 2 files changed, 2 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/thermal/gov_bang_bang.c >> b/drivers/thermal/gov_bang_bang.c >> index 991a1c54296d..c292a69845bb 100644 >> --- a/drivers/thermal/gov_bang_bang.c >> +++ b/drivers/thermal/gov_bang_bang.c >> @@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct >> thermal_zone_device *tz, int trip) >> mutex_lock(&instance->cdev->lock); >> instance->cdev->updated = false; /* cdev needs update >> */ >> mutex_unlock(&instance->cdev->lock); >> + thermal_cdev_update(instance->cdev); >> } >> >> mutex_unlock(&tz->lock); >> @@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct >> thermal_zone_device *tz, int trip) >> */ >> static int bang_bang_control(struct thermal_zone_device *tz, int >> trip) >> { >> - struct thermal_instance *instance; >> - >> thermal_zone_trip_update(tz, trip); >> >> - mutex_lock(&tz->lock); >> - >> - list_for_each_entry(instance, &tz->thermal_instances, tz_node) >> - thermal_cdev_update(instance->cdev); >> - >> - mutex_unlock(&tz->lock); >> - >> return 0; >> } >> >> diff --git a/drivers/thermal/step_wise.c >> b/drivers/thermal/step_wise.c >> index 2ae7198d3067..298eedac0293 100644 >> --- a/drivers/thermal/step_wise.c >> +++ b/drivers/thermal/step_wise.c >> @@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct >> thermal_zone_device *tz, int trip) >> mutex_lock(&instance->cdev->lock); >> instance->cdev->updated = false; /* cdev needs update >> */ >> mutex_unlock(&instance->cdev->lock); >> + thermal_cdev_update(instance->cdev); >> } >> >> mutex_unlock(&tz->lock); >> @@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct >> thermal_zone_device *tz, int trip) >> */ >> static int step_wise_throttle(struct thermal_zone_device *tz, int >> trip) >> { >> - struct thermal_instance *instance; >> - >> thermal_zone_trip_update(tz, trip); >> >> if (tz->forced_passive) >> thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE); >> >> - mutex_lock(&tz->lock); >> - >> - list_for_each_entry(instance, &tz->thermal_instances, tz_node) >> - thermal_cdev_update(instance->cdev); >> - >> - mutex_unlock(&tz->lock); >> - >> return 0; >> } >> >
On Fri, 2020-04-10 at 13:26 +0200, Daniel Lezcano wrote: > On 10/04/2020 12:14, Zhang Rui wrote: > > Hi, Daniel, > > > > On Thu, 2020-04-09 at 17:15 +0200, Daniel Lezcano wrote: > > > The call to the thermal_cdev_update() function is done after > > > browsing > > > the thermal instances which sets the updated flag by browsing > > > them > > > again. > > > > > > Instead of doing this, let's move the call right after setting > > > the > > > cooling device 'updated' flag as it is done in the other > > > governors. > > > > The reason we do this in two steps is that we want to avoid > > redundant > > cooling device state changes. > > > > Further more, I think it is better to move the thermal_cdev_update > > out > > of .throllte() callback, to thermal_zone_device_update(). So that > > we do > > not need to update the cooling device for each trip point. > > > > is there any specific reason we need to do thermal_cdev_update() > > for > > every potential change? > > I agree we can go further and move the cooling device update in the > thermal_zone_device_update() by letting the throttle callback let us > know an update is needed with the return value. > > Makes sense to provide more changes on top of those two patches ? Hmmm, without the update flag, we can only updating all the cooling devices blindly. And this is time consuming for some cooling devices. thanks, rui > > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > > --- > > > drivers/thermal/gov_bang_bang.c | 10 +--------- > > > drivers/thermal/step_wise.c | 10 +--------- > > > 2 files changed, 2 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/thermal/gov_bang_bang.c > > > b/drivers/thermal/gov_bang_bang.c > > > index 991a1c54296d..c292a69845bb 100644 > > > --- a/drivers/thermal/gov_bang_bang.c > > > +++ b/drivers/thermal/gov_bang_bang.c > > > @@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct > > > thermal_zone_device *tz, int trip) > > > mutex_lock(&instance->cdev->lock); > > > instance->cdev->updated = false; /* cdev needs update > > > */ > > > mutex_unlock(&instance->cdev->lock); > > > + thermal_cdev_update(instance->cdev); > > > } > > > > > > mutex_unlock(&tz->lock); > > > @@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct > > > thermal_zone_device *tz, int trip) > > > */ > > > static int bang_bang_control(struct thermal_zone_device *tz, int > > > trip) > > > { > > > - struct thermal_instance *instance; > > > - > > > thermal_zone_trip_update(tz, trip); > > > > > > - mutex_lock(&tz->lock); > > > - > > > - list_for_each_entry(instance, &tz->thermal_instances, tz_node) > > > - thermal_cdev_update(instance->cdev); > > > - > > > - mutex_unlock(&tz->lock); > > > - > > > return 0; > > > } > > > > > > diff --git a/drivers/thermal/step_wise.c > > > b/drivers/thermal/step_wise.c > > > index 2ae7198d3067..298eedac0293 100644 > > > --- a/drivers/thermal/step_wise.c > > > +++ b/drivers/thermal/step_wise.c > > > @@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct > > > thermal_zone_device *tz, int trip) > > > mutex_lock(&instance->cdev->lock); > > > instance->cdev->updated = false; /* cdev needs update > > > */ > > > mutex_unlock(&instance->cdev->lock); > > > + thermal_cdev_update(instance->cdev); > > > } > > > > > > mutex_unlock(&tz->lock); > > > @@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct > > > thermal_zone_device *tz, int trip) > > > */ > > > static int step_wise_throttle(struct thermal_zone_device *tz, > > > int > > > trip) > > > { > > > - struct thermal_instance *instance; > > > - > > > thermal_zone_trip_update(tz, trip); > > > > > > if (tz->forced_passive) > > > thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE); > > > > > > - mutex_lock(&tz->lock); > > > - > > > - list_for_each_entry(instance, &tz->thermal_instances, tz_node) > > > - thermal_cdev_update(instance->cdev); > > > - > > > - mutex_unlock(&tz->lock); > > > - > > > return 0; > > > } > > > > >
diff --git a/drivers/thermal/gov_bang_bang.c b/drivers/thermal/gov_bang_bang.c index 991a1c54296d..c292a69845bb 100644 --- a/drivers/thermal/gov_bang_bang.c +++ b/drivers/thermal/gov_bang_bang.c @@ -64,6 +64,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip) mutex_lock(&instance->cdev->lock); instance->cdev->updated = false; /* cdev needs update */ mutex_unlock(&instance->cdev->lock); + thermal_cdev_update(instance->cdev); } mutex_unlock(&tz->lock); @@ -98,17 +99,8 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip) */ static int bang_bang_control(struct thermal_zone_device *tz, int trip) { - struct thermal_instance *instance; - thermal_zone_trip_update(tz, trip); - mutex_lock(&tz->lock); - - list_for_each_entry(instance, &tz->thermal_instances, tz_node) - thermal_cdev_update(instance->cdev); - - mutex_unlock(&tz->lock); - return 0; } diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c index 2ae7198d3067..298eedac0293 100644 --- a/drivers/thermal/step_wise.c +++ b/drivers/thermal/step_wise.c @@ -167,6 +167,7 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip) mutex_lock(&instance->cdev->lock); instance->cdev->updated = false; /* cdev needs update */ mutex_unlock(&instance->cdev->lock); + thermal_cdev_update(instance->cdev); } mutex_unlock(&tz->lock); @@ -185,20 +186,11 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip) */ static int step_wise_throttle(struct thermal_zone_device *tz, int trip) { - struct thermal_instance *instance; - thermal_zone_trip_update(tz, trip); if (tz->forced_passive) thermal_zone_trip_update(tz, THERMAL_TRIPS_NONE); - mutex_lock(&tz->lock); - - list_for_each_entry(instance, &tz->thermal_instances, tz_node) - thermal_cdev_update(instance->cdev); - - mutex_unlock(&tz->lock); - return 0; }
The call to the thermal_cdev_update() function is done after browsing the thermal instances which sets the updated flag by browsing them again. Instead of doing this, let's move the call right after setting the cooling device 'updated' flag as it is done in the other governors. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/thermal/gov_bang_bang.c | 10 +--------- drivers/thermal/step_wise.c | 10 +--------- 2 files changed, 2 insertions(+), 18 deletions(-)