linux-kernel - Re: [PATCH v3 1/2] driver core: Rework logic in __driver_deferred_probe_check_state to allow EPROBE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200220052739.87057-1-saravanak@google.com>
Date:   Wed, 19 Feb 2020 21:27:38 -0800
From:   Saravana Kannan <saravanak@...gle.com>
To:     john.stultz@...aro.org, lkml <linux-kernel@...r.kernel.org>,
        Rob Herring <robh@...nel.org>
Cc:     bjorn.andersson@...aro.org, broonie@...nel.org,
        gregkh@...uxfoundation.org, khilman@...nel.org,
        len.brown@...el.com, lgirdwood@...il.com, linux-pm@...r.kernel.org,
        pavel@....cz, rjw@...ysocki.net, tkjos@...gle.com,
        ulf.hansson@...aro.org, Saravana Kannan <saravanak@...gle.com>,
        kernel-team@...roid.com
Subject: Re: [PATCH v3 1/2] driver core: Rework logic in __driver_deferred_probe_check_state
 to allow EPROBE_DEFER to be returned for longer

Apologies in advance for replying to this one email but discussing the
points raised in all the other replies. I'm not cc'ed in this thread and
replying to each email individually is a pain.

On Tue, Feb 18, 2020 at 4:07 PM John Stultz wrote:
> Due to commit e01afc3250255 ("PM / Domains: Stop deferring probe
> at the end of initcall"), along with commit 25b4e70dcce9
> ("driver core: allow stopping deferred probe after init") after
> late_initcall, drivers will stop getting EPROBE_DEFER, and
> instead see an error causing the driver to fail to load.

Both of those patches were the best solution at that point in time. But
the kernel has changed a lot since then. Power domain and IOMMU drivers
can work as modules now. We have of_devlink and sync_state().

So, while a delay might have been the ideal solution back then, I think
we need to work towards removing arbitrary timeouts instead of making
the timeout mechanism more elaborate.

I think driver_deferred_probe_check_state() logic should boiled down
to something like:

int driver_deferred_probe_check_state(struct device *dev)
{
	/* No modules and init done, deferred probes are pointless from
	 * now. */
	if (!defined(CONFIG_MODULES) && initcall_done)
		return -ENODEV;

	/* If modules and of_devlink then you clean want dependencies to
	 * be enforced.
	 */
	if (defined(CONFIG_MODULES) && of_devlink)
		return -EPROBE_DEFER;

	/* Whatever other complexity (including timeouts) we want to
	 * add. Hopefully none - we can discuss in this thread. */
	if (.....)
		return -Exxxx;

	/* When in doubt, allow probe deferral. */
	return -EPROBE_DEFER;
}

Rob, for the original use case those two patches were added for, do they need
to support CONFIG_MODULES?

> That change causes trouble when trying to use many clk drivers
> as modules, as the clk modules may not load until much later
> after init has started. If a dependent driver loads and gets an
> error instead of EPROBE_DEFER, it won't try to reload later when
> the dependency is met, and will thus fail to load.

Once we add of_devlink support for power-domains, you won't even hit the
genpd error path if you have of_devlink enabled. I believe in the case
you are testing DB 845c, of_devlink is enabled?

If of_devlink is enabled, the devices depending on the unprobed power
domains would be deferred without even calling the driver's probe()
function.

Adding power-domain support to of_devlink is a 2 line change. I'll send
it out soon.

Also, regulator_init_complete() can be replaced by a sync_state() based
implementation. I have a downstream implementation that works. But it's
definitely not upstream ready. I plan to rewrite it and send it upstream
at some point, but it's fairly straightforward if anyone else want to
implement it. My main point being, we shouldn't have to make the timeout
logic more complex (or even need one) for regulator clean up either.

On Tue, Feb 18, 2020 at 6:07 PM Rob Herring wrote:
> The one complication which I mentioned already is with consoles. A
> timeout (and dependencies in modules) there doesn't work. You have to
> probe and register the console before init is done.

Rob,

I've seen you say this a couple of times before. But I don't think this
is true any more. With of_devlink enabled I've booted hardware countless
times with the console device probing after userspace comes up. The only
limitation for console drivers is that they need to be built-in if they
need to support earlycon. If you don't care to support earlycon (very
useful for bringup debugging), I think the console driver can even be a
module. I don't think even of_devlink needs to be enabled technically if
you load the modules in the right order.

Thanks,
Saravana