diff mbox series

thermal/core/power_allocator: avoid cdev->state can not be reset

Message ID 20230309135515.1232-1-di.shen@unisoc.com
State New
Headers
Series thermal/core/power_allocator: avoid cdev->state can not be reset |

Commit Message

Di Shen March 9, 2023, 1:55 p.m. UTC 
  Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
add a update flag to update cooling device only once when temp is low.
But when the switch_on_temp is set to be a higher value, the cooling device state
may not be reset to max, because the last_temp is smaller than the switch_on_temp.

For example:
First:
swicth_on_temp=70 control_temp=85;

Then userspace change the trip_temp:
swicth_on_temp=45 control_temp=55 cur_temp=54

Then userspace reset the trip_temp:
swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54

At this time, the cooling device state should be reset to be max.
However, because cur_temp(57) < switch_on_temp(70)
last_temp(54) < swicth_on_temp(70) --> update = false
When update is false, the cooling device state can not be reset.

So delete the update condition, so that the cooling device state
could be reset.

Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
Signed-off-by: Di Shen <di.shen@unisoc.com>
---
 drivers/thermal/gov_power_allocator.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Di Shen March 10, 2023, 7:31 a.m. UTC | #1 
Hi all,
any comments for this patch? Looking forward to your reply. Thank you.
Lukasz Luba March 10, 2023, 3:13 p.m. UTC | #2 
Hi Di,

On 3/9/23 13:55, Di Shen wrote:
> Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> add a update flag to update cooling device only once when temp is low.
> But when the switch_on_temp is set to be a higher value, the cooling device state
> may not be reset to max, because the last_temp is smaller than the switch_on_temp.
> 
> For example:
> First:
> swicth_on_temp=70 control_temp=85;
> 
> Then userspace change the trip_temp:
> swicth_on_temp=45 control_temp=55 cur_temp=54
> 
> Then userspace reset the trip_temp:
> swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
> 
> At this time, the cooling device state should be reset to be max.
> However, because cur_temp(57) < switch_on_temp(70)
> last_temp(54) < swicth_on_temp(70) --> update = false
> When update is false, the cooling device state can not be reset.

That's a tricky use case. How is that now possible,
> 
> So delete the update condition, so that the cooling device state
> could be reset.

IMO this is not the desired solution. Daniel reported the issue that
IPA triggers the event sent to user-space even when there is no need.
That's the motivation for the 0952177f2a1f change.

To address your scenario properly, we need an interface which allows
to respond properly for such situation when someone from user-space
writes a new value to something fundamental as trip point.

You also have a kernel config enabled:
CONFIG_THERMAL_WRITABLE_TRIPS
which IMO is only for debug kernels for system integrator (according
to the Kconfig description).

When you disable this config in your deploy/product kernel
than this issue would disappear.

> 
> Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> Signed-off-by: Di Shen <di.shen@unisoc.com>
> ---
>   drivers/thermal/gov_power_allocator.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 

That's why IMO this is not the solution.

Regards,
Lukasz
Xuewen Yan March 13, 2023, 1:40 a.m. UTC | #3 
Hi Lukasz

On Fri, Mar 10, 2023 at 11:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Di,
>
> On 3/9/23 13:55, Di Shen wrote:
> > Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> > add a update flag to update cooling device only once when temp is low.
> > But when the switch_on_temp is set to be a higher value, the cooling device state
> > may not be reset to max, because the last_temp is smaller than the switch_on_temp.
> >
> > For example:
> > First:
> > swicth_on_temp=70 control_temp=85;
> >
> > Then userspace change the trip_temp:
> > swicth_on_temp=45 control_temp=55 cur_temp=54
> >
> > Then userspace reset the trip_temp:
> > swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
> >
> > At this time, the cooling device state should be reset to be max.
> > However, because cur_temp(57) < switch_on_temp(70)
> > last_temp(54) < swicth_on_temp(70) --> update = false
> > When update is false, the cooling device state can not be reset.
>
> That's a tricky use case. How is that now possible,

We use the trip_temp in the Android System. Often, we set different
control temperatures in different scenarios,
and when we change the switch_on_temp from small to bigger, we find
the power can not be reset to be max.


> >
> > So delete the update condition, so that the cooling device state
> > could be reset.
>
> IMO this is not the desired solution. Daniel reported the issue that
> IPA triggers the event sent to user-space even when there is no need.
> That's the motivation for the 0952177f2a1f change.
>
> To address your scenario properly, we need an interface which allows
> to respond properly for such situation when someone from user-space
> writes a new value to something fundamental as trip point.
>
> You also have a kernel config enabled:
> CONFIG_THERMAL_WRITABLE_TRIPS
> which IMO is only for debug kernels for system integrator (according
> to the Kconfig description).

 Yes, we use it to meet the temperature control needs of different scenarios.
And now in android with google's GKI2.0, the config must be opened.

>
> When you disable this config in your deploy/product kernel
> than this issue would disappear.
>
> >
> > Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> > Signed-off-by: Di Shen <di.shen@unisoc.com>
> > ---
> >   drivers/thermal/gov_power_allocator.c | 9 +++------
> >   1 file changed, 3 insertions(+), 6 deletions(-)
> >
>
> That's why IMO this is not the solution.

Yes, but I think we should fix the bug, although the
CONFIG_THERMAL_WRITABLE_TRIPS is just for debugging.
How about record the last_trip_temp, and when the last_temp >
last_trip_temp, set the update tobe true?

Thanks!
BR,
xuewen

>
> Regards,
> Lukasz
Lukasz Luba March 13, 2023, 9:35 a.m. UTC | #4 
Hi Xuewen,

On 3/13/23 01:40, Xuewen Yan wrote:
> Hi Lukasz
> 
> On Fri, Mar 10, 2023 at 11:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Di,
>>
>> On 3/9/23 13:55, Di Shen wrote:
>>> Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
>>> add a update flag to update cooling device only once when temp is low.
>>> But when the switch_on_temp is set to be a higher value, the cooling device state
>>> may not be reset to max, because the last_temp is smaller than the switch_on_temp.
>>>
>>> For example:
>>> First:
>>> swicth_on_temp=70 control_temp=85;
>>>
>>> Then userspace change the trip_temp:
>>> swicth_on_temp=45 control_temp=55 cur_temp=54
>>>
>>> Then userspace reset the trip_temp:
>>> swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
>>>
>>> At this time, the cooling device state should be reset to be max.
>>> However, because cur_temp(57) < switch_on_temp(70)
>>> last_temp(54) < swicth_on_temp(70) --> update = false
>>> When update is false, the cooling device state can not be reset.
>>
>> That's a tricky use case. How is that now possible,
> 
> We use the trip_temp in the Android System. Often, we set different
> control temperatures in different scenarios,
> and when we change the switch_on_temp from small to bigger, we find
> the power can not be reset to be max.

I see, thanks for letting me know that this is Android.

> 
> 
>>>
>>> So delete the update condition, so that the cooling device state
>>> could be reset.
>>
>> IMO this is not the desired solution. Daniel reported the issue that
>> IPA triggers the event sent to user-space even when there is no need.
>> That's the motivation for the 0952177f2a1f change.
>>
>> To address your scenario properly, we need an interface which allows
>> to respond properly for such situation when someone from user-space
>> writes a new value to something fundamental as trip point.
>>
>> You also have a kernel config enabled:
>> CONFIG_THERMAL_WRITABLE_TRIPS
>> which IMO is only for debug kernels for system integrator (according
>> to the Kconfig description).
> 
>   Yes, we use it to meet the temperature control needs of different scenarios.
> And now in android with google's GKI2.0, the config must be opened.

OK

> 
>>
>> When you disable this config in your deploy/product kernel
>> than this issue would disappear.
>>
>>>
>>> Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
>>> Signed-off-by: Di Shen <di.shen@unisoc.com>
>>> ---
>>>    drivers/thermal/gov_power_allocator.c | 9 +++------
>>>    1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>
>> That's why IMO this is not the solution.
> 
> Yes, but I think we should fix the bug, although the
> CONFIG_THERMAL_WRITABLE_TRIPS is just for debugging.
> How about record the last_trip_temp, and when the last_temp >
> last_trip_temp, set the update tobe true?

Yes, if that config is used in Android then we must fix it.

That last_trip_temp makes sense (but maybe name it last_switch_on_temp).
Please put that new field into the IPA local
struct power_allocator_params. We should store the trip temp
value there every time power_allocator_throttle() is called.
That can be called due to a write from user-space w/ a new trip point
value, so should be OK.

Regards,
Lukasz
Xuewen Yan March 13, 2023, 11:10 a.m. UTC | #5 
Hi Lukasz

On Mon, Mar 13, 2023 at 5:35 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Xuewen,
>
> On 3/13/23 01:40, Xuewen Yan wrote:
> > Hi Lukasz
> >
> > On Fri, Mar 10, 2023 at 11:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Hi Di,
> >>
> >> On 3/9/23 13:55, Di Shen wrote:
> >>> Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> >>> add a update flag to update cooling device only once when temp is low.
> >>> But when the switch_on_temp is set to be a higher value, the cooling device state
> >>> may not be reset to max, because the last_temp is smaller than the switch_on_temp.
> >>>
> >>> For example:
> >>> First:
> >>> swicth_on_temp=70 control_temp=85;
> >>>
> >>> Then userspace change the trip_temp:
> >>> swicth_on_temp=45 control_temp=55 cur_temp=54
> >>>
> >>> Then userspace reset the trip_temp:
> >>> swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
> >>>
> >>> At this time, the cooling device state should be reset to be max.
> >>> However, because cur_temp(57) < switch_on_temp(70)
> >>> last_temp(54) < swicth_on_temp(70) --> update = false
> >>> When update is false, the cooling device state can not be reset.
> >>
> >> That's a tricky use case. How is that now possible,
> >
> > We use the trip_temp in the Android System. Often, we set different
> > control temperatures in different scenarios,
> > and when we change the switch_on_temp from small to bigger, we find
> > the power can not be reset to be max.
>
> I see, thanks for letting me know that this is Android.
>
> >
> >
> >>>
> >>> So delete the update condition, so that the cooling device state
> >>> could be reset.
> >>
> >> IMO this is not the desired solution. Daniel reported the issue that
> >> IPA triggers the event sent to user-space even when there is no need.
> >> That's the motivation for the 0952177f2a1f change.
> >>
> >> To address your scenario properly, we need an interface which allows
> >> to respond properly for such situation when someone from user-space
> >> writes a new value to something fundamental as trip point.
> >>
> >> You also have a kernel config enabled:
> >> CONFIG_THERMAL_WRITABLE_TRIPS
> >> which IMO is only for debug kernels for system integrator (according
> >> to the Kconfig description).
> >
> >   Yes, we use it to meet the temperature control needs of different scenarios.
> > And now in android with google's GKI2.0, the config must be opened.
>
> OK
>
> >
> >>
> >> When you disable this config in your deploy/product kernel
> >> than this issue would disappear.
> >>
> >>>
> >>> Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> >>> Signed-off-by: Di Shen <di.shen@unisoc.com>
> >>> ---
> >>>    drivers/thermal/gov_power_allocator.c | 9 +++------
> >>>    1 file changed, 3 insertions(+), 6 deletions(-)
> >>>
> >>
> >> That's why IMO this is not the solution.
> >
> > Yes, but I think we should fix the bug, although the
> > CONFIG_THERMAL_WRITABLE_TRIPS is just for debugging.
> > How about record the last_trip_temp, and when the last_temp >
> > last_trip_temp, set the update tobe true?
>
> Yes, if that config is used in Android then we must fix it.
>
> That last_trip_temp makes sense (but maybe name it last_switch_on_temp).
> Please put that new field into the IPA local
> struct power_allocator_params. We should store the trip temp
> value there every time power_allocator_throttle() is called.
> That can be called due to a write from user-space w/ a new trip point
> value, so should be OK.

Thanks for the suggestion. We would send the patch-v2 as soon as possible.

Thanks!
BR
xuewen
Lukasz Luba March 13, 2023, 11:18 a.m. UTC | #6 
On 3/13/23 11:10, Xuewen Yan wrote:
> Hi Lukasz
> 
> On Mon, Mar 13, 2023 at 5:35 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Hi Xuewen,
>>
>> On 3/13/23 01:40, Xuewen Yan wrote:
>>> Hi Lukasz
>>>
>>> On Fri, Mar 10, 2023 at 11:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>>>
>>>> Hi Di,
>>>>
>>>> On 3/9/23 13:55, Di Shen wrote:
>>>>> Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
>>>>> add a update flag to update cooling device only once when temp is low.
>>>>> But when the switch_on_temp is set to be a higher value, the cooling device state
>>>>> may not be reset to max, because the last_temp is smaller than the switch_on_temp.
>>>>>
>>>>> For example:
>>>>> First:
>>>>> swicth_on_temp=70 control_temp=85;
>>>>>
>>>>> Then userspace change the trip_temp:
>>>>> swicth_on_temp=45 control_temp=55 cur_temp=54
>>>>>
>>>>> Then userspace reset the trip_temp:
>>>>> swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
>>>>>
>>>>> At this time, the cooling device state should be reset to be max.
>>>>> However, because cur_temp(57) < switch_on_temp(70)
>>>>> last_temp(54) < swicth_on_temp(70) --> update = false
>>>>> When update is false, the cooling device state can not be reset.
>>>>
>>>> That's a tricky use case. How is that now possible,
>>>
>>> We use the trip_temp in the Android System. Often, we set different
>>> control temperatures in different scenarios,
>>> and when we change the switch_on_temp from small to bigger, we find
>>> the power can not be reset to be max.
>>
>> I see, thanks for letting me know that this is Android.
>>
>>>
>>>
>>>>>
>>>>> So delete the update condition, so that the cooling device state
>>>>> could be reset.
>>>>
>>>> IMO this is not the desired solution. Daniel reported the issue that
>>>> IPA triggers the event sent to user-space even when there is no need.
>>>> That's the motivation for the 0952177f2a1f change.
>>>>
>>>> To address your scenario properly, we need an interface which allows
>>>> to respond properly for such situation when someone from user-space
>>>> writes a new value to something fundamental as trip point.
>>>>
>>>> You also have a kernel config enabled:
>>>> CONFIG_THERMAL_WRITABLE_TRIPS
>>>> which IMO is only for debug kernels for system integrator (according
>>>> to the Kconfig description).
>>>
>>>    Yes, we use it to meet the temperature control needs of different scenarios.
>>> And now in android with google's GKI2.0, the config must be opened.
>>
>> OK
>>
>>>
>>>>
>>>> When you disable this config in your deploy/product kernel
>>>> than this issue would disappear.
>>>>
>>>>>
>>>>> Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
>>>>> Signed-off-by: Di Shen <di.shen@unisoc.com>
>>>>> ---
>>>>>     drivers/thermal/gov_power_allocator.c | 9 +++------
>>>>>     1 file changed, 3 insertions(+), 6 deletions(-)
>>>>>
>>>>
>>>> That's why IMO this is not the solution.
>>>
>>> Yes, but I think we should fix the bug, although the
>>> CONFIG_THERMAL_WRITABLE_TRIPS is just for debugging.
>>> How about record the last_trip_temp, and when the last_temp >
>>> last_trip_temp, set the update tobe true?
>>
>> Yes, if that config is used in Android then we must fix it.
>>
>> That last_trip_temp makes sense (but maybe name it last_switch_on_temp).
>> Please put that new field into the IPA local
>> struct power_allocator_params. We should store the trip temp
>> value there every time power_allocator_throttle() is called.
>> That can be called due to a write from user-space w/ a new trip point
>> value, so should be OK.
> 
> Thanks for the suggestion. We would send the patch-v2 as soon as possible.

Thanks!
I'll review that and check on my board.
BTW, which board/device you use with this IPA? Maybe I can buy it
and also test to capture such regression in future.
Xuewen Yan March 14, 2023, 2:41 a.m. UTC | #7 
add Orson and lyra

Hi Lukasz

On Mon, Mar 13, 2023 at 7:18 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 3/13/23 11:10, Xuewen Yan wrote:
> > Hi Lukasz
> >
> > On Mon, Mar 13, 2023 at 5:35 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>
> >> Hi Xuewen,
> >>
> >> On 3/13/23 01:40, Xuewen Yan wrote:
> >>> Hi Lukasz
> >>>
> >>> On Fri, Mar 10, 2023 at 11:56 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
> >>>>
> >>>> Hi Di,
> >>>>
> >>>> On 3/9/23 13:55, Di Shen wrote:
> >>>>> Commit 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> >>>>> add a update flag to update cooling device only once when temp is low.
> >>>>> But when the switch_on_temp is set to be a higher value, the cooling device state
> >>>>> may not be reset to max, because the last_temp is smaller than the switch_on_temp.
> >>>>>
> >>>>> For example:
> >>>>> First:
> >>>>> swicth_on_temp=70 control_temp=85;
> >>>>>
> >>>>> Then userspace change the trip_temp:
> >>>>> swicth_on_temp=45 control_temp=55 cur_temp=54
> >>>>>
> >>>>> Then userspace reset the trip_temp:
> >>>>> swicth_on_temp=70 control_temp=85 cur_temp=57 last_temp=54
> >>>>>
> >>>>> At this time, the cooling device state should be reset to be max.
> >>>>> However, because cur_temp(57) < switch_on_temp(70)
> >>>>> last_temp(54) < swicth_on_temp(70) --> update = false
> >>>>> When update is false, the cooling device state can not be reset.
> >>>>
> >>>> That's a tricky use case. How is that now possible,
> >>>
> >>> We use the trip_temp in the Android System. Often, we set different
> >>> control temperatures in different scenarios,
> >>> and when we change the switch_on_temp from small to bigger, we find
> >>> the power can not be reset to be max.
> >>
> >> I see, thanks for letting me know that this is Android.
> >>
> >>>
> >>>
> >>>>>
> >>>>> So delete the update condition, so that the cooling device state
> >>>>> could be reset.
> >>>>
> >>>> IMO this is not the desired solution. Daniel reported the issue that
> >>>> IPA triggers the event sent to user-space even when there is no need.
> >>>> That's the motivation for the 0952177f2a1f change.
> >>>>
> >>>> To address your scenario properly, we need an interface which allows
> >>>> to respond properly for such situation when someone from user-space
> >>>> writes a new value to something fundamental as trip point.
> >>>>
> >>>> You also have a kernel config enabled:
> >>>> CONFIG_THERMAL_WRITABLE_TRIPS
> >>>> which IMO is only for debug kernels for system integrator (according
> >>>> to the Kconfig description).
> >>>
> >>>    Yes, we use it to meet the temperature control needs of different scenarios.
> >>> And now in android with google's GKI2.0, the config must be opened.
> >>
> >> OK
> >>
> >>>
> >>>>
> >>>> When you disable this config in your deploy/product kernel
> >>>> than this issue would disappear.
> >>>>
> >>>>>
> >>>>> Fixes: 0952177f2a1f (thermal/core/power_allocator: Update once cooling devices when temp is low)
> >>>>> Signed-off-by: Di Shen <di.shen@unisoc.com>
> >>>>> ---
> >>>>>     drivers/thermal/gov_power_allocator.c | 9 +++------
> >>>>>     1 file changed, 3 insertions(+), 6 deletions(-)
> >>>>>
> >>>>
> >>>> That's why IMO this is not the solution.
> >>>
> >>> Yes, but I think we should fix the bug, although the
> >>> CONFIG_THERMAL_WRITABLE_TRIPS is just for debugging.
> >>> How about record the last_trip_temp, and when the last_temp >
> >>> last_trip_temp, set the update tobe true?
> >>
> >> Yes, if that config is used in Android then we must fix it.
> >>
> >> That last_trip_temp makes sense (but maybe name it last_switch_on_temp).
> >> Please put that new field into the IPA local
> >> struct power_allocator_params. We should store the trip temp
> >> value there every time power_allocator_throttle() is called.
> >> That can be called due to a write from user-space w/ a new trip point
> >> value, so should be OK.
> >
> > Thanks for the suggestion. We would send the patch-v2 as soon as possible.
>
> Thanks!
> I'll review that and check on my board.
> BTW, which board/device you use with this IPA? Maybe I can buy it
> and also test to capture such regression in future.

We use our company(unisoc)'s mobile phone, and the development board
with the same chip should be available in the market. And Orson would
help and then reply to you.

Thanks!
BR
xuewen
diff mbox series

Patch

diff --git a/drivers/thermal/gov_power_allocator.c b/drivers/thermal/gov_power_allocator.c
index 0eaf1527d3e3..153bf528b98c 100644
--- a/drivers/thermal/gov_power_allocator.c
+++ b/drivers/thermal/gov_power_allocator.c
@@ -560,7 +560,7 @@  static void reset_pid_controller(struct power_allocator_params *params)
 	params->prev_err = 0;
 }
 
-static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
+static void allow_maximum_power(struct thermal_zone_device *tz)
 {
 	struct thermal_instance *instance;
 	struct power_allocator_params *params = tz->governor_data;
@@ -582,8 +582,7 @@  static void allow_maximum_power(struct thermal_zone_device *tz, bool update)
 		 */
 		cdev->ops->get_requested_power(cdev, &req_power);
 
-		if (update)
-			__thermal_cdev_update(instance->cdev);
+		__thermal_cdev_update(instance->cdev);
 
 		mutex_unlock(&instance->cdev->lock);
 	}
@@ -697,7 +696,6 @@  static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
 	struct power_allocator_params *params = tz->governor_data;
 	struct thermal_trip trip;
 	int ret;
-	bool update;
 
 	lockdep_assert_held(&tz->lock);
 
@@ -710,10 +708,9 @@  static int power_allocator_throttle(struct thermal_zone_device *tz, int trip_id)
 
 	ret = __thermal_zone_get_trip(tz, params->trip_switch_on, &trip);
 	if (!ret && (tz->temperature < trip.temperature)) {
-		update = (tz->last_temperature >= trip.temperature);
 		tz->passive = 0;
 		reset_pid_controller(params);
-		allow_maximum_power(tz, update);
+		allow_maximum_power(tz);
 		return 0;
 	}