Received: by 2002:ac0:946b:0:0:0:0:0 with SMTP id j40csp4529090imj; Tue, 12 Feb 2019 18:37:31 -0800 (PST) X-Google-Smtp-Source: AHgI3Ib77GgvmweHQV+nNiQC2Vk5uk5j5TW7pa96JGDgyBhz6B5TkZiBRTl+R8x+DvYKHWOAmVA/ X-Received: by 2002:a17:902:2e03:: with SMTP id q3mr7463164plb.330.1550025451730; Tue, 12 Feb 2019 18:37:31 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1550025451; cv=none; d=google.com; s=arc-20160816; b=gK5WE8+VWhbJYvn1IZ8VuhiXxCg8qzQ5Ge122N3XLF/SXtCLr3n0l7t3D5V8QJfqw3 0q6IdrCgHMgLuCarNS1L6LdORdMyL3kn3zAw1hJXoAmW8zm5GSXBUccw1IKXNxz1chld TkitlGi9LDVL4oNdK3N7ONa+GFUEg2kVqti4J+JbjPaaqhgqBuz0moDt1egpcamB/Sq8 3wQTCG5XOG8DF02862gE3qcUQJuhsoX8OHpL0GWEc1xLEz+0RnbPZOj10fPlODlUqlpi PtGqdDGcNyW1qExxLhPH6RpuPK7PBvlNzlQj78fgXtTPLl5KEyA9JqCLbL19WYvmkLGY QY4A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=QINtGpLKekzMH3z8PqzqOcmzTqT22tsMTZrK5qGIpLU=; b=gHN8r6ynmsPT/z8He6T+IlFHV5DBdnQqTC+yL8EnkEBPFdghG0Vlf19uHdDQ3eBdjb o0rvfUPV1XQrmGzrVJQz5Eeq/f76lRU8n76rqjiLbup8ccOqgKarpm3J9+YsPK9tkYek Su/MFnXCKK7u4cshRE/gHV70S52+5UawP0eFp7pz6dj1+ZgMrWaMTooSySQAvw9LkInp kW4Fi0laaYRk69A5AI5LHOJCNFsxE50gknSHc/5n7TmD9oax8LHch+QL9HQWF3LacMge RgNCTxvWbnVQpkbR9QK/kZzMH5OWAjjFKsezAglpkt5dmVOsLjFqOV0Oad7B7ofZSvHm zlVQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b="WdWbO/9n"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id f12si13988159pgd.68.2019.02.12.18.37.16; Tue, 12 Feb 2019 18:37:31 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b="WdWbO/9n"; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732944AbfBMAs2 (ORCPT + 99 others); Tue, 12 Feb 2019 19:48:28 -0500 Received: from mail-pf1-f196.google.com ([209.85.210.196]:33661 "EHLO mail-pf1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727871AbfBMAs2 (ORCPT ); Tue, 12 Feb 2019 19:48:28 -0500 Received: by mail-pf1-f196.google.com with SMTP id c123so310112pfb.0 for ; Tue, 12 Feb 2019 16:48:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=QINtGpLKekzMH3z8PqzqOcmzTqT22tsMTZrK5qGIpLU=; b=WdWbO/9nC3/cqHLKWG9Ngk6ftliD34qP4LDuYwATeEBZSbWDivQ2Y0FPfrqufsp/pi VJQB0gpdbYiZsF7Lr570HLsTQVjm/4XIf5K7D5wiw7vjMeU+D/w5v6r0vJun2xG2isMY 9Qe0wjiWGJ7E6CI9fPSVxz+SkRaouYWkWzLTI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=QINtGpLKekzMH3z8PqzqOcmzTqT22tsMTZrK5qGIpLU=; b=Zn1OkaT+aMCIGdPnS3yL1wPEV+aKDop5qJSNaDDrXqNlKQ55Yw/Mk6qbD1S7RaFaod UwVlZ6rF52ZKE6FLdJG30cjvI9R4t8meKyMyPLVUbBPXHQj9lXAoG/E+sFbmnxuJGhAj 0ee0zEbL8kLlm6MCVFXUoBVcUAiVMske2xSZrMPnREVoGaNcZA53P6f7R49jB/NWR3f2 PM8gZeJXUnHy9CLcDfuIB1pK538Wa0vTlv/i894WV8N/y1ur5bsvUQrOFtBcT1FgFrqO LJznUaoZO+C0qFAI2K6/r5HazdzE1QMvy2Xb+qUSHHPABS72F9ANFqudRQaTH8qRh6QZ NWsQ== X-Gm-Message-State: AHQUAua9tcaes7bT9yb1H7Gu3A3xkcPB5hOghozP1fzpGcU9P/WH4KL2 hm4PB6VDlmVLwq8vM2SpirtOoQ== X-Received: by 2002:a63:94:: with SMTP id 142mr6172397pga.74.1550018907356; Tue, 12 Feb 2019 16:48:27 -0800 (PST) Received: from localhost ([2620:15c:202:1:75a:3f6e:21d:9374]) by smtp.gmail.com with ESMTPSA id z9sm24982625pfd.99.2019.02.12.16.48.26 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 12 Feb 2019 16:48:26 -0800 (PST) Date: Tue, 12 Feb 2019 16:48:26 -0800 From: Matthias Kaehlcke To: Lukasz Luba Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, b.zolnierkie@samsung.com, myungjoo.ham@samsung.com, cw00.choi@samsung.com, kyungmin.park@samsung.com, m.szyprowski@samsung.com, s.nawrocki@samsung.com, tkjos@google.com, joel@joelfernandes.org, chris.diamand@arm.com Subject: Re: [PATCH v2 1/2] drivers: devfreq: change devfreq workqueue mechanism Message-ID: <20190213004826.GW117604@google.com> References: <1549899005-7760-1-git-send-email-l.luba@partner.samsung.com> <1549899005-7760-2-git-send-email-l.luba@partner.samsung.com> <20190211214252.GR117604@google.com> <01644c1c-a419-9864-3b34-0837a114b799@partner.samsung.com> <20190212201228.GU117604@google.com> <6eb6fc90-27ef-87d2-1123-3d71f0a5102e@partner.samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <6eb6fc90-27ef-87d2-1123-3d71f0a5102e@partner.samsung.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >>>> --- > >>>> 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