[PATCH v4 0/6] driver core: Try to improve and cleanup driver_deferred_probe_check_state()

[PATCH v4 0/6] driver core: Try to improve and cleanup driver_deferred_probe_check_state()

[John Stultz]

This series tries to improve and cleanup the
driver_deferred_probe_check_state() code in the driver core.

This is most important 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 use successfully various clk drivers as modules on
the db845c board. And without it, those drivers have to
be statically built in to work.

The first patch (or two) is really the most critical for me,
but as I was working to understand the code (and with some
prodding), it seemed a further cleanup was in order.

If folks have concerns about the tail end of the patch set, I'm
fine to defer that. The main issue for me is the inability to
load module dependencies after init starts. 

New in v4:
* Split earlier version of the patches apart
* Defaulting deferred_probe_timeout to 30 seconds if modules
  are enabled.
* Deeper cleanup on driver_deferred_probe_check_state_continue()

Feedback would be greatly appreciated!

thanks
-john

Cc: Rob Herring <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org

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             | 78 ++++++++++++-----------------------
 drivers/pinctrl/devicetree.c  |  9 ++--
 drivers/regulator/core.c      | 25 ++++++-----
 include/linux/device/driver.h |  2 +-
 4 files changed, 47 insertions(+), 67 deletions(-)

-- 
2.17.1

[PATCH v4 1/6] driver core: Fix driver_deferred_probe_check_state() logic

[John Stultz]

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 deferred_probe_timeout is set, it returns -EPROBE_DEFER
  until the timeout, afterwhich it returns -ETIMEDOUT.

* If deferred_probe_timeout is not set (-1), it returns
  -EPROBE_DEFER until late_initcall, after which it returns

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 <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
Change-Id: I8349b7a403ce8cbce485ea0a0a5512fddffb635c
---
v4:
* Simplified logic suggested by Andy Shevchenko
* Clarified commit message to focus on logic change
---
 drivers/base/dd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index b25bcab2a26b..bb383dca39c1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -237,7 +237,7 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
 
 static int __driver_deferred_probe_check_state(struct device *dev)
 {
-	if (!initcalls_done)
+	if (!initcalls_done || deferred_probe_timeout > 0)
 		return -EPROBE_DEFER;
 
 	if (!deferred_probe_timeout) {
@@ -252,9 +252,11 @@ static int __driver_deferred_probe_check_state(struct device *dev)
  * 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.
+ * Returnes -EPROBE_DEFER if initcalls have not completed, or the deferred
+ * probe timeout is set, but not expried.
+ * Returns -ETIMEDOUT if the probe timeout was set and has expired.
+ * Returns -ENODEV if initcalls have completed and the deferred probe timeout
+ * was not set.
  *
  * Drivers or subsystems can opt-in to calling this function instead of directly
  * returning -EPROBE_DEFER.
-- 
2.17.1

[PATCH v4 2/6] driver core: Set deferred_probe_timeout to a longer default if CONFIG_MODULES is set

[John Stultz]

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 <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
Change-Id: I9c5a02a54915ff53f9f14d49c601f41d7105e05e
---
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 bb383dca39c1..fa138f24e2d3 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

[PATCH v4 3/6] pinctrl: Remove use of driver_deferred_probe_check_state_continue()

[John Stultz]

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 <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
Change-Id: If72682e0a7641b33edf56f188fc067c68bbc571e
---
 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

[PATCH v4 4/6] driver core: Remove driver_deferred_probe_check_state_continue()

[John Stultz]

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 <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
Change-Id: Id5cd5e9264cfb0fbd70a702715174cc4b10006f4
---
 drivers/base/dd.c             | 49 +++++------------------------------
 include/linux/device/driver.h |  1 -
 2 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index fa138f24e2d3..408e4da081da 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 (!initcalls_done || deferred_probe_timeout > 0)
-		return -EPROBE_DEFER;
-
-	if (!deferred_probe_timeout) {
-		dev_WARN(dev, "deferred probe timeout, ignoring dependency");
-		return -ETIMEDOUT;
-	}
-
-	return 0;
-}
-
 /**
  * driver_deferred_probe_check_state() - Check deferred probe state
  * @dev: device to check
@@ -272,43 +259,19 @@ static int __driver_deferred_probe_check_state(struct device *dev)
  */
 int driver_deferred_probe_check_state(struct device *dev)
 {
-	int ret;
+	if (!initcalls_done || deferred_probe_timeout > 0)
+		return -EPROBE_DEFER;
 
-	ret = __driver_deferred_probe_check_state(dev);
-	if (ret < 0)
-		return ret;
+	if (!deferred_probe_timeout) {
+		dev_WARN(dev, "deferred probe timeout, ignoring dependency");
+		return -ETIMEDOUT;
+	}
 
 	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;
-
-	ret = __driver_deferred_probe_check_state(dev);
-	if (ret < 0)
-		return ret;
-
-	return -EPROBE_DEFER;
-}
-
 static void deferred_probe_timeout_work_func(struct work_struct *work)
 {
 	struct device_private *private, *p;
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

[PATCH v4 5/6] driver core: Rename deferred_probe_timeout and make it global

[John Stultz]

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 <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
Change-Id: I92ee3b392004ecc9217c5337b54eda48c2d7f3ee
---
v4:
* Split out into its own patch as suggested by Mark
* Renamed deferred_probe_timeout as suggested by Greg
---
 drivers/base/dd.c             | 18 ++++++++++--------
 include/linux/device/driver.h |  1 +
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 408e4da081da..39f1ce6d4f1c 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);
@@ -259,10 +261,10 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
  */
 int driver_deferred_probe_check_state(struct device *dev)
 {
-	if (!initcalls_done || deferred_probe_timeout > 0)
+	if (!initcalls_done || driver_deferred_probe_timeout > 0)
 		return -EPROBE_DEFER;
 
-	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

[PATCH v4 6/6] regulator: Use driver_deferred_probe_timeout for regulator_init_complete_work

[John Stultz]

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 if it is set, otherwise we
directly call the regulator_init_complete_work_function().

Cc: Rob Herring <robh@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Len Brown <len.brown@intel.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
Change-Id: I9fa2411abbb91ed4dd0edc41e8cc8583577c005b
---
v4:
* Split out into its own patch, as suggested by Mark
---
 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..394e7b11576a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5767,18 +5767,21 @@ 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(&regulator_init_complete_work,
-			      msecs_to_jiffies(30000));
+	if (driver_deferred_probe_timeout >= 0)
+		schedule_delayed_work(&regulator_init_complete_work,
+				      driver_deferred_probe_timeout * HZ);
+	else
+		regulator_init_complete_work_function(NULL);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v4 0/6] driver core: Try to improve and cleanup driver_deferred_probe_check_state()

[Rafael J. Wysocki]

On Thu, Feb 20, 2020 at 6:04 AM John Stultz <john.stultz@linaro.org> wrote:
>
> This series tries to improve and cleanup the
> driver_deferred_probe_check_state() code in the driver core.

"Do. Or do not. There is no try."
 - Master Yoda

Re: [PATCH v4 1/6] driver core: Fix driver_deferred_probe_check_state() logic

[Rafael J. Wysocki]

On Thu, Feb 20, 2020 at 6:05 AM John Stultz <john.stultz@linaro.org> 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 deferred_probe_timeout is set, it returns -EPROBE_DEFER
>   until the timeout, afterwhich it returns -ETIMEDOUT.
>
> * If deferred_probe_timeout is not set (-1), it returns
>   -EPROBE_DEFER until late_initcall, after which it returns
>
> 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 <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I8349b7a403ce8cbce485ea0a0a5512fddffb635c
> ---
> v4:
> * Simplified logic suggested by Andy Shevchenko
> * Clarified commit message to focus on logic change
> ---
>  drivers/base/dd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index b25bcab2a26b..bb383dca39c1 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -237,7 +237,7 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>
>  static int __driver_deferred_probe_check_state(struct device *dev)
>  {
> -       if (!initcalls_done)
> +       if (!initcalls_done || deferred_probe_timeout > 0)
>                 return -EPROBE_DEFER;

Makes sense to me.

>
>         if (!deferred_probe_timeout) {
> @@ -252,9 +252,11 @@ static int __driver_deferred_probe_check_state(struct device *dev)
>   * 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.
> + * Returnes -EPROBE_DEFER if initcalls have not completed, or the deferred

s/Returnes/Returns/

And I would write

* Return:
* -EPROBE_DEFER if initcalls have not completed, or the deferred
*  probe timeout is set, but not expried.
* -ETIMEDOUT if the deferred probe timeout was set and has expired.
* -ENODEV if initcalls have completed and the deferred probe timeout
was not set.

> + * probe timeout is set, but not expried.
> + * Returns -ETIMEDOUT if the probe timeout was set and has expired.
> + * Returns -ENODEV if initcalls have completed and the deferred probe timeout
> + * was not set.
>   *
>   * Drivers or subsystems can opt-in to calling this function instead of directly
>   * returning -EPROBE_DEFER.
> --

Re: [PATCH v4 2/6] driver core: Set deferred_probe_timeout to a longer default if CONFIG_MODULES is set

[Rafael J. Wysocki]

On Thu, Feb 20, 2020 at 6:05 AM John Stultz <john.stultz@linaro.org> wrote:
>
> 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 <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I9c5a02a54915ff53f9f14d49c601f41d7105e05e
> ---
> 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 bb383dca39c1..fa138f24e2d3 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

Looks reasonable to me.

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

>  static int __init deferred_probe_timeout_setup(char *str)
>  {
>         int timeout;
> --

Re: [PATCH v4 4/6] driver core: Remove driver_deferred_probe_check_state_continue()

[Rafael J. Wysocki]

On Thu, Feb 20, 2020 at 6:05 AM John Stultz <john.stultz@linaro.org> wrote:
>
> 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 <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: Id5cd5e9264cfb0fbd70a702715174cc4b10006f4

Nice!

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/base/dd.c             | 49 +++++------------------------------
>  include/linux/device/driver.h |  1 -
>  2 files changed, 6 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index fa138f24e2d3..408e4da081da 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 (!initcalls_done || deferred_probe_timeout > 0)
> -               return -EPROBE_DEFER;
> -
> -       if (!deferred_probe_timeout) {
> -               dev_WARN(dev, "deferred probe timeout, ignoring dependency");
> -               return -ETIMEDOUT;
> -       }
> -
> -       return 0;
> -}
> -
>  /**
>   * driver_deferred_probe_check_state() - Check deferred probe state
>   * @dev: device to check
> @@ -272,43 +259,19 @@ static int __driver_deferred_probe_check_state(struct device *dev)
>   */
>  int driver_deferred_probe_check_state(struct device *dev)
>  {
> -       int ret;
> +       if (!initcalls_done || deferred_probe_timeout > 0)
> +               return -EPROBE_DEFER;
>
> -       ret = __driver_deferred_probe_check_state(dev);
> -       if (ret < 0)
> -               return ret;
> +       if (!deferred_probe_timeout) {
> +               dev_WARN(dev, "deferred probe timeout, ignoring dependency");
> +               return -ETIMEDOUT;
> +       }
>
>         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;
> -
> -       ret = __driver_deferred_probe_check_state(dev);
> -       if (ret < 0)
> -               return ret;
> -
> -       return -EPROBE_DEFER;
> -}
> -
>  static void deferred_probe_timeout_work_func(struct work_struct *work)
>  {
>         struct device_private *private, *p;
> 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);
>
>  /**
> --

Re: [PATCH v4 5/6] driver core: Rename deferred_probe_timeout and make it global

[Rafael J. Wysocki]

On Thu, Feb 20, 2020 at 6:05 AM John Stultz <john.stultz@linaro.org> wrote:
>
> 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.

Fair enough.

> Cc: Rob Herring <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I92ee3b392004ecc9217c5337b54eda48c2d7f3ee

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
> v4:
> * Split out into its own patch as suggested by Mark
> * Renamed deferred_probe_timeout as suggested by Greg
> ---
>  drivers/base/dd.c             | 18 ++++++++++--------
>  include/linux/device/driver.h |  1 +
>  2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 408e4da081da..39f1ce6d4f1c 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);
> @@ -259,10 +261,10 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>   */
>  int driver_deferred_probe_check_state(struct device *dev)
>  {
> -       if (!initcalls_done || deferred_probe_timeout > 0)
> +       if (!initcalls_done || driver_deferred_probe_timeout > 0)
>                 return -EPROBE_DEFER;
>
> -       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
>

Re: [PATCH v4 1/6] driver core: Fix driver_deferred_probe_check_state() logic

[Bjorn Andersson]

On Wed 19 Feb 21:04 PST 2020, John Stultz 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 deferred_probe_timeout is set, it returns -EPROBE_DEFER
>   until the timeout, afterwhich it returns -ETIMEDOUT.
> 
> * If deferred_probe_timeout is not set (-1), it returns
>   -EPROBE_DEFER until late_initcall, after which it returns
> 
> 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 <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I8349b7a403ce8cbce485ea0a0a5512fddffb635c

Please drop the Change-Id.

> ---
> v4:
> * Simplified logic suggested by Andy Shevchenko
> * Clarified commit message to focus on logic change
> ---
>  drivers/base/dd.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index b25bcab2a26b..bb383dca39c1 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -237,7 +237,7 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>  
>  static int __driver_deferred_probe_check_state(struct device *dev)
>  {
> -	if (!initcalls_done)
> +	if (!initcalls_done || deferred_probe_timeout > 0)
>  		return -EPROBE_DEFER;
>  
>  	if (!deferred_probe_timeout) {
> @@ -252,9 +252,11 @@ static int __driver_deferred_probe_check_state(struct device *dev)
>   * 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.
> + * Returnes -EPROBE_DEFER if initcalls have not completed, or the deferred

As pointed out by Rafael, this should be Return:

With that addressed, you have my
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> + * probe timeout is set, but not expried.
> + * Returns -ETIMEDOUT if the probe timeout was set and has expired.
> + * Returns -ENODEV if initcalls have completed and the deferred probe timeout
> + * was not set.
>   *
>   * Drivers or subsystems can opt-in to calling this function instead of directly
>   * returning -EPROBE_DEFER.
> -- 
> 2.17.1
> 

Re: [PATCH v4 2/6] driver core: Set deferred_probe_timeout to a longer default if CONFIG_MODULES is set

[Bjorn Andersson]

On Wed 19 Feb 21:04 PST 2020, John Stultz wrote:

> 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 <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I9c5a02a54915ff53f9f14d49c601f41d7105e05e

Change-Id...

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> 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 bb383dca39c1..fa138f24e2d3 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
> 

Re: [PATCH v4 4/6] driver core: Remove driver_deferred_probe_check_state_continue()

[Bjorn Andersson]

On Wed 19 Feb 21:04 PST 2020, John Stultz wrote:

> 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 <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: Id5cd5e9264cfb0fbd70a702715174cc4b10006f4

Change-Id...

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/base/dd.c             | 49 +++++------------------------------
>  include/linux/device/driver.h |  1 -
>  2 files changed, 6 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index fa138f24e2d3..408e4da081da 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 (!initcalls_done || deferred_probe_timeout > 0)
> -		return -EPROBE_DEFER;
> -
> -	if (!deferred_probe_timeout) {
> -		dev_WARN(dev, "deferred probe timeout, ignoring dependency");
> -		return -ETIMEDOUT;
> -	}
> -
> -	return 0;
> -}
> -
>  /**
>   * driver_deferred_probe_check_state() - Check deferred probe state
>   * @dev: device to check
> @@ -272,43 +259,19 @@ static int __driver_deferred_probe_check_state(struct device *dev)
>   */
>  int driver_deferred_probe_check_state(struct device *dev)
>  {
> -	int ret;
> +	if (!initcalls_done || deferred_probe_timeout > 0)
> +		return -EPROBE_DEFER;
>  
> -	ret = __driver_deferred_probe_check_state(dev);
> -	if (ret < 0)
> -		return ret;
> +	if (!deferred_probe_timeout) {
> +		dev_WARN(dev, "deferred probe timeout, ignoring dependency");
> +		return -ETIMEDOUT;
> +	}
>  
>  	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;
> -
> -	ret = __driver_deferred_probe_check_state(dev);
> -	if (ret < 0)
> -		return ret;
> -
> -	return -EPROBE_DEFER;
> -}
> -
>  static void deferred_probe_timeout_work_func(struct work_struct *work)
>  {
>  	struct device_private *private, *p;
> 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
> 

Re: [PATCH v4 5/6] driver core: Rename deferred_probe_timeout and make it global

[Bjorn Andersson]

On Wed 19 Feb 21:04 PST 2020, John Stultz wrote:

> 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 <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I92ee3b392004ecc9217c5337b54eda48c2d7f3ee

Change-Id...

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> v4:
> * Split out into its own patch as suggested by Mark
> * Renamed deferred_probe_timeout as suggested by Greg
> ---
>  drivers/base/dd.c             | 18 ++++++++++--------
>  include/linux/device/driver.h |  1 +
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 408e4da081da..39f1ce6d4f1c 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);
> @@ -259,10 +261,10 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>   */
>  int driver_deferred_probe_check_state(struct device *dev)
>  {
> -	if (!initcalls_done || deferred_probe_timeout > 0)
> +	if (!initcalls_done || driver_deferred_probe_timeout > 0)
>  		return -EPROBE_DEFER;
>  
> -	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
> 

Re: [PATCH v4 6/6] regulator: Use driver_deferred_probe_timeout for regulator_init_complete_work

[Bjorn Andersson]

On Wed 19 Feb 21:04 PST 2020, 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.
> 
> 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 if it is set, otherwise we
> directly call the regulator_init_complete_work_function().
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I9fa2411abbb91ed4dd0edc41e8cc8583577c005b

Change-Id...

> ---
> v4:
> * Split out into its own patch, as suggested by Mark
> ---
>  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..394e7b11576a 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5767,18 +5767,21 @@ 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(&regulator_init_complete_work,
> -			      msecs_to_jiffies(30000));
> +	if (driver_deferred_probe_timeout >= 0)
> +		schedule_delayed_work(&regulator_init_complete_work,
> +				      driver_deferred_probe_timeout * HZ);
> +	else
> +		regulator_init_complete_work_function(NULL);

Why not schedule_delayed_work(..., 0) in this case, to get it off the
initcall context and to avoid the difference in execution paths?

Regards,
Bjorn

>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v4 1/6] driver core: Fix driver_deferred_probe_check_state() logic

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

Hi!

> * If deferred_probe_timeout is not set (-1), it returns
>   -EPROBE_DEFER until late_initcall, after which it returns
>

You may want to complete the sentence here :-).

Best regards,
							Pavel
							
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 1/6] driver core: Fix driver_deferred_probe_check_state() logic

[John Stultz]

On Thu, Feb 20, 2020 at 3:40 PM Pavel Machek <pavel@denx.de> wrote:
>
> Hi!
>
> > * If deferred_probe_timeout is not set (-1), it returns
> >   -EPROBE_DEFER until late_initcall, after which it returns
> >
>
> You may want to complete the sentence here :-).
>

Yes. I somehow cut the line accidentally. Its already fixed in my tree. :)

thanks for the review!
-john

Re: [PATCH v4 3/6] pinctrl: Remove use of driver_deferred_probe_check_state_continue()

[Linus Walleij]

On Thu, Feb 20, 2020 at 6:05 AM John Stultz <john.stultz@linaro.org> 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 <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: If72682e0a7641b33edf56f188fc067c68bbc571e

I sure trust that you know what you're doing here.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

I assume you will merge this through device core?

Yours,
Linus Walleij

Re: [PATCH v4 3/6] pinctrl: Remove use of driver_deferred_probe_check_state_continue()

[John Stultz]

On Fri, Feb 21, 2020 at 7:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Feb 20, 2020 at 6:05 AM John Stultz <john.stultz@linaro.org> 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 <robh@kernel.org>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Kevin Hilman <khilman@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Len Brown <len.brown@intel.com>
> > Cc: Todd Kjos <tkjos@google.com>
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Liam Girdwood <lgirdwood@gmail.com>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Thierry Reding <treding@nvidia.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > Change-Id: If72682e0a7641b33edf56f188fc067c68bbc571e
>
> I sure trust that you know what you're doing here.

Classic mistake. :)

> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> I assume you will merge this through device core?

I guess? I'm going to resend it again as I think its a reasonable
cleanup, but the urgent need for it from me may be solved by
of_devlink Saravana's patch instead.

thanks
-john