Re: [PATCH] thermal/core/power_allocator: avoid cdev->state can not be reset

[Xuewen Yan]

Hi Lukasz

On Mon, Mar 13, 2023 at 5:35 PM Lukasz Luba <lukasz.luba@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>>    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