| Message ID | 20180524175024.19874-2-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/8] driver core: make deferring probe after init optional | expand |
On Thu, May 24, 2018 at 1:56 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote: >> 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 <agraf@suse.de> >> 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) { >> + dev_WARN(dev, "ignoring dependency for device, assuming no driver"); > > You really only need dev_warn(), here, right? No, the screaming is on purpose. Bjorn had concerns about debugging/supporting subtle problems that could stem from this. Such as electrical settings not quite right that cause intermittent errors. Rob
On Fri, May 25, 2018 at 7:20 AM, Robin Murphy <robin.murphy@arm.com> wrote: > On 24/05/18 21:57, Rob Herring wrote: >> >> On Thu, May 24, 2018 at 2:00 PM, Greg Kroah-Hartman >> <gregkh@linuxfoundation.org> wrote: >>> >>> On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote: >>>> >>>> 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 <agraf@suse.de> >>>> 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. >>> >>> 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. >> >>> 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
On Tue, May 29, 2018 at 12:12 AM, Frank Rowand <frowand.list@gmail.com> wrote: > On 05/24/18 11:18, Mark Brown wrote: >> On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote: >> >>> 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. >> >> Should userspace have some involvement in this decision? It knows if >> it's got any intention of loading modules for example. Kernel config >> checks might be good enough, though it's going to be a pain to work out >> if the relevant driver is built as a module for example. >> > > A parallel issue is that loading an overlay could provide the resource > that will allow the deferred probe to complete. (That is, once we > finish implementing the run time overlays feature.) I'd like to see an actual example where that could happen. I agree you could craft it, but would it really be a valid partitioning? For example, SoC pinctrl, iommu, or power domains defined in an overlay would not be something valid to apply during kernel boot or after boot (though putting those into overlays is exactly what Alex wants to do). Rob
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) { + dev_WARN(dev, "ignoring dependency for device, assuming no driver"); + return -ENODEV; + } + + return -EPROBE_DEFER; +} + /** * deferred_probe_initcall() - Enable probing of deferred devices * @@ -240,6 +250,13 @@ static int deferred_probe_initcall(void) /* Sort as many dependencies as possible before exiting initcalls */ flush_work(&deferred_probe_work); initcalls_done = true; + + /* + * Trigger deferred probe again, this time we won't defer anything + * that is optional + */ + driver_deferred_probe_trigger(); + flush_work(&deferred_probe_work); return 0; } late_initcall(deferred_probe_initcall); diff --git a/include/linux/device.h b/include/linux/device.h index 477956990f5e..f3dafd44c285 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -334,6 +334,8 @@ struct device *driver_find_device(struct device_driver *drv, struct device *start, void *data, int (*match)(struct device *dev, void *data)); +int driver_deferred_probe_check_init_done(struct device *dev, bool optional); + /** * struct subsys_interface - interfaces to device functions * @name: name of the device function
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 <agraf@suse.de> Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/base/dd.c | 17 +++++++++++++++++ include/linux/device.h | 2 ++ 2 files changed, 19 insertions(+) -- 2.17.0