Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 5044 bytes --]

On Mon, 5 Dec 2011 12:25:42 +0200 Ohad Ben-Cohen <ohad@wizery.com> wrote:

> Hi Neil,
> 
> On Mon, Dec 5, 2011 at 3:54 AM, NeilBrown <neilb@suse.de> wrote:
> >  I've been chasing down a problem with the SDIO-attached wifi card in the
> >  GTA04 (www.gta04.org).
> 
> 8686, right ?

yep, that's the one!

> 
> >  The problem seems very similar to that described in
> >
> >    http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg04289.html
> 
> Don't go that far back, Joe just reported this exact problem in :
> 
> http://comments.gmane.org/gmane.linux.kernel.mmc/11231

Indeed.

> 
> > commit 08da834a24312157f512224691ad1fddd11c1073
> > Author: Daniel Drake <dsd@laptop.org>
> > Date:   Wed Jul 20 17:39:22 2011 +0100
> >
> >    mmc: enable runtime PM by default
> >
> >
> > and if I revert that commit so that runtime PM is not enabled the problem
> > goes away.
> 
> And this is most likely what we're going to do, unless someone with
> the 8686 can further look into the problem.

Challenge accepted.

Even if I don't find a better solution, this seems backwards.  Sure the
default should be that PM is enabled, but individual board can request no
PM on MMC interfaces where it is a problem.

> 
> > (The wifi chip is a libertas_sdio supported chip connected to an mmc
> > interface on an omap3, and has a separate regulator for power supply, plus a
> > GPIO pin for reset, just like in the email thread.  The problem is
> > exemplified by:
> > [   64.643981] libertas_sdio: probe of mmc1:0001:1 failed with error -16
> > ).
> 
> Yes, the same issue that Joe is seeing.
> 
> > I finally managed to track down exactly what was happening - runtime PM was
> > putting the interface to sleep before the SDIO functions were probed so
> > initialising them failed.
> 
> Yeah, but the question is why it fails.

The chip has a requirement that the reset line is held down during power-on,
and raised shortly after (I don't know exactly how short).  So if you just
remove power and give it back, the chip doesn't come up properly.

> 
> It's perfectly fine to power the card down before the functions are
> probed, because just before they are probed the bus is responsible to
> power the card up again.

"It *should be* perfectly fine ..." :-)

We just have to make sure the bug powers up the card properly.
Maybe I need to create a virtual regulator that powers on the real regulator,
then raises the reset line.  I wonder how hard that is.


> 
> > From: NeilBrown <neilb@suse.de>
> > Date: Mon, 5 Dec 2011 11:34:47 +1100
> > Subject: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.
> 
> You can't tell when probe is finished. In fact, in can happen very far
> in the future or even never at all (e.g. when the function driver
> isn't loaded).

Ahhh... I naively assumed that 

	/*
	 * ...then the SDIO functions.
	 */
	for (i = 0;i < funcs;i++) {
		err = sdio_add_func(host->card->sdio_func[i]);
		if (err)
			goto remove_added;
	}

would add all the functions.  but as you say: the drivers might not be loaded
yet.


> 
> > mmc_attach_sdio currently enables runtime power management on
> > the mmc interface before it completes the probe of functions.
> > This means that it might be asleep during the probe and the probe
> > will fail.
> 
> No - the sdio bus powers the card up before probing the drivers. See
> sdio_bus_probe.
> 
> > In particular, if 'drivers_autoprobe' is enabled on the mmc bus, then
> > device_attach() will be called by bus_probe_device() from device_add()
> > and it will pm_runtime_get_noresume()/pm_runtime_put_sync().
> >
> > As runtime power management this the device is running
> > (pm_runtime_set_active() in mmc_attach_sdio()) and rtpm is enabled
> > (pm_runtime_enable()), the pm_runtime_put_sync() call puts the mmc
> > interface to sleep,
> 
> This is fine
> 
> > so function probing fails.
> 
> The question is why. Again - sdio_bus_probe should power up the card.
> For some reason, it (or something else) fails to do so with the 8686
> on some setups. Other chips might have similar issues, too - we just
> don't know (all we know is that it works great with the wl12xx, and
> with the Daniel's 8686 setup).
> 
> > So this patch moves the call to pm_runtime_enable to after all the
> > calls to sdio_add_func.
> 
> Looks like this will have the undesired side-effect of keeping the
> chip powered up, even if it doesn't have any driver loaded for it.
> 
> And this doesn't really address the problem: we still don't know why
> the 8686 fails to power up at this point.
> 
> Can you please tell us where exactly is the first failure coming from
> ? is this from libertas own probe function ? is this sdio_bus_probe
> getting an error from calling pm_runtime_get_sync ?
> 
> Thanks,
> Ohad

I'll spend some more time on this and get back to you - probably next week.

Thanks for the hints and perspective.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]