| Message ID | 20190214013042.254790-2-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>님이 작성: > > The field ->stop_polling indicates whether load monitoring should be/is > stopped, it is set in devfreq_monitor_suspend(). Change the variable to > hold the general state of load monitoring (stopped, running, suspended). > Besides improving readability of conditions involving the field and this > prepares the terrain for moving some duplicated code from the governors > into the devfreq core. > > Hold the devfreq lock in devfreq_monitor_start/stop() to ensure proper > synchronization. IMHO, I'm not sure that there are any benefits changing from 'stop_polling' to 'monitor_state'. I have no objections if Myungjoo confirms it. And I agree to move the initialization of work under if statement according to the value of polling_ms variable in devfreq_monitor_start(). > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/devfreq/devfreq.c | 34 +++++++++++++++++++++++++--------- > include/linux/devfreq.h | 4 ++-- > 2 files changed, 27 insertions(+), 11 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 0ae3de76833b7..1d3a43f8b3a10 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -29,6 +29,10 @@ > #include <linux/of.h> > #include "governor.h" > > +#define DEVFREQ_MONITOR_STOPPED 0 > +#define DEVFREQ_MONITOR_RUNNING 1 > +#define DEVFREQ_MONITOR_SUSPENDED 2 > + > static struct class *devfreq_class; > > /* > @@ -407,10 +411,17 @@ static void devfreq_monitor(struct work_struct *work) > */ > void devfreq_monitor_start(struct devfreq *devfreq) > { > - INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); > - if (devfreq->profile->polling_ms) > + mutex_lock(&devfreq->lock); > + > + if (devfreq->profile->polling_ms) { > + INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); > queue_delayed_work(devfreq_wq, &devfreq->work, > msecs_to_jiffies(devfreq->profile->polling_ms)); > + > + devfreq->monitor_state = DEVFREQ_MONITOR_RUNNING; > + } > + > + mutex_unlock(&devfreq->lock); > } > EXPORT_SYMBOL(devfreq_monitor_start); > > @@ -425,6 +436,10 @@ EXPORT_SYMBOL(devfreq_monitor_start); > void devfreq_monitor_stop(struct devfreq *devfreq) > { > cancel_delayed_work_sync(&devfreq->work); > + > + mutex_lock(&devfreq->lock); > + devfreq->monitor_state = DEVFREQ_MONITOR_STOPPED; > + mutex_unlock(&devfreq->lock); > } > EXPORT_SYMBOL(devfreq_monitor_stop); > > @@ -443,13 +458,13 @@ EXPORT_SYMBOL(devfreq_monitor_stop); > void devfreq_monitor_suspend(struct devfreq *devfreq) > { > mutex_lock(&devfreq->lock); > - if (devfreq->stop_polling) { > + if (devfreq->monitor_state != DEVFREQ_MONITOR_RUNNING) { > mutex_unlock(&devfreq->lock); > return; > } > > devfreq_update_status(devfreq, devfreq->previous_freq); > - devfreq->stop_polling = true; > + devfreq->monitor_state = DEVFREQ_MONITOR_SUSPENDED; > mutex_unlock(&devfreq->lock); > cancel_delayed_work_sync(&devfreq->work); > } > @@ -468,7 +483,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > unsigned long freq; > > mutex_lock(&devfreq->lock); > - if (!devfreq->stop_polling) > + if (devfreq->monitor_state == DEVFREQ_MONITOR_STOPPED) > goto out; > > if (!delayed_work_pending(&devfreq->work) && > @@ -477,7 +492,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > msecs_to_jiffies(devfreq->profile->polling_ms)); > > devfreq->last_stat_updated = jiffies; > - devfreq->stop_polling = false; > + devfreq->monitor_state = DEVFREQ_MONITOR_RUNNING; > > if (devfreq->profile->get_cur_freq && > !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) > @@ -504,13 +519,14 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) > mutex_lock(&devfreq->lock); > devfreq->profile->polling_ms = new_delay; > > - if (devfreq->stop_polling) > + if (devfreq->monitor_state == DEVFREQ_MONITOR_SUSPENDED) > goto out; > > /* if new delay is zero, stop polling */ > if (!new_delay) { > mutex_unlock(&devfreq->lock); > cancel_delayed_work_sync(&devfreq->work); > + devfreq->monitor_state == DEVFREQ_MONITOR_STOPPED; > return; > } > > @@ -526,7 +542,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) > mutex_unlock(&devfreq->lock); > cancel_delayed_work_sync(&devfreq->work); > mutex_lock(&devfreq->lock); > - if (!devfreq->stop_polling) > + if (devfreq->monitor_state != DEVFREQ_MONITOR_SUSPENDED) > queue_delayed_work(devfreq_wq, &devfreq->work, > msecs_to_jiffies(devfreq->profile->polling_ms)); > } > @@ -1372,7 +1388,7 @@ static ssize_t trans_stat_show(struct device *dev, > int i, j; > unsigned int max_state = devfreq->profile->max_state; > > - if (!devfreq->stop_polling && > + if ((devfreq->monitor_state == DEVFREQ_MONITOR_RUNNING) && > devfreq_update_status(devfreq, devfreq->previous_freq)) > return 0; > if (max_state == 0) > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index fbffa74bfc1bb..0a618bbb8b294 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -130,7 +130,7 @@ struct devfreq_dev_profile { > * @max_freq: Limit maximum frequency requested by user (0: none) > * @scaling_min_freq: Limit minimum frequency requested by OPP interface > * @scaling_max_freq: Limit maximum frequency requested by OPP interface > - * @stop_polling: devfreq polling status of a device. > + * @monitor_state: State of the load monitor. > * @suspend_freq: frequency of a device set during suspend phase. > * @resume_freq: frequency of a device set in resume phase. > * @suspend_count: suspend requests counter for a device. > @@ -168,7 +168,7 @@ struct devfreq { > unsigned long max_freq; > unsigned long scaling_min_freq; > unsigned long scaling_max_freq; > - bool stop_polling; > + int monitor_state; > > unsigned long suspend_freq; > unsigned long resume_freq; > -- > 2.20.1.791.gb4d0f1c61a-goog >
Hi Chanwoo, On Thu, Feb 14, 2019 at 11:25:52PM +0900, Chanwoo Choi wrote: > Hi Matthias, > > 2019년 2월 14일 (목) 오후 7:16, Matthias Kaehlcke <mka@chromium.org>님이 작성: > > > > The field ->stop_polling indicates whether load monitoring should be/is > > stopped, it is set in devfreq_monitor_suspend(). Change the variable to > > hold the general state of load monitoring (stopped, running, suspended). > > Besides improving readability of conditions involving the field and this > > prepares the terrain for moving some duplicated code from the governors > > into the devfreq core. > > > > Hold the devfreq lock in devfreq_monitor_start/stop() to ensure proper > > synchronization. > > IMHO, I'm not sure that there are any benefits changing > from 'stop_polling' to 'monitor_state'. I have no objections > if Myungjoo confirms it. I agree that as an isolated change there isn't a clear benefit. However in the context of the series the change is needed to avoid resuming a load monitor that wasn't even started. In case this series isn't accepted I'd still suggest to change the name from 'stop_polling' to 'suspended'. I read 'stop_polling' as a call for action, while 'suspended' is a state. IMO at least in some contexts conditions using a state is clearer. Cheers Matthias
Hi Matthias, On 19. 2. 15. 오전 1:59, Matthias Kaehlcke wrote: > Hi Chanwoo, > > On Thu, Feb 14, 2019 at 11:25:52PM +0900, Chanwoo Choi wrote: >> Hi Matthias, >> >> 2019년 2월 14일 (목) 오후 7:16, Matthias Kaehlcke <mka@chromium.org>님이 작성: >>> >>> The field ->stop_polling indicates whether load monitoring should be/is >>> stopped, it is set in devfreq_monitor_suspend(). Change the variable to >>> hold the general state of load monitoring (stopped, running, suspended). >>> Besides improving readability of conditions involving the field and this >>> prepares the terrain for moving some duplicated code from the governors >>> into the devfreq core. >>> >>> Hold the devfreq lock in devfreq_monitor_start/stop() to ensure proper >>> synchronization. >> >> IMHO, I'm not sure that there are any benefits changing >> from 'stop_polling' to 'monitor_state'. I have no objections >> if Myungjoo confirms it. > > I agree that as an isolated change there isn't a clear benefit. > However in the context of the series the change is needed to > avoid resuming a load monitor that wasn't even started. > > In case this series isn't accepted I'd still suggest to change the > name from 'stop_polling' to 'suspended'. I read 'stop_polling' as a > call for action, while 'suspended' is a state. IMO at least in some > contexts conditions using a state is clearer. I agree to change the variable name 'stop_polling' to 'suspended' for using the correct meaningful name. > > Cheers > > Matthias > >
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 0ae3de76833b7..1d3a43f8b3a10 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -29,6 +29,10 @@ #include <linux/of.h> #include "governor.h" +#define DEVFREQ_MONITOR_STOPPED 0 +#define DEVFREQ_MONITOR_RUNNING 1 +#define DEVFREQ_MONITOR_SUSPENDED 2 + static struct class *devfreq_class; /* @@ -407,10 +411,17 @@ static void devfreq_monitor(struct work_struct *work) */ void devfreq_monitor_start(struct devfreq *devfreq) { - INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); - if (devfreq->profile->polling_ms) + mutex_lock(&devfreq->lock); + + if (devfreq->profile->polling_ms) { + INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms)); + + devfreq->monitor_state = DEVFREQ_MONITOR_RUNNING; + } + + mutex_unlock(&devfreq->lock); } EXPORT_SYMBOL(devfreq_monitor_start); @@ -425,6 +436,10 @@ EXPORT_SYMBOL(devfreq_monitor_start); void devfreq_monitor_stop(struct devfreq *devfreq) { cancel_delayed_work_sync(&devfreq->work); + + mutex_lock(&devfreq->lock); + devfreq->monitor_state = DEVFREQ_MONITOR_STOPPED; + mutex_unlock(&devfreq->lock); } EXPORT_SYMBOL(devfreq_monitor_stop); @@ -443,13 +458,13 @@ EXPORT_SYMBOL(devfreq_monitor_stop); void devfreq_monitor_suspend(struct devfreq *devfreq) { mutex_lock(&devfreq->lock); - if (devfreq->stop_polling) { + if (devfreq->monitor_state != DEVFREQ_MONITOR_RUNNING) { mutex_unlock(&devfreq->lock); return; } devfreq_update_status(devfreq, devfreq->previous_freq); - devfreq->stop_polling = true; + devfreq->monitor_state = DEVFREQ_MONITOR_SUSPENDED; mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work); } @@ -468,7 +483,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) unsigned long freq; mutex_lock(&devfreq->lock); - if (!devfreq->stop_polling) + if (devfreq->monitor_state == DEVFREQ_MONITOR_STOPPED) goto out; if (!delayed_work_pending(&devfreq->work) && @@ -477,7 +492,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) msecs_to_jiffies(devfreq->profile->polling_ms)); devfreq->last_stat_updated = jiffies; - devfreq->stop_polling = false; + devfreq->monitor_state = DEVFREQ_MONITOR_RUNNING; if (devfreq->profile->get_cur_freq && !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) @@ -504,13 +519,14 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) mutex_lock(&devfreq->lock); devfreq->profile->polling_ms = new_delay; - if (devfreq->stop_polling) + if (devfreq->monitor_state == DEVFREQ_MONITOR_SUSPENDED) goto out; /* if new delay is zero, stop polling */ if (!new_delay) { mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work); + devfreq->monitor_state == DEVFREQ_MONITOR_STOPPED; return; } @@ -526,7 +542,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work); mutex_lock(&devfreq->lock); - if (!devfreq->stop_polling) + if (devfreq->monitor_state != DEVFREQ_MONITOR_SUSPENDED) queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms)); } @@ -1372,7 +1388,7 @@ static ssize_t trans_stat_show(struct device *dev, int i, j; unsigned int max_state = devfreq->profile->max_state; - if (!devfreq->stop_polling && + if ((devfreq->monitor_state == DEVFREQ_MONITOR_RUNNING) && devfreq_update_status(devfreq, devfreq->previous_freq)) return 0; if (max_state == 0) diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index fbffa74bfc1bb..0a618bbb8b294 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -130,7 +130,7 @@ struct devfreq_dev_profile { * @max_freq: Limit maximum frequency requested by user (0: none) * @scaling_min_freq: Limit minimum frequency requested by OPP interface * @scaling_max_freq: Limit maximum frequency requested by OPP interface - * @stop_polling: devfreq polling status of a device. + * @monitor_state: State of the load monitor. * @suspend_freq: frequency of a device set during suspend phase. * @resume_freq: frequency of a device set in resume phase. * @suspend_count: suspend requests counter for a device. @@ -168,7 +168,7 @@ struct devfreq { unsigned long max_freq; unsigned long scaling_min_freq; unsigned long scaling_max_freq; - bool stop_polling; + int monitor_state; unsigned long suspend_freq; unsigned long resume_freq;
The field ->stop_polling indicates whether load monitoring should be/is stopped, it is set in devfreq_monitor_suspend(). Change the variable to hold the general state of load monitoring (stopped, running, suspended). Besides improving readability of conditions involving the field and this prepares the terrain for moving some duplicated code from the governors into the devfreq core. Hold the devfreq lock in devfreq_monitor_start/stop() to ensure proper synchronization. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/devfreq/devfreq.c | 34 +++++++++++++++++++++++++--------- include/linux/devfreq.h | 4 ++-- 2 files changed, 27 insertions(+), 11 deletions(-)