From patchwork Fri Nov 22 09:11:37 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 21676 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pa0-f71.google.com (mail-pa0-f71.google.com [209.85.220.71]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 76C992096D for ; Fri, 22 Nov 2013 09:11:48 +0000 (UTC) Received: by mail-pa0-f71.google.com with SMTP id kl14sf1827407pab.2 for ; Fri, 22 Nov 2013 01:11:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:date:from:user-agent :mime-version:to:cc:subject:references:in-reply-to:x-original-sender :x-original-authentication-results:precedence:mailing-list:list-id :list-post:list-help:list-archive:list-unsubscribe:content-type :content-transfer-encoding; bh=MrS7CeN0D11kY+QTKdTHk6HRlW86rSPauakJYcfLk0E=; b=MVB1PY4pwiftNonX9pZPjwi2VXTsfeb94UsA9umZxtDxu8VqceRpPPuOWq4+EUXfbs OCfVcdGFIOsVgwDT0ooWs/l3ZZfH2TgRVxRehZMM//zyQw592xl4BxTK5+wCoSxqVf6e zrKE9QXdHDiH0oS+j9/dv3z1HfaEe7vV0o6odFssUsf0PUCk+LdsG5m1+bTg1kZ03kQ/ reaxbIaT1T3PtW6uC9a4mhWyK8/vsQrWUIQ8C8R/lOoegY6/5qSskgqSkvDYxCH9eC7w ZQwancZbJLlmgG7Axc3W7ju6o0we941C6T/G8SLR2aOysmFTHPwHtWjgIob6r0m4MAq/ kz+w== X-Gm-Message-State: ALoCoQkBb0Vk/BUFRXm0WEI2fJkot/ru7hDq8yf3URqRwsPfD7Ri0rYN1qv5t9ViCc4zHq8Xlbee X-Received: by 10.66.66.163 with SMTP id g3mr3746446pat.3.1385111507020; Fri, 22 Nov 2013 01:11:47 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.49.36.136 with SMTP id q8ls11967qej.36.gmail; Fri, 22 Nov 2013 01:11:46 -0800 (PST) X-Received: by 10.58.108.196 with SMTP id hm4mr900829veb.28.1385111506771; Fri, 22 Nov 2013 01:11:46 -0800 (PST) Received: from mail-ve0-f181.google.com (mail-ve0-f181.google.com [209.85.128.181]) by mx.google.com with ESMTPS id hs8si12332127veb.98.2013.11.22.01.11.46 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 22 Nov 2013 01:11:46 -0800 (PST) Received-SPF: neutral (google.com: 209.85.128.181 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.128.181; Received: by mail-ve0-f181.google.com with SMTP id oy12so704395veb.12 for ; Fri, 22 Nov 2013 01:11:46 -0800 (PST) X-Received: by 10.58.208.130 with SMTP id me2mr10364850vec.13.1385111506410; Fri, 22 Nov 2013 01:11:46 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patches@linaro.org Received: by 10.220.174.196 with SMTP id u4csp25824vcz; Fri, 22 Nov 2013 01:11:45 -0800 (PST) X-Received: by 10.49.107.131 with SMTP id hc3mr19434697qeb.60.1385111505583; Fri, 22 Nov 2013 01:11:45 -0800 (PST) Received: from mail-qe0-f51.google.com (mail-qe0-f51.google.com [209.85.128.51]) by mx.google.com with ESMTPS id p1si5100308qad.36.2013.11.22.01.11.45 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 22 Nov 2013 01:11:45 -0800 (PST) Received-SPF: neutral (google.com: 209.85.128.51 is neither permitted nor denied by best guess record for domain of viresh.kumar@linaro.org) client-ip=209.85.128.51; Received: by mail-qe0-f51.google.com with SMTP id d4so697990qej.38 for ; Fri, 22 Nov 2013 01:11:45 -0800 (PST) X-Received: by 10.224.124.134 with SMTP id u6mr19128761qar.79.1385111504803; Fri, 22 Nov 2013 01:11:44 -0800 (PST) Received: from [127.0.0.1] (git.linaro.org. [54.235.93.228]) by mx.google.com with ESMTPSA id a5sm87400442qae.2.2013.11.22.01.11.40 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 22 Nov 2013 01:11:44 -0800 (PST) Message-ID: <528F1FC9.4070306@linaro.org> Date: Fri, 22 Nov 2013 14:41:37 +0530 From: viresh kumar User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0 MIME-Version: 1.0 To: "Rafael J. Wysocki" CC: Lists linaro-kernel , Patch Tracking , "cpufreq@vger.kernel.org" , "linux-pm@vger.kernel.org" , Linux Kernel Mailing List , Lan Tianyu , Nishanth Menon , jinchoi@broadcom.com, Sebastian Capella , "Srivatsa S. Bhat" Subject: Re: [PATCH] cpufreq: suspend/resume governors with PM notifiers References: <0e476d16e0ae4b001f5dd4f9ac191f899095928a.1384510171.git.viresh.kumar@linaro.org> <1531674.UoqyyWSbOg@vostro.rjw.lan> <2642790.BqNcqy9aCb@vostro.rjw.lan> In-Reply-To: <2642790.BqNcqy9aCb@vostro.rjw.lan> X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: viresh.kumar@linaro.org X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.128.181 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Precedence: list Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org List-ID: X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On Friday 22 November 2013 03:44 AM, Rafael J. Wysocki wrote: > Short-term. To be precise, governors may be stopped at the beginning of > dpm_suspend_noirq() (that is, where cpuidle_pause() is called). Analogously, > they may be started again in dpm_resume_noirq(), where cpuidle_resume() is > called. That at least would be consistent with what cpuidle already does. Ahh, I mentioned the location to be after "freeze" as I thought CPUs are removed before calling dpm_suspend_noirq(). And yes I was *wrong*.. So, dpm_suspend_noirq() and resume_noirq() looks to be the right place to get that stuff in.. And that will fit cleanly in the existing code as well.. Not many changes would be required in the $subject patch.. > That said in my opinion the appropriate long-term approach would be to split > CPU offline and online each into two parts, the "core" part and the "extras" > part, such that the "core" parts would only do the offline/online of the > cores themselves. The rest, such as cpufreq/cpuidle "offline/online" would > be done in the "extras" part. > > Then, system suspend/resume will only use the "core" parts of CPU offline/online > and the handling of the things belonging to "extras" would be carried out > through CPU device suspend/resume callbacks. In turn, the "runtime" CPU offline > and online would carry out both the "extras" and "core" parts as it does today. > > Makes sense? Yes it does. Very much. So, I will probably float a initial patch with the dpm_{suspend|resume}_noirq() approach to get things fixed for now. And then will do what you suggested. And yes logically this makes sense, a lot of sense. cpuidle/freq are about managing CPUs and so we better have a CPU driver here, to take care of suspend/resume paths. I have few questions regarding the long term solution. There can be only one driver for any device, this is how device-driver model is. But there can be many users of cpu driver. Like ACPI (which we already have: drivers/acpi/processor_driver.c), CPUFreq, CPUIdle and maybe more.. To get all these serviced together we probably need to write another layer on top of these to which these will register their callbacks. Then I started looking into kernel code to understand different frameworks we are using and came across: subsys_interface. This is the comment over it: * Simple interfaces attached to a subsystem. Multiple interfaces can * attach to a subsystem and its devices. Unlike drivers, they do not * exclusively claim or control devices. Interfaces usually represent * a specific functionality of a subsystem/class of devices. And it exactly fits our purpose. We don't really need a CPU driver as there are multiple frameworks that need it for the same device. And probably we just need a interface which would call user specific callbacks (user being: cpufreq, cpuidle, maybe more).. So, what about something like this ? diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index f48370d..523c0bc 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -120,6 +120,45 @@ static DEVICE_ATTR(release, S_IWUSR, NULL, cpu_release_store); #endif /* CONFIG_ARCH_CPU_PROBE_RELEASE */ #endif /* CONFIG_HOTPLUG_CPU */ +int cpu_subsys_suspend_noirq(struct device *dev) +{ + struct bus_type *bus = dev->bus; + struct subsys_interface *sif; + int ret = 0; + + list_for_each_entry(sif, &bus->p->interfaces, node) { + if (sif->pm && sif->pm->suspend_noirq) { + ret = sif->suspend_noirq(dev); + if (ret) + break; + } + } + + return ret; +} + +int cpu_subsys_resume_noirq(struct device *dev) +{ + struct bus_type *bus = dev->bus; + struct subsys_interface *sif; + int ret = 0; + + list_for_each_entry(sif, &bus->p->interfaces, node) { + if (sif->pm && sif->pm->resume_noirq) { + ret = sif->resume_noirq(dev); + if (ret) + break; + } + } + + return ret; +} + +static const struct dev_pm_ops cpu_subsys_pm_ops = { + .suspend_noirq = cpu_subsys_suspend_noirq, + .resume_noirq = cpu_subsys_resume_noirq, +}; + struct bus_type cpu_subsys = { .name = "cpu", .dev_name = "cpu", @@ -128,6 +167,7 @@ struct bus_type cpu_subsys = { .online = cpu_subsys_online, .offline = cpu_subsys_offline, #endif + .pm = &cpu_subsys_pm_ops, }; EXPORT_SYMBOL_GPL(cpu_subsys); diff --git a/include/linux/device.h b/include/linux/device.h index b025925..fa01273 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -298,11 +298,16 @@ struct device *driver_find_device(struct device_driver *drv, * @node: the list of functions registered at the subsystem * @add_dev: device hookup to device function handler * @remove_dev: device hookup to device function handler + * @pm: Power management operations of this interface. * * Simple interfaces attached to a subsystem. Multiple interfaces can * attach to a subsystem and its devices. Unlike drivers, they do not * exclusively claim or control devices. Interfaces usually represent * a specific functionality of a subsystem/class of devices. + * + * PM callbacks are called from individual subsystems instead of PM core. And + * hence might not be available for all subsystems. Currently present for: + * cpu_subsys. */ struct subsys_interface { const char *name; @@ -310,6 +315,7 @@ struct subsys_interface { struct list_head node; int (*add_dev)(struct device *dev, struct subsys_interface *sif); int (*remove_dev)(struct device *dev, struct subsys_interface *sif); + const struct dev_pm_ops *pm; }; int subsys_interface_register(struct subsys_interface *sif);