Messages in this thread | | | Subject | Re: [PATCH v2 1/2] drivers: devfreq: change devfreq workqueue mechanism | From | Lukasz Luba <> | Date | Tue, 12 Feb 2019 22:37:20 +0100 |
| |
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 <l.luba@partner.samsung.com> >>>> --- >>>> 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... Do you like to write a patch for them (I can test it) or should I do it?
Regards, Lukasz > > Thanks > > Matthias > >
| |