[PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

Commit Message

Guennadi Liakhovetski April 26, 2011, 10:44 a.m. UTC

Hi

On Mon, 25 Apr 2011, Rafael J. Wysocki wrote:

> On Saturday, April 23, 2011, Rafael J. Wysocki wrote:
> > On Friday, April 22, 2011, Alan Stern wrote:
> > > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote:
> > > 
> > > > > The barrier would not prevent the race between the notifier and runtie PM
> > > > > from taking place.  Why don't we do something like this instead:
> > > > > 
> > > > > ---
> > > > >  drivers/base/dd.c |    3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > Index: linux-2.6/drivers/base/dd.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/base/dd.c
> > > > > +++ linux-2.6/drivers/base/dd.c
> > > > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru
> > > > >  						     BUS_NOTIFY_UNBIND_DRIVER,
> > > > >  						     dev);
> > > > >  
> > > > > +		pm_runtime_put_sync(dev);
> > > > > +
> > > > 
> > > > In fact, I think this one may be _noidle.  If we allow the bus/driver
> > > > to do what they wont, we might as well let them handle the "device idle"
> > > > case from ->remove().
> > > 
> > > Maybe...  But keeping it put_sync doesn't do any harm.  In Guennadi's 
> > > case, it might allow him to get rid of the pm_runtime_suspend() call in 
> > > the remove routine.
> > > 
> > > > >  		if (dev->bus && dev->bus->remove)
> > > > >  			dev->bus->remove(dev);
> > > > >  		else if (drv->remove)
> > > > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru
> > > > >  						     BUS_NOTIFY_UNBOUND_DRIVER,
> > > > >  						     dev);
> > > > >  
> > > > > -		pm_runtime_put_sync(dev);
> > > > >  	}
> > > > >  }
> > > 
> > > Basically this is okay with me, and it should allow Guennadi to avoid
> > > the extra put/get pair.
> > 
> > OK, so I'm going to put the appended patch into my linux-next branch
> > (hopefully, the problem is explained sufficiently in the changelog).
> 
> I thought about that a bit more and came to the conclusion that we should
> do things a bit differently in __device_release_driver().  Namely, the fact
> that the device can be resumed (either synchronously or asynchronously) after
> the pm_runtime_barrier() has returned may be problematic too, because it
> may race with the bus notifier in some cases.  For this reason, I think it
> would be better to do pm_runtime_get_sync() instead of the
> pm_runtime_get_noresume() and pm_runtime_barrier().
> 
> So, I think the appended patch would be better than the previous one.

I refrained in taking part in the general rtpm API behaviour, I'd rather 
rely on others here. If you push this your patch, I'll have to change my 
TMIO/SDHI and MMCIF patches as follows:


Is this your patch final and shall I submit updated versions of my patches 
or shall I wait for your patch to take its final form and hit "next?"

Thanks
Guennadi

> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <[e-mail]>
> Subject: PM / Runtime: Rework runtime PM handling during driver removal
> 
> The driver core tries to prevent race conditions between runtime PM
> and driver removal from happening by incrementing the runtime PM
> usage counter of the device and executing pm_runtime_barrier() before
> running the bus notifier and the ->remove() callbacks provided by the
> device's subsystem or driver.  This guarantees that, if a future
> runtime suspend of the device has been scheduled or a runtime resume
> or idle request has been queued up right before the driver removal,
> it will be canceled or waited for to complete and no other
> asynchronous runtime suspend or idle requests for the device will be
> put into the PM workqueue until the ->remove() callback returns.
> However, it doesn't prevent resume requests from being queued up
> after pm_runtime_barrier() has been called and it doesn't prevent
> pm_runtime_resume() from executing the device subsystem's runtime
> resume callback.  Morever, it prevents the device's subsystem or
> driver from putting the device into the suspended state by calling
> pm_runtime_suspend() from its ->remove() routine.  This turns out to
> be a major inconvenience for some subsystems and drivers that want to
> leave the devices they handle in the suspended state.
> 
> To really prevent runtime PM callbacks from racing with the bus
> notifier callback in __device_release_driver(), which is necessary,
> because the notifier is used by some subsystems to carry out
> operations affecting the runtime PM functionality, use
> pm_runtime_get_sync() instead of the combination of
> pm_runtime_get_noresume() and pm_runtime_barrier().  This will resume
> the device if it's in the suspended state and will prevent it from
> being suspended again until pm_runtime_put_*() is called.
> 
> To allow subsystems and drivers to put devices into the suspended
> state by calling pm_runtime_suspend() from their ->remove() routines,
> execute pm_runtime_put_sync() after running the bus notifier in
> __device_release_driver().  This will require subsystems and drivers
> to make their ->remove() callbacks avoid races with runtime PM
> directly, but it will allow of more flexibility in the handling of
> devices during the removal of their drivers.
> 
> Signed-off-by: Rafael J. Wysocki <[e-mail]>
> ---
>  drivers/base/dd.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/base/dd.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/dd.c
> +++ linux-2.6/drivers/base/dd.c
> @@ -316,8 +316,7 @@ static void __device_release_driver(stru
>  
>  	drv = dev->driver;
>  	if (drv) {
> -		pm_runtime_get_noresume(dev);
> -		pm_runtime_barrier(dev);
> +		pm_runtime_get_sync(dev);
>  
>  		driver_sysfs_remove(dev);
>  
> @@ -326,6 +325,8 @@ static void __device_release_driver(stru
>  						     BUS_NOTIFY_UNBIND_DRIVER,
>  						     dev);
>  
> +		pm_runtime_put_sync(dev);
> +
>  		if (dev->bus && dev->bus->remove)
>  			dev->bus->remove(dev);
>  		else if (drv->remove)
> @@ -338,7 +339,6 @@ static void __device_release_driver(stru
>  						     BUS_NOTIFY_UNBOUND_DRIVER,
>  						     dev);
>  
> -		pm_runtime_put_sync(dev);
>  	}
>  }
>  
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Comments

Rafael Wysocki April 26, 2011, 11:51 a.m. UTC | #1

On Tuesday, April 26, 2011, Guennadi Liakhovetski wrote:
> Hi

Hi,

> On Mon, 25 Apr 2011, Rafael J. Wysocki wrote:
> 
> > On Saturday, April 23, 2011, Rafael J. Wysocki wrote:
> > > On Friday, April 22, 2011, Alan Stern wrote:
> > > > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote:
> > > > 
> > > > > > The barrier would not prevent the race between the notifier and runtie PM
> > > > > > from taking place.  Why don't we do something like this instead:
> > > > > > 
> > > > > > ---
> > > > > >  drivers/base/dd.c |    3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > Index: linux-2.6/drivers/base/dd.c
> > > > > > ===================================================================
> > > > > > --- linux-2.6.orig/drivers/base/dd.c
> > > > > > +++ linux-2.6/drivers/base/dd.c
> > > > > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru
> > > > > >  						     BUS_NOTIFY_UNBIND_DRIVER,
> > > > > >  						     dev);
> > > > > >  
> > > > > > +		pm_runtime_put_sync(dev);
> > > > > > +
> > > > > 
> > > > > In fact, I think this one may be _noidle.  If we allow the bus/driver
> > > > > to do what they wont, we might as well let them handle the "device idle"
> > > > > case from ->remove().
> > > > 
> > > > Maybe...  But keeping it put_sync doesn't do any harm.  In Guennadi's 
> > > > case, it might allow him to get rid of the pm_runtime_suspend() call in 
> > > > the remove routine.
> > > > 
> > > > > >  		if (dev->bus && dev->bus->remove)
> > > > > >  			dev->bus->remove(dev);
> > > > > >  		else if (drv->remove)
> > > > > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru
> > > > > >  						     BUS_NOTIFY_UNBOUND_DRIVER,
> > > > > >  						     dev);
> > > > > >  
> > > > > > -		pm_runtime_put_sync(dev);
> > > > > >  	}
> > > > > >  }
> > > > 
> > > > Basically this is okay with me, and it should allow Guennadi to avoid
> > > > the extra put/get pair.
> > > 
> > > OK, so I'm going to put the appended patch into my linux-next branch
> > > (hopefully, the problem is explained sufficiently in the changelog).
> > 
> > I thought about that a bit more and came to the conclusion that we should
> > do things a bit differently in __device_release_driver().  Namely, the fact
> > that the device can be resumed (either synchronously or asynchronously) after
> > the pm_runtime_barrier() has returned may be problematic too, because it
> > may race with the bus notifier in some cases.  For this reason, I think it
> > would be better to do pm_runtime_get_sync() instead of the
> > pm_runtime_get_noresume() and pm_runtime_barrier().
> > 
> > So, I think the appended patch would be better than the previous one.
> 
> I refrained in taking part in the general rtpm API behaviour, I'd rather 
> rely on others here. If you push this your patch, I'll have to change my 
> TMIO/SDHI and MMCIF patches as follows:
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 1889d64..3a22e55 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -1117,6 +1117,8 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
>  	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
>  	int irq[2];
>  
> +	pm_runtime_get_sync(&pdev->dev);
> +
>  	mmc_remove_host(host->mmc);
>  	sh_mmcif_release_dma(host);
>  
> @@ -1137,7 +1139,6 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev)
>  	mmc_free_host(host->mmc);
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> -	pm_runtime_get_noresume(&pdev->dev);
>  
>  	return 0;
>  }
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 26598f1..86eaa68 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -956,17 +956,9 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host)
>  	iounmap(host->ctl);
>  	mmc_free_host(host->mmc);
>  
> -	/*
> -	 * Now rtpm usage_count = 2, because we incremented it once in probe()
> -	 * above, and dd.c incremented it again, before calling .release(). So.
> -	 * to power the device down we have to decrement the counter to 0 and
> -	 * suspend it, because after our disable() suspending from dd.c will
> -	 * only decrement the counter, but not call any callbacks
> -	 */
> -	pm_runtime_put_noidle(&pdev->dev);
> +	/* Compensate for pm_runtime_get_sync() in probe() above */
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> -	pm_runtime_get_noresume(&pdev->dev);
>  }
>  EXPORT_SYMBOL(tmio_mmc_host_remove);

So you won't get the reversed _put/_get calls any more here, good. :-)

> Is this your patch final and shall I submit updated versions of my patches 

This patch will be final if there are no objections from other people.

> or shall I wait for your patch to take its final form and hit "next?"

It generally is better to wait for a patch to appear in linux-next before
basing you work on top of it.

Thanks,
Rafael

Patch

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 1889d64..3a22e55 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -1117,6 +1117,8 @@  static int __devexit sh_mmcif_remove(struct platform_device *pdev)
 	struct sh_mmcif_host *host = platform_get_drvdata(pdev);
 	int irq[2];
 
+	pm_runtime_get_sync(&pdev->dev);
+
 	mmc_remove_host(host->mmc);
 	sh_mmcif_release_dma(host);
 
@@ -1137,7 +1139,6 @@  static int __devexit sh_mmcif_remove(struct platform_device *pdev)
 	mmc_free_host(host->mmc);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
 
 	return 0;
 }
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 26598f1..86eaa68 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -956,17 +956,9 @@  void tmio_mmc_host_remove(struct tmio_mmc_host *host)
 	iounmap(host->ctl);
 	mmc_free_host(host->mmc);
 
-	/*
-	 * Now rtpm usage_count = 2, because we incremented it once in probe()
-	 * above, and dd.c incremented it again, before calling .release(). So.
-	 * to power the device down we have to decrement the counter to 0 and
-	 * suspend it, because after our disable() suspending from dd.c will
-	 * only decrement the counter, but not call any callbacks
-	 */
-	pm_runtime_put_noidle(&pdev->dev);
+	/* Compensate for pm_runtime_get_sync() in probe() above */
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	pm_runtime_get_noresume(&pdev->dev);
 }
 EXPORT_SYMBOL(tmio_mmc_host_remove);