Date:	Sat, 17 Oct 2015 09:56:17 -0700
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Rob Herring <robh+dt@...nel.org>
Cc:	Tomeu Vizoso <tomeu.vizoso@...labora.com>,
	Russell King <linux@....linux.org.uk>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...eaurora.org>,
	Vinod Koul <vinod.koul@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Thierry Reding <thierry.reding@...il.com>,
	David Airlie <airlied@...ux.ie>,
	Terje Bergström <tbergstrom@...dia.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Wolfram Sang <wsa@...-dreams.de>,
	Frank Rowand <frowand.list@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	Kishon Vijay Abraham I <kishon@...com>,
	Sebastian Reichel <sre@...nel.org>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>, Felipe Balbi <balbi@...com>,
	Jingoo Han <jingoohan1@...il.com>,
	Lee Jones <lee.jones@...aro.org>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Tomi Valkeinen <tomi.valkeinen@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-clk@...r.kernel.org, dmaengine@...r.kernel.org,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Linux PWM List <linux-pwm@...r.kernel.org>,
	Linux USB List <linux-usb@...r.kernel.org>,
	"linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>
Subject: Re: [GIT PULL] On-demand device probing

On Sat, Oct 17, 2015 at 11:28:29AM -0500, Rob Herring wrote:
> On Sat, Oct 17, 2015 at 10:47 AM, Greg Kroah-Hartman
> <gregkh@...uxfoundation.org> wrote:
> > On Sat, Oct 17, 2015 at 10:04:55AM -0500, Rob Herring wrote:
> >> On Sat, Oct 17, 2015 at 1:57 AM, Greg Kroah-Hartman
> >> <gregkh@...uxfoundation.org> wrote:
> >> > On Wed, Oct 14, 2015 at 10:34:00AM +0200, Tomeu Vizoso wrote:
> >> >> Hi Rob,
> >> >>
> >> >> here is the pull request you asked for, with no changes from the version
> >> >> that I posted last to the list.
> >> >>
> >> >> The following changes since commit 6ff33f3902c3b1c5d0db6b1e2c70b6d76fba357f:
> >> >>
> >> >> Linux 4.3-rc1 (2015-09-12 16:35:56 -0700)
> >> >>
> >> >> are available in the git repository at:
> >> >>
> >> >> git+ssh://git.collabora.co.uk/git/user/tomeu/linux.git
> >> >> on-demand-probes-for-next
> >> >
> >> > That's not a signed tag :(
> >> >
> >> > Anyway, I REALLY don't like this series (sorry for the delay in
> >> > reviewing them, normally I trust Rob's judgement...)
> >>
> >> We've seen a lot of attempts here. This is really the best solution so
> >> far in that it is simple, uses existing data from DT, and was low risk
> >> for breaking platforms (at least I thought it would be). Anyway,
> >> getting more exposure is why I've put it into -next.
> >
> > Exposure is good, now we know it breaks some builds, which was useful :)
> 
> Now that I've looked at them, they are somewhat questionable failures.
> They do show the fragile nature of probe ordering and the implicit
> dependencies we have.
> 
> >> > I can't see adding calls like this all over the tree just to solve a
> >> > bus-specific problem, you are adding of_* calls where they aren't
> >> > needed, or wanted, at all.
> >>
> >> I think Linus W, Mark B, and I all said a similar thing initially in
> >> that dependencies should be handled in the driver core. We went down
> >> the path of making this not firmware (aka bus) specific and an earlier
> >> version had just that (with fwnode_* calls). That turned out to be
> >> pointless as the calling locations were almost always in DT specific
> >> code anyway. If you notice, the calls are next to other DT specific
> >> calls generally (usually a "get"). So yes, I'd prefer not to have to
> >> touch every subsystem, but we had to do that anyway to add DT support.
> >
> > If they are "next" to a call like that, why not put it in that call?  I
> > really object to having to "sprinkle" this all over the kernel, for no
> > obvious reason why that is happening at all (look at the USB patch for
> > one such example.)
> 
> Looking at it again, they are in DT specific code already. The USB one
> is in devm_usb_get_phy_by_node() which is a DT specific call.

But that's not very obvious, right?  Especially given that you now have
to add a new .h file, which implies that suddenly this file is now
touching a new subsystem.

> >> We've generally split the DT code into the core (in drivers/of) and
> >> the binding specific (in subsystems). Extracting dependency
> >> information the DT is going to require binding specific knowledge, so
> >> subsystem changes are probably unavoidable.
> >>
> >> The alternative is we put binding specific knowledge into the core DT
> >> code to parse dependencies.
> >>
> >> > What is the root-problem of your delay in device probing?  I read your
> >> > last patch series and I can't seem to figure out what the issue is that
> >> > this is solving in any "better" way from the existing deferred probing.
> >>
> >> It saves 2 seconds in the boot time as re-probing takes time. That
> >> alone seems compelling to me.
> >
> > 2 seconds is _forever_, and really seems like some other driver is
> > sleeping and causing this problem.  What does the bootlog time-chart say
> > is really causing this long delay?  There's no way we are stuck in some
> > sort of logic loop for that long (i.e. having to walk the list of
> > devices somehow.)  This sounds like a driver-specific problem that is
> > being worked around by having to touch all subsystems, which isn't nice.
> 
> I don't think it is one driver as the improvement is seen on multiple
> platforms. I'll let Tomeu comment further on where the time was spent.

That would be good to know, as 2 seconds is forever (my whole machine
boots to a gnome login faster than that.)

> > Hint, we didn't have to do this type of thing to solve boot delays on
> > x86 when we had hardware that was slow to initialize, why should DT be
> > special?  :)
> 
> x86 did not need deferred probe either (though we probably can find
> some initcall ordering hacks). This is an embedded problem, not a DT
> problem.

x86 is embedded :)

> I'm guessing the time is a matter of probing and undoing the probes
> rather than slow h/w. We could maybe improve things by making sure
> drivers move what they defer on to the beginning of probe, but that
> seems like a horrible, fragile hack.

How can calling probe and failing cause 2 seconds?  How many different
probe calls are failing here?  Again, a boot log graph would be great to
see as it will show the root cause, not just guessing at this.

> >> Another downside to deferred probing is you have to touch every driver
> >> and subsystem to support it. This contains the problem to the
> >> subsystems.
> >
> > But we have deferred probing already, only those drivers that need/want
> > it have to do anything, why create yet-another model here?
> 
> Yes, the only ones needing it are drivers dependent on clocks, gpio,
> regulators, pwm, pin-ctrl, dma, etc. That's not a small number. This
> is a side benefit and wouldn't take this series for that reason alone.
> 
> I've used the deferred probing is good enough argument myself on
> previous attempts. The boot time improvements convinced me it is not
> good enough except for simple cases.

Then let's fix deferred probing to do it "correctly", let's not add
yet-another-way-to-probe instead please, as we will be forever
sprinkling these calls around subsystems in a cargo-cult-like manner for
forever.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists