Re: [PATCH 2/4] PM / devfreq: Handle monitor suspend/resume in the devfreq core

[Matthias Kaehlcke <mka@chromium.org>]

Hi Chanwoo,

On Thu, Feb 14, 2019 at 11:10:00PM +0900, Chanwoo Choi wrote:
> Hi Matthias,
> 
> 2019년 2월 14일 (목) 오후 7:16, Matthias Kaehlcke <mka@chromium.org>님이 작성:
> >
> > devfreq expects governors to call devfreq_monitor_suspend/resume()
> > in response to DEVFREQ_GOV_SUSPEND/RESUME events. Since the devfreq
> > core itself generates these events and invokes the governor's event
> > handler the suspend/resume of the load monitoring can be done in the
> > common code.
> >
> > Call devfreq_monitor_suspend/resume() when the governor reports a
> > successful handling of DEVFREQ_GOV_SUSPEND/RESUME and remove these
> > calls from the simpleondemand and tegra governors. Make
> > devfreq_monitor_suspend/resume() static since these functions are now
> > only used by the devfreq core.
> 
> The devfreq core generates the all events including
> DEVFREQ_GOV_START/STOP/INTERVAL.
> It is possible to make 'devfreq_monitor_*()' function as the static
> instead of exported function.
> And call them in devfreq.c as this patch as following:
> 
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -728,6 +728,8 @@ struct devfreq *devfreq_add_device(struct device *dev,
>                 goto err_init;
>         }
> 
> +       devfreq_monitor_start(devfreq);
> +
>         list_add(&devfreq->node, &devfreq_list);
> 
>         mutex_unlock(&devfreq_list_lock);
> @@ -760,6 +762,7 @@ int devfreq_remove_device(struct devfreq *devfreq)
>         if (devfreq->governor)
>                 devfreq->governor->event_handler(devfreq,
>                                                  DEVFREQ_GOV_STOP, NULL);
> +       devfreq_monitor_stop(devfreq);
>         device_unregister(&devfreq->dev);
> 
>         return 0;
> @@ -1259,6 +1262,9 @@ static ssize_t polling_interval_store(struct device *dev,
>         df->governor->event_handler(df, DEVFREQ_GOV_INTERVAL, &value);
>         ret = count;
> 
> +       if (!ret)
> +               devfreq_interval_update(devfreq, (unsigned int *)data);
> +
>         return ret;
>  }
>  static DEVICE_ATTR_RW(polling_interval);
> diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
> index 79efa1e..515fb85 100644
> --- a/drivers/devfreq/tegra-devfreq.c
> +++ b/drivers/devfreq/tegra-devfreq.c
> @@ -580,13 +580,11 @@ static int tegra_governor_event_handler(struct
> devfreq *devfreq,
> 
>         switch (event) {
>         case DEVFREQ_GOV_START:
> -               devfreq_monitor_start(devfreq);
>                 tegra_actmon_enable_interrupts(tegra);
>                 break;
> 
>         case DEVFREQ_GOV_STOP:
>                 tegra_actmon_disable_interrupts(tegra);
> -               devfreq_monitor_stop(devfreq);
>                 break;

indeed, that's similar to "[4/4] PM / devfreq: Handle monitor
start/stop in the devfreq core" of this series.

> Instead,
> 
> If the governor should execute some codes before and after of
> DEVFREQ_GOV_SUSPEND, DEVFREQ_GOV_RESUME,
> it is impossible to change the order between devfreq_monitor_*() function
> and the specific governor in the event_handler callback function of
> each governor.
> 
> For example, if some govenor requires the following sequencue,
> after this patch, it is not possible.
> 
> case DEVFREQ_GOV_SUSPEND:
>         /* execute some code before devfreq_monitor_suspend() */
>         devfreq_monitor_suspend()
>         /* execute some code after devfreq_monitor_suspend() */

I agree that the patch introduces this limitation, however I'm not
convinced it is a problem in practice.

For suspend we'd essentially end up with:

governor_suspend
  governor->suspend

  monitor_suspend
    update_status

    stop polling

What would a governor want to do after polling is stopped? Is there
any real world or halfway reasonable hypothetical example?


And for resume:

governor_resume
  governor->resume

  monitor_resume
    start polling

Same question here, what would the governor realistically want to do
after polling has started that it couldn't have done before?

Cheers

Matthias