| Message ID | 1643307093-22501-1-git-send-email-quic_manafm@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] drivers: thermal: clear all mitigation when thermal zone is disabled | expand |
Hi Manaf, On 1/27/22 6:11 PM, Manaf Meethalavalappu Pallikunhi wrote: > Whenever a thermal zone is in trip violated state, there is a chance > that the same thermal zone mode can be disabled either via > thermal core API or via thermal zone sysfs. Once it is disabled, > the framework bails out any re-evaluation of thermal zone. It leads > to a case where if it is already in mitigation state, it will stay > the same state forever. > > To avoid above mentioned issue, add support to bind/unbind > governor from thermal zone during thermal zone mode change request > and clear all existing throttling in governor unbind_from_tz() > callback. I have one use case: This would be a bit dangerous, e.g. to switch governors while there is a high temperature. Although, sounds reasonable to left a 'default' state for a next governor. > > Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com> > --- > drivers/thermal/gov_power_allocator.c | 3 +++ > drivers/thermal/gov_step_wise.c | 26 ++++++++++++++++++++++++++ > drivers/thermal/thermal_core.c | 31 +++++++++++++++++++++++++++---- > 3 files changed, 56 insertions(+), 4 deletions(-) Why only two governors need that change and not all? Because they don't have 'bind/unbind' callbacks, then maybe we should change that as well to make it consistent? Regards, Lukasz
On 1/31/2022 12:55 PM, Lukasz Luba wrote: > Hi Manaf, > > On 1/27/22 6:11 PM, Manaf Meethalavalappu Pallikunhi wrote: >> Whenever a thermal zone is in trip violated state, there is a chance >> that the same thermal zone mode can be disabled either via >> thermal core API or via thermal zone sysfs. Once it is disabled, >> the framework bails out any re-evaluation of thermal zone. It leads >> to a case where if it is already in mitigation state, it will stay >> the same state forever. >> >> To avoid above mentioned issue, add support to bind/unbind >> governor from thermal zone during thermal zone mode change request >> and clear all existing throttling in governor unbind_from_tz() >> callback. > > I have one use case: > This would be a bit dangerous, e.g. to switch governors while there is a > high temperature. Although, sounds reasonable to left a 'default' state > for a next governor. > I believe only way to change the governror via userspace at runtime. Just re-evaluate thermal zone (thermal_zone_device_update) immediately after thermal_zone_device_set_policy() in same policy_store() context, isn't it good enough ? Not sure how a "default" state can be reverted once governor change is done. Re-evaluating thermal zone doesn't guarantee that it will recover previous set default state for all governors, right ? >> >> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> Signed-off-by: Manaf Meethalavalappu Pallikunhi >> <quic_manafm@quicinc.com> >> --- >> drivers/thermal/gov_power_allocator.c | 3 +++ >> drivers/thermal/gov_step_wise.c | 26 ++++++++++++++++++++++++++ >> drivers/thermal/thermal_core.c | 31 >> +++++++++++++++++++++++++++---- >> 3 files changed, 56 insertions(+), 4 deletions(-) > > Why only two governors need that change and not all? > Because they don't have 'bind/unbind' callbacks, then maybe we should > change that as well to make it consistent? I will update other governors as well in v6 > > Regards, > Lukasz
On 2/14/22 8:00 PM, Manaf Meethalavalappu Pallikunhi wrote: > > On 1/31/2022 12:55 PM, Lukasz Luba wrote: >> Hi Manaf, >> >> On 1/27/22 6:11 PM, Manaf Meethalavalappu Pallikunhi wrote: >>> Whenever a thermal zone is in trip violated state, there is a chance >>> that the same thermal zone mode can be disabled either via >>> thermal core API or via thermal zone sysfs. Once it is disabled, >>> the framework bails out any re-evaluation of thermal zone. It leads >>> to a case where if it is already in mitigation state, it will stay >>> the same state forever. >>> >>> To avoid above mentioned issue, add support to bind/unbind >>> governor from thermal zone during thermal zone mode change request >>> and clear all existing throttling in governor unbind_from_tz() >>> callback. >> >> I have one use case: >> This would be a bit dangerous, e.g. to switch governors while there is a >> high temperature. Although, sounds reasonable to left a 'default' state >> for a next governor. >> > I believe only way to change the governror via userspace at runtime. > > Just re-evaluate thermal zone (thermal_zone_device_update) immediately > after > > thermal_zone_device_set_policy() in same policy_store() context, isn't > it good enough ? It depends. The code would switch the governors very fast, in the meantime notifying about possible full speed of CPU (cooling state = 0). If the task scheduler goes via schedutil (cpufreq governor) at that moment and decides to set this max frequency, it will be set. This is situation with your patch, since you added in IPA unbind 'allow_maximum_power()'. Then the new governor is bind, evaluates the max cooling state, the notification about reduced max freq is sent to schedutil (a workqueue will call .sugov_limits() callback) and lower freq would be set. Now there are things which are not greatly covered by these 4 involved sub-systems (thermal fwk, schedutil, scheduler, HW). It takes time. It also depends when the actual HW freq is possible to be set. It might take a few milli-seconds or even a dozes of milli-seconds (depends on HW). Without your change, we avoid such situation while switching the thermal governors. For your requirement, which is 'mode' enable/disable it OK to un-throttle. It's probably something to Rafael and Daniel to judge if we want to pay that cost and introduce this racy time slot. Maybe there is a way to implement your needed feature differently. Unfortunately, I'm super busy with other stuff this month so I cannot spent much time investigating this. > > Not sure how a "default" state can be reverted once governor change is > done. > > Re-evaluating thermal zone doesn't guarantee that it will recover previous > > set default state for all governors, right ? > >>> >>> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> Signed-off-by: Manaf Meethalavalappu Pallikunhi >>> <quic_manafm@quicinc.com> >>> --- >>> drivers/thermal/gov_power_allocator.c | 3 +++ >>> drivers/thermal/gov_step_wise.c | 26 ++++++++++++++++++++++++++ >>> drivers/thermal/thermal_core.c | 31 >>> +++++++++++++++++++++++++++---- >>> 3 files changed, 56 insertions(+), 4 deletions(-) >> >> Why only two governors need that change and not all? >> Because they don't have 'bind/unbind' callbacks, then maybe we should >> change that as well to make it consistent? > I will update other governors as well in v6 Sounds reasonable based on your code (you've added the unbind_from_tz() callback to step_wise, but not for others).
On 27/01/2022 19:11, Manaf Meethalavalappu Pallikunhi wrote: > Whenever a thermal zone is in trip violated state, there is a chance > that the same thermal zone mode can be disabled either via > thermal core API or via thermal zone sysfs. Once it is disabled, > the framework bails out any re-evaluation of thermal zone. It leads > to a case where if it is already in mitigation state, it will stay > the same state forever. > > To avoid above mentioned issue, add support to bind/unbind > governor from thermal zone during thermal zone mode change request > and clear all existing throttling in governor unbind_from_tz() > callback. Manaf, can you clarify when / why is needed to disable a thermal zone at runtime? > Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com> > --- > drivers/thermal/gov_power_allocator.c | 3 +++ > drivers/thermal/gov_step_wise.c | 26 ++++++++++++++++++++++++++ > drivers/thermal/thermal_core.c | 31 +++++++++++++++++++++++++++---- > 3 files changed, 56 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c > index 13e3757..9ff0c5f 100644 > --- a/drivers/thermal/gov_power_allocator.c > +++ b/drivers/thermal/gov_power_allocator.c > @@ -696,6 +696,9 @@ static void power_allocator_unbind(struct thermal_zone_device *tz) > > dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id); > > + tz->passive = 0; > + allow_maximum_power(tz, true); > + > if (params->allocated_tzp) { > kfree(tz->tzp); > tz->tzp = NULL; > diff --git a/drivers/thermal/gov_step_wise.c b/drivers/thermal/gov_step_wise.c > index 12acb12..2d2186b 100644 > --- a/drivers/thermal/gov_step_wise.c > +++ b/drivers/thermal/gov_step_wise.c > @@ -168,6 +168,31 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip) > } > > /** > + * step_wise_unbind() - unbind the step_wise governor to a thermal zone > + * @tz: thermal zone to unbind it to > + * > + * Clear all previous throttling and reset passive counter. > + * > + */ > +static void step_wise_unbind(struct thermal_zone_device *tz) > +{ > + struct thermal_instance *instance; > + > + dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id); > + > + mutex_lock(&tz->lock); > + tz->passive = 0; > + list_for_each_entry(instance, &tz->thermal_instances, tz_node) { > + instance->initialized = false; > + instance->target = THERMAL_NO_TARGET; > + mutex_lock(&instance->cdev->lock); > + __thermal_cdev_update(instance->cdev); > + mutex_unlock(&instance->cdev->lock); > + } > + mutex_unlock(&tz->lock); > +} > + > +/** > * step_wise_throttle - throttles devices associated with the given zone > * @tz: thermal_zone_device > * @trip: trip point index > @@ -196,6 +221,7 @@ static int step_wise_throttle(struct thermal_zone_device *tz, int trip) > > static struct thermal_governor thermal_gov_step_wise = { > .name = "step_wise", > + .unbind_from_tz = step_wise_unbind, > .throttle = step_wise_throttle, > }; > THERMAL_GOVERNOR_DECLARE(thermal_gov_step_wise); > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 82654dc..34726d3 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -274,6 +274,26 @@ static int __init thermal_register_governors(void) > return ret; > } > > +static void thermal_governor_attach(struct thermal_zone_device *tz) > +{ > + mutex_lock(&thermal_governor_lock); > + if (tz->governor && tz->governor->bind_to_tz) { > + if (tz->governor->bind_to_tz(tz)) > + dev_err(&tz->device, > + "governor %s failed to bind to thermal zone %s\n", > + tz->governor->name, tz->type); > + } > + mutex_unlock(&thermal_governor_lock); > +} > + > +static void thermal_governor_detach(struct thermal_zone_device *tz) > +{ > + mutex_lock(&thermal_governor_lock); > + if (tz->governor && tz->governor->unbind_from_tz) > + tz->governor->unbind_from_tz(tz); > + mutex_unlock(&thermal_governor_lock); > +} > + > /* > * Zone update section: main control loop applied to each zone while monitoring > * > @@ -449,12 +469,15 @@ static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, > > mutex_unlock(&tz->lock); > > - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > - > - if (mode == THERMAL_DEVICE_ENABLED) > + if (mode == THERMAL_DEVICE_ENABLED) { > + thermal_governor_attach(tz); > + thermal_zone_device_init(tz); > + thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > thermal_notify_tz_enable(tz->id); > - else > + } else { > + thermal_governor_detach(tz); > thermal_notify_tz_disable(tz->id); > + } > > return ret; > }
On 15/02/2022 10:57, Lukasz Luba wrote: > > > On 2/14/22 8:00 PM, Manaf Meethalavalappu Pallikunhi wrote: >> >> On 1/31/2022 12:55 PM, Lukasz Luba wrote: >>> Hi Manaf, >>> >>> On 1/27/22 6:11 PM, Manaf Meethalavalappu Pallikunhi wrote: >>>> Whenever a thermal zone is in trip violated state, there is a chance >>>> that the same thermal zone mode can be disabled either via >>>> thermal core API or via thermal zone sysfs. Once it is disabled, >>>> the framework bails out any re-evaluation of thermal zone. It leads >>>> to a case where if it is already in mitigation state, it will stay >>>> the same state forever. >>>> >>>> To avoid above mentioned issue, add support to bind/unbind >>>> governor from thermal zone during thermal zone mode change request >>>> and clear all existing throttling in governor unbind_from_tz() >>>> callback. >>> >>> I have one use case: >>> This would be a bit dangerous, e.g. to switch governors while there is a >>> high temperature. Although, sounds reasonable to left a 'default' state >>> for a next governor. >>> >> I believe only way to change the governror via userspace at runtime. >> >> Just re-evaluate thermal zone (thermal_zone_device_update) >> immediately after >> >> thermal_zone_device_set_policy() in same policy_store() context, >> isn't it good enough ? > > It depends. The code would switch the governors very fast, in the > meantime notifying about possible full speed of CPU (cooling state = 0). > If the task scheduler goes via schedutil (cpufreq governor) at that > moment and decides to set this max frequency, it will be set. > This is situation with your patch, since you added in IPA unbind > 'allow_maximum_power()'. > Then the new governor is bind, evaluates the max cooling state, the > notification about reduced max freq is sent to schedutil (a workqueue > will call .sugov_limits() callback) and lower freq would be set. > > Now there are things which are not greatly covered by these 4 > involved sub-systems (thermal fwk, schedutil, scheduler, HW). > It takes time. It also depends when the actual HW freq is possible to be > set. It might take a few milli-seconds or even a dozes of milli-seconds > (depends on HW). > > Without your change, we avoid such situation while switching the > thermal governors. > > For your requirement, which is 'mode' enable/disable it OK to > un-throttle. > > It's probably something to Rafael and Daniel to judge if we want to > pay that cost and introduce this racy time slot. > > Maybe there is a way to implement your needed feature differently. > Unfortunately, I'm super busy with other stuff this month so I cannot > spent much time investigating this. IMO, we should be able to disable a thermal zone, no mitigation will happen but somehow we can keep the hot / critical trip points enabled, so if the temperature crosses these that would trigger an action for safety. However, for me it is still unclear what means "disabling a thermal zone" exactly ? Maybe we should clarify that before going further
diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c index 13e3757..9ff0c5f 100644 --- a/drivers/thermal/gov_power_allocator.c +++ b/drivers/thermal/gov_power_allocator.c @@ -696,6 +696,9 @@ static void power_allocator_unbind(struct thermal_zone_device *tz) dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id); + tz->passive = 0; + allow_maximum_power(tz, true); + if (params->allocated_tzp) { kfree(tz->tzp); tz->tzp = NULL; diff --git a/drivers/thermal/gov_step_wise.c b/drivers/thermal/gov_step_wise.c index 12acb12..2d2186b 100644 --- a/drivers/thermal/gov_step_wise.c +++ b/drivers/thermal/gov_step_wise.c @@ -168,6 +168,31 @@ static void thermal_zone_trip_update(struct thermal_zone_device *tz, int trip) } /** + * step_wise_unbind() - unbind the step_wise governor to a thermal zone + * @tz: thermal zone to unbind it to + * + * Clear all previous throttling and reset passive counter. + * + */ +static void step_wise_unbind(struct thermal_zone_device *tz) +{ + struct thermal_instance *instance; + + dev_dbg(&tz->device, "Unbinding from thermal zone %d\n", tz->id); + + mutex_lock(&tz->lock); + tz->passive = 0; + list_for_each_entry(instance, &tz->thermal_instances, tz_node) { + instance->initialized = false; + instance->target = THERMAL_NO_TARGET; + mutex_lock(&instance->cdev->lock); + __thermal_cdev_update(instance->cdev); + mutex_unlock(&instance->cdev->lock); + } + mutex_unlock(&tz->lock); +} + +/** * step_wise_throttle - throttles devices associated with the given zone * @tz: thermal_zone_device * @trip: trip point index @@ -196,6 +221,7 @@ static int step_wise_throttle(struct thermal_zone_device *tz, int trip) static struct thermal_governor thermal_gov_step_wise = { .name = "step_wise", + .unbind_from_tz = step_wise_unbind, .throttle = step_wise_throttle, }; THERMAL_GOVERNOR_DECLARE(thermal_gov_step_wise); diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 82654dc..34726d3 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -274,6 +274,26 @@ static int __init thermal_register_governors(void) return ret; } +static void thermal_governor_attach(struct thermal_zone_device *tz) +{ + mutex_lock(&thermal_governor_lock); + if (tz->governor && tz->governor->bind_to_tz) { + if (tz->governor->bind_to_tz(tz)) + dev_err(&tz->device, + "governor %s failed to bind to thermal zone %s\n", + tz->governor->name, tz->type); + } + mutex_unlock(&thermal_governor_lock); +} + +static void thermal_governor_detach(struct thermal_zone_device *tz) +{ + mutex_lock(&thermal_governor_lock); + if (tz->governor && tz->governor->unbind_from_tz) + tz->governor->unbind_from_tz(tz); + mutex_unlock(&thermal_governor_lock); +} + /* * Zone update section: main control loop applied to each zone while monitoring * @@ -449,12 +469,15 @@ static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, mutex_unlock(&tz->lock); - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); - - if (mode == THERMAL_DEVICE_ENABLED) + if (mode == THERMAL_DEVICE_ENABLED) { + thermal_governor_attach(tz); + thermal_zone_device_init(tz); + thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); thermal_notify_tz_enable(tz->id); - else + } else { + thermal_governor_detach(tz); thermal_notify_tz_disable(tz->id); + } return ret; }
Whenever a thermal zone is in trip violated state, there is a chance that the same thermal zone mode can be disabled either via thermal core API or via thermal zone sysfs. Once it is disabled, the framework bails out any re-evaluation of thermal zone. It leads to a case where if it is already in mitigation state, it will stay the same state forever. To avoid above mentioned issue, add support to bind/unbind governor from thermal zone during thermal zone mode change request and clear all existing throttling in governor unbind_from_tz() callback. Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org> Signed-off-by: Manaf Meethalavalappu Pallikunhi <quic_manafm@quicinc.com> --- drivers/thermal/gov_power_allocator.c | 3 +++ drivers/thermal/gov_step_wise.c | 26 ++++++++++++++++++++++++++ drivers/thermal/thermal_core.c | 31 +++++++++++++++++++++++++++---- 3 files changed, 56 insertions(+), 4 deletions(-)