This series goal is to improve and cleanup the
driver_deferred_probe_check_state() code in the driver core.
This series is useful for being able to support modules
dependencies which may be loaded by userland, far after
late_initcall is done. For instance, this series allows us to
successfully use various clk drivers as modules on the db845c
board. And without it, those drivers have to be statically built
in to work.
Since I first sent out this patch, Saravana suggested an
alternative approach which also works for our needs, and is a
bit simpler:
https://lore.kernel.org/lkml/[email protected]/T/#u
However, while that patch provides the functionality we need,
I still suspect the driver_deferred_probe_check_state() code
could benefit from the cleanup in this patch, as the existing
logic is somewhat muddy.
New in v5:
* Reworked the driver_deferred_probe_check_state() logic as
suggested by Saravana to tie the initcall_done checking with
modules being enabled.
* Cleanup some comment wording as suggested by Rafael
* Try to slightly simplify the regulator logic as suggested by
Bjorn
Thanks so much to Bjorn, Saravana and Rafael for their reviews
and suggestions! Additional review and feedback is always greatly
appreciated!
thanks
-john
Cc: Rob Herring <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
John Stultz (6):
driver core: Fix driver_deferred_probe_check_state() logic
driver core: Set deferred_probe_timeout to a longer default if
CONFIG_MODULES is set
pinctrl: Remove use of driver_deferred_probe_check_state_continue()
driver core: Remove driver_deferred_probe_check_state_continue()
driver core: Rename deferred_probe_timeout and make it global
regulator: Use driver_deferred_probe_timeout for
regulator_init_complete_work
drivers/base/dd.c | 82 +++++++++++++----------------------
drivers/pinctrl/devicetree.c | 9 ++--
drivers/regulator/core.c | 25 ++++++-----
include/linux/device/driver.h | 2 +-
4 files changed, 49 insertions(+), 69 deletions(-)
--
2.17.1
driver_deferred_probe_check_state() has some uninituitive behavior.
* From boot to late_initcall, it returns -EPROBE_DEFER
* From late_initcall to the deferred_probe_timeout (if set)
it returns -ENODEV
* If the deferred_probe_timeout it set, after it fires, it
returns -ETIMEDOUT
This is a bit confusing, as its useful to have the function
return -EPROBE_DEFER while the timeout is still running. This
behavior has resulted in the somwhat duplicative
driver_deferred_probe_check_state_continue() function being
added.
Thus this patch tries to improve the logic, so that it behaves
as such:
* If late_initcall has passed, and modules are not enabled
it returns -ENODEV
* If modules are enabled and deferred_probe_timeout is set,
it returns -EPROBE_DEFER until the timeout, afterwhich it
returns -ETIMEDOUT.
* In all other cases, it returns -EPROBE_DEFER
This will make the deferred_probe_timeout value much more
functional, and will allow us to consolidate the
driver_deferred_probe_check_state() and
driver_deferred_probe_check_state_continue() logic in a later
patch.
Cc: Rob Herring <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
v4:
* Simplified logic suggested by Andy Shevchenko
* Clarified commit message to focus on logic change
v5:
* Cleanup comment wording as suggested by Rafael
* Tweaked the logic to use Saravana's suggested conditionals
---
drivers/base/dd.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index b25bcab2a26b..d75b34de6964 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -237,24 +237,26 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
static int __driver_deferred_probe_check_state(struct device *dev)
{
- if (!initcalls_done)
- return -EPROBE_DEFER;
+ if (!IS_ENABLED(CONFIG_MODULES) && initcalls_done)
+ return -ENODEV;
if (!deferred_probe_timeout) {
dev_WARN(dev, "deferred probe timeout, ignoring dependency");
return -ETIMEDOUT;
}
- return 0;
+ return -EPROBE_DEFER;
}
/**
* driver_deferred_probe_check_state() - Check deferred probe state
* @dev: device to check
*
- * Returns -ENODEV if init is done and all built-in drivers have had a chance
- * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
- * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
+ * Return:
+ * -ENODEV if initcalls have completed and modules are disabled.
+ * -ETIMEDOUT if the deferred probe timeout was set and has expired
+ * and modules are enabled.
+ * -EPROBE_DEFER in other cases.
*
* Drivers or subsystems can opt-in to calling this function instead of directly
* returning -EPROBE_DEFER.
@@ -264,7 +266,7 @@ int driver_deferred_probe_check_state(struct device *dev)
int ret;
ret = __driver_deferred_probe_check_state(dev);
- if (ret < 0)
+ if (ret != -ENODEV)
return ret;
dev_warn(dev, "ignoring dependency for device, assuming no driver");
@@ -292,7 +294,7 @@ int driver_deferred_probe_check_state_continue(struct device *dev)
int ret;
ret = __driver_deferred_probe_check_state(dev);
- if (ret < 0)
+ if (ret != -ENODEV)
return ret;
return -EPROBE_DEFER;
--
2.17.1
When using modules, its common for the modules not to be loaded
until quite late by userland. With the current code,
driver_deferred_probe_check_state() will stop returning
EPROBE_DEFER after late_initcall, which can cause module
dependency resolution to fail after that.
So allow a longer window of 30 seconds (picked somewhat
arbitrarily, but influenced by the similar regulator core
timeout value) in the case where modules are enabled.
Cc: Rob Herring <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Reviewed-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
v4:
* Split out into its own patch as suggested by Mark
* Made change conditional on CONFIG_MODULES
---
drivers/base/dd.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index d75b34de6964..fe26f2574a6d 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -224,7 +224,16 @@ static int deferred_devs_show(struct seq_file *s, void *data)
}
DEFINE_SHOW_ATTRIBUTE(deferred_devs);
+#ifdef CONFIG_MODULES
+/*
+ * In the case of modules, set the default probe timeout to
+ * 30 seconds to give userland some time to load needed modules
+ */
+static int deferred_probe_timeout = 30;
+#else
+/* In the case of !modules, no probe timeout needed */
static int deferred_probe_timeout = -1;
+#endif
static int __init deferred_probe_timeout_setup(char *str)
{
int timeout;
--
2.17.1
With the earlier sanity fixes to
driver_deferred_probe_check_state() it should be usable for the
pinctrl logic here.
So tweak the logic to use driver_deferred_probe_check_state()
instead of driver_deferred_probe_check_state_continue()
Cc: Rob Herring <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Acked-by: Linus Walleij <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
drivers/pinctrl/devicetree.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index 9357f7c46cf3..1ed20ac2243f 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -127,11 +127,12 @@ static int dt_to_map_one_config(struct pinctrl *p,
np_pctldev = of_get_next_parent(np_pctldev);
if (!np_pctldev || of_node_is_root(np_pctldev)) {
of_node_put(np_pctldev);
+ ret = driver_deferred_probe_check_state(p->dev);
/* keep deferring if modules are enabled unless we've timed out */
- if (IS_ENABLED(CONFIG_MODULES) && !allow_default)
- return driver_deferred_probe_check_state_continue(p->dev);
-
- return driver_deferred_probe_check_state(p->dev);
+ if (IS_ENABLED(CONFIG_MODULES) && !allow_default &&
+ (ret == -ENODEV))
+ ret = -EPROBE_DEFER;
+ return ret;
}
/* If we're creating a hog we can use the passed pctldev */
if (hog_pctldev && (np_pctldev == p->dev->of_node)) {
--
2.17.1
Now that driver_deferred_probe_check_state() works better, and
we've converted the only user of
driver_deferred_probe_check_state_continue() we can simply
remove it and simplify some of the logic.
Cc: Rob Herring <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Reviewed-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
drivers/base/dd.c | 53 ++++++-----------------------------
include/linux/device/driver.h | 1 -
2 files changed, 8 insertions(+), 46 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index fe26f2574a6d..c09e4e7277d4 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -244,19 +244,6 @@ static int __init deferred_probe_timeout_setup(char *str)
}
__setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
-static int __driver_deferred_probe_check_state(struct device *dev)
-{
- if (!IS_ENABLED(CONFIG_MODULES) && initcalls_done)
- return -ENODEV;
-
- if (!deferred_probe_timeout) {
- dev_WARN(dev, "deferred probe timeout, ignoring dependency");
- return -ETIMEDOUT;
- }
-
- return -EPROBE_DEFER;
-}
-
/**
* driver_deferred_probe_check_state() - Check deferred probe state
* @dev: device to check
@@ -272,39 +259,15 @@ static int __driver_deferred_probe_check_state(struct device *dev)
*/
int driver_deferred_probe_check_state(struct device *dev)
{
- int ret;
-
- ret = __driver_deferred_probe_check_state(dev);
- if (ret != -ENODEV)
- return ret;
-
- dev_warn(dev, "ignoring dependency for device, assuming no driver");
-
- return -ENODEV;
-}
-
-/**
- * driver_deferred_probe_check_state_continue() - check deferred probe state
- * @dev: device to check
- *
- * Returns -ETIMEDOUT if deferred probe debug timeout has expired, or
- * -EPROBE_DEFER otherwise.
- *
- * Drivers or subsystems can opt-in to calling this function instead of
- * directly returning -EPROBE_DEFER.
- *
- * This is similar to driver_deferred_probe_check_state(), but it allows the
- * subsystem to keep deferring probe after built-in drivers have had a chance
- * to probe. One scenario where that is useful is if built-in drivers rely on
- * resources that are provided by modular drivers.
- */
-int driver_deferred_probe_check_state_continue(struct device *dev)
-{
- int ret;
+ if (!IS_ENABLED(CONFIG_MODULES) && initcalls_done) {
+ dev_warn(dev, "ignoring dependency for device, assuming no driver");
+ return -ENODEV;
+ }
- ret = __driver_deferred_probe_check_state(dev);
- if (ret != -ENODEV)
- return ret;
+ if (!deferred_probe_timeout) {
+ dev_WARN(dev, "deferred probe timeout, ignoring dependency");
+ return -ETIMEDOUT;
+ }
return -EPROBE_DEFER;
}
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index 1188260f9a02..5242afabfaba 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -238,7 +238,6 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
void driver_deferred_probe_add(struct device *dev);
int driver_deferred_probe_check_state(struct device *dev);
-int driver_deferred_probe_check_state_continue(struct device *dev);
void driver_init(void);
/**
--
2.17.1
The regulator_init_complete_work logic defers the cleanup for an
arbitrary 30 seconds of time to allow modules loaded by userland
to start.
This arbitrary timeout is similar to the
driver_deferred_probe_timeout value, and its been suggested we
align these so users have a method to extend the timeouts as
needed.
So this patch changes the logic to use the
driver_deferred_probe_timeout value for the delay value if it
is set (using a delay of 0 if it is not).
Cc: Rob Herring <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
v4:
* Split out into its own patch, as suggested by Mark
v5:
* Try to simplify the logic a touch as suggested by Bjorn
---
drivers/regulator/core.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d015d99cb59d..51b6a2dea717 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5757,6 +5757,10 @@ static DECLARE_DELAYED_WORK(regulator_init_complete_work,
static int __init regulator_init_complete(void)
{
+ int delay = driver_deferred_probe_timeout;
+
+ if (delay < 0)
+ delay = 0;
/*
* Since DT doesn't provide an idiomatic mechanism for
* enabling full constraints and since it's much more natural
@@ -5767,18 +5771,17 @@ static int __init regulator_init_complete(void)
has_full_constraints = true;
/*
- * We punt completion for an arbitrary amount of time since
- * systems like distros will load many drivers from userspace
- * so consumers might not always be ready yet, this is
- * particularly an issue with laptops where this might bounce
- * the display off then on. Ideally we'd get a notification
- * from userspace when this happens but we don't so just wait
- * a bit and hope we waited long enough. It'd be better if
- * we'd only do this on systems that need it, and a kernel
- * command line option might be useful.
+ * If driver_deferred_probe_timeout is set, we punt
+ * completion for that many seconds since systems like
+ * distros will load many drivers from userspace so consumers
+ * might not always be ready yet, this is particularly an
+ * issue with laptops where this might bounce the display off
+ * then on. Ideally we'd get a notification from userspace
+ * when this happens but we don't so just wait a bit and hope
+ * we waited long enough. It'd be better if we'd only do
+ * this on systems that need it.
*/
- schedule_delayed_work(®ulator_init_complete_work,
- msecs_to_jiffies(30000));
+ schedule_delayed_work(®ulator_init_complete_work, delay * HZ);
return 0;
}
--
2.17.1
Since other subsystems (like regulator) have similar arbitrary
timeouts for how long they try to resolve driver dependencies,
rename deferred_probe_timeout to driver_deferred_probe_timeout
and set it as global, so it can be shared.
Cc: Rob Herring <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Reviewed-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
v4:
* Split out into its own patch as suggested by Mark
* Renamed deferred_probe_timeout as suggested by Greg
---
drivers/base/dd.c | 16 +++++++++-------
include/linux/device/driver.h | 1 +
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index c09e4e7277d4..76888a7459d8 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -229,17 +229,19 @@ DEFINE_SHOW_ATTRIBUTE(deferred_devs);
* In the case of modules, set the default probe timeout to
* 30 seconds to give userland some time to load needed modules
*/
-static int deferred_probe_timeout = 30;
+int driver_deferred_probe_timeout = 30;
#else
/* In the case of !modules, no probe timeout needed */
-static int deferred_probe_timeout = -1;
+int driver_deferred_probe_timeout = -1;
#endif
+EXPORT_SYMBOL_GPL(driver_deferred_probe_timeout);
+
static int __init deferred_probe_timeout_setup(char *str)
{
int timeout;
if (!kstrtoint(str, 10, &timeout))
- deferred_probe_timeout = timeout;
+ driver_deferred_probe_timeout = timeout;
return 1;
}
__setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
@@ -264,7 +266,7 @@ int driver_deferred_probe_check_state(struct device *dev)
return -ENODEV;
}
- if (!deferred_probe_timeout) {
+ if (!driver_deferred_probe_timeout) {
dev_WARN(dev, "deferred probe timeout, ignoring dependency");
return -ETIMEDOUT;
}
@@ -276,7 +278,7 @@ static void deferred_probe_timeout_work_func(struct work_struct *work)
{
struct device_private *private, *p;
- deferred_probe_timeout = 0;
+ driver_deferred_probe_timeout = 0;
driver_deferred_probe_trigger();
flush_work(&deferred_probe_work);
@@ -310,9 +312,9 @@ static int deferred_probe_initcall(void)
driver_deferred_probe_trigger();
flush_work(&deferred_probe_work);
- if (deferred_probe_timeout > 0) {
+ if (driver_deferred_probe_timeout > 0) {
schedule_delayed_work(&deferred_probe_timeout_work,
- deferred_probe_timeout * HZ);
+ driver_deferred_probe_timeout * HZ);
}
return 0;
}
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index 5242afabfaba..ee7ba5b5417e 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
}
#endif
+extern int driver_deferred_probe_timeout;
void driver_deferred_probe_add(struct device *dev);
int driver_deferred_probe_check_state(struct device *dev);
void driver_init(void);
--
2.17.1
On Tue, Feb 25, 2020 at 05:08:28AM +0000, John Stultz wrote:
> The regulator_init_complete_work logic defers the cleanup for an
> arbitrary 30 seconds of time to allow modules loaded by userland
> to start.
Acked-by: Mark Brown <[email protected]>
On Tue, Feb 25, 2020 at 6:08 AM John Stultz <[email protected]> wrote:
>
> driver_deferred_probe_check_state() has some uninituitive behavior.
>
> * From boot to late_initcall, it returns -EPROBE_DEFER
>
> * From late_initcall to the deferred_probe_timeout (if set)
> it returns -ENODEV
>
> * If the deferred_probe_timeout it set, after it fires, it
> returns -ETIMEDOUT
>
> This is a bit confusing, as its useful to have the function
> return -EPROBE_DEFER while the timeout is still running. This
> behavior has resulted in the somwhat duplicative
> driver_deferred_probe_check_state_continue() function being
> added.
>
> Thus this patch tries to improve the logic, so that it behaves
> as such:
>
> * If late_initcall has passed, and modules are not enabled
> it returns -ENODEV
>
> * If modules are enabled and deferred_probe_timeout is set,
> it returns -EPROBE_DEFER until the timeout, afterwhich it
> returns -ETIMEDOUT.
>
> * In all other cases, it returns -EPROBE_DEFER
>
> This will make the deferred_probe_timeout value much more
> functional, and will allow us to consolidate the
> driver_deferred_probe_check_state() and
> driver_deferred_probe_check_state_continue() logic in a later
> patch.
>
> Cc: Rob Herring <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
> ---
> v4:
> * Simplified logic suggested by Andy Shevchenko
> * Clarified commit message to focus on logic change
> v5:
> * Cleanup comment wording as suggested by Rafael
> * Tweaked the logic to use Saravana's suggested conditionals
> ---
> drivers/base/dd.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index b25bcab2a26b..d75b34de6964 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -237,24 +237,26 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>
> static int __driver_deferred_probe_check_state(struct device *dev)
> {
> - if (!initcalls_done)
> - return -EPROBE_DEFER;
> + if (!IS_ENABLED(CONFIG_MODULES) && initcalls_done)
> + return -ENODEV;
>
> if (!deferred_probe_timeout) {
> dev_WARN(dev, "deferred probe timeout, ignoring dependency");
> return -ETIMEDOUT;
> }
>
> - return 0;
> + return -EPROBE_DEFER;
> }
>
> /**
> * driver_deferred_probe_check_state() - Check deferred probe state
> * @dev: device to check
> *
> - * Returns -ENODEV if init is done and all built-in drivers have had a chance
> - * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
> - * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
> + * Return:
> + * -ENODEV if initcalls have completed and modules are disabled.
> + * -ETIMEDOUT if the deferred probe timeout was set and has expired
> + * and modules are enabled.
> + * -EPROBE_DEFER in other cases.
> *
> * Drivers or subsystems can opt-in to calling this function instead of directly
> * returning -EPROBE_DEFER.
> @@ -264,7 +266,7 @@ int driver_deferred_probe_check_state(struct device *dev)
> int ret;
>
> ret = __driver_deferred_probe_check_state(dev);
> - if (ret < 0)
> + if (ret != -ENODEV)
> return ret;
>
> dev_warn(dev, "ignoring dependency for device, assuming no driver");
> @@ -292,7 +294,7 @@ int driver_deferred_probe_check_state_continue(struct device *dev)
> int ret;
>
> ret = __driver_deferred_probe_check_state(dev);
> - if (ret < 0)
> + if (ret != -ENODEV)
> return ret;
>
> return -EPROBE_DEFER;
> --
> 2.17.1
>
Sending again because of accidental HTML email.
On Mon, Feb 24, 2020 at 9:08 PM John Stultz <[email protected]> wrote:
>
> With the earlier sanity fixes to
> driver_deferred_probe_check_state() it should be usable for the
> pinctrl logic here.
>
> So tweak the logic to use driver_deferred_probe_check_state()
> instead of driver_deferred_probe_check_state_continue()
>
> Cc: Rob Herring <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Acked-by: Linus Walleij <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> drivers/pinctrl/devicetree.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> index 9357f7c46cf3..1ed20ac2243f 100644
> --- a/drivers/pinctrl/devicetree.c
> +++ b/drivers/pinctrl/devicetree.c
> @@ -127,11 +127,12 @@ static int dt_to_map_one_config(struct pinctrl *p,
> np_pctldev = of_get_next_parent(np_pctldev);
> if (!np_pctldev || of_node_is_root(np_pctldev)) {
> of_node_put(np_pctldev);
> + ret = driver_deferred_probe_check_state(p->dev);
> /* keep deferring if modules are enabled unless we've timed out */
> - if (IS_ENABLED(CONFIG_MODULES) && !allow_default)
> - return driver_deferred_probe_check_state_continue(p->dev);
> -
> - return driver_deferred_probe_check_state(p->dev);
> + if (IS_ENABLED(CONFIG_MODULES) && !allow_default &&
Is this IS_ENABLED(CONFIG_MODULES) still necessary? At the end of this
series, doesn't driver_deferred_probe_check_state() already return
-EPROBE_DEFER if modules are enabled and timeout hasn't happened?
-Saravana
> + (ret == -ENODEV))
> + ret = -EPROBE_DEFER;
> + return ret;
> }
> /* If we're creating a hog we can use the passed pctldev */
> if (hog_pctldev && (np_pctldev == p->dev->of_node)) {
> --
> 2.17.1
>
On Tue, Feb 25, 2020 at 6:11 PM Saravana Kannan <[email protected]> wrote:
> On Mon, Feb 24, 2020 at 9:08 PM John Stultz <[email protected]> wrote:
> >
> > With the earlier sanity fixes to
> > driver_deferred_probe_check_state() it should be usable for the
> > pinctrl logic here.
> >
> > So tweak the logic to use driver_deferred_probe_check_state()
> > instead of driver_deferred_probe_check_state_continue()
> >
> > Cc: Rob Herring <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Kevin Hilman <[email protected]>
> > Cc: Ulf Hansson <[email protected]>
> > Cc: Pavel Machek <[email protected]>
> > Cc: Len Brown <[email protected]>
> > Cc: Todd Kjos <[email protected]>
> > Cc: Saravana Kannan <[email protected]>
> > Cc: Bjorn Andersson <[email protected]>
> > Cc: Liam Girdwood <[email protected]>
> > Cc: Mark Brown <[email protected]>
> > Cc: Thierry Reding <[email protected]>
> > Cc: Linus Walleij <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: [email protected]
> > Acked-by: Linus Walleij <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > drivers/pinctrl/devicetree.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
> > index 9357f7c46cf3..1ed20ac2243f 100644
> > --- a/drivers/pinctrl/devicetree.c
> > +++ b/drivers/pinctrl/devicetree.c
> > @@ -127,11 +127,12 @@ static int dt_to_map_one_config(struct pinctrl *p,
> > np_pctldev = of_get_next_parent(np_pctldev);
> > if (!np_pctldev || of_node_is_root(np_pctldev)) {
> > of_node_put(np_pctldev);
> > + ret = driver_deferred_probe_check_state(p->dev);
> > /* keep deferring if modules are enabled unless we've timed out */
> > - if (IS_ENABLED(CONFIG_MODULES) && !allow_default)
> > - return driver_deferred_probe_check_state_continue(p->dev);
> > -
> > - return driver_deferred_probe_check_state(p->dev);
> > + if (IS_ENABLED(CONFIG_MODULES) && !allow_default &&
>
> Is this IS_ENABLED(CONFIG_MODULES) still necessary? At the end of this
> series, doesn't driver_deferred_probe_check_state() already return
> -EPROBE_DEFER if modules are enabled and timeout hasn't happened?
>
Yea, good point. With the reworked logic in v5, the
IS_ENABLED(CONFIG_MODULES) check could go.
thanks
-john
On Tue, Feb 25, 2020 at 05:08:22AM +0000, John Stultz wrote:
> This series goal is to improve and cleanup the
> driver_deferred_probe_check_state() code in the driver core.
>
> This series is useful for being able to support modules
> dependencies which may be loaded by userland, far after
> late_initcall is done. For instance, this series allows us to
> successfully use various clk drivers as modules on the db845c
> board. And without it, those drivers have to be statically built
> in to work.
>
> Since I first sent out this patch, Saravana suggested an
> alternative approach which also works for our needs, and is a
> bit simpler:
> https://lore.kernel.org/lkml/[email protected]/T/#u
>
> However, while that patch provides the functionality we need,
> I still suspect the driver_deferred_probe_check_state() code
> could benefit from the cleanup in this patch, as the existing
> logic is somewhat muddy.
This looks much better, thanks for sticking with it, all now queued up.
greg k-h
Hi John,
Cc: linux-renesas-soc
On Tue, Feb 25, 2020 at 05:08:22AM +0000, John Stultz wrote:
> This series goal is to improve and cleanup the
> driver_deferred_probe_check_state() code in the driver core.
>
> This series is useful for being able to support modules
> dependencies which may be loaded by userland, far after
> late_initcall is done. For instance, this series allows us to
> successfully use various clk drivers as modules on the db845c
> board. And without it, those drivers have to be statically built
> in to work.
>
> Since I first sent out this patch, Saravana suggested an
> alternative approach which also works for our needs, and is a
> bit simpler:
> https://lore.kernel.org/lkml/[email protected]/T/#u
>
> However, while that patch provides the functionality we need,
> I still suspect the driver_deferred_probe_check_state() code
> could benefit from the cleanup in this patch, as the existing
> logic is somewhat muddy.
>
> New in v5:
> * Reworked the driver_deferred_probe_check_state() logic as
> suggested by Saravana to tie the initcall_done checking with
> modules being enabled.
> * Cleanup some comment wording as suggested by Rafael
> * Try to slightly simplify the regulator logic as suggested by
> Bjorn
>
> Thanks so much to Bjorn, Saravana and Rafael for their reviews
> and suggestions! Additional review and feedback is always greatly
> appreciated!
Building a recent [0] kernel using vanilla arm64 defconfig
and booting it on H3ULCB, I get buried into backtraces [1].
After reverting this series, up to and including its first commit,
booting goes back to normal [2].
Any chance to get a fix or at least some hints where to dig into?
[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=18bf34080c4c3b
("Merge branch 'akpm' (patches from Andrew)")
[1] https://gist.github.com/erosca/ac779c348dd272c448e162c406c48f4a
[2] https://gist.github.com/erosca/5eea2bc5e82be651d405ba038d0ad036
--
Best regards,
Eugeniu Rosca
On Tue, Apr 21, 2020 at 4:59 PM Eugeniu Rosca <[email protected]> wrote:
>
> Hi John,
> Cc: linux-renesas-soc
>
> On Tue, Feb 25, 2020 at 05:08:22AM +0000, John Stultz wrote:
> > This series goal is to improve and cleanup the
> > driver_deferred_probe_check_state() code in the driver core.
> >
> > This series is useful for being able to support modules
> > dependencies which may be loaded by userland, far after
> > late_initcall is done. For instance, this series allows us to
> > successfully use various clk drivers as modules on the db845c
> > board. And without it, those drivers have to be statically built
> > in to work.
> >
> > Since I first sent out this patch, Saravana suggested an
> > alternative approach which also works for our needs, and is a
> > bit simpler:
> > https://lore.kernel.org/lkml/[email protected]/T/#u
> >
> > However, while that patch provides the functionality we need,
> > I still suspect the driver_deferred_probe_check_state() code
> > could benefit from the cleanup in this patch, as the existing
> > logic is somewhat muddy.
> >
> > New in v5:
> > * Reworked the driver_deferred_probe_check_state() logic as
> > suggested by Saravana to tie the initcall_done checking with
> > modules being enabled.
> > * Cleanup some comment wording as suggested by Rafael
> > * Try to slightly simplify the regulator logic as suggested by
> > Bjorn
> >
> > Thanks so much to Bjorn, Saravana and Rafael for their reviews
> > and suggestions! Additional review and feedback is always greatly
> > appreciated!
>
> Building a recent [0] kernel using vanilla arm64 defconfig
> and booting it on H3ULCB, I get buried into backtraces [1].
>
> After reverting this series, up to and including its first commit,
> booting goes back to normal [2].
>
> Any chance to get a fix or at least some hints where to dig into?
Yea. There's two patch sets I have for this. The first quiets down the
warnings(we don't need stack dumps for these):
https://lore.kernel.org/lkml/[email protected]/
The second reverts the default timeout back to 0:
https://lore.kernel.org/lkml/[email protected]/
Let me know if those work for you, or if you're still having trouble
afterwards. I need to resubmit the set as I'm guessing they've been
overlooked.
thanks
-john
Hi John,
On Tue, Apr 21, 2020 at 06:16:31PM -0700, John Stultz wrote:
> On Tue, Apr 21, 2020 at 4:59 PM Eugeniu Rosca <[email protected]> wrote:
> >
> > Building a recent [0] kernel using vanilla arm64 defconfig
> > and booting it on H3ULCB, I get buried into backtraces [1].
> >
> > After reverting this series, up to and including its first commit,
> > booting goes back to normal [2].
> >
> > Any chance to get a fix or at least some hints where to dig into?
>
> Yea. There's two patch sets I have for this. The first quiets down the
> warnings(we don't need stack dumps for these):
> https://lore.kernel.org/lkml/[email protected]/
>
> The second reverts the default timeout back to 0:
> https://lore.kernel.org/lkml/[email protected]/
>
>
> Let me know if those work for you, or if you're still having trouble
> afterwards. I need to resubmit the set as I'm guessing they've been
> overlooked.
The patches fix the issue on my end. Thanks!
One note is that they carry a slight mutual conflict, but that's up to
the maintainer to complain about.
Many thanks again!
Hope the patches will be picked up soon!
--
Best regards,
Eugeniu Rosca
On Tue, Apr 21, 2020 at 06:16:31PM -0700, John Stultz wrote:
> The second reverts the default timeout back to 0:
> https://lore.kernel.org/lkml/[email protected]/
If you're reverting the timeout we should revert the regulator change
too I think.
On Wed, Apr 22, 2020 at 12:54 AM Mark Brown <[email protected]> wrote:
> On Tue, Apr 21, 2020 at 06:16:31PM -0700, John Stultz wrote:
>
> > The second reverts the default timeout back to 0:
> > https://lore.kernel.org/lkml/[email protected]/
>
> If you're reverting the timeout we should revert the regulator change
> too I think.
Maybe? The main issue for me was my change was clearly breaking users
with dts with missing dependencies where their setup was working
before. I sort of feel like having a dtb with missing dependencies is
less valid than wanting to load module dependencies from userland, but
they were working first, so we have to keep them happy :) And at least
now the latter can add the timeout boot argument to make it work.
For your case, I'm not sure if the timeout would run afoul on the nfs
root mounting case this one tripped over.
thanks
-john
Hi John,
On Wed, Apr 22, 2020 at 10:46 PM John Stultz <[email protected]> wrote:
> On Wed, Apr 22, 2020 at 12:54 AM Mark Brown <[email protected]> wrote:
> > On Tue, Apr 21, 2020 at 06:16:31PM -0700, John Stultz wrote:
> > > The second reverts the default timeout back to 0:
> > > https://lore.kernel.org/lkml/[email protected]/
> >
> > If you're reverting the timeout we should revert the regulator change
> > too I think.
>
> Maybe? The main issue for me was my change was clearly breaking users
> with dts with missing dependencies where their setup was working
> before. I sort of feel like having a dtb with missing dependencies is
> less valid than wanting to load module dependencies from userland, but
> they were working first, so we have to keep them happy :) And at least
> now the latter can add the timeout boot argument to make it work.
IOMMU support is optional.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Apr 22, 2020 at 01:45:49PM -0700, John Stultz wrote:
> On Wed, Apr 22, 2020 at 12:54 AM Mark Brown <[email protected]> wrote:
> > If you're reverting the timeout we should revert the regulator change
> > too I think.
> Maybe? The main issue for me was my change was clearly breaking users
> with dts with missing dependencies where their setup was working
> before. I sort of feel like having a dtb with missing dependencies is
> less valid than wanting to load module dependencies from userland, but
> they were working first, so we have to keep them happy :) And at least
> now the latter can add the timeout boot argument to make it work.
> For your case, I'm not sure if the timeout would run afoul on the nfs
> root mounting case this one tripped over.
Given that it's basically entirely about glitching displays rather than
an unrecoverable break I suspect that anyone using NFS root is some
combination of unaffected or doesn't care if they see the timeout kick
in.