[{"id":1763408,"web_url":"http://patchwork.ozlabs.org/comment/1763408/","msgid":"","list_archive_url":null,"date":"2017-09-05T14:40:22","subject":"Re: [RFT][PATCH 1/2] i2c: designware: Clean up PM handling in\n\tdw_i2c_plat_probe()","submitter":{"id":43309,"url":"http://patchwork.ozlabs.org/api/people/43309/","name":"Jarkko Nikula","email":"jarkko.nikula@linux.intel.com"},"content":"Hi\n\nOn 09/04/2017 02:08 AM, Rafael J. Wysocki wrote:\n> From: Rafael J. Wysocki \n> \n> The power management handling in dw_i2c_plat_probe() is somewhat\n> messy and it is rather hard to figure out the code intention for\n> the case when pm_disabled is set. In that case, the driver doesn't\n> enable runtime PM at all, but in addition to that it calls\n> pm_runtime_forbid() as though it wasn't sure if runtime PM might\n> be enabled for the device later by someone else.\n> \n> Although that concern doesn't seem to be actually valid, the\n> device is clearly still expected to be PM-capable even in the\n> pm_disabled set case, so a better approach would be to enable\n> runtime PM for it unconditionally and then prevent it from\n> being runtime suspended by using pm_runtime_forbid().\n> \n> Make the driver do that as that will help to clean up its system\n> sleep handling in a relatively straightforward way.\n> \n> Signed-off-by: Rafael J. Wysocki \n> ---\n> drivers/i2c/busses/i2c-designware-platdrv.c | 36 +++++++++++++++++++---------\n> 1 file changed, 25 insertions(+), 11 deletions(-)\n> \n> Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c\n> ===================================================================\n> --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c\n> +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c\n> @@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct\n> \t}\n> }\n> \n> +static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)\n> +{\n> +\tpm_runtime_get_noresume(dev->dev);\n> +\tif (dev->pm_disabled)\n> +\t\tpm_runtime_allow(dev->dev);\n> +\n> +\tpm_runtime_disable(dev->dev);\n> +\tpm_runtime_put_noidle(dev->dev);\n> +}\n> +\n> static int dw_i2c_plat_probe(struct platform_device *pdev)\n> {\n> \tstruct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);\n> @@ -362,14 +372,19 @@ static int dw_i2c_plat_probe(struct plat\n> \tACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));\n> \tadap->dev.of_node = pdev->dev.of_node;\n> \n> -\tif (dev->pm_disabled) {\n> +\t/* The code below assumes runtime PM to be disabled. */\n> +\tWARN_ON(pm_runtime_enabled(&pdev->dev));\n> +\n> +\tpm_runtime_set_autosuspend_delay(&pdev->dev, 1000);\n> +\tpm_runtime_use_autosuspend(&pdev->dev);\n> +\tpm_runtime_set_active(&pdev->dev);\n> +\n> +\tpm_runtime_get_noresume(&pdev->dev);\n> +\tpm_runtime_enable(&pdev->dev);\n> +\tif (dev->pm_disabled)\n> \t\tpm_runtime_forbid(&pdev->dev);\n> -\t} else {\n> -\t\tpm_runtime_set_autosuspend_delay(&pdev->dev, 1000);\n> -\t\tpm_runtime_use_autosuspend(&pdev->dev);\n> -\t\tpm_runtime_set_active(&pdev->dev);\n> -\t\tpm_runtime_enable(&pdev->dev);\n> -\t}\n> +\n> +\tpm_runtime_put_noidle(&pdev->dev);\n> \nIs pm_runtime_get_noresume()/pm_runtime_put_noidle() cycle needed here? \nMy vague memory tells platform device won't power off instantly because \nof plain pm_runtime_enable() even if dev->power.usage_count is zero.\n\nI guess it was the pm_request_idle() in driver_probe_device() that \ntriggered the power transition after probe.\n\ndrivers/base/dd.c: driver_probe_device():\n\tpm_runtime_barrier(dev);\n\tret = really_probe(dev, drv);\n\tpm_request_idle(dev);","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 3xmqBk3HLkz9t1t\n\tfor ;\n\tWed, 6 Sep 2017 00:40:30 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751238AbdIEOk2 (ORCPT );\n\tTue, 5 Sep 2017 10:40:28 -0400","from mga09.intel.com ([134.134.136.24]:38389 \"EHLO mga09.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751031AbdIEOk2 (ORCPT );\n\tTue, 5 Sep 2017 10:40:28 -0400","from fmsmga003.fm.intel.com ([10.253.24.29])\n\tby orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t05 Sep 2017 07:40:27 -0700","from mylly.fi.intel.com (HELO [10.237.72.56]) ([10.237.72.56])\n\tby FMSMGA003.fm.intel.com with ESMTP; 05 Sep 2017 07:40:23 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,480,1498546800\"; d=\"scan'208\";a=\"897264101\"","Subject":"Re: [RFT][PATCH 1/2] i2c: designware: Clean up PM handling in\n\tdw_i2c_plat_probe()","To":"\"Rafael J. Wysocki\" , linux-pm@vger.kernel.org,\n\tlinux-i2c@vger.kernel.org","Cc":"Wolfram Sang , linux-acpi@vger.kernel.org,\n\tKevin Hilman ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach , Ulf Hansson ","References":"<3023226.l5IfJK6GIc@aspire.rjw.lan>\n\t<57866801.UnLEtqmHq8@aspire.rjw.lan>","From":"Jarkko Nikula ","Message-ID":"","Date":"Tue, 5 Sep 2017 17:40:22 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<57866801.UnLEtqmHq8@aspire.rjw.lan>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1763419,"web_url":"http://patchwork.ozlabs.org/comment/1763419/","msgid":"<3992663.3QJSSQ4alv@aspire.rjw.lan>","list_archive_url":null,"date":"2017-09-05T14:41:34","subject":"Re: [RFT][PATCH 1/2] i2c: designware: Clean up PM handling in\n\tdw_i2c_plat_probe()","submitter":{"id":26536,"url":"http://patchwork.ozlabs.org/api/people/26536/","name":"Rafael J. Wysocki","email":"rjw@rjwysocki.net"},"content":"On Tuesday, September 5, 2017 4:40:22 PM CEST Jarkko Nikula wrote:\n> Hi\n> \n> On 09/04/2017 02:08 AM, Rafael J. Wysocki wrote:\n> > From: Rafael J. Wysocki \n> > \n> > The power management handling in dw_i2c_plat_probe() is somewhat\n> > messy and it is rather hard to figure out the code intention for\n> > the case when pm_disabled is set. In that case, the driver doesn't\n> > enable runtime PM at all, but in addition to that it calls\n> > pm_runtime_forbid() as though it wasn't sure if runtime PM might\n> > be enabled for the device later by someone else.\n> > \n> > Although that concern doesn't seem to be actually valid, the\n> > device is clearly still expected to be PM-capable even in the\n> > pm_disabled set case, so a better approach would be to enable\n> > runtime PM for it unconditionally and then prevent it from\n> > being runtime suspended by using pm_runtime_forbid().\n> > \n> > Make the driver do that as that will help to clean up its system\n> > sleep handling in a relatively straightforward way.\n> > \n> > Signed-off-by: Rafael J. Wysocki \n> > ---\n> > drivers/i2c/busses/i2c-designware-platdrv.c | 36 +++++++++++++++++++---------\n> > 1 file changed, 25 insertions(+), 11 deletions(-)\n> > \n> > Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c\n> > ===================================================================\n> > --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c\n> > +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c\n> > @@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct\n> > \t}\n> > }\n> > \n> > +static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)\n> > +{\n> > +\tpm_runtime_get_noresume(dev->dev);\n> > +\tif (dev->pm_disabled)\n> > +\t\tpm_runtime_allow(dev->dev);\n> > +\n> > +\tpm_runtime_disable(dev->dev);\n> > +\tpm_runtime_put_noidle(dev->dev);\n> > +}\n> > +\n> > static int dw_i2c_plat_probe(struct platform_device *pdev)\n> > {\n> > \tstruct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);\n> > @@ -362,14 +372,19 @@ static int dw_i2c_plat_probe(struct plat\n> > \tACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));\n> > \tadap->dev.of_node = pdev->dev.of_node;\n> > \n> > -\tif (dev->pm_disabled) {\n> > +\t/* The code below assumes runtime PM to be disabled. */\n> > +\tWARN_ON(pm_runtime_enabled(&pdev->dev));\n> > +\n> > +\tpm_runtime_set_autosuspend_delay(&pdev->dev, 1000);\n> > +\tpm_runtime_use_autosuspend(&pdev->dev);\n> > +\tpm_runtime_set_active(&pdev->dev);\n> > +\n> > +\tpm_runtime_get_noresume(&pdev->dev);\n> > +\tpm_runtime_enable(&pdev->dev);\n> > +\tif (dev->pm_disabled)\n> > \t\tpm_runtime_forbid(&pdev->dev);\n> > -\t} else {\n> > -\t\tpm_runtime_set_autosuspend_delay(&pdev->dev, 1000);\n> > -\t\tpm_runtime_use_autosuspend(&pdev->dev);\n> > -\t\tpm_runtime_set_active(&pdev->dev);\n> > -\t\tpm_runtime_enable(&pdev->dev);\n> > -\t}\n> > +\n> > +\tpm_runtime_put_noidle(&pdev->dev);\n> > \n> Is pm_runtime_get_noresume()/pm_runtime_put_noidle() cycle needed here? \n> My vague memory tells platform device won't power off instantly because \n> of plain pm_runtime_enable() even if dev->power.usage_count is zero.\n\nNot by itself, but as a result of something running in parallel with the\nprobe it may in theory.\n\n> I guess it was the pm_request_idle() in driver_probe_device() that \n> triggered the power transition after probe.\n> \n> drivers/base/dd.c: driver_probe_device():\n> \tpm_runtime_barrier(dev);\n> \tret = really_probe(dev, drv);\n> \tpm_request_idle(dev);\n\nThis doesn't prevent runtime PM transitions from occurring in parallel\nwith really_probe() and the extra counter incrementation/decrementation\ndoesn't hurt. :-)\n\nTo me, the rule of thumb for runtime PM should be quite analogous to the one\nfor interrupts: expect it to happen immediately after you have enabled it\nunless you know for a fact that there are protections in place.\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 3xmqQR1fxdz9s9Y\n\tfor ;\n\tWed, 6 Sep 2017 00:50:39 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751808AbdIEOuZ (ORCPT );\n\tTue, 5 Sep 2017 10:50:25 -0400","from cloudserver094114.home.net.pl ([79.96.170.134]:56752 \"EHLO\n\tcloudserver094114.home.net.pl\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751698AbdIEOuX (ORCPT\n\t); Tue, 5 Sep 2017 10:50:23 -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 97db060fda0ac763; Tue, 5 Sep 2017 16:50:21 +0200"],"From":"\"Rafael J. Wysocki\" ","To":"Jarkko Nikula ","Cc":"linux-pm@vger.kernel.org, linux-i2c@vger.kernel.org,\n\tWolfram Sang , linux-acpi@vger.kernel.org,\n\tKevin Hilman ,\n\tAndy Shevchenko ,\n\tMika Westerberg ,\n\tJisheng Zhang ,\n\tJohn Stultz , Guodong Xu ,\n\tSumit Semwal ,\n\tHaojian Zhuang ,\n\tJohannes Stezenbach , Ulf Hansson ","Subject":"Re: [RFT][PATCH 1/2] i2c: designware: Clean up PM handling in\n\tdw_i2c_plat_probe()","Date":"Tue, 05 Sep 2017 16:41:34 +0200","Message-ID":"<3992663.3QJSSQ4alv@aspire.rjw.lan>","In-Reply-To":"","References":"<3023226.l5IfJK6GIc@aspire.rjw.lan>\n\t<57866801.UnLEtqmHq8@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"}}]