Re: use of pm_runtime_disable() from driver probe?

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday, July 12, 2012, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Wednesday, July 11, 2012, Alan Stern wrote:
> >> On Wed, 11 Jul 2012, Rafael J. Wysocki wrote:
> >> 
> >> > > This opens up the possibility of calling probe while a runtime resume
> >> > > or suspend is in progress.  (On the other hand, the existing code
> >> > > doesn't prevent a concurrent runtime resume.)  Maybe it would be best
> >> > > to leave the pm_runtime_barrier().
> >> > 
> >> > That wouldn't close the race, though, because the suspend/resume may still
> >> > be started right after _barrier has returned.
> >> 
> >> True, but see below.
> >> 
> >> > The race is only possible if runtime PM is enabled by a subsystem or PM domain
> >> > code before the first eligible driver is registered and if that code is not
> >> > careful enough to get ready for driver registration.  I'm not sure how likely
> >> > it is to happen in practice.
> >> 
> >> It's not just the first eligible driver.  Drivers can be bound and 
> >> unbound dynamically, and suspends/resume operations can sit on the wait 
> >> queue or wait until a timer expires.  We don't want an old request 
> >> suddenly to take effect in the middle of a probe.
> >> 
> >> The barrier will get rid of any old requests.  New ones would have to 
> >> be added after the probe starts, which as you say, is unlikely.
> >
> > OK, I'll keep the barrier, then.
> >
> > Kevin, can you please double check the patch below?
> 
> Yup, this verion looks right and I tested it and verified that it still
> fixes the problem I'm seeing.
> 
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Tested-by: Kevin Hilman <khilman@ti.com>

Cool, thanks!

I'll submit it officially (with a changelog and tags) tomorrow.

Rafael


> > ---
> >  drivers/base/dd.c |    6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > Index: linux/drivers/base/dd.c
> > ===================================================================
> > --- linux.orig/drivers/base/dd.c
> > +++ linux/drivers/base/dd.c
> > @@ -356,10 +356,9 @@ int driver_probe_device(struct device_dr
> >  	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
> >  		 drv->bus->name, __func__, dev_name(dev), drv->name);
> >  
> > -	pm_runtime_get_noresume(dev);
> >  	pm_runtime_barrier(dev);
> >  	ret = really_probe(dev, drv);
> > -	pm_runtime_put_sync(dev);
> > +	pm_runtime_idle(dev);
> >  
> >  	return ret;
> >  }
> > @@ -406,9 +405,8 @@ int device_attach(struct device *dev)
> >  			ret = 0;
> >  		}
> >  	} else {
> > -		pm_runtime_get_noresume(dev);
> >  		ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
> > -		pm_runtime_put_sync(dev);
> > +		pm_runtime_idle(dev);
> >  	}
> >  out_unlock:
> >  	device_unlock(dev);
> 
>