Message-ID: <20120518084525.GG4073@opensource.wolfsonmicro.com>
Date:	Fri, 18 May 2012 09:45:25 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	Linus Walleij <linus.walleij@...ricsson.com>,
	linux-kernel@...r.kernel.org, "Rafael J. Wysocki" <rjw@...k.pl>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Kevin Hilman <khilman@...com>,
	Alan Stern <stern@...land.harvard.edu>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jesse Barnes <jbarnes@...tuousgeek.org>
Subject: Re: [PATCH] gpiolib: Defer failed gpio requests by default

On Thu, May 17, 2012 at 10:32:19PM -0600, Grant Likely wrote:

> be done is to move the device to the end of the list.  The agreement
> when this was last discussed on the ML was to make the drivers do an
> explicit move immediately after it has validated all its dependencies.
> That is why it doesn't work to return -EPROBE_DEFER by default from
> helpers; because it becomes really hard to audit if the drivers are
> doing the right thing.

Ah, oh dear - I missed that bit of the discussion I'm afraid and had
thought we'd gone with your option 2 below.  This means that the ASoC
usage is currently not helping anything (though it's no worse than what
we had before), and the regulator framework usage won't do the right
thing (though in practice I don't expect it to actually go off and
there's fun and games associated with that if we actually need to
suspend regulators from software anyway).

> I see two solutions to this though;
> 1) add a new .preprobe hook to device drivers that will respect
>    -EPROBE_DEFER and ignore -EPROBE_DEFER on the regular .probe.
>    Probing a driver will call both, but if the .preprobe hook is
>    populated, then it will also reorder dpm_list between the two
>    calls.
>    - This still requires modifying drivers, but at least it is
>      auditable by mere-mortal reviewers.
>    - It also means all bus_type code has to add a new hook.  Blech.

Yeah, this seems very painful, especially the bit where we have to
change a good selection of buses and drivers to implement and use it.

> 2) simply force devices to the end of dpm_list before each probe
>    attempt (provided that it doesn't have any children).
>    - I've avoided this because it adds a claim of the dpm_list_mtx
>      mutex on every single probe call and adds a lot more manipulation
>      of the list....
>    - However, it should make your patch here actually safe.  If a
>      device gets deferred, it will be guaranteed to be at the end of
>      the list because it gets moved there on every probe attempt.
>    - also after reading through the code (see my notes below; I'm a
>      bit frustrated with the whole thing) I think it is probably the
>      right thing to do.

This does feel much more robust to me than manually reordering in
drivers, we'll have to see if it causes a performance problem though and
if it does perhaps we can handle it.

I guess the other thing is that if we get a lot of drivers implementing
deferral and we stop playing around with initcall stuff then we'll loose
a reasonable proportion of the advantage of not taking the mutex and
reordering.

Either of these options would avoid the surprise that currently exists
with the API.

> Will you be at Connect?  I could use some time sitting in a room with
> smart people to dig through this code and talk out what it should be
> doing.

I won't be unfortunately.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists