[{"id":1759652,"web_url":"http://patchwork.ozlabs.org/comment/1759652/","msgid":"<1957183.ZU1CWrAZoR@aspire.rjw.lan>","list_archive_url":null,"date":"2017-08-29T20:19:43","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Tuesday, August 29, 2017 4:56:42 PM CEST Ulf Hansson wrote:\n> The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,\n> isn't well optimized for system sleep.\n> \n> What makes this driver particularly interesting is because it's a cross-SoC\n> driver, which sometimes means there is an ACPI PM domain attached to the i2c\n> device and sometimes not. The driver is being used on both x86 and ARM.\n> \n> In principle, to optimize the system sleep support in i2c driver, this series\n> enables the proven runtime PM centric path for the i2c driver. However, to do\n> that the ACPI PM domain also have to collaborate and understand this behaviour.\n> From earlier versions, Rafael has also pointed out that also the PM core needs\n> to be involved.\n\nEarlier today I realized that drivers pointing their ->suspend_late and\n->resume_early callbacks, respectively, to pm_runtime_force_suspend() and\npm_runtime_force_resume(), are fundamentally incompatible with any bus type\ndoing nontrivial PM and with almost any nontrivial PM domains, for two reasons.\n\nFirst, it basically requires the bus type or PM domain to expect that its\n->runtime_suspend callback may or may not be indirectly invoked from its\nown ->suspend_late callback, depending on the driver (and analogously\nfor ->runtime_resume and ->resume early), which is insane.\n\nSecond, it is a layering violation, because it makes the driver effectively\noverride the upper layer's decisions about what code to run.\n\nThat's why I'm afraid that we've reached a dead end here. :-(\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xhgFT3fVHz9sQl\n\tfor ;\n\tWed, 30 Aug 2017 06:28:29 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751419AbdH2U21 (ORCPT );\n\tTue, 29 Aug 2017 16:28:27 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:54227 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751250AbdH2U2Z (ORCPT\n\t); Tue, 29 Aug 2017 16:28:25 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id ee08f67d5e0703dd; Tue, 29 Aug 2017 22:28:23 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tlinux-acpi@vger.kernel.org, linux-pm@vger.kernel.org,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\tlinux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","Date":"Tue, 29 Aug 2017 22:19:43 +0200","Message-ID":"<1957183.ZU1CWrAZoR@aspire.rjw.lan>","In-Reply-To":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1760005,"web_url":"http://patchwork.ozlabs.org/comment/1760005/","msgid":"","list_archive_url":null,"date":"2017-08-30T09:57:28","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":21036,"url":"http://patchwork.ozlabs.org/api/people/21036/","name":"Ulf Hansson","email":"ulf.hansson@linaro.org"},"content":"On 29 August 2017 at 22:19, Rafael J. Wysocki wrote:\n> On Tuesday, August 29, 2017 4:56:42 PM CEST Ulf Hansson wrote:\n>> The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,\n>> isn't well optimized for system sleep.\n>>\n>> What makes this driver particularly interesting is because it's a cross-SoC\n>> driver, which sometimes means there is an ACPI PM domain attached to the i2c\n>> device and sometimes not. The driver is being used on both x86 and ARM.\n>>\n>> In principle, to optimize the system sleep support in i2c driver, this series\n>> enables the proven runtime PM centric path for the i2c driver. However, to do\n>> that the ACPI PM domain also have to collaborate and understand this behaviour.\n>> From earlier versions, Rafael has also pointed out that also the PM core needs\n>> to be involved.\n>\n> Earlier today I realized that drivers pointing their ->suspend_late and\n> ->resume_early callbacks, respectively, to pm_runtime_force_suspend() and\n> pm_runtime_force_resume(), are fundamentally incompatible with any bus type\n> doing nontrivial PM and with almost any nontrivial PM domains, for two reasons.\n>\n> First, it basically requires the bus type or PM domain to expect that its\n> ->runtime_suspend callback may or may not be indirectly invoked from its\n> own ->suspend_late callback, depending on the driver (and analogously\n> for ->runtime_resume and ->resume early), which is insane.\n>\n> Second, it is a layering violation, because it makes the driver effectively\n> override the upper layer's decisions about what code to run.\n\nYou are right that for more complex bus types and PM domains, those\nneeds to play along. So that is what I am trying to implement for the\nACPI PM domain in this series.\n\nThe generic PM domain, is simple in this regards. There is only a\nminor adaptation for the ->runtime_suspend|resume() callbacks, which\navoids validating dev_pm_qos constraints during system sleep. Nothing\nspecial is needed in ->suspend_late|noirq callbacks, etc.\n\nFor most other simple bus types, like the platform bus, spi, i2c,\namba, no particular adoptions is needed at all. Instead those just\ntrust the drivers to do the right thing.\n\nBefore we had the direct_complete path, using the\npm_runtime_force_suspend|resume() helpers, was the only good way for\nthese kind of drivers, to in an optimized manner, deal with system\nsleep when runtime PM also was enabled for their devices. Now this\nmethod has become widely deployed, unfortunate whether you like it or\nnot.\n\nBesides the slightly better optimizations you get when using\npm_runtime_force_suspend|resume(), comparing to the direct_complete\npath - I think it's also worth to consider, how easy it becomes for\ndrivers to deploy system sleep support. In many cases, only two lines\nof code is needed to add system sleep support in a driver.\n\nNow, some complex code always needs to be implemented somewhere. When\nusing the runtime PM centric path, that code consist of the\npm_runtime_force_suspend|resume() helpers itself - and some\nadaptations in buses/PM domains in cases when those needs special\ncare.\n\nMy point is, the runtime PM centric path, allows us to keep the\ncomplex part of the code at a few centralized places, instead of\nhaving it spread/duplicated into drivers.\n\nSo yes, you could consider it insane, but to me and many others, it\nseems to work out quite well.\n\nYeah, and the laying violation is undoubtedly the most controversial\npart of the runtime PM centric path - I agree to that! The\ndirect_complete path don't have this, as you implemented it. :-)\n\nOn the other hand, one could consider that these upper layers, in many\ncases anyway needs to play along with the behavior of the driver. So,\nI guess it depends on how one see it.\n\n>\n> That's why I'm afraid that we've reached a dead end here. :-(\n\nThat's said news. Is was really hoping I could find a way to move this\nforward. You don't have any other ideas on how I can adjust the series\nto make you happy?\n\n>\n> Thanks,\n> Rafael\n>\n\nKind regards\nUffe","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"jl8hxeK7\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xj1C06h1pz9sNn\n\tfor ;\n\tWed, 30 Aug 2017 19:57:32 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751358AbdH3J5b (ORCPT );\n\tWed, 30 Aug 2017 05:57:31 -0400","from mail-io0-f177.google.com ([209.85.223.177]:38284 \"EHLO\n\tmail-io0-f177.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751298AbdH3J53 (ORCPT\n\t); Wed, 30 Aug 2017 05:57:29 -0400","by mail-io0-f177.google.com with SMTP id 81so3458591ioj.5\n\tfor ; Wed, 30 Aug 2017 02:57:29 -0700 (PDT)","by 10.2.96.38 with HTTP; Wed, 30 Aug 2017 02:57:28 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=oNZ0nFlT/0ox637VDpuaZmezigONBFIUNckEfNpXsu8=;\n\tb=jl8hxeK7ZTpKbOlcuqxhf5bpgB7bnMRdCVavr9BmLL8w69JGDtzEuIFEevvWo+resi\n\tG9IeskK+psUjSYODY1EWWPz5g4aP+zUsXNb/2xaidoRhi+YQ5hTuBqYFo5OUH+8/3jDz\n\tEpDPwXaK2Egn8CtVO0d6XYZKSC7caf1vHlef0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=oNZ0nFlT/0ox637VDpuaZmezigONBFIUNckEfNpXsu8=;\n\tb=hNJzFQWtE51RNIh40DO4hKzlZfuYz/AT1InHWBML01Q9HvV/RxR6Exusv5be5V5s92\n\t3g9hXWzBjCpGwRadcYTHjBt+240VmQ/sJUHuLHh/3X6RPtQnaNuRcX0peaoCdBUxedm8\n\tXOkJWRz2Jre13IkA+Bwbvur0gwZUPNNXxW55/ecseJiEOS8QK/QVntIYUgcx3wgQ/EM2\n\tcoJWs+NjF3v6c4FljyR2bsDD2+kpJxyaQKk0syt1bGmlAXsJKh2tCQsT6u6U8PYXvuTt\n\t/Yn0kPWYca+9GchTx+1PjD9OABrZQlY8vBlIbAXXmxf2RwQiBms7LiDnWlz0MPbGcEdl\n\tlvBg==","X-Gm-Message-State":"AHPjjUjo/86rlpqVPDmDO7bB96PJoZwlIJMlNxS+jmgqSRJxtWAjt369\n\tgFQ66xefEOgOa3xT3yJ35cSiLcGzqcVA","X-Received":"by 10.107.158.12 with SMTP id h12mr766558ioe.222.1504087049011; \n\tWed, 30 Aug 2017 02:57:29 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1957183.ZU1CWrAZoR@aspire.rjw.lan>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<1957183.ZU1CWrAZoR@aspire.rjw.lan>","From":"Ulf Hansson ","Date":"Wed, 30 Aug 2017 11:57:28 +0200","Message-ID":"","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","To":"\"Rafael J. Wysocki\" ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1760583,"web_url":"http://patchwork.ozlabs.org/comment/1760583/","msgid":"<3033151.kbPpEQGlRq@aspire.rjw.lan>","list_archive_url":null,"date":"2017-08-31T00:17:41","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"Disclaimer: I'm falling asleep, so I probably shouldn't reply to email right\nnow, but tomorrow I may not be able to get to email at all, so I'll try anyway.\n\nOn Wednesday, August 30, 2017 11:57:28 AM CEST Ulf Hansson wrote:\n> On 29 August 2017 at 22:19, Rafael J. Wysocki wrote:\n> > On Tuesday, August 29, 2017 4:56:42 PM CEST Ulf Hansson wrote:\n> >> The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,\n> >> isn't well optimized for system sleep.\n> >>\n> >> What makes this driver particularly interesting is because it's a cross-SoC\n> >> driver, which sometimes means there is an ACPI PM domain attached to the i2c\n> >> device and sometimes not. The driver is being used on both x86 and ARM.\n> >>\n> >> In principle, to optimize the system sleep support in i2c driver, this series\n> >> enables the proven runtime PM centric path for the i2c driver. However, to do\n> >> that the ACPI PM domain also have to collaborate and understand this behaviour.\n> >> From earlier versions, Rafael has also pointed out that also the PM core needs\n> >> to be involved.\n> >\n> > Earlier today I realized that drivers pointing their ->suspend_late and\n> > ->resume_early callbacks, respectively, to pm_runtime_force_suspend() and\n> > pm_runtime_force_resume(), are fundamentally incompatible with any bus type\n> > doing nontrivial PM and with almost any nontrivial PM domains, for two reasons.\n> >\n> > First, it basically requires the bus type or PM domain to expect that its\n> > ->runtime_suspend callback may or may not be indirectly invoked from its\n> > own ->suspend_late callback, depending on the driver (and analogously\n> > for ->runtime_resume and ->resume early), which is insane.\n> >\n> > Second, it is a layering violation, because it makes the driver effectively\n> > override the upper layer's decisions about what code to run.\n> \n> You are right that for more complex bus types and PM domains, those\n> needs to play along. So that is what I am trying to implement for the\n> ACPI PM domain in this series.\n\nWell, \"play along\" is a bit of an understatement here.\n\nThey would need to turn into horrible mess and that's not going to happen.\n\n> The generic PM domain, is simple in this regards. There is only a\n> minor adaptation for the ->runtime_suspend|resume() callbacks, which\n> avoids validating dev_pm_qos constraints during system sleep. Nothing\n> special is needed in ->suspend_late|noirq callbacks, etc.\n> \n> For most other simple bus types, like the platform bus, spi, i2c,\n> amba, no particular adoptions is needed at all. Instead those just\n> trust the drivers to do the right thing.\n\nThey are the trivial ones.\n\n> Before we had the direct_complete path, using the\n> pm_runtime_force_suspend|resume() helpers, was the only good way for\n> these kind of drivers, to in an optimized manner, deal with system\n> sleep when runtime PM also was enabled for their devices. Now this\n> method has become widely deployed, unfortunate whether you like it or\n> not.\n\nSo can you please remind me why the _force_ wrappers are needed?\n\nIn particular, why can't drivers arrange their callbacks the way I did that\nin https://patchwork.kernel.org/patch/9928583/ ?\n\n> Besides the slightly better optimizations you get when using\n> pm_runtime_force_suspend|resume(), comparing to the direct_complete\n> path - I think it's also worth to consider, how easy it becomes for\n> drivers to deploy system sleep support. In many cases, only two lines\n> of code is needed to add system sleep support in a driver.\n\nYou are doing a wrong comparison here IMO. You essentially are comparing two\nbandaids with each other and arguing that one of them somehow is better.\n\nWhat about doing something which is not a bandaid instead?\n\n> Now, some complex code always needs to be implemented somewhere. When\n> using the runtime PM centric path, that code consist of the\n> pm_runtime_force_suspend|resume() helpers itself - and some\n> adaptations in buses/PM domains in cases when those needs special\n> care.\n> \n> My point is, the runtime PM centric path, allows us to keep the\n> complex part of the code at a few centralized places, instead of\n> having it spread/duplicated into drivers.\n> \n> So yes, you could consider it insane, but to me and many others, it\n> seems to work out quite well.\n\nBecause it only has been used with trivial middle layer code so far\nand I'm quite disappointed that you don't seem to see a problem here. :-/\n\nI mean something like\n\nPM core => bus type / PM domain ->suspend_late => driver ->suspend_late\n\nis far more straightforward than\n\nPM core => bus type / PM domain ->suspend_late => driver ->suspend_late =>\n\tbus type / PM domain ->runtime_suspend => driver ->runtime_suspend\n\nwith the bus type / PM domain having to figure out somehow at the\n->suspend_late time whether or not its ->runtume_suspend is going to be invoked\nin the middle of it.\n\nApart from this just being aesthetically disgusting to me, which admittedly is\na matter of personal opinion, it makes debugging new driver code harder (if it\nhappens to not work) and reviewing it almost impossible, because now you need\nto take all of the tangling between callbacks into accont and sometimes not\njust for one bus type / PM domain.\n\n> Yeah, and the laying violation is undoubtedly the most controversial\n> part of the runtime PM centric path - I agree to that! The\n> direct_complete path don't have this, as you implemented it. :-)\n> \n> On the other hand, one could consider that these upper layers, in many\n> cases anyway needs to play along with the behavior of the driver. So,\n> I guess it depends on how one see it.\n> \n> >\n> > That's why I'm afraid that we've reached a dead end here. :-(\n> \n> That's said news. Is was really hoping I could find a way to move this\n> forward. You don't have any other ideas on how I can adjust the series\n> to make you happy?\n\nSo to be precise, patches [2-3/8] are basically fine by me. Patch [4/8]\nsort of works too, but I'd do the splitting slightly differently and I don't\nsee much value in it alone.\n\nThe rest of the ACPI changes is mostly not acceptable to me, mostly because\nof what is done to the PM domain's ->runtime_suspend/resume and\n->suspend_late/->resume_early callbacks.\n\nI guess the only way that could be made work for me would be by not using\n_force_suspend/resume() at all, but that would defeat the point, right?\n\nI don't like the flag too, but that might be worked out.\n\nAlso, when I looked at _force_suspend/resume() again, I got concerned.\nThere is stuff in there that shouldn't be necessary in a driver's\n->late_suspend/->early_resume and some things in there just made me\nscratch my head.\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjNTZ4Dxzz9s83\n\tfor ;\n\tThu, 31 Aug 2017 10:26:26 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750973AbdHaA0Y (ORCPT );\n\tWed, 30 Aug 2017 20:26:24 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:41309 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1750885AbdHaA0Y (ORCPT\n\t); Wed, 30 Aug 2017 20:26:24 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 8f7e48eca07b2e14; Thu, 31 Aug 2017 02:26:22 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","Date":"Thu, 31 Aug 2017 02:17:41 +0200","Message-ID":"<3033151.kbPpEQGlRq@aspire.rjw.lan>","In-Reply-To":"","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<1957183.ZU1CWrAZoR@aspire.rjw.lan>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1761579,"web_url":"http://patchwork.ozlabs.org/comment/1761579/","msgid":"","list_archive_url":null,"date":"2017-09-01T10:42:35","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":21036,"url":"http://patchwork.ozlabs.org/api/people/21036/","name":"Ulf Hansson","email":"ulf.hansson@linaro.org"},"content":"On 31 August 2017 at 02:17, Rafael J. Wysocki wrote:\n> Disclaimer: I'm falling asleep, so I probably shouldn't reply to email right\n> now, but tomorrow I may not be able to get to email at all, so I'll try anyway.\n>\n> On Wednesday, August 30, 2017 11:57:28 AM CEST Ulf Hansson wrote:\n>> On 29 August 2017 at 22:19, Rafael J. Wysocki wrote:\n>> > On Tuesday, August 29, 2017 4:56:42 PM CEST Ulf Hansson wrote:\n>> >> The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,\n>> >> isn't well optimized for system sleep.\n>> >>\n>> >> What makes this driver particularly interesting is because it's a cross-SoC\n>> >> driver, which sometimes means there is an ACPI PM domain attached to the i2c\n>> >> device and sometimes not. The driver is being used on both x86 and ARM.\n>> >>\n>> >> In principle, to optimize the system sleep support in i2c driver, this series\n>> >> enables the proven runtime PM centric path for the i2c driver. However, to do\n>> >> that the ACPI PM domain also have to collaborate and understand this behaviour.\n>> >> From earlier versions, Rafael has also pointed out that also the PM core needs\n>> >> to be involved.\n>> >\n>> > Earlier today I realized that drivers pointing their ->suspend_late and\n>> > ->resume_early callbacks, respectively, to pm_runtime_force_suspend() and\n>> > pm_runtime_force_resume(), are fundamentally incompatible with any bus type\n>> > doing nontrivial PM and with almost any nontrivial PM domains, for two reasons.\n>> >\n>> > First, it basically requires the bus type or PM domain to expect that its\n>> > ->runtime_suspend callback may or may not be indirectly invoked from its\n>> > own ->suspend_late callback, depending on the driver (and analogously\n>> > for ->runtime_resume and ->resume early), which is insane.\n>> >\n>> > Second, it is a layering violation, because it makes the driver effectively\n>> > override the upper layer's decisions about what code to run.\n>>\n>> You are right that for more complex bus types and PM domains, those\n>> needs to play along. So that is what I am trying to implement for the\n>> ACPI PM domain in this series.\n>\n> Well, \"play along\" is a bit of an understatement here.\n>\n> They would need to turn into horrible mess and that's not going to happen.\n\nI absolutely agree, there must be no mess what so ever!\n\nBut, I don't want to give up yet, I still believe I can make this\nseries into a nice couple of changes for the ACPI PM domain.\nEspecially if you continue giving me your guidance.\n\n>\n>> The generic PM domain, is simple in this regards. There is only a\n>> minor adaptation for the ->runtime_suspend|resume() callbacks, which\n>> avoids validating dev_pm_qos constraints during system sleep. Nothing\n>> special is needed in ->suspend_late|noirq callbacks, etc.\n>>\n>> For most other simple bus types, like the platform bus, spi, i2c,\n>> amba, no particular adoptions is needed at all. Instead those just\n>> trust the drivers to do the right thing.\n>\n> They are the trivial ones.\n\nYes.\n\nHowever, the platform bus is also very commonly used in kernel. I\nthink that's an important thing to consider.\n\n>\n>> Before we had the direct_complete path, using the\n>> pm_runtime_force_suspend|resume() helpers, was the only good way for\n>> these kind of drivers, to in an optimized manner, deal with system\n>> sleep when runtime PM also was enabled for their devices. Now this\n>> method has become widely deployed, unfortunate whether you like it or\n>> not.\n>\n> So can you please remind me why the _force_ wrappers are needed?\n\nSee below.\n\n>\n> In particular, why can't drivers arrange their callbacks the way I did that\n> in https://patchwork.kernel.org/patch/9928583/ ?\n\nI was preparing a reply to that patch, but let me summarize that here instead.\n\nLet me be clear, the patch is an improvement of the behavior of the\ndriver and it addresses the issues you point out in the change log.\nRe-using the runtime PM callbacks for system sleep, is nice as it\navoids open coding, which is of curse also one of the reason of using\npm_runtime_force_suspend|resume().\n\nStill there are a couple of things I am worried about in this patch.\n*)\nTo be able to re-use the same callbacks for system sleep and runtime\nPM, some boilerplate code is added to the driver, as to cope with the\ndifferent conditions inside the callbacks. That pattern would become\nrepeated to many drivers dealing with similar issues.\n\n**)\nThe ->resume_early() callback powers on the device, in case it was\nruntime resumed when the ->suspend_late() callback was invoked. That\nis in many cases completely unnecessary, causing us to waste power and\nincrease system resume time, for absolutely no reason. However, I\nunderstand the patch didn't try to address this, but to really fix\nthis, there has to be an even closer collaboration between runtime PM\nand the system sleep callbacks.\n\nSo, to remind you why the pm_runtime_force_suspend|resume() helpers is\npreferred, that's because both of the above two things becomes taken\ncare of.\n\n>\n>> Besides the slightly better optimizations you get when using\n>> pm_runtime_force_suspend|resume(), comparing to the direct_complete\n>> path - I think it's also worth to consider, how easy it becomes for\n>> drivers to deploy system sleep support. In many cases, only two lines\n>> of code is needed to add system sleep support in a driver.\n>\n> You are doing a wrong comparison here IMO. You essentially are comparing two\n> bandaids with each other and arguing that one of them somehow is better.\n\nI just wanted to compare against something...\n\n>\n> What about doing something which is not a bandaid instead?\n\nI don't have a problem working on something new, but I am not sure\nwhat that should be.\n\nUnless you re-consider moving forward in some form, with the current\nsuggested approach for the ACPI PM domain, can you give me some\npointers on what you have in mind?\n\nTo remind you of my current view, the direct_complete path is useful\nfor PM domains, like the ACPI PM domain as it impacts all its devices.\nUsing pm_runtime_force_suspend|resume() offers the next steps to\nachieve a fully optimized behavior of a device during system sleep, as\nalready been proven by now. It would be great to both options\nsupported by the ACPI PM domain.\n\nAnother related thing that is causing lots of problems during system\nsleep of devices, but not related to optimizations, is to have the\ncorrect order of how to suspend/resume the devices. We have talked\nabout this, but it's a separate problem and it's rather a deployment\nissue, than having to implements something entirely new (we have\nsupplies/consumers links you invented for this).\n\n>\n>> Now, some complex code always needs to be implemented somewhere. When\n>> using the runtime PM centric path, that code consist of the\n>> pm_runtime_force_suspend|resume() helpers itself - and some\n>> adaptations in buses/PM domains in cases when those needs special\n>> care.\n>>\n>> My point is, the runtime PM centric path, allows us to keep the\n>> complex part of the code at a few centralized places, instead of\n>> having it spread/duplicated into drivers.\n>>\n>> So yes, you could consider it insane, but to me and many others, it\n>> seems to work out quite well.\n>\n> Because it only has been used with trivial middle layer code so far\n> and I'm quite disappointed that you don't seem to see a problem here. :-/\n>\n> I mean something like\n>\n> PM core => bus type / PM domain ->suspend_late => driver ->suspend_late\n>\n> is far more straightforward than\n>\n> PM core => bus type / PM domain ->suspend_late => driver ->suspend_late =>\n> bus type / PM domain ->runtime_suspend => driver ->runtime_suspend\n>\n> with the bus type / PM domain having to figure out somehow at the\n> ->suspend_late time whether or not its ->runtume_suspend is going to be invoked\n> in the middle of it.\n>\n> Apart from this just being aesthetically disgusting to me, which admittedly is\n> a matter of personal opinion, it makes debugging new driver code harder (if it\n> happens to not work) and reviewing it almost impossible, because now you need\n> to take all of the tangling between callbacks into accont and sometimes not\n> just for one bus type / PM domain.\n\nI am wondering that perhaps you may be overlooking some of the\ninternals of runtime PM. Or maybe not? :-)\n\nI mean, the hole thing is build upon that anyone can call runtime PM\nfunctions to runtime resume/suspend a device. Doing that, makes the\nhierarchy of the runtime PM callbacks being walked and invoked, of\ncourse properly managed by the runtime PM core.\n\nMy point is that, the runtime PM core still controls this behavior,\neven when the pm_runtime_force_suspend|resume() helpers are being\ninvoked. The only difference is that it allows runtime PM for the\ndevice to be disabled, and still correctly invoked the callbacks. That\nis what it is all about.\n\n>\n>> Yeah, and the laying violation is undoubtedly the most controversial\n>> part of the runtime PM centric path - I agree to that! The\n>> direct_complete path don't have this, as you implemented it. :-)\n>>\n>> On the other hand, one could consider that these upper layers, in many\n>> cases anyway needs to play along with the behavior of the driver. So,\n>> I guess it depends on how one see it.\n>>\n>> >\n>> > That's why I'm afraid that we've reached a dead end here. :-(\n>>\n>> That's said news. Is was really hoping I could find a way to move this\n>> forward. You don't have any other ideas on how I can adjust the series\n>> to make you happy?\n>\n> So to be precise, patches [2-3/8] are basically fine by me. Patch [4/8]\n> sort of works too, but I'd do the splitting slightly differently and I don't\n> see much value in it alone.\n>\n> The rest of the ACPI changes is mostly not acceptable to me, mostly because\n> of what is done to the PM domain's ->runtime_suspend/resume and\n> ->suspend_late/->resume_early callbacks.\n>\n> I guess the only way that could be made work for me would be by not using\n> _force_suspend/resume() at all, but that would defeat the point, right?\n\nYes, it would.\n\n>\n> I don't like the flag too, but that might be worked out.\n\nYeah, I am open to any suggestion.\n\n>\n> Also, when I looked at _force_suspend/resume() again, I got concerned.\n> There is stuff in there that shouldn't be necessary in a driver's\n> ->late_suspend/->early_resume and some things in there just made me\n> scratch my head.\n\nYes, there are some complexity in there, I will be happy to answer any\nspecific question about it.\n\nThe main thing is, that it tries to conform to the regular rules set\nby the runtime PM core when runtime PM is enabled for the device - and\nthen apply those to the device when runtime PM has been disabled for\nit.\n\nAgain, thanks for being patient and reviewing!\n\nKind regards\nUffe","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"Dm/gAG0s\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkG683JpNz9t32\n\tfor ;\n\tFri, 1 Sep 2017 20:42:40 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751864AbdIAKmi (ORCPT );\n\tFri, 1 Sep 2017 06:42:38 -0400","from mail-io0-f180.google.com ([209.85.223.180]:32771 \"EHLO\n\tmail-io0-f180.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751725AbdIAKmh (ORCPT\n\t); Fri, 1 Sep 2017 06:42:37 -0400","by mail-io0-f180.google.com with SMTP id b2so14142123iof.0\n\tfor ; Fri, 01 Sep 2017 03:42:36 -0700 (PDT)","by 10.2.96.38 with HTTP; Fri, 1 Sep 2017 03:42:35 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=mdFwhCaj9YvvcSznbjyqtrmatZCiI6HvZcMs3glZ6to=;\n\tb=Dm/gAG0s7nhu9/sbX0AlxavXlRBefrfCYLYZKXbyJACybj78HHK15AINCgu0+d58Eo\n\tJvoM8/lADyWXCOCN0jqtddbyzTV2FL97+0RLalybp+KAUNeecXMKz3BKwZWUQmySqvAc\n\t5iPMWEvEdWFngAaOWd2PxhTDAIER+yMwyr4ew=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=mdFwhCaj9YvvcSznbjyqtrmatZCiI6HvZcMs3glZ6to=;\n\tb=VJQRSOXznImCd4qc8R+ixKpYpHLpYQ0lHbBbtZRy53bnxuI9ZazEtf5tfrI305Ujax\n\t3cYhWW9xwIEoJY3b5GT+Z+Z2t4X9SXMcPavUM0hvtiGzhFzdHpI3QzHgEXkJeRq7uIIr\n\t5lUkyiOjs6p9Omnmyw7tssdJB0LL08Pmsd+trme++tJBLQK2M+1c+X5pZ1EX5eixjOgA\n\tQmI8UM1wKySrnPCmHQHHwbw0LmHTuswBWLqgaUQnz7paEdcBaRt9ziAmZIxgWn77KtTm\n\t8g0wbGdO/YimD92ykzvOsq7q7Kt3HajgJWyECPvCzsd29xZxKqXtawTlG5nVG9pYZfNn\n\tlXFg==","X-Gm-Message-State":"AHPjjUgebrtQq0Wgy6M3X7RDMAYNYUKwGKaveAoi6Vkivx8MtRNrEdW1\n\tDV34uABQq7Cajk0IvRprqjuAZLKxFKWT","X-Google-Smtp-Source":"ADKCNb4/2d8NpMz1byBo1BPM112p1cM1mCgEupDS7/RTuoH6DxD6N97/7KgFstN9yj5Zj4jBQ0SbMn8R4xFVQsVYCVc=","X-Received":"by 10.107.138.155 with SMTP id c27mr1037849ioj.103.1504262556195;\n\tFri, 01 Sep 2017 03:42:36 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<3033151.kbPpEQGlRq@aspire.rjw.lan>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<1957183.ZU1CWrAZoR@aspire.rjw.lan>\n\t\n\t<3033151.kbPpEQGlRq@aspire.rjw.lan>","From":"Ulf Hansson ","Date":"Fri, 1 Sep 2017 12:42:35 +0200","Message-ID":"","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","To":"\"Rafael J. Wysocki\" ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1762360,"web_url":"http://patchwork.ozlabs.org/comment/1762360/","msgid":"<1668277.7DWLdgHGid@aspire.rjw.lan>","list_archive_url":null,"date":"2017-09-04T00:17:21","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Friday, September 1, 2017 12:42:35 PM CEST Ulf Hansson wrote:\n> On 31 August 2017 at 02:17, Rafael J. Wysocki wrote:\n> > Disclaimer: I'm falling asleep, so I probably shouldn't reply to email right\n> > now, but tomorrow I may not be able to get to email at all, so I'll try anyway.\n> >\n> > On Wednesday, August 30, 2017 11:57:28 AM CEST Ulf Hansson wrote:\n> >> On 29 August 2017 at 22:19, Rafael J. Wysocki wrote:\n> >> > On Tuesday, August 29, 2017 4:56:42 PM CEST Ulf Hansson wrote:\n> >> >> The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c,\n> >> >> isn't well optimized for system sleep.\n> >> >>\n> >> >> What makes this driver particularly interesting is because it's a cross-SoC\n> >> >> driver, which sometimes means there is an ACPI PM domain attached to the i2c\n> >> >> device and sometimes not. The driver is being used on both x86 and ARM.\n> >> >>\n> >> >> In principle, to optimize the system sleep support in i2c driver, this series\n> >> >> enables the proven runtime PM centric path for the i2c driver. However, to do\n> >> >> that the ACPI PM domain also have to collaborate and understand this behaviour.\n> >> >> From earlier versions, Rafael has also pointed out that also the PM core needs\n> >> >> to be involved.\n> >> >\n> >> > Earlier today I realized that drivers pointing their ->suspend_late and\n> >> > ->resume_early callbacks, respectively, to pm_runtime_force_suspend() and\n> >> > pm_runtime_force_resume(), are fundamentally incompatible with any bus type\n> >> > doing nontrivial PM and with almost any nontrivial PM domains, for two reasons.\n> >> >\n> >> > First, it basically requires the bus type or PM domain to expect that its\n> >> > ->runtime_suspend callback may or may not be indirectly invoked from its\n> >> > own ->suspend_late callback, depending on the driver (and analogously\n> >> > for ->runtime_resume and ->resume early), which is insane.\n> >> >\n> >> > Second, it is a layering violation, because it makes the driver effectively\n> >> > override the upper layer's decisions about what code to run.\n> >>\n> >> You are right that for more complex bus types and PM domains, those\n> >> needs to play along. So that is what I am trying to implement for the\n> >> ACPI PM domain in this series.\n> >\n> > Well, \"play along\" is a bit of an understatement here.\n> >\n> > They would need to turn into horrible mess and that's not going to happen.\n> \n> I absolutely agree, there must be no mess what so ever!\n> \n> But, I don't want to give up yet, I still believe I can make this\n> series into a nice couple of changes for the ACPI PM domain.\n\nWell, as far as I'm concerned, this is not going to get any further.\n\n> Especially if you continue giving me your guidance.\n> \n> >\n> >> The generic PM domain, is simple in this regards. There is only a\n> >> minor adaptation for the ->runtime_suspend|resume() callbacks, which\n> >> avoids validating dev_pm_qos constraints during system sleep. Nothing\n> >> special is needed in ->suspend_late|noirq callbacks, etc.\n> >>\n> >> For most other simple bus types, like the platform bus, spi, i2c,\n> >> amba, no particular adoptions is needed at all. Instead those just\n> >> trust the drivers to do the right thing.\n> >\n> > They are the trivial ones.\n> \n> Yes.\n> \n> However, the platform bus is also very commonly used in kernel. I\n> think that's an important thing to consider.\n\nWell, so is ACPI, and PCI.\n\nAnd the platform bus doesn't do any kind of PM handling by itself,\nwhereas the above do. Therefore trying to look at the platform bus as\nan example to follow by them is rather not useful IMO.\n\n> >\n> >> Before we had the direct_complete path, using the\n> >> pm_runtime_force_suspend|resume() helpers, was the only good way for\n> >> these kind of drivers, to in an optimized manner, deal with system\n> >> sleep when runtime PM also was enabled for their devices. Now this\n> >> method has become widely deployed, unfortunate whether you like it or\n> >> not.\n> >\n> > So can you please remind me why the _force_ wrappers are needed?\n> \n> See below.\n> \n> >\n> > In particular, why can't drivers arrange their callbacks the way I did that\n> > in https://patchwork.kernel.org/patch/9928583/ ?\n> \n> I was preparing a reply to that patch, but let me summarize that here instead.\n> \n> Let me be clear, the patch is an improvement of the behavior of the\n> driver and it addresses the issues you point out in the change log.\n> Re-using the runtime PM callbacks for system sleep, is nice as it\n> avoids open coding, which is of curse also one of the reason of using\n> pm_runtime_force_suspend|resume().\n> \n> Still there are a couple of things I am worried about in this patch.\n> *)\n> To be able to re-use the same callbacks for system sleep and runtime\n> PM, some boilerplate code is added to the driver, as to cope with the\n> different conditions inside the callbacks. That pattern would become\n> repeated to many drivers dealing with similar issues.\n\nI'm not worried about that as long as there are good examples and\ndocumented best practices.\n\nThere aren't any right now, which is a problem, but that certainly is\nfixable.\n\n> **)\n> The ->resume_early() callback powers on the device, in case it was\n> runtime resumed when the ->suspend_late() callback was invoked. That\n> is in many cases completely unnecessary, causing us to waste power and\n> increase system resume time, for absolutely no reason. However, I\n> understand the patch didn't try to address this, but to really fix\n> this, there has to be an even closer collaboration between runtime PM\n> and the system sleep callbacks.\n\nI don't quite agree and here's why.\n\nIf a device was not runtime-suspended right before system suspend, then quite\nlikely it was in use then. Therefore it is quite likely to be resumed\nimmediately after system resume anyway.\n\nNow, if that's just one device, it probably doesn't matter, but if there are\nmore devices like that, they will be resumed after system suspend when they\nare accessed and quite likely they will be accessed one-by-one rather than in\nparallel with each other, so the latencies related to that will add up. In\nthat case it is better to resume them upfront during system resume as they will\nbe resumed in parallel with each other then. And that also is *way* simpler.\n\nThis means that the benefit from avoiding to resume devices during system\nresume is not quite so obvious and the whole point above is highly\nquestionable.\n\n> \n> So, to remind you why the pm_runtime_force_suspend|resume() helpers is\n> preferred, that's because both of the above two things becomes taken\n> care of.\n\nAnd that is why there is this stuff about parents and usage counters, right?\n\nI'm not liking it at all.\n\n> \n> >\n> >> Besides the slightly better optimizations you get when using\n> >> pm_runtime_force_suspend|resume(), comparing to the direct_complete\n> >> path - I think it's also worth to consider, how easy it becomes for\n> >> drivers to deploy system sleep support. In many cases, only two lines\n> >> of code is needed to add system sleep support in a driver.\n> >\n> > You are doing a wrong comparison here IMO. You essentially are comparing two\n> > bandaids with each other and arguing that one of them somehow is better.\n> \n> I just wanted to compare against something...\n> \n> >\n> > What about doing something which is not a bandaid instead?\n> \n> I don't have a problem working on something new, but I am not sure\n> what that should be.\n> \n> Unless you re-consider moving forward in some form, with the current\n> suggested approach for the ACPI PM domain, can you give me some\n> pointers on what you have in mind?\n\nYes.\n\nDo what was indended from the start and make drivers re-use runtime\nPM callbacks for ->suspend_late and ->resume_early.\n\nFirst, that can be done.\n\nSecond, it is *conceptually* much more straightforward than things like\n_force_suspend/resume().\n\nNext, the drivers have full control on what they do in that case and\ncan be made work with any middle-layer core easily enough.\n\nNo layering violations, no insane callback chains.\n\n> To remind you of my current view, the direct_complete path is useful\n> for PM domains, like the ACPI PM domain as it impacts all its devices.\n> Using pm_runtime_force_suspend|resume() offers the next steps to\n\nI completely disagree at this point.\n\nSo to be clear, the invocation of moddle-layer callbacks instead of\n*driver* callbacks in pm_runtime_force_suspend|resume() is a grave mistake.\nThey would have been almost possible to work with had they just invoke\ndriver callbacks.\n\nOTOH I'm starting to think that direct_complete is only theoretically useful\nand may not be actually set very often in practice, whereas it adds significant\ncomplexity to the code, so I'm not sure about it any more.\n\n> achieve a fully optimized behavior of a device during system sleep, as\n> already been proven by now. It would be great to both options\n> supported by the ACPI PM domain.\n\nNo.\n\n> Another related thing that is causing lots of problems during system\n> sleep of devices, but not related to optimizations, is to have the\n> correct order of how to suspend/resume the devices. We have talked\n> about this, but it's a separate problem and it's rather a deployment\n> issue, than having to implements something entirely new (we have\n> supplies/consumers links you invented for this).\n\nYes, that is a separate issue.\n\n> >\n> >> Now, some complex code always needs to be implemented somewhere. When\n> >> using the runtime PM centric path, that code consist of the\n> >> pm_runtime_force_suspend|resume() helpers itself - and some\n> >> adaptations in buses/PM domains in cases when those needs special\n> >> care.\n> >>\n> >> My point is, the runtime PM centric path, allows us to keep the\n> >> complex part of the code at a few centralized places, instead of\n> >> having it spread/duplicated into drivers.\n> >>\n> >> So yes, you could consider it insane, but to me and many others, it\n> >> seems to work out quite well.\n> >\n> > Because it only has been used with trivial middle layer code so far\n> > and I'm quite disappointed that you don't seem to see a problem here. :-/\n> >\n> > I mean something like\n> >\n> > PM core => bus type / PM domain ->suspend_late => driver ->suspend_late\n> >\n> > is far more straightforward than\n> >\n> > PM core => bus type / PM domain ->suspend_late => driver ->suspend_late =>\n> > bus type / PM domain ->runtime_suspend => driver ->runtime_suspend\n> >\n> > with the bus type / PM domain having to figure out somehow at the\n> > ->suspend_late time whether or not its ->runtume_suspend is going to be invoked\n> > in the middle of it.\n> >\n> > Apart from this just being aesthetically disgusting to me, which admittedly is\n> > a matter of personal opinion, it makes debugging new driver code harder (if it\n> > happens to not work) and reviewing it almost impossible, because now you need\n> > to take all of the tangling between callbacks into accont and sometimes not\n> > just for one bus type / PM domain.\n> \n> I am wondering that perhaps you may be overlooking some of the\n> internals of runtime PM. Or maybe not? :-)\n> \n> I mean, the hole thing is build upon that anyone can call runtime PM\n> functions to runtime resume/suspend a device.\n\nWell, right in general, except that _force_suspend/resume() invoke\n*callbacks* and *not* runtime PM functions.\n\n> Doing that, makes the\n> hierarchy of the runtime PM callbacks being walked and invoked, of\n> course properly managed by the runtime PM core.\n> \n> My point is that, the runtime PM core still controls this behavior,\n> even when the pm_runtime_force_suspend|resume() helpers are being\n> invoked. The only difference is that it allows runtime PM for the\n> device to be disabled, and still correctly invoked the callbacks. That\n> is what it is all about.\n\nSo why is it even useful to call ->runtime_suspend from a middle layer\nin pm_runtime_force_suspend(), for example?\n\n> >\n> >> Yeah, and the laying violation is undoubtedly the most controversial\n> >> part of the runtime PM centric path - I agree to that! The\n> >> direct_complete path don't have this, as you implemented it. :-)\n> >>\n> >> On the other hand, one could consider that these upper layers, in many\n> >> cases anyway needs to play along with the behavior of the driver. So,\n> >> I guess it depends on how one see it.\n> >>\n> >> >\n> >> > That's why I'm afraid that we've reached a dead end here. :-(\n> >>\n> >> That's said news. Is was really hoping I could find a way to move this\n> >> forward. You don't have any other ideas on how I can adjust the series\n> >> to make you happy?\n> >\n> > So to be precise, patches [2-3/8] are basically fine by me. Patch [4/8]\n> > sort of works too, but I'd do the splitting slightly differently and I don't\n> > see much value in it alone.\n> >\n> > The rest of the ACPI changes is mostly not acceptable to me, mostly because\n> > of what is done to the PM domain's ->runtime_suspend/resume and\n> > ->suspend_late/->resume_early callbacks.\n> >\n> > I guess the only way that could be made work for me would be by not using\n> > _force_suspend/resume() at all, but that would defeat the point, right?\n> \n> Yes, it would.\n> \n> >\n> > I don't like the flag too, but that might be worked out.\n> \n> Yeah, I am open to any suggestion.\n> \n> >\n> > Also, when I looked at _force_suspend/resume() again, I got concerned.\n> > There is stuff in there that shouldn't be necessary in a driver's\n> > ->late_suspend/->early_resume and some things in there just made me\n> > scratch my head.\n> \n> Yes, there are some complexity in there, I will be happy to answer any\n> specific question about it.\n\nOK\n\nOf course they require runtime PM to be enabled by drivers using them as\ntheir callbacks, but I suppose that you realize that.\n\nWhy to disabe/renable runtime PM in there in the first place? That should\nhave been done by the core when these functions are intended to be called.\n\nSecond, why to use RPM_GET_CALLBACK in there?\n\nNext, how is the parent actually runtime-resumed by pm_runtime_force_resume()\nwhich the comment in pm_runtime_force_suspend() talks about?\n\n> \n> The main thing is, that it tries to conform to the regular rules set\n> by the runtime PM core when runtime PM is enabled for the device - and\n> then apply those to the device when runtime PM has been disabled for\n> it.\n\nSorry, I'm not sure what this means really ...\n\n> Again, thanks for being patient and reviewing!\n\nWell, no problem.\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xlrHV4gFcz9s8J\n\tfor ;\n\tMon, 4 Sep 2017 10:26:14 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753111AbdIDA0K (ORCPT );\n\tSun, 3 Sep 2017 20:26:10 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:47613 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1753046AbdIDA0J (ORCPT\n\t); Sun, 3 Sep 2017 20:26:09 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 977448f64dceb8fe; Mon, 4 Sep 2017 02:26:06 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","Date":"Mon, 04 Sep 2017 02:17:21 +0200","Message-ID":"<1668277.7DWLdgHGid@aspire.rjw.lan>","In-Reply-To":"","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<3033151.kbPpEQGlRq@aspire.rjw.lan>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1762428,"web_url":"http://patchwork.ozlabs.org/comment/1762428/","msgid":"<20170904054637.GA23707@wunner.de>","list_archive_url":null,"date":"2017-09-04T05:46:37","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":68499,"url":"http://patchwork.ozlabs.org/api/people/68499/","name":"Lukas Wunner","email":"lukas@wunner.de"},"content":"On Mon, Sep 04, 2017 at 02:17:21AM +0200, Rafael J. Wysocki wrote:\n> OTOH I'm starting to think that direct_complete is only theoretically\n> useful and may not be actually set very often in practice, whereas it\n> adds significant complexity to the code, so I'm not sure about it any\n> more.\n\nThat makes me come out of the woodwork as a direct_complete fan:\n\nRuntime resuming a discrete GPU on a modern dual GPU laptop takes about\n1.5 sec, runtime resuming Thunderbolt controllers more than 2 sec.\nA discrete GPU easily consumes 10W, a Thunderbolt controller 2W.\n\nSo not having direct_complete would noticeably delay system suspend and\nresume as well as reduce battery life.\n\nLukas","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xlzPH0l2kz9s7M\n\tfor ;\n\tMon, 4 Sep 2017 15:46:43 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751620AbdIDFql (ORCPT );\n\tMon, 4 Sep 2017 01:46:41 -0400","from mailout2.hostsharing.net ([83.223.90.233]:45169 \"EHLO\n\tmailout2.hostsharing.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750782AbdIDFqk (ORCPT\n\t); Mon, 4 Sep 2017 01:46:40 -0400","from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby mailout2.hostsharing.net (Postfix) with ESMTPS id 26DF1101712DC;\n\tMon, 4 Sep 2017 07:46:38 +0200 (CEST)","by h08.hostsharing.net (Postfix, from userid 100393)\n\tid E7D8C7C35F9; Mon, 4 Sep 2017 07:46:37 +0200 (CEST)"],"Date":"Mon, 4 Sep 2017 07:46:37 +0200","From":"Lukas Wunner ","To":"\"Rafael J. Wysocki\" ","Cc":"Ulf Hansson ,\n\tWolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","Message-ID":"<20170904054637.GA23707@wunner.de>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<3033151.kbPpEQGlRq@aspire.rjw.lan>\n\t\n\t<1668277.7DWLdgHGid@aspire.rjw.lan>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1668277.7DWLdgHGid@aspire.rjw.lan>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1762552,"web_url":"http://patchwork.ozlabs.org/comment/1762552/","msgid":"<3410095.RTqPGnhnfq@aspire.rjw.lan>","list_archive_url":null,"date":"2017-09-04T10:04:58","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Monday, September 4, 2017 7:46:37 AM CEST Lukas Wunner wrote:\n> On Mon, Sep 04, 2017 at 02:17:21AM +0200, Rafael J. Wysocki wrote:\n> > OTOH I'm starting to think that direct_complete is only theoretically\n> > useful and may not be actually set very often in practice, whereas it\n> > adds significant complexity to the code, so I'm not sure about it any\n> > more.\n> \n> That makes me come out of the woodwork as a direct_complete fan:\n> \n> Runtime resuming a discrete GPU on a modern dual GPU laptop takes about\n> 1.5 sec, runtime resuming Thunderbolt controllers more than 2 sec.\n> A discrete GPU easily consumes 10W, a Thunderbolt controller 2W.\n> \n> So not having direct_complete would noticeably delay system suspend and\n> resume as well as reduce battery life.\n\nWell, that's a good reason for having it. :-)\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm5KT4lJLz9s7m\n\tfor ;\n\tMon, 4 Sep 2017 20:13:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753469AbdIDKNr (ORCPT );\n\tMon, 4 Sep 2017 06:13:47 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:61974 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1753344AbdIDKNr (ORCPT\n\t); Mon, 4 Sep 2017 06:13:47 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 195fe62584e15445; Mon, 4 Sep 2017 12:13:45 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Lukas Wunner ","Cc":"Ulf Hansson ,\n\tWolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","Date":"Mon, 04 Sep 2017 12:04:58 +0200","Message-ID":"<3410095.RTqPGnhnfq@aspire.rjw.lan>","In-Reply-To":"<20170904054637.GA23707@wunner.de>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<1668277.7DWLdgHGid@aspire.rjw.lan>\n\t<20170904054637.GA23707@wunner.de>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1762607,"web_url":"http://patchwork.ozlabs.org/comment/1762607/","msgid":"","list_archive_url":null,"date":"2017-09-04T12:55:37","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":21036,"url":"http://patchwork.ozlabs.org/api/people/21036/","name":"Ulf Hansson","email":"ulf.hansson@linaro.org"},"content":"[...]\n\n>> > So can you please remind me why the _force_ wrappers are needed?\n>>\n>> See below.\n>>\n>> >\n>> > In particular, why can't drivers arrange their callbacks the way I did that\n>> > in https://patchwork.kernel.org/patch/9928583/ ?\n>>\n>> I was preparing a reply to that patch, but let me summarize that here instead.\n>>\n>> Let me be clear, the patch is an improvement of the behavior of the\n>> driver and it addresses the issues you point out in the change log.\n>> Re-using the runtime PM callbacks for system sleep, is nice as it\n>> avoids open coding, which is of curse also one of the reason of using\n>> pm_runtime_force_suspend|resume().\n>>\n>> Still there are a couple of things I am worried about in this patch.\n>> *)\n>> To be able to re-use the same callbacks for system sleep and runtime\n>> PM, some boilerplate code is added to the driver, as to cope with the\n>> different conditions inside the callbacks. That pattern would become\n>> repeated to many drivers dealing with similar issues.\n>\n> I'm not worried about that as long as there are good examples and\n> documented best practices.\n>\n> There aren't any right now, which is a problem, but that certainly is\n> fixable.\n>\n>> **)\n>> The ->resume_early() callback powers on the device, in case it was\n>> runtime resumed when the ->suspend_late() callback was invoked. That\n>> is in many cases completely unnecessary, causing us to waste power and\n>> increase system resume time, for absolutely no reason. However, I\n>> understand the patch didn't try to address this, but to really fix\n>> this, there has to be an even closer collaboration between runtime PM\n>> and the system sleep callbacks.\n>\n> I don't quite agree and here's why.\n>\n> If a device was not runtime-suspended right before system suspend, then quite\n> likely it was in use then. Therefore it is quite likely to be resumed\n> immediately after system resume anyway.\n\nUnfortunate, to always make that assumption, leads to a non-optimized\nbehavior of system sleep. I think we can do better than that!\n\nLet me give you a concrete example, where the above assumption would\nlead to an non-optimized behavior.\n\nTo put an MMC card into low power state during system suspend\n(covering eMMC, SD, SDIO) the mmc core needs to send a couple of\ncommands over the MMC interface to the card, as to conform with the\n(e)MMC/SD/SDIO spec. To do this, the mmc driver for the mmc controller\nmust runtime resume its device, as to be able to send the commands\nover the interface.\n\nNow, when the system resumes, there is absolutely no reason to runtime\nresume the device for the MMC controller, just because it was runtime\nresumed during system suspend. Instead that is better to be postponed\nto when the MMC card is really needed and thus via runtime PM instead.\n\nThis scenario shouldn't be specific to only MMC controllers/cards, but\nshould apply to any external devices/controllers that needs some\nspecial treatment to be put into low power state during system\nsuspend. Particularly also when those external devices may be left in\nthat low power state until those are really needed. A couple of cases\nI know of pops up in my head, WiFi chips, persistent storage devices,\netc. There should be plenty.\n\nAnother common case, is when a subsystem core layer flushes a request\nqueue during system suspend, which may cause a controller device to be\nruntime resumed. Making the assumption that, because flushing the\nqueue was done during system suspend, we must also power up the\ncontroller during system resume, again would lead to a non-optimized\nbehavior.\n\n>\n> Now, if that's just one device, it probably doesn't matter, but if there are\n> more devices like that, they will be resumed after system suspend when they\n> are accessed and quite likely they will be accessed one-by-one rather than in\n> parallel with each other, so the latencies related to that will add up. In\n> that case it is better to resume them upfront during system resume as they will\n> be resumed in parallel with each other then. And that also is *way* simpler.\n>\n> This means that the benefit from avoiding to resume devices during system\n> resume is not quite so obvious and the whole point above is highly\n> questionable.\n\nI hope my reasoning above explains why I think it shouldn't be\nconsidered as questionable.\n\nIf you like, I can also provide some real data/logs - showing you\nwhat's happening.\n\n>\n>>\n>> So, to remind you why the pm_runtime_force_suspend|resume() helpers is\n>> preferred, that's because both of the above two things becomes taken\n>> care of.\n>\n> And that is why there is this stuff about parents and usage counters, right?\n\nCorrect. Perhaps this commit tells you a little more.\n\ncommit 1d9174fbc55ec99ccbfcafa3de2528ef78a849aa\nAuthor: Ulf Hansson \nDate: Thu Oct 13 16:58:54 2016 +0200\n\n PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()\n\n[...]\n\n>> >\n>> > PM core => bus type / PM domain ->suspend_late => driver ->suspend_late\n>> >\n>> > is far more straightforward than\n>> >\n>> > PM core => bus type / PM domain ->suspend_late => driver ->suspend_late =>\n>> > bus type / PM domain ->runtime_suspend => driver ->runtime_suspend\n>> >\n>> > with the bus type / PM domain having to figure out somehow at the\n>> > ->suspend_late time whether or not its ->runtume_suspend is going to be invoked\n>> > in the middle of it.\n>> >\n>> > Apart from this just being aesthetically disgusting to me, which admittedly is\n>> > a matter of personal opinion, it makes debugging new driver code harder (if it\n>> > happens to not work) and reviewing it almost impossible, because now you need\n>> > to take all of the tangling between callbacks into accont and sometimes not\n>> > just for one bus type / PM domain.\n>>\n>> I am wondering that perhaps you may be overlooking some of the\n>> internals of runtime PM. Or maybe not? :-)\n>>\n>> I mean, the hole thing is build upon that anyone can call runtime PM\n>> functions to runtime resume/suspend a device.\n>\n> Well, right in general, except that _force_suspend/resume() invoke\n> *callbacks* and *not* runtime PM functions.\n\nI am considering pm_runtime_force_suspend|resume() being a part of the\nruntime PM API, except that those may be called only during system\nsleep.\n\nComparing a call to pm_runtime_resume(); this may trigger rpm_resume()\nto invoke the callbacks. To me, the difference is that the conditions\nlooked at in rpm_resume(), when runtime PM is enabled, becomes\ndifferent for system sleep when runtime PM is disabled - and that is\ntaken care of in pm_runtime_force_suspend|resume().\n\n>\n>> Doing that, makes the\n>> hierarchy of the runtime PM callbacks being walked and invoked, of\n>> course properly managed by the runtime PM core.\n>>\n>> My point is that, the runtime PM core still controls this behavior,\n>> even when the pm_runtime_force_suspend|resume() helpers are being\n>> invoked. The only difference is that it allows runtime PM for the\n>> device to be disabled, and still correctly invoked the callbacks. That\n>> is what it is all about.\n>\n> So why is it even useful to call ->runtime_suspend from a middle layer\n> in pm_runtime_force_suspend(), for example?\n\nPerhaps I don't understand the question correctly.\n\nAnyway, the answer I think of, is probably because of the same reason\nto why the runtime PM core invokes it, when it runs rpm_suspend() for\na device. My point is, we want the similar behavior.\n\n[...]\n\n>> >\n>> > Also, when I looked at _force_suspend/resume() again, I got concerned.\n>> > There is stuff in there that shouldn't be necessary in a driver's\n>> > ->late_suspend/->early_resume and some things in there just made me\n>> > scratch my head.\n>>\n>> Yes, there are some complexity in there, I will be happy to answer any\n>> specific question about it.\n>\n> OK\n>\n> Of course they require runtime PM to be enabled by drivers using them as\n> their callbacks, but I suppose that you realize that.\n>\n> Why to disabe/renable runtime PM in there in the first place? That should\n> have been done by the core when these functions are intended to be called.\n\nThe reason is because we didn't want to re-strict them to be used only\nin ->suspend_late() and ->resume_early(), but also for ->suspend() and\n->resume(), which is when runtime PM still is enabled.\n\n>\n> Second, why to use RPM_GET_CALLBACK in there?\n\nTo follow the same rules/hierarchy, as being done in rpm_suspend|resume().\n\n>\n> Next, how is the parent actually runtime-resumed by pm_runtime_force_resume()\n> which the comment in pm_runtime_force_suspend() talks about?\n\nI think the relevant use case here is when a parent and a child, both\nhave subsystems/drivers using pm_runtime_force_suspend|resume(). If\nthat isn't the case, we expect that the parent is always resumed\nduring system resume. It's a bit fragile approach, so we perhaps we\nshould deal with it, even if the hole thing is used as opt-in.\n\nAnyway, let's focus on the case which I think is most relevant to your question:\n\nA couple of conditions to start with.\n*) The PM core system suspends a child prior a parent, which leads to\npm_runtime_force_suspend() being called for the child first.\n**) The PM core system resumes a parents before a child, thus\npm_runtime_force_resume() is called for the parent first.\n\nIn case a child don't need to be resumed when\npm_runtime_force_resume() is called for it, likely doesn't its parent.\nHowever, to control that, in system suspend the\npm_runtime_force_suspend() increases the usage counter for the parent,\nas to indicate if it needs to be resumed when\npm_runtime_force_resume() is called for it.\n\nFinally, when the child becomes resumed in pm_runtime_force_resume(),\npm_runtime_set_active() is called for it. This verifies that the\nparent also has been resumed properly.\n\n[...]\n\nKind regards\nUffe","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"c4AGTNOT\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xm8wJ64hGz9sNr\n\tfor ;\n\tMon, 4 Sep 2017 22:55:44 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753542AbdIDMzl (ORCPT );\n\tMon, 4 Sep 2017 08:55:41 -0400","from mail-it0-f52.google.com ([209.85.214.52]:36644 \"EHLO\n\tmail-it0-f52.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753497AbdIDMzj (ORCPT\n\t); Mon, 4 Sep 2017 08:55:39 -0400","by mail-it0-f52.google.com with SMTP id j17so2502843iti.1\n\tfor ; Mon, 04 Sep 2017 05:55:39 -0700 (PDT)","by 10.2.96.38 with HTTP; Mon, 4 Sep 2017 05:55:37 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=yjxnPDpLCf2yCjcd0ZoCpKX4Y5FapV/v3zxDTSipIys=;\n\tb=c4AGTNOT5kODFukoYUuQHFMsGA5nES0B9wSeLxKLnZyUFPFXDKQgLqhFObJdInjMFX\n\tNV6tE1S4oBziWGQ75mjqjTlPIaw6y63nRAiHoKJmpi7gm5Oo110bpFVAdd5o1V8fkUX1\n\tcccxJv7MwNS7U9IdyO9QCUSDvPJ73p9cPoxew=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=yjxnPDpLCf2yCjcd0ZoCpKX4Y5FapV/v3zxDTSipIys=;\n\tb=sbMgyYuihFExTj9UsBgQ3ltDOn5N/9N/SriFF5cbv6R1T+ZaDZ0hLzxjMw4OJLIyXQ\n\tmh6sLnLcV+GXjCUW9pPb+SUt92uq7krKAjj5nO1m4Hnci1aYSpQG93G6PPKjzMxuLHB2\n\tfUwdJqX2uCvqyVaa9NV6VziaMZzewIZNEAGhPOnsIXpBwmUCgJ9sle2KnU39e2SBuTnW\n\t7EakKmgbdAFCnmi0GQ47MeOAb8YPRuUjCE8fHjkh/kPppQYbvNTnP5h+y4V7mCrspsjB\n\ti6OQpncBUtZKa52brHIyqvEdvv9JR4vFPSQwqQ6Y8TPdTXeiW8Swpk9wTLdJcav4aohP\n\tNHcw==","X-Gm-Message-State":"AHPjjUjAl9uqmjeXOQv2UoTQlOTBL0Z2ybL6EtaO9i6xxQ0S4N8mbNAn\n\tHQAEbA1nIXUR9HHNQCOAF3+S/vtR4nVJ","X-Google-Smtp-Source":"ADKCNb7RtD7/MHb0knWGMAgxtiVjBTXHV3QmKwMx1yyNIWuwN0xewI3MY9DcPtT9WPMWa/QXjvNQ9/gGxMsv4WAGUgU=","X-Received":"by 10.36.192.69 with SMTP id u66mr706090itf.25.1504529738713;\n\tMon, 04 Sep 2017 05:55:38 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1668277.7DWLdgHGid@aspire.rjw.lan>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<3033151.kbPpEQGlRq@aspire.rjw.lan>\n\t\n\t<1668277.7DWLdgHGid@aspire.rjw.lan>","From":"Ulf Hansson ","Date":"Mon, 4 Sep 2017 14:55:37 +0200","Message-ID":"","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","To":"\"Rafael J. Wysocki\" ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1763755,"web_url":"http://patchwork.ozlabs.org/comment/1763755/","msgid":"<1883292.VFru06qUYG@aspire.rjw.lan>","list_archive_url":null,"date":"2017-09-06T00:52:59","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Monday, September 4, 2017 2:55:37 PM CEST Ulf Hansson wrote:\n> [...]\n> \n> >> > So can you please remind me why the _force_ wrappers are needed?\n> >>\n> >> See below.\n> >>\n> >> >\n> >> > In particular, why can't drivers arrange their callbacks the way I did that\n> >> > in https://patchwork.kernel.org/patch/9928583/ ?\n> >>\n> >> I was preparing a reply to that patch, but let me summarize that here instead.\n> >>\n> >> Let me be clear, the patch is an improvement of the behavior of the\n> >> driver and it addresses the issues you point out in the change log.\n> >> Re-using the runtime PM callbacks for system sleep, is nice as it\n> >> avoids open coding, which is of curse also one of the reason of using\n> >> pm_runtime_force_suspend|resume().\n> >>\n> >> Still there are a couple of things I am worried about in this patch.\n> >> *)\n> >> To be able to re-use the same callbacks for system sleep and runtime\n> >> PM, some boilerplate code is added to the driver, as to cope with the\n> >> different conditions inside the callbacks. That pattern would become\n> >> repeated to many drivers dealing with similar issues.\n> >\n> > I'm not worried about that as long as there are good examples and\n> > documented best practices.\n> >\n> > There aren't any right now, which is a problem, but that certainly is\n> > fixable.\n> >\n> >> **)\n> >> The ->resume_early() callback powers on the device, in case it was\n> >> runtime resumed when the ->suspend_late() callback was invoked. That\n> >> is in many cases completely unnecessary, causing us to waste power and\n> >> increase system resume time, for absolutely no reason. However, I\n> >> understand the patch didn't try to address this, but to really fix\n> >> this, there has to be an even closer collaboration between runtime PM\n> >> and the system sleep callbacks.\n> >\n> > I don't quite agree and here's why.\n> >\n> > If a device was not runtime-suspended right before system suspend, then quite\n> > likely it was in use then. Therefore it is quite likely to be resumed\n> > immediately after system resume anyway.\n> \n> Unfortunate, to always make that assumption, leads to a non-optimized\n> behavior of system sleep. I think we can do better than that!\n> \n> Let me give you a concrete example, where the above assumption would\n> lead to an non-optimized behavior.\n> \n> To put an MMC card into low power state during system suspend\n> (covering eMMC, SD, SDIO) the mmc core needs to send a couple of\n> commands over the MMC interface to the card, as to conform with the\n> (e)MMC/SD/SDIO spec. To do this, the mmc driver for the mmc controller\n> must runtime resume its device, as to be able to send the commands\n> over the interface.\n> \n> Now, when the system resumes, there is absolutely no reason to runtime\n> resume the device for the MMC controller, just because it was runtime\n> resumed during system suspend. Instead that is better to be postponed\n> to when the MMC card is really needed and thus via runtime PM instead.\n\nYes, in this particular case it makes more sense to defer the resume of\nthe device, but there also are cases in which doing that leads to\nsuboptimal behavior.\n\n> This scenario shouldn't be specific to only MMC controllers/cards, but\n> should apply to any external devices/controllers that needs some\n> special treatment to be put into low power state during system\n> suspend. Particularly also when those external devices may be left in\n> that low power state until those are really needed. A couple of cases\n> I know of pops up in my head, WiFi chips, persistent storage devices,\n> etc. There should be plenty.\n> \n> Another common case, is when a subsystem core layer flushes a request\n> queue during system suspend, which may cause a controller device to be\n> runtime resumed. Making the assumption that, because flushing the\n> queue was done during system suspend, we must also power up the\n> controller during system resume, again would lead to a non-optimized\n> behavior.\n\nI understand that.\n\nHowever, from a driver perspective, the most straightforward thing to do\nis to restore the previous state of the device during system resume,\nbecause that guarantees correctness. Anything else is tricky and need to\nbe done with extra care. Drivers *must* know what they are doing when\nthey are doing such things.\n\n> >\n> > Now, if that's just one device, it probably doesn't matter, but if there are\n> > more devices like that, they will be resumed after system suspend when they\n> > are accessed and quite likely they will be accessed one-by-one rather than in\n> > parallel with each other, so the latencies related to that will add up. In\n> > that case it is better to resume them upfront during system resume as they will\n> > be resumed in parallel with each other then. And that also is *way* simpler.\n> >\n> > This means that the benefit from avoiding to resume devices during system\n> > resume is not quite so obvious and the whole point above is highly\n> > questionable.\n> \n> I hope my reasoning above explains why I think it shouldn't be\n> considered as questionable.\n> \n> If you like, I can also provide some real data/logs - showing you\n> what's happening.\n> \n\nThat's not necessary, this behavior can be useful and there are arguments for\ndoing it in *some* cases, but all of this argumentation applies to devices\nthat aren't going to be used right after system resume. If they *are* going\nto be used then, it very well may be better to resume them as part of\nsystem resume instead of deferring that.\n\nThe tricky part is that at the point the resume callbacks run it is not known\nwhether or not the device is going to be accessed shortly and the decision made\neither way may be suboptimal.\n\n[Note: I know that people mostly care about seeing the screen on, but in fact\nthey should *also* care about the touch panel being ready to respond to\ntouches, for example. If it isn't ready and the system suspends again\nafter a while because of that, the experience is somehwat less than fantastic.]\n\n> >>\n> >> So, to remind you why the pm_runtime_force_suspend|resume() helpers is\n> >> preferred, that's because both of the above two things becomes taken\n> >> care of.\n> >\n> > And that is why there is this stuff about parents and usage counters, right?\n> \n> Correct. Perhaps this commit tells you a little more.\n> \n> commit 1d9174fbc55ec99ccbfcafa3de2528ef78a849aa\n> Author: Ulf Hansson \n> Date: Thu Oct 13 16:58:54 2016 +0200\n> \n> PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()\n> \n> [...]\n> \n> >> >\n> >> > PM core => bus type / PM domain ->suspend_late => driver ->suspend_late\n> >> >\n> >> > is far more straightforward than\n> >> >\n> >> > PM core => bus type / PM domain ->suspend_late => driver ->suspend_late =>\n> >> > bus type / PM domain ->runtime_suspend => driver ->runtime_suspend\n> >> >\n> >> > with the bus type / PM domain having to figure out somehow at the\n> >> > ->suspend_late time whether or not its ->runtume_suspend is going to be invoked\n> >> > in the middle of it.\n> >> >\n> >> > Apart from this just being aesthetically disgusting to me, which admittedly is\n> >> > a matter of personal opinion, it makes debugging new driver code harder (if it\n> >> > happens to not work) and reviewing it almost impossible, because now you need\n> >> > to take all of the tangling between callbacks into accont and sometimes not\n> >> > just for one bus type / PM domain.\n> >>\n> >> I am wondering that perhaps you may be overlooking some of the\n> >> internals of runtime PM. Or maybe not? :-)\n> >>\n> >> I mean, the hole thing is build upon that anyone can call runtime PM\n> >> functions to runtime resume/suspend a device.\n> >\n> > Well, right in general, except that _force_suspend/resume() invoke\n> > *callbacks* and *not* runtime PM functions.\n> \n> I am considering pm_runtime_force_suspend|resume() being a part of the\n> runtime PM API, except that those may be called only during system\n> sleep.\n> \n> Comparing a call to pm_runtime_resume(); this may trigger rpm_resume()\n> to invoke the callbacks. To me, the difference is that the conditions\n> looked at in rpm_resume(), when runtime PM is enabled, becomes\n> different for system sleep when runtime PM is disabled - and that is\n> taken care of in pm_runtime_force_suspend|resume().\n\nSo actually invoking runtime PM from a *driver* ->suspend callback for the\nsame device it was called for is fishy at best and may be a bug. I'm not\nsure why I had been thinking that it might have been fine at all. It isn't.\n\nThe reason why is because runtime PM *potentially* involves invoking middle\nlayer callbacks an they generally may look like\n\n->runtime_resume:\n\t(1) do A\n\t(2) call driver ->runtime_resume\n\t(3) do B\n\nNow, a middle layer ->suspend callback generally may look like this:\n\n->suspend:\n\t(1) do C\n\t(2) call driver ->suspend\n\t(3) do D\n\nand if you stick the middle layer ->runtime_suspend invocation into the\ndriver ->suspend (which effectively is what running runtime PM in there means),\nyou get something like\n\ndo C\n...\ndo A\ncall driver ->runtime_resume\ndo B\n...\ndo D\n\nand there's no guarantee whatever that \"do C\" can go before \"do A\" and\n\"do B\" can go before \"do D\". That depends on how the middle layer is designed\nand there may be good reasons for how it works.\n\n> >\n> >> Doing that, makes the\n> >> hierarchy of the runtime PM callbacks being walked and invoked, of\n> >> course properly managed by the runtime PM core.\n> >>\n> >> My point is that, the runtime PM core still controls this behavior,\n> >> even when the pm_runtime_force_suspend|resume() helpers are being\n> >> invoked. The only difference is that it allows runtime PM for the\n> >> device to be disabled, and still correctly invoked the callbacks. That\n> >> is what it is all about.\n> >\n> > So why is it even useful to call ->runtime_suspend from a middle layer\n> > in pm_runtime_force_suspend(), for example?\n> \n> Perhaps I don't understand the question correctly.\n> \n> Anyway, the answer I think of, is probably because of the same reason\n> to why the runtime PM core invokes it, when it runs rpm_suspend() for\n> a device. My point is, we want the similar behavior.\n\nNot really. The context is different, so why to expect the behavior to be\nthe same?\n\n> [...]\n> \n> >> >\n> >> > Also, when I looked at _force_suspend/resume() again, I got concerned.\n> >> > There is stuff in there that shouldn't be necessary in a driver's\n> >> > ->late_suspend/->early_resume and some things in there just made me\n> >> > scratch my head.\n> >>\n> >> Yes, there are some complexity in there, I will be happy to answer any\n> >> specific question about it.\n> >\n> > OK\n> >\n> > Of course they require runtime PM to be enabled by drivers using them as\n> > their callbacks, but I suppose that you realize that.\n> >\n> > Why to disabe/renable runtime PM in there in the first place? That should\n> > have been done by the core when these functions are intended to be called.\n> \n> The reason is because we didn't want to re-strict them to be used only\n> in ->suspend_late() and ->resume_early(), but also for ->suspend() and\n> ->resume(), which is when runtime PM still is enabled.\n\nWell, that means disabling runtime PM for some devices earlier which isn't\nparticularly consistent overall.\n\n> >\n> > Second, why to use RPM_GET_CALLBACK in there?\n> \n> To follow the same rules/hierarchy, as being done in rpm_suspend|resume().\n\nNo, you don't use the same hierarchy, which is the key point of my objection.\n\nYou run *already* in the context of a middle layer PM callback, so by very\ndefinition it is *not* the same situation as running runtime PM elsewhere.\n\nThis is the second or maybe even the third time I have repeated this point\nand I'm not going to do so again.\n\n> >\n> > Next, how is the parent actually runtime-resumed by pm_runtime_force_resume()\n> > which the comment in pm_runtime_force_suspend() talks about?\n> \n> I think the relevant use case here is when a parent and a child, both\n> have subsystems/drivers using pm_runtime_force_suspend|resume(). If\n> that isn't the case, we expect that the parent is always resumed\n> during system resume.\n\nWhy?\n\n> It's a bit fragile approach, so we perhaps we\n> should deal with it, even if the hole thing is used as opt-in.\n> \n> Anyway, let's focus on the case which I think is most relevant to your question:\n> \n> A couple of conditions to start with.\n> *) The PM core system suspends a child prior a parent, which leads to\n> pm_runtime_force_suspend() being called for the child first.\n> **) The PM core system resumes a parents before a child, thus\n> pm_runtime_force_resume() is called for the parent first.\n> \n> In case a child don't need to be resumed when\n> pm_runtime_force_resume() is called for it, likely doesn't its parent.\n> However, to control that, in system suspend the\n> pm_runtime_force_suspend() increases the usage counter for the parent,\n> as to indicate if it needs to be resumed when\n> pm_runtime_force_resume() is called for it.\n\nOK, I see.\n\nWhy is usage_count > 1 used as the condition to trigger this behavior?\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xn4zj05tRz9sP3\n\tfor ;\n\tWed, 6 Sep 2017 11:01:53 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753116AbdIFBBv (ORCPT );\n\tTue, 5 Sep 2017 21:01:51 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:43672 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1752145AbdIFBBu (ORCPT\n\t); Tue, 5 Sep 2017 21:01:50 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 504d7c0833a68916; Wed, 6 Sep 2017 03:01:48 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","Date":"Wed, 06 Sep 2017 02:52:59 +0200","Message-ID":"<1883292.VFru06qUYG@aspire.rjw.lan>","In-Reply-To":"","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<1668277.7DWLdgHGid@aspire.rjw.lan>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1764000,"web_url":"http://patchwork.ozlabs.org/comment/1764000/","msgid":"<8456507.jc97SKmauk@aspire.rjw.lan>","list_archive_url":null,"date":"2017-09-06T10:46:34","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Wednesday, September 6, 2017 2:52:59 AM CEST Rafael J. Wysocki wrote:\n> On Monday, September 4, 2017 2:55:37 PM CEST Ulf Hansson wrote:\n> > [...]\n\nI guess I can wrap it up, because all of the points seem to have been stated\nand repeating them would not be useful.\n\nMy summary of the discussion is as follows.\n\nIt only is valid to use pm_runtime_force_suspend/resume() as *driver*\ncallbacks for system suspend/resume if both the driver itself and all of\nthe middle layers it has to work with carry out the same sequence of\noperations in order to suspend the device both in runtime PM and for\nsystem sleep (and analogously for resuming). [The middle layers need\nto meet additional conditions, but that's less relevant.]\n\nUnfortunately, for the ACPI PM domain and the PCI bus type the situation is\ndifferent, because they generally need to do different things to suspend\ndevices for system sleep than they do for runtime PM (which mostly is\nrelated to the handling of ACPI-defined sleep states and device/system\nwakeup, but not limited to that). This clearly means that drivers needing\nto work with the ACPI PM domain and PCI drivers cannot use\npm_runtime_force_suspend/resume() as their PM callbacks for system\nsuspend/resume (quite fundamentally).\n\n[Note that for i2c-designware-platdrv the situation is even more complicated,\nbecause on some platforms it has to work with the ACPI PM domain (or the\nACPI LPSS driver), on some platforms its parent is a PCI device and on\nsome other platforms there's none of them.]\n\nHowever, for drivers that need to work with the ACPI PM domain and\nPCI drivers the differences in the device handling between runtime PM and\nsystem suspend/resume are *very* often (even though not always) covered\nentirely by the middle layer code. Then, the driver itself actually\nalways carries out the same sequence of operations in order to suspend\nthe device (or to resume it, analogously). The driver then can re-use\nits runtime PM callbacks for system suspend/resume (but at the driver\nlevel only) and it would be good to make that easy (or easier) for these\ndrivers somehow.\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnL8d3CV5z9sRV\n\tfor ;\n\tWed, 6 Sep 2017 20:55:29 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753390AbdIFKz1 (ORCPT );\n\tWed, 6 Sep 2017 06:55:27 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:42872 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1753510AbdIFKzY (ORCPT\n\t); Wed, 6 Sep 2017 06:55:24 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id cf015b5817f578cb; Wed, 6 Sep 2017 12:55:22 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","Date":"Wed, 06 Sep 2017 12:46:34 +0200","Message-ID":"<8456507.jc97SKmauk@aspire.rjw.lan>","In-Reply-To":"<1883292.VFru06qUYG@aspire.rjw.lan>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t\n\t<1883292.VFru06qUYG@aspire.rjw.lan>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1764117,"web_url":"http://patchwork.ozlabs.org/comment/1764117/","msgid":"","list_archive_url":null,"date":"2017-09-06T13:54:06","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":21036,"url":"http://patchwork.ozlabs.org/api/people/21036/","name":"Ulf Hansson","email":"ulf.hansson@linaro.org"},"content":"[...]\n\n>\n>> >\n>> > Now, if that's just one device, it probably doesn't matter, but if there are\n>> > more devices like that, they will be resumed after system suspend when they\n>> > are accessed and quite likely they will be accessed one-by-one rather than in\n>> > parallel with each other, so the latencies related to that will add up. In\n>> > that case it is better to resume them upfront during system resume as they will\n>> > be resumed in parallel with each other then. And that also is *way* simpler.\n>> >\n>> > This means that the benefit from avoiding to resume devices during system\n>> > resume is not quite so obvious and the whole point above is highly\n>> > questionable.\n>>\n>> I hope my reasoning above explains why I think it shouldn't be\n>> considered as questionable.\n>>\n>> If you like, I can also provide some real data/logs - showing you\n>> what's happening.\n>>\n>\n> That's not necessary, this behavior can be useful and there are arguments for\n> doing it in *some* cases, but all of this argumentation applies to devices\n> that aren't going to be used right after system resume. If they *are* going\n> to be used then, it very well may be better to resume them as part of\n> system resume instead of deferring that.\n>\n> The tricky part is that at the point the resume callbacks run it is not known\n> whether or not the device is going to be accessed shortly and the decision made\n> either way may be suboptimal.\n\nYou have a point and it seems like this is what everything boils done\nto, except for the reasons about that you dislike how\npm_runtime_force_suspend|resume() is being used by drivers.\n\nTo clarify, let me bring up yet another typical scenario, observed\noften in cases when pm_runtime_force_suspend|resume is not used.\n\nDuring system resume the device gets resumed, then shortly after the\nsystem resume sequence has completed, it become runtime suspened,\nbecause pm_runtime_put() is called in device_complete(). Then, soon\nafter the system has resumed, the device becomes runtime resumed\nagain, which is because there is a request for it to really be used.\n\nThis means we end up resuming the device, suspending it then and\nresuming it again, all within a very short time frame. I guess this is\nalso one of those tricky cases you refer to above, because one just\ncan know how long after the system has resumed it takes for the device\nto be requested to be used again, thus we end up runtime suspending\nthe device in-between.\n\nTo me, spending lot of time in the world of embedded battery driven\ndevices, this behavior isn't good enough, because it increases system\nresume time and may waste some power. Apologize if you find me\nrepeating myself.\n\nAnyway, this leads to my final question, do you want this behavior to\nbe better addressed by the ACPI PM domain, if it can be solved nicely,\nor are you fine with how works today?\n\n>\n> [Note: I know that people mostly care about seeing the screen on, but in fact\n> they should *also* care about the touch panel being ready to respond to\n> touches, for example. If it isn't ready and the system suspends again\n> after a while because of that, the experience is somehwat less than fantastic.]\n>\n\nYep!\n\n[...]\n\n>> Comparing a call to pm_runtime_resume(); this may trigger rpm_resume()\n>> to invoke the callbacks. To me, the difference is that the conditions\n>> looked at in rpm_resume(), when runtime PM is enabled, becomes\n>> different for system sleep when runtime PM is disabled - and that is\n>> taken care of in pm_runtime_force_suspend|resume().\n>\n> So actually invoking runtime PM from a *driver* ->suspend callback for the\n> same device it was called for is fishy at best and may be a bug. I'm not\n> sure why I had been thinking that it might have been fine at all. It isn't.\n\nHuh, now you lost me. :-)\n\n>\n> The reason why is because runtime PM *potentially* involves invoking middle\n> layer callbacks an they generally may look like\n>\n> ->runtime_resume:\n> (1) do A\n> (2) call driver ->runtime_resume\n> (3) do B\n>\n> Now, a middle layer ->suspend callback generally may look like this:\n>\n> ->suspend:\n> (1) do C\n> (2) call driver ->suspend\n> (3) do D\n>\n> and if you stick the middle layer ->runtime_suspend invocation into the\n> driver ->suspend (which effectively is what running runtime PM in there means),\n> you get something like\n>\n> do C\n> ...\n> do A\n> call driver ->runtime_resume\n> do B\n> ...\n> do D\n>\n> and there's no guarantee whatever that \"do C\" can go before \"do A\" and\n> \"do B\" can go before \"do D\". That depends on how the middle layer is designed\n> and there may be good reasons for how it works.\n\nFor ARM SoCs, not using the ACPI PM domain, many drivers needs to be\nable to use runtime PM during system suspend, simply because the PM\ndomain/middle layer, has no knowledge of what the driver needs to put\nits device into low power state during system suspend.\n\nFor many of the simple cases, the PM domain/middle layer act\ntransparent to this, which means leaving what needs to be done to the\ndriver (platform, spi, i2c, amba, genpd etc).\n\nI understand there may be some cases where the situation becomes more\ncomplex and interaction between the driver and the PM domain/middle\nlayer is required, like what it seems for in ACPI PM domain and PCI,\nbut is that a reason turn the world upside down for everybody else?\n\nOr perhaps I don't understand what your are suggesting here.\n\n[...]\n\n>\n>> I think the relevant use case here is when a parent and a child, both\n>> have subsystems/drivers using pm_runtime_force_suspend|resume(). If\n>> that isn't the case, we expect that the parent is always resumed\n>> during system resume.\n>\n> Why?\n\nBecause the child may rely on that for it to be resumed.\n\nMoreover, the expectation is that the parent likely doesn't support\nruntime PM, or that it not yet supports the optimized method of using\npm_runtime_force_suspend|resume() during system sleep, and will thus\nresume its device always during system resume.\n\n>\n>> It's a bit fragile approach, so we perhaps we\n>> should deal with it, even if the hole thing is used as opt-in.\n>>\n>> Anyway, let's focus on the case which I think is most relevant to your question:\n>>\n>> A couple of conditions to start with.\n>> *) The PM core system suspends a child prior a parent, which leads to\n>> pm_runtime_force_suspend() being called for the child first.\n>> **) The PM core system resumes a parents before a child, thus\n>> pm_runtime_force_resume() is called for the parent first.\n>>\n>> In case a child don't need to be resumed when\n>> pm_runtime_force_resume() is called for it, likely doesn't its parent.\n>> However, to control that, in system suspend the\n>> pm_runtime_force_suspend() increases the usage counter for the parent,\n>> as to indicate if it needs to be resumed when\n>> pm_runtime_force_resume() is called for it.\n>\n> OK, I see.\n>\n> Why is usage_count > 1 used as the condition to trigger this behavior?\n\nIt takes into account that the PM core increases the usage count in\ndevice_prepare(), but which isn't because it needs the device to be\noperational.\n\nKind regards\nUffe","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"Rc1/y6Ga\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnQ6v1qBrz9s9Y\n\tfor ;\n\tWed, 6 Sep 2017 23:54:15 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932250AbdIFNyM (ORCPT );\n\tWed, 6 Sep 2017 09:54:12 -0400","from mail-it0-f49.google.com ([209.85.214.49]:37116 \"EHLO\n\tmail-it0-f49.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1754801AbdIFNyI (ORCPT\n\t); Wed, 6 Sep 2017 09:54:08 -0400","by mail-it0-f49.google.com with SMTP id k189so10371574itk.0\n\tfor ; Wed, 06 Sep 2017 06:54:08 -0700 (PDT)","by 10.2.96.38 with HTTP; Wed, 6 Sep 2017 06:54:06 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=M+SkkJacXOxWso9B22opU1xKDGhm37Lfl1WsIk4eGr4=;\n\tb=Rc1/y6Gag2Czdd2UDNm2hJjjcu2EP4myIL/gY7yKMQcjOofYHAIcfxN8huLftEMeJj\n\tyWhGui9IgUIQo3FkDEdnlTmtiEbcG8iyuTJ/5HqIQ8v19wcPmZu5mj5PB1Zwcqn9Xn3S\n\t7q3ScKSPH2Z8OIw2zYFN3+Xde4LJ2bzQE9w1A=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=M+SkkJacXOxWso9B22opU1xKDGhm37Lfl1WsIk4eGr4=;\n\tb=YKFZAYPFm4H4u070FCiMJ+spgr45NLqDC6NpuW27CEAgHfgPt9AS3K6czAXsoHOLD2\n\thWSNHQORadVGAfY+yCMfYedljGBwgNe6zpwlgsRBm70QbcqhRSWmsMgzXpcNiA8USPaM\n\t03nbEPvqB43zy4y4qlLWeHa+G0fgyfLv1j0EfRu0s9WIdOKasC06wGboVmDqQ8fsdQko\n\tFgeHfO17By7Mge0ATwHu8oiUMwfrwPwKAwT+P/U5yp/LhfEbpNxDraI3J1Bwog8P8+k6\n\tghcDCyGY0ExpccPFOff1liqWW3EG/5cC9zM8GKCLKoDQETydLtMPashhQ4vxx+YEV/0f\n\thAYg==","X-Gm-Message-State":"AHPjjUhtTML+vOta1B8STocLIa5gBdU/TWgBJJVcLBCjKgjkCZEirhGn\n\tP19C7tl7zwrxu+NjOMGCQaesI3GVDvIW","X-Google-Smtp-Source":"ADKCNb4pxQEDvXr5g2ftNGDRAKUexXJ1jxMdUz03haf/o7uGHzI6255sAOxolbG2lkUknEo3kjn+JS9jZOX93mN6oHU=","X-Received":"by 10.36.44.200 with SMTP id i191mr3067457iti.136.1504706047319; \n\tWed, 06 Sep 2017 06:54:07 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1883292.VFru06qUYG@aspire.rjw.lan>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<1668277.7DWLdgHGid@aspire.rjw.lan>\n\t\n\t<1883292.VFru06qUYG@aspire.rjw.lan>","From":"Ulf Hansson ","Date":"Wed, 6 Sep 2017 15:54:06 +0200","Message-ID":"","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","To":"\"Rafael J. Wysocki\" ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1764123,"web_url":"http://patchwork.ozlabs.org/comment/1764123/","msgid":"","list_archive_url":null,"date":"2017-09-06T13:59:16","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":21036,"url":"http://patchwork.ozlabs.org/api/people/21036/","name":"Ulf Hansson","email":"ulf.hansson@linaro.org"},"content":"On 6 September 2017 at 12:46, Rafael J. Wysocki wrote:\n> On Wednesday, September 6, 2017 2:52:59 AM CEST Rafael J. Wysocki wrote:\n>> On Monday, September 4, 2017 2:55:37 PM CEST Ulf Hansson wrote:\n>> > [...]\n>\n> I guess I can wrap it up, because all of the points seem to have been stated\n> and repeating them would not be useful.\n>\n> My summary of the discussion is as follows.\n>\n> It only is valid to use pm_runtime_force_suspend/resume() as *driver*\n> callbacks for system suspend/resume if both the driver itself and all of\n> the middle layers it has to work with carry out the same sequence of\n> operations in order to suspend the device both in runtime PM and for\n> system sleep (and analogously for resuming). [The middle layers need\n> to meet additional conditions, but that's less relevant.]\n>\n> Unfortunately, for the ACPI PM domain and the PCI bus type the situation is\n> different, because they generally need to do different things to suspend\n> devices for system sleep than they do for runtime PM (which mostly is\n> related to the handling of ACPI-defined sleep states and device/system\n> wakeup, but not limited to that). This clearly means that drivers needing\n> to work with the ACPI PM domain and PCI drivers cannot use\n> pm_runtime_force_suspend/resume() as their PM callbacks for system\n> suspend/resume (quite fundamentally).\n>\n> [Note that for i2c-designware-platdrv the situation is even more complicated,\n> because on some platforms it has to work with the ACPI PM domain (or the\n> ACPI LPSS driver), on some platforms its parent is a PCI device and on\n> some other platforms there's none of them.]\n\nThat is also why it makes it really interesting. I am guessing we will\nbe seeing more of these cases sooner or later.\n\nTo make it even more complex, I can guess we can expect cases when\ngenpd is mixed with the ACPI PM domain.\n\n>\n> However, for drivers that need to work with the ACPI PM domain and\n> PCI drivers the differences in the device handling between runtime PM and\n> system suspend/resume are *very* often (even though not always) covered\n> entirely by the middle layer code. Then, the driver itself actually\n> always carries out the same sequence of operations in order to suspend\n> the device (or to resume it, analogously). The driver then can re-use\n> its runtime PM callbacks for system suspend/resume (but at the driver\n> level only) and it would be good to make that easy (or easier) for these\n> drivers somehow.\n\nThis is a very nice summary so far, thanks for putting it together.\n\nKind regards\nUffe","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=linaro.org header.i=@linaro.org\n\theader.b=\"GYovbC4v\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnQDt1ySCz9t2R\n\tfor ;\n\tWed, 6 Sep 2017 23:59:26 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932209AbdIFN7X (ORCPT );\n\tWed, 6 Sep 2017 09:59:23 -0400","from mail-io0-f178.google.com ([209.85.223.178]:38280 \"EHLO\n\tmail-io0-f178.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S932339AbdIFN7S (ORCPT\n\t); Wed, 6 Sep 2017 09:59:18 -0400","by mail-io0-f178.google.com with SMTP id q64so24000296iod.5\n\tfor ; Wed, 06 Sep 2017 06:59:17 -0700 (PDT)","by 10.2.96.38 with HTTP; Wed, 6 Sep 2017 06:59:16 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=Txb81XLQVtpJGB1IaFy4nzgUbqfK3rasyh6LBqEt334=;\n\tb=GYovbC4vHxnAuDftgqdv6syAx9MQUVNtpk2u7oqWlQvLcqzOq8VES70uv7N91OzOjb\n\tBH8EF6n1EkT7Y1r+jeCMf3Qsk1DyR2kfSawzgcCJyFc+8CRZlAWmNLoxr4vYA8584vP5\n\tIwJc60LXnUk9YeKw2G3dUaO0tdDu60q9fSKdo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=Txb81XLQVtpJGB1IaFy4nzgUbqfK3rasyh6LBqEt334=;\n\tb=pb8gTqkOZCNr14BbXVmvKe79DR9VLRNhDrW4YtjYkPEhBwb3P+gv7rpQjQtrdSYJbC\n\t/TUGGdOSKNKYUgt0d8q8bCsMz6ucDahXSalVjaZz2/S9xaYx8wZrLosIF4o0Ox8t3hCM\n\tDIxM1PnSH+I1Ps/fsV10TAJ52624UYIv/d7XBytg2UO4SxPbcJyBLfTQ2FZfGr1anKCO\n\tutw99ZFpUJSmoAD2s9kWybDf6FSk735K35BJqm1CTMbYfMUKJQkQQ8J8WiA8v3PNBRn4\n\tuwXAstYpWn0WytBed4sFkvPCBxqxApEbxxpjSBZewV1G7hmwF7JcizbFPic8h/0Twttx\n\tJGDg==","X-Gm-Message-State":"AHPjjUhxSUODMVXnkC/NRbB5NjzOU0cam4kcj3j9cHaKjs6sLxWpZ7w3\n\tZjCgWI2ex+Ub7r6Lz5xSR+VV4XVW5hzp","X-Google-Smtp-Source":"AOwi7QADsZGm8+mKJmeu5+OZBnBVWeiAnImimDzzcFhfX3KutBdfrUGSCvOTc1pHJFuuArEvUt5U33Jv+8zLTcvOrbs=","X-Received":"by 10.107.68.16 with SMTP id r16mr3064643ioa.34.1504706357254;\n\tWed, 06 Sep 2017 06:59:17 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<8456507.jc97SKmauk@aspire.rjw.lan>","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t\n\t<1883292.VFru06qUYG@aspire.rjw.lan>\n\t<8456507.jc97SKmauk@aspire.rjw.lan>","From":"Ulf Hansson ","Date":"Wed, 6 Sep 2017 15:59:16 +0200","Message-ID":"","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","To":"\"Rafael J. Wysocki\" ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1764401,"web_url":"http://patchwork.ozlabs.org/comment/1764401/","msgid":"<7722016.zqrrHyr8aS@aspire.rjw.lan>","list_archive_url":null,"date":"2017-09-06T21:39:01","subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Wednesday, September 6, 2017 3:59:16 PM CEST Ulf Hansson wrote:\n> On 6 September 2017 at 12:46, Rafael J. Wysocki wrote:\n> > On Wednesday, September 6, 2017 2:52:59 AM CEST Rafael J. Wysocki wrote:\n> >> On Monday, September 4, 2017 2:55:37 PM CEST Ulf Hansson wrote:\n> >> > [...]\n> >\n> > I guess I can wrap it up, because all of the points seem to have been stated\n> > and repeating them would not be useful.\n> >\n> > My summary of the discussion is as follows.\n> >\n> > It only is valid to use pm_runtime_force_suspend/resume() as *driver*\n> > callbacks for system suspend/resume if both the driver itself and all of\n> > the middle layers it has to work with carry out the same sequence of\n> > operations in order to suspend the device both in runtime PM and for\n> > system sleep (and analogously for resuming). [The middle layers need\n> > to meet additional conditions, but that's less relevant.]\n> >\n> > Unfortunately, for the ACPI PM domain and the PCI bus type the situation is\n> > different, because they generally need to do different things to suspend\n> > devices for system sleep than they do for runtime PM (which mostly is\n> > related to the handling of ACPI-defined sleep states and device/system\n> > wakeup, but not limited to that). This clearly means that drivers needing\n> > to work with the ACPI PM domain and PCI drivers cannot use\n> > pm_runtime_force_suspend/resume() as their PM callbacks for system\n> > suspend/resume (quite fundamentally).\n> >\n> > [Note that for i2c-designware-platdrv the situation is even more complicated,\n> > because on some platforms it has to work with the ACPI PM domain (or the\n> > ACPI LPSS driver), on some platforms its parent is a PCI device and on\n> > some other platforms there's none of them.]\n> \n> That is also why it makes it really interesting. I am guessing we will\n> be seeing more of these cases sooner or later.\n> \n> To make it even more complex, I can guess we can expect cases when\n> genpd is mixed with the ACPI PM domain.\n> \n> >\n> > However, for drivers that need to work with the ACPI PM domain and\n> > PCI drivers the differences in the device handling between runtime PM and\n> > system suspend/resume are *very* often (even though not always) covered\n> > entirely by the middle layer code. Then, the driver itself actually\n> > always carries out the same sequence of operations in order to suspend\n> > the device (or to resume it, analogously). The driver then can re-use\n> > its runtime PM callbacks for system suspend/resume (but at the driver\n> > level only) and it would be good to make that easy (or easier) for these\n> > drivers somehow.\n> \n> This is a very nice summary so far, thanks for putting it together.\n\nNo problem.\n\nI actually have an idea on how to move forward, but let me start a new thread\nfor discussing that.\n\nThanks,\nRafael","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xncdR2cnCz9s8J\n\tfor ;\n\tThu, 7 Sep 2017 07:47:55 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753111AbdIFVrx (ORCPT );\n\tWed, 6 Sep 2017 17:47:53 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:58818 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1752313AbdIFVrw (ORCPT\n\t); Wed, 6 Sep 2017 17:47:52 -0400","from 79.184.253.199.ipv4.supernova.orange.pl (79.184.253.199)\n\t(HELO aspire.rjw.lan)\n\tby serwer1319399.home.pl (79.96.170.134) with SMTP (IdeaSmtpServer\n\t0.82) id 7883038369968e14; Wed, 6 Sep 2017 23:47:50 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Ulf Hansson ","Cc":"Wolfram Sang , Len Brown ,\n\tACPI Devel Maling List ,\n\t\"linux-pm@vger.kernel.org\" ,\n\tKevin Hilman ,\n\tJarkko Nikula ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach ,\n\t\"linux-arm-kernel@lists.infradead.org\" \n\t,\n\t\"linux-i2c@vger.kernel.org\" ","Subject":"Re: [PATCH v3 0/8] PM / ACPI / i2c: Deploy runtime PM centric path\n\tfor system sleep","Date":"Wed, 06 Sep 2017 23:39:01 +0200","Message-ID":"<7722016.zqrrHyr8aS@aspire.rjw.lan>","In-Reply-To":"","References":"<1504018610-10822-1-git-send-email-ulf.hansson@linaro.org>\n\t<8456507.jc97SKmauk@aspire.rjw.lan>\n\t","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}}]