| Message ID | 20190214013042.254790-3-mka@chromium.org |
|---|---|
| State | Deferred |
| Headers | show |
| Series | PM / devfreq: Refactor load monitoring | expand |
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; 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() */ > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/devfreq/devfreq.c | 18 ++++++++---------- > drivers/devfreq/governor.h | 2 -- > drivers/devfreq/governor_simpleondemand.c | 8 -------- > drivers/devfreq/tegra-devfreq.c | 2 -- > 4 files changed, 8 insertions(+), 22 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 1d3a43f8b3a10..7fab6c4cf719b 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -447,15 +447,13 @@ EXPORT_SYMBOL(devfreq_monitor_stop); > * devfreq_monitor_suspend() - Suspend load monitoring of a devfreq instance > * @devfreq: the devfreq instance. > * > - * Helper function to suspend devfreq device load monitoing. Function > - * to be called from governor in response to DEVFREQ_GOV_SUSPEND > - * event or when polling interval is set to zero. > + * Helper function to suspend devfreq device load monitoring. > * > * Note: Though this function is same as devfreq_monitor_stop(), > * intentionally kept separate to provide hooks for collecting > * transition statistics. > */ > -void devfreq_monitor_suspend(struct devfreq *devfreq) > +static void devfreq_monitor_suspend(struct devfreq *devfreq) > { > mutex_lock(&devfreq->lock); > if (devfreq->monitor_state != DEVFREQ_MONITOR_RUNNING) { > @@ -468,17 +466,14 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) > mutex_unlock(&devfreq->lock); > cancel_delayed_work_sync(&devfreq->work); > } > -EXPORT_SYMBOL(devfreq_monitor_suspend); > > /** > * devfreq_monitor_resume() - Resume load monitoring of a devfreq instance > * @devfreq: the devfreq instance. > * > - * Helper function to resume devfreq device load monitoing. Function > - * to be called from governor in response to DEVFREQ_GOV_RESUME > - * event or when polling interval is set to non-zero. > + * Helper function to resume devfreq device load monitoring. > */ > -void devfreq_monitor_resume(struct devfreq *devfreq) > +static void devfreq_monitor_resume(struct devfreq *devfreq) > { > unsigned long freq; > > @@ -501,7 +496,6 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > out: > mutex_unlock(&devfreq->lock); > } > -EXPORT_SYMBOL(devfreq_monitor_resume); > > /** > * devfreq_interval_update() - Update device devfreq monitoring interval > @@ -903,6 +897,8 @@ int devfreq_suspend_device(struct devfreq *devfreq) > DEVFREQ_GOV_SUSPEND, NULL); > if (ret) > return ret; > + > + devfreq_monitor_suspend(devfreq); > } > > if (devfreq->suspend_freq) { > @@ -944,6 +940,8 @@ int devfreq_resume_device(struct devfreq *devfreq) > DEVFREQ_GOV_RESUME, NULL); > if (ret) > return ret; > + > + devfreq_monitor_resume(devfreq); > } > > return 0; > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h > index f53339ca610fc..d136792c0cc91 100644 > --- a/drivers/devfreq/governor.h > +++ b/drivers/devfreq/governor.h > @@ -59,8 +59,6 @@ struct devfreq_governor { > > extern void devfreq_monitor_start(struct devfreq *devfreq); > extern void devfreq_monitor_stop(struct devfreq *devfreq); > -extern void devfreq_monitor_suspend(struct devfreq *devfreq); > -extern void devfreq_monitor_resume(struct devfreq *devfreq); > extern void devfreq_interval_update(struct devfreq *devfreq, > unsigned int *delay); > > diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c > index c0417f0e081e0..52eb0c734b312 100644 > --- a/drivers/devfreq/governor_simpleondemand.c > +++ b/drivers/devfreq/governor_simpleondemand.c > @@ -103,14 +103,6 @@ static int devfreq_simple_ondemand_handler(struct devfreq *devfreq, > devfreq_interval_update(devfreq, (unsigned int *)data); > break; > > - case DEVFREQ_GOV_SUSPEND: > - devfreq_monitor_suspend(devfreq); > - break; > - > - case DEVFREQ_GOV_RESUME: > - devfreq_monitor_resume(devfreq); > - break; > - > default: > break; > } > diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c > index c59d2eee5d309..79efa1e51bd06 100644 > --- a/drivers/devfreq/tegra-devfreq.c > +++ b/drivers/devfreq/tegra-devfreq.c > @@ -591,11 +591,9 @@ static int tegra_governor_event_handler(struct devfreq *devfreq, > > case DEVFREQ_GOV_SUSPEND: > tegra_actmon_disable_interrupts(tegra); > - devfreq_monitor_suspend(devfreq); > break; > > case DEVFREQ_GOV_RESUME: > - devfreq_monitor_resume(devfreq); > tegra_actmon_enable_interrupts(tegra); > break; > } > -- > 2.20.1.791.gb4d0f1c61a-goog >
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
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 1d3a43f8b3a10..7fab6c4cf719b 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -447,15 +447,13 @@ EXPORT_SYMBOL(devfreq_monitor_stop); * devfreq_monitor_suspend() - Suspend load monitoring of a devfreq instance * @devfreq: the devfreq instance. * - * Helper function to suspend devfreq device load monitoing. Function - * to be called from governor in response to DEVFREQ_GOV_SUSPEND - * event or when polling interval is set to zero. + * Helper function to suspend devfreq device load monitoring. * * Note: Though this function is same as devfreq_monitor_stop(), * intentionally kept separate to provide hooks for collecting * transition statistics. */ -void devfreq_monitor_suspend(struct devfreq *devfreq) +static void devfreq_monitor_suspend(struct devfreq *devfreq) { mutex_lock(&devfreq->lock); if (devfreq->monitor_state != DEVFREQ_MONITOR_RUNNING) { @@ -468,17 +466,14 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work); } -EXPORT_SYMBOL(devfreq_monitor_suspend); /** * devfreq_monitor_resume() - Resume load monitoring of a devfreq instance * @devfreq: the devfreq instance. * - * Helper function to resume devfreq device load monitoing. Function - * to be called from governor in response to DEVFREQ_GOV_RESUME - * event or when polling interval is set to non-zero. + * Helper function to resume devfreq device load monitoring. */ -void devfreq_monitor_resume(struct devfreq *devfreq) +static void devfreq_monitor_resume(struct devfreq *devfreq) { unsigned long freq; @@ -501,7 +496,6 @@ void devfreq_monitor_resume(struct devfreq *devfreq) out: mutex_unlock(&devfreq->lock); } -EXPORT_SYMBOL(devfreq_monitor_resume); /** * devfreq_interval_update() - Update device devfreq monitoring interval @@ -903,6 +897,8 @@ int devfreq_suspend_device(struct devfreq *devfreq) DEVFREQ_GOV_SUSPEND, NULL); if (ret) return ret; + + devfreq_monitor_suspend(devfreq); } if (devfreq->suspend_freq) { @@ -944,6 +940,8 @@ int devfreq_resume_device(struct devfreq *devfreq) DEVFREQ_GOV_RESUME, NULL); if (ret) return ret; + + devfreq_monitor_resume(devfreq); } return 0; diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h index f53339ca610fc..d136792c0cc91 100644 --- a/drivers/devfreq/governor.h +++ b/drivers/devfreq/governor.h @@ -59,8 +59,6 @@ struct devfreq_governor { extern void devfreq_monitor_start(struct devfreq *devfreq); extern void devfreq_monitor_stop(struct devfreq *devfreq); -extern void devfreq_monitor_suspend(struct devfreq *devfreq); -extern void devfreq_monitor_resume(struct devfreq *devfreq); extern void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay); diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c index c0417f0e081e0..52eb0c734b312 100644 --- a/drivers/devfreq/governor_simpleondemand.c +++ b/drivers/devfreq/governor_simpleondemand.c @@ -103,14 +103,6 @@ static int devfreq_simple_ondemand_handler(struct devfreq *devfreq, devfreq_interval_update(devfreq, (unsigned int *)data); break; - case DEVFREQ_GOV_SUSPEND: - devfreq_monitor_suspend(devfreq); - break; - - case DEVFREQ_GOV_RESUME: - devfreq_monitor_resume(devfreq); - break; - default: break; } diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c index c59d2eee5d309..79efa1e51bd06 100644 --- a/drivers/devfreq/tegra-devfreq.c +++ b/drivers/devfreq/tegra-devfreq.c @@ -591,11 +591,9 @@ static int tegra_governor_event_handler(struct devfreq *devfreq, case DEVFREQ_GOV_SUSPEND: tegra_actmon_disable_interrupts(tegra); - devfreq_monitor_suspend(devfreq); break; case DEVFREQ_GOV_RESUME: - devfreq_monitor_resume(devfreq); tegra_actmon_enable_interrupts(tegra); break; }
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. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/devfreq/devfreq.c | 18 ++++++++---------- drivers/devfreq/governor.h | 2 -- drivers/devfreq/governor_simpleondemand.c | 8 -------- drivers/devfreq/tegra-devfreq.c | 2 -- 4 files changed, 8 insertions(+), 22 deletions(-)