[RFT,1/2] i2c: designware: Clean up PM handling in dw_i2c_plat_probe()

Message ID 57866801.UnLEtqmHq8@aspire.rjw.lan
State New
Headers show
Series
  • i2c: designware: Runtime PM aware system sleep handling
Related show

Commit Message

Rafael J. Wysocki Sept. 3, 2017, 11:08 p.m.
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The power management handling in dw_i2c_plat_probe() is somewhat
messy and it is rather hard to figure out the code intention for
the case when pm_disabled is set.  In that case, the driver doesn't
enable runtime PM at all, but in addition to that it calls
pm_runtime_forbid() as though it wasn't sure if runtime PM might
be enabled for the device later by someone else.

Although that concern doesn't seem to be actually valid, the
device is clearly still expected to be PM-capable even in the
pm_disabled set case, so a better approach would be to enable
runtime PM for it unconditionally and then prevent it from
being runtime suspended by using pm_runtime_forbid().

Make the driver do that as that will help to clean up its system
sleep handling in a relatively straightforward way.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   36 +++++++++++++++++++---------
 1 file changed, 25 insertions(+), 11 deletions(-)

Comments

Jarkko Nikula Sept. 5, 2017, 2:40 p.m. | #1
Hi

On 09/04/2017 02:08 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The power management handling in dw_i2c_plat_probe() is somewhat
> messy and it is rather hard to figure out the code intention for
> the case when pm_disabled is set.  In that case, the driver doesn't
> enable runtime PM at all, but in addition to that it calls
> pm_runtime_forbid() as though it wasn't sure if runtime PM might
> be enabled for the device later by someone else.
> 
> Although that concern doesn't seem to be actually valid, the
> device is clearly still expected to be PM-capable even in the
> pm_disabled set case, so a better approach would be to enable
> runtime PM for it unconditionally and then prevent it from
> being runtime suspended by using pm_runtime_forbid().
> 
> Make the driver do that as that will help to clean up its system
> sleep handling in a relatively straightforward way.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c |   36 +++++++++++++++++++---------
>   1 file changed, 25 insertions(+), 11 deletions(-)
> 
> Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
> ===================================================================
> --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct
>   	}
>   }
>   
> +static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
> +{
> +	pm_runtime_get_noresume(dev->dev);
> +	if (dev->pm_disabled)
> +		pm_runtime_allow(dev->dev);
> +
> +	pm_runtime_disable(dev->dev);
> +	pm_runtime_put_noidle(dev->dev);
> +}
> +
>   static int dw_i2c_plat_probe(struct platform_device *pdev)
>   {
>   	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -362,14 +372,19 @@ static int dw_i2c_plat_probe(struct plat
>   	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>   	adap->dev.of_node = pdev->dev.of_node;
>   
> -	if (dev->pm_disabled) {
> +	/* The code below assumes runtime PM to be disabled. */
> +	WARN_ON(pm_runtime_enabled(&pdev->dev));
> +
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +	if (dev->pm_disabled)
>   		pm_runtime_forbid(&pdev->dev);
> -	} else {
> -		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> -		pm_runtime_use_autosuspend(&pdev->dev);
> -		pm_runtime_set_active(&pdev->dev);
> -		pm_runtime_enable(&pdev->dev);
> -	}
> +
> +	pm_runtime_put_noidle(&pdev->dev);
>   
Is pm_runtime_get_noresume()/pm_runtime_put_noidle() cycle needed here? 
My vague memory tells platform device won't power off instantly because 
of plain pm_runtime_enable() even if dev->power.usage_count is zero.

I guess it was the pm_request_idle() in driver_probe_device() that 
triggered the power transition after probe.

drivers/base/dd.c: driver_probe_device():
	pm_runtime_barrier(dev);
	ret = really_probe(dev, drv);
	pm_request_idle(dev);
Rafael J. Wysocki Sept. 5, 2017, 2:41 p.m. | #2
On Tuesday, September 5, 2017 4:40:22 PM CEST Jarkko Nikula wrote:
> Hi
> 
> On 09/04/2017 02:08 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The power management handling in dw_i2c_plat_probe() is somewhat
> > messy and it is rather hard to figure out the code intention for
> > the case when pm_disabled is set.  In that case, the driver doesn't
> > enable runtime PM at all, but in addition to that it calls
> > pm_runtime_forbid() as though it wasn't sure if runtime PM might
> > be enabled for the device later by someone else.
> > 
> > Although that concern doesn't seem to be actually valid, the
> > device is clearly still expected to be PM-capable even in the
> > pm_disabled set case, so a better approach would be to enable
> > runtime PM for it unconditionally and then prevent it from
> > being runtime suspended by using pm_runtime_forbid().
> > 
> > Make the driver do that as that will help to clean up its system
> > sleep handling in a relatively straightforward way.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-platdrv.c |   36 +++++++++++++++++++---------
> >   1 file changed, 25 insertions(+), 11 deletions(-)
> > 
> > Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
> > ===================================================================
> > --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct
> >   	}
> >   }
> >   
> > +static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
> > +{
> > +	pm_runtime_get_noresume(dev->dev);
> > +	if (dev->pm_disabled)
> > +		pm_runtime_allow(dev->dev);
> > +
> > +	pm_runtime_disable(dev->dev);
> > +	pm_runtime_put_noidle(dev->dev);
> > +}
> > +
> >   static int dw_i2c_plat_probe(struct platform_device *pdev)
> >   {
> >   	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > @@ -362,14 +372,19 @@ static int dw_i2c_plat_probe(struct plat
> >   	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> >   	adap->dev.of_node = pdev->dev.of_node;
> >   
> > -	if (dev->pm_disabled) {
> > +	/* The code below assumes runtime PM to be disabled. */
> > +	WARN_ON(pm_runtime_enabled(&pdev->dev));
> > +
> > +	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > +	pm_runtime_use_autosuspend(&pdev->dev);
> > +	pm_runtime_set_active(&pdev->dev);
> > +
> > +	pm_runtime_get_noresume(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +	if (dev->pm_disabled)
> >   		pm_runtime_forbid(&pdev->dev);
> > -	} else {
> > -		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > -		pm_runtime_use_autosuspend(&pdev->dev);
> > -		pm_runtime_set_active(&pdev->dev);
> > -		pm_runtime_enable(&pdev->dev);
> > -	}
> > +
> > +	pm_runtime_put_noidle(&pdev->dev);
> >   
> Is pm_runtime_get_noresume()/pm_runtime_put_noidle() cycle needed here? 
> My vague memory tells platform device won't power off instantly because 
> of plain pm_runtime_enable() even if dev->power.usage_count is zero.

Not by itself, but as a result of something running in parallel with the
probe it may in theory.

> I guess it was the pm_request_idle() in driver_probe_device() that 
> triggered the power transition after probe.
> 
> drivers/base/dd.c: driver_probe_device():
> 	pm_runtime_barrier(dev);
> 	ret = really_probe(dev, drv);
> 	pm_request_idle(dev);

This doesn't prevent runtime PM transitions from occurring in parallel
with really_probe() and the extra counter incrementation/decrementation
doesn't hurt. :-)

To me, the rule of thumb for runtime PM should be quite analogous to the one
for interrupts: expect it to happen immediately after you have enabled it
unless you know for a fact that there are protections in place.

Thanks,
Rafael

Patch

Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
===================================================================
--- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
+++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -249,6 +249,16 @@  static void dw_i2c_set_fifo_size(struct
 	}
 }
 
+static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
+{
+	pm_runtime_get_noresume(dev->dev);
+	if (dev->pm_disabled)
+		pm_runtime_allow(dev->dev);
+
+	pm_runtime_disable(dev->dev);
+	pm_runtime_put_noidle(dev->dev);
+}
+
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -362,14 +372,19 @@  static int dw_i2c_plat_probe(struct plat
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
-	if (dev->pm_disabled) {
+	/* The code below assumes runtime PM to be disabled. */
+	WARN_ON(pm_runtime_enabled(&pdev->dev));
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	if (dev->pm_disabled)
 		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+
+	pm_runtime_put_noidle(&pdev->dev);
 
 	if (dev->mode == DW_IC_SLAVE)
 		ret = i2c_dw_probe_slave(dev);
@@ -382,8 +397,7 @@  static int dw_i2c_plat_probe(struct plat
 	return ret;
 
 exit_probe:
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
 exit_reset:
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);
@@ -402,8 +416,8 @@  static int dw_i2c_plat_remove(struct pla
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_disabled)
-		pm_runtime_disable(&pdev->dev);
+	dw_i2c_plat_pm_cleanup(dev);
+
 	if (!IS_ERR_OR_NULL(dev->rst))
 		reset_control_assert(dev->rst);