On Fri, May 25, 2018 at 7:20 AM, Robin Murphy [off-list ref] wrote:

On 24/05/18 21:57, Rob Herring wrote:

quoted

On Thu, May 24, 2018 at 2:00 PM, Greg Kroah-Hartman
[off-list ref] wrote:

quoted

On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote:

quoted

Deferred probe will currently wait forever on dependent devices to
probe,
but sometimes a driver will never exist. It's also not always critical
for
a driver to exist. Platforms can rely on default configuration from the
bootloader or reset defaults for things such as pinctrl and power
domains.
This is often the case with initial platform support until various
drivers
get enabled. There's at least 2 scenarios where deferred probe can
render
a platform broken. Both involve using a DT which has more devices and
dependencies than the kernel supports. The 1st case is a driver may be
disabled in the kernel config. The 2nd case is the kernel version may
simply not have the dependent driver. This can happen if using a newer
DT
(provided by firmware perhaps) with a stable kernel version.

Subsystems or drivers may opt-in to this behavior by calling
driver_deferred_probe_check_init_done() instead of just returning
-EPROBE_DEFER. They may use additional information from DT or kernel's
config to decide whether to continue to defer probe or not.

Cc: Alexander Graf <redacted>
Signed-off-by: Rob Herring <robh@kernel.org>
---
  drivers/base/dd.c      | 17 +++++++++++++++++
  include/linux/device.h |  2 ++
  2 files changed, 19 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index c9f54089429b..d6034718da6f 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c

@@ -226,6 +226,16 @@ void device_unblock_probing(void)
       driver_deferred_probe_trigger();
  }

+int driver_deferred_probe_check_init_done(struct device *dev, bool

optional)
+{
+     if (optional && initcalls_done) {

Wait, what's the "optional" mess here?

My intent was that subsystems just always call this function and never
return EPROBE_DEFER themselves. Then the driver core can make
decisions as to what to do (such as the timeout added in the next
patch). Or it can print common error/debug messages. So optional is a
hint to allow subsystems per device control.

Maybe just driver_defer_probe() might be a more descriptive name? To me,
calling "foo_check_x()" with a parameter that says "I don't actually care
about x" is the really unintuitive bit.

All the other (though static or internal to driver core) functions are
prefixed driver_deferred_probe_* so I was trying to remain consistent
there. You're right though, with the timeout it's not just whether
initcalls are done. It's really "get the return value depending on the
core's deferred probe state". So perhaps one of these:

driver_deferred_probe_get_return_val()
driver_deferred_probe_handle_return()

The other option would be a more straight-forward functions that just
returns a bool on whether to continue deferring and leave the return
code handling to the caller:

if (driver_deferred_probe_enabled_for_builtin(dev))
  return -EPROBE_DEFER;
else
  return -ENODEV;

The pinctrl case would look like this:

builtin_only = of_property_read_bool(np_pctldev, "pinctrl-use-default");
if (builtin_only && driver_deferred_probe_enabled_for_builtin(dev))
  return -EPROBE_DEFER;
else if (!builtin_only && driver_deferred_probe_enabled(dev))
  return -EPROBE_DEFER;
else
  return -ENODEV;

I still prefer the former, picking the bike shed color is easier with
the latter.

quoted

quoted

The caller knows this value, so why do you need to even pass it in here?

Because regardless of the value, we always stop deferring when/if we
hit the timeout and the caller doesn't know about the timeout. If we
get rid of it, we'd need functions for both init done and for deferred
timeout.

quoted

And bool values that are not obvious are horrid.  I had to go look this
up when reading the later patches that just passed "true" in this
variable as I had no idea what that meant.

Perhaps inverting it and calling it "keep_deferring" would be better.
However, the flag is ignored if we have timed out.

Perhaps an enum (or bitmask of named flags) then? That would allow the most
readability at callsites, plus it seems quite likely that we may want
intermediate degrees of "deferral strictness" eventually.

A bitmask is just 32 booleans stuffed into one parameter which I can
guess Greg's opinion on.

I can't really think of other flags we might need here. If we added
some userspace trigger saying module loading is done, I don't think
we'd need that to be per caller.

Rob