driver core: Improve and cleanup driver_deferred_probe_check_state()

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(-)

drivers/base/dd.c +10 -9

index b25bcab2a26b..d75b34de6964 100644
@@ -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

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(-)

drivers/pinctrl/devicetree.c +0 -0

index 9357f7c46cf3..1ed20ac2243f 100644

drivers/pinctrl/devicetree.c +3 -1

+                           (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

Quoted 40 lines · 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 <[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(-)

drivers/pinctrl/devicetree.c +1 -1

index 9357f7c46cf3..1ed20ac2243f 100644
@@ -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)

drivers/pinctrl/devicetree.c static int dt_to_map_one_config(struct pinctrl *p, · line 132

Quoted 4 lines

drivers/pinctrl/devicetree.c +1 -3

-                               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?

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 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 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!

Hi John,
Cc: linux-renesas-soc

Quoted 28 lines · John Stultz

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

Quoted 3 lines

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?

Quoted 8 lines · Eugeniu Rosca

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 second reverts the default timeout back to 0:
https://lore.kernel.org/lkml/[email protected]/

Quoted 2 lines · John Stultz

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.

Quoted 6 lines · Mark Brown

Quoted 2 lines · John Stultz

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.

Quoted 2 lines · Mark Brown

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.