Hi Lukasz, On Tue, Feb 12, 2019 at 10:37:20PM +0100, Lukasz Luba wrote: > Hi Matthias, > > On 2/12/19 9:12 PM, Matthias Kaehlcke wrote: > > On Tue, Feb 12, 2019 at 12:20:42PM +0100, Lukasz Luba wrote: > >> Hi Matthias, > >> > >> On 2/11/19 10:42 PM, Matthias Kaehlcke wrote: > >>> Hi Lukasz, > >>> > >>> On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote: > >>>> There is no need for creating another workqueue in the system, > >>>> the existing one should meet the requirements. > >>>> This patch removes devfreq's custom workqueue and uses system one. > >>>> It switches from queue_delayed_work() to schedule_delayed_work(). > >>>> It also does not wake up the system when it enters suspend (this > >>>> functionality stays the same). > >>>> > >>>> Signed-off-by: Lukasz Luba <[email protected]> > >>>> --- > >>>> drivers/devfreq/devfreq.c | 25 ++++++------------------- > >>>> 1 file changed, 6 insertions(+), 19 deletions(-) > >>>> > >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > >>>> index 0ae3de7..882e717 100644 > >>>> --- a/drivers/devfreq/devfreq.c > >>>> +++ b/drivers/devfreq/devfreq.c > >>>> @@ -31,13 +31,6 @@ > >>>> > >>>> static struct class *devfreq_class; > >>>> > >>>> -/* > >>>> - * devfreq core provides delayed work based load monitoring helper > >>>> - * functions. Governors can use these or can implement their own > >>>> - * monitoring mechanism. > >>>> - */ > >>>> -static struct workqueue_struct *devfreq_wq; > >>>> - > >>>> /* The list of all device-devfreq governors */ > >>>> static LIST_HEAD(devfreq_governor_list); > >>>> /* The list of all device-devfreq */ > >>>> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work) > >>>> if (err) > >>>> dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", > >>>> err); > >>>> > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> - > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> + schedule_delayed_work(&devfreq->work, > >>>> + > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> mutex_unlock(&devfreq->lock); > >>>> } > >>>> > >>>> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq) > >>>> { > >>>> INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); > >>>> if (devfreq->profile->polling_ms) > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> + schedule_delayed_work(&devfreq->work, > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> } > >>>> EXPORT_SYMBOL(devfreq_monitor_start); > >>>> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > >>>> > >>>> if (!delayed_work_pending(&devfreq->work) && > >>>> devfreq->profile->polling_ms) > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> + schedule_delayed_work(&devfreq->work, > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> > >>>> devfreq->last_stat_updated = jiffies; > >>>> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq > >>>> *devfreq, unsigned int *delay) > >>>> > >>>> /* if current delay is zero, start polling with new delay */ > >>>> if (!cur_delay) { > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> + schedule_delayed_work(&devfreq->work, > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> goto out; > >>>> } > >>>> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq > >>>> *devfreq, unsigned int *delay) > >>>> cancel_delayed_work_sync(&devfreq->work); > >>>> mutex_lock(&devfreq->lock); > >>>> if (!devfreq->stop_polling) > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> + schedule_delayed_work(&devfreq->work, > >>>> > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> } > >>>> out: > >>>> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void) > >>>> return PTR_ERR(devfreq_class); > >>>> } > >>>> > >>>> - devfreq_wq = create_freezable_workqueue("devfreq_wq"); > >>>> - if (!devfreq_wq) { > >>>> - class_destroy(devfreq_class); > >>>> - pr_err("%s: couldn't create workqueue\n", __FILE__); > >>>> - return -ENOMEM; > >>>> - } > >>>> devfreq_class->dev_groups = devfreq_groups; > >>>> > >>>> return 0; > >>> > >>> As commented on v1, the change from a custom to a system workqueue > >>> seems reasonable to me. However this patch also changes from a > >>> freezable workqueue to a non-freezable one. C&P of my comments on v1: > >>> > >>> ``WQ_FREEZABLE`` > >>> A freezable wq participates in the freeze phase of the system > >>> suspend operations. Work items on the wq are drained and no > >>> new work item starts execution until thawed. > >>> > >>> I'm not entirely sure what the impact of this is. > >>> > >>> I imagine suspend is potentially quicker because the wq isn't drained, > >>> but could works that execute during the suspend phase be a problem? > >> The devfreq supports suspend from v4.20-rc6, which picks OPP for a > >> device based on its DT 'opp-suspend'. For the devices which do not > >> choose the suspend OPP it is possible to enter that state with any > >> frequency. Queuing work for calling governor during suspend which > >> calculates the device's frequency for the next period is IMO not needed, > >> The 'next period' is actually suspend and is not related to > >> 'predicted' load by the governor. > > > > If I am not mistaken the monitor can still be running after a device > > was suspended: > > > > devfreq_suspend > > list_for_each_entry(devfreq, &devfreq_list, node) > > devfreq_suspend_device > > devfreq->governor->event_handler(devfreq, > > DEVFREQ_GOV_SUSPEND, NULL); > > > > According to the comment of devfreq_monitor_suspend() the function is > > supposed to be called by the governor in response to > > DEVFREQ_GOV_SUSPEND, however this doesn't seem to be universally the case: > > > > git grep devfreq_monitor_suspend > > drivers/devfreq/governor_simpleondemand.c: > > devfreq_monitor_suspend(devfreq); > > drivers/devfreq/tegra-devfreq.c: > > devfreq_monitor_suspend(devfreq); > > > > i.e. the other governors don't seem to call devfreq_monitor_suspend(). > > > > Am I missing something? > Probably not. > Good catch, these governors should support case DEVFREQ_GOV_SUSPEND. > The system suspend which calls 'devfreq_suspend' does it when the > workqueues are frozen and sets the desired OPP for later resume. > The other use use cases (like pm_suspend) might assume that these > governors are ready for DEVFREQ_GOV_SUSPEND...
Thanks for the confirmation! > Do you like to write a patch for them (I can test it) or should I do it? I can send a patch, testing will be appreciated :) Thanks Matthias
