Re: [PATCH 3/3] powercap/drivers/idle_inject: Specify the idle state to inject

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Tue, 24 Sep 2019 at 07:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
>
> Hi Mathieu,
>
> On 18/09/2019 21:35, Mathieu Poirier wrote:
> > On Mon, Sep 09, 2019 at 04:50:15PM +0200, Daniel Lezcano wrote:
> >> Currently the idle injection framework only allows to inject the
> >> deepest idle state available on the system.
> >>
> >> Give the opportunity to specify which idle state we want to inject by
> >> adding a new function helper to set the state and use it when calling
> >> play_idle().
> >>
> >> There is no functional changes, the cpuidle state is the deepest one.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
>
> [ ... ]
>
> >> +/**
> >> + * idle_inject_set_state - set the idle state to inject
> >> + * @state: an integer for the idle state to inject
> >> + */
> >> +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state)
> >> +{
> >> +    if (state >= CPUIDLE_STATE_NOUSE && state < CPUIDLE_STATE_MAX)
> >> +            WRITE_ONCE(ii_dev->state, state);
> >> +}
> >> +
> >>  /**
> >>   * idle_inject_start - start idle injections
> >>   * @ii_dev: idle injection control device structure
> >> @@ -298,6 +310,7 @@ struct idle_inject_device *idle_inject_register(struct cpumask *cpumask)
> >>      cpumask_copy(to_cpumask(ii_dev->cpumask), cpumask);
> >>      hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> >>      ii_dev->timer.function = idle_inject_timer_fn;
> >> +    ii_dev->state = cpuidle_find_deepest_state();
> >>
> >>      for_each_cpu(cpu, to_cpumask(ii_dev->cpumask)) {
> >>
> >> diff --git a/include/linux/idle_inject.h b/include/linux/idle_inject.h
> >> index a445cd1a36c5..e2b26b9ccd34 100644
> >> --- a/include/linux/idle_inject.h
> >> +++ b/include/linux/idle_inject.h
> >> @@ -26,4 +26,7 @@ void idle_inject_set_duration(struct idle_inject_device *ii_dev,
> >>  void idle_inject_get_duration(struct idle_inject_device *ii_dev,
> >>                               unsigned int *run_duration_us,
> >>                               unsigned int *idle_duration_us);
> >> +
> >> +void idle_inject_set_state(struct idle_inject_device *ii_dev, int state);
> >> +
> >
> > The above function is not used in this patch and as such should be introduce as
> > part of future work.  Otherwise I agree that this patch does not carry any
> > functional changes.
> >
> > Without function idle_inject_set_state():
>
> I was about to remove the function but actually it may not make sense as
> the idle_inject is a framework providing the different API to do the
> idle injection and the function is an helper to set the state value. It
> comes with the addition of the state number in the structure.

I agree with you but there is still no current user of the API.

>
> Next patch is the idle cooling device and makes use of it.

Then why not simply introduce it then?

>
> Can I still consider your acked-by valid?

Whether the API is introduced here or in the next patchset has no
effect on the validity of the code and as such my approbation is still
valid.

Thanks,
Mathieu

>
> --
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>