> On 27 September 2012 13:50, MyungJoo Ham <myungjoo....@samsung.com> wrote:
> >> Prepare devfreq core framework to support devices which
> >> can idle. When device idleness is detected perhaps through
> >> runtime-pm, need some mechanism to suspend devfreq load
> >> monitoring and resume back when device is online. Present
> >> code continues monitoring unless device is removed from
> >> devfreq core.
> >>
> >> This patch introduces following design changes,
> >>
> >> - use per device work instead of global work to monitor device
> >> load. This enables suspend/resume of device devfreq and
> >> reduces monitoring code complexity.
> >> - decouple delayed work based load monitoring logic from core
> >> by introducing helpers functions to be used by governors. This
> >> provides flexibility for governors either to use delayed work
> >> based monitoring functions or to implement their own mechanism.
> >> - devfreq core interacts with governors via events to perform
> >> specific actions. These events include start/stop devfreq.
> >> This sets ground for adding suspend/resume events.
> >>
> >> The devfreq apis are not modified and are kept intact.
> >>
> >> Signed-off-by: Rajagopal Venkat <rajagopal.ven...@linaro.org>
> >
> > Hello,
> >
> >
> > I'll do more through review tomorrow (sorry, I was occuppied by
> > something other than Linux tasks for a while again); however,
> > there are two concerns in this patch.
> >
> > 1. (minor but may bothersome in some rare but not-ignorable cases)
> > Serialization issue between suspend/resume
> > functions; this may happen with some failure or interrupts while entering
> > STR or
> > unexpected usage of the API at drivers.
>
> Regarding the invalid usage of suspend/resume apis, we can have
> additional checks
> something like,
>
> void devfreq_monitor_suspend(struct devfreq *devfreq)
> {
> .....
> if (devfreq->stop_polling)
> return;
> ......
> }
>
> void devfreq_monitor_resume(struct devfreq *devfreq)
> {
> .....
> if (!devfreq->stop_polling)
> return;
> ......
> }
>
> >
> > For example, if devfreq_monitor_suspend() and devfreq_montir_resume() are
> > called
> > almost simultaneously, we may execute 1) locked part of suspend, 2) locked
> > part of
> > resume, 3) cancel_delayed_work_sync of suspend.
> >
> > Then, we may have stop_polling = false w/ cancel_delayed_work_sync() in
> > effect.
> >
> > Let's not assume that suspend() and resume() may called almost
> > simultaneously,
> > especially in subsystem core code.
(sorry, I missed "not be" between "may" and "called" here)
> >
>
> These devfreq_monitor_suspend() and devfreq_monitor_resume() functions are
> executed when device idleness is detected. Perhaps,
> - using runtime-pm: the runtime_suspend() and runtime_resume() are mutually
> exclusive and is guaranteed not to run in parallel.
> - driver may have its own mechanism: in my opinion, driver should ensure
> suspend/resume are sequential even for it to know its devfreq status.
>
> Assuming even if above sequence occurs, I don't see any problem having
> stop_polling = false w/ cancel_delayed_work_sync() in effect. Since the
> suspend
> is the last one to complete, monitoring will not continue.
Why don't you simply extend the mutex-locked context?
I.e.,
+ mutex_lock(&devfreq->lock);
+ devfreq->stop_polling = true;
+ mutex_unlock(&devfreq->lock);
+ cancel_delayed_work_sync(&devfreq->work);
-->
+ mutex_lock(&devfreq->lock);
+ devfreq->stop_polling = true;
+ cancel_delayed_work_sync(&devfreq->work);
+ mutex_unlock(&devfreq->lock);
This serializes data-update and the execution based on the data-update,
resolving any inconsistency issues with the queue-status and devfreq
variable.
It doesn't have a heavy overhead to extend it and we have the
probably of inconsistency due to serialization issues.
>
> >
> > 2. What if polling_ms = 0 w/ active governors (such as ondemand)?
> >
> > Users may declare the initial polling_ms = 0 w/ simple-ondemand in order to
> > pause sampling at boot-time and start sampling at run-time some time later.
> >
> > It appears that this patch will start forcibly at boot-time in such a case.
>
> Yes. This is a valid case which can be handled by
>
> void devfreq_monitor_start(struct devfreq *devfreq)
> {
> INIT_DELAYED_WORK_DEFERRABLE(&devfreq->work, devfreq_monitor);
> + if (devfreq->profile->polling_ms)
> queue_delayed_work(devfreq_wq, &devfreq->work,
> msecs_to_jiffies(devfreq->profile->polling_ms));
> }
Please add the checking statement to every queue_delayed_work() statement:
resume and interval-update functions.
Cheers!
MyungJoo
