[RFC,v2,2/2] driver core: Ensure wait_for_device_probe() waits until the deferred_probe_timeout fires

Commit Message

John Stultz April 8, 2020, 7:26 a.m. UTC

In commit c8c43cee29f6 ("driver core: Fix
driver_deferred_probe_check_state() logic"), we set the default
driver_deferred_probe_timeout value to 30 seconds to allow for
drivers that are missing dependencies to have some time so that
the dependency may be loaded from userland after initcalls_done
is set.

However, Yoshihiro Shimoda reported that on his device that
expects to have unmet dependencies (due to "optional links" in
its devicetree), was failing to mount the NFS root.

In digging further, it seemed the problem was that while the
device properly probes after waiting 30 seconds for any missing
modules to load, the ip_auto_config() had already failed,
resulting in NFS to fail. This was due to ip_auto_config()
calling wait_for_device_probe() which doesn't wait for the
driver_deferred_probe_timeout to fire.

This patch tries to fix the issue by creating a waitqueue
for the driver_deferred_probe_timeout, and calling wait_event()
to make sure driver_deferred_probe_timeout is zero in
wait_for_device_probe() to make sure all the probing is
finished.

The downside to this solution is that kernel functionality that
uses wait_for_device_probe(), will block until the
driver_deferred_probe_timeout fires, regardless of if there is
any missing dependencies.

However, the previous patch reverts the default timeout value to
zero, so this side-effect will only affect users who specify a
driver_deferred_probe_timeout= value as a boot argument, where
the additional delay would be beneficial to allow modules to
load later during boot.

Thanks to Geert for chasing down that ip_auto_config was why NFS
was failing in this case!

Cc: "David S. Miller" <[e-mail]>
Cc: Alexey Kuznetsov <[e-mail]>
Cc: Hideaki YOSHIFUJI <[e-mail]>
Cc: Jakub Kicinski <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: Rafael J. Wysocki <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Geert Uytterhoeven <[e-mail]>
Cc: Yoshihiro Shimoda <[e-mail]>
Cc: netdev <[e-mail]>
Cc: [e-mail]
Reported-by: Yoshihiro Shimoda <[e-mail]>
Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic")
Signed-off-by: John Stultz <[e-mail]>
---
* v2: Split patch, and apply it as a follow-on to setting
      the driver_deferred_probe_timeout defalt back to zero
---
 drivers/base/dd.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Yoshihiro Shimoda April 8, 2020, 10:29 a.m. UTC | #1

Hi John,

> From: John Stultz, Sent: Wednesday, April 8, 2020 4:27 PM
> 
> In commit c8c43cee29f6 ("driver core: Fix
> driver_deferred_probe_check_state() logic"), we set the default
> driver_deferred_probe_timeout value to 30 seconds to allow for
> drivers that are missing dependencies to have some time so that
> the dependency may be loaded from userland after initcalls_done
> is set.
> 
> However, Yoshihiro Shimoda reported that on his device that
> expects to have unmet dependencies (due to "optional links" in
> its devicetree), was failing to mount the NFS root.
> 
> In digging further, it seemed the problem was that while the
> device properly probes after waiting 30 seconds for any missing
> modules to load, the ip_auto_config() had already failed,
> resulting in NFS to fail. This was due to ip_auto_config()
> calling wait_for_device_probe() which doesn't wait for the
> driver_deferred_probe_timeout to fire.
> 
> This patch tries to fix the issue by creating a waitqueue
> for the driver_deferred_probe_timeout, and calling wait_event()
> to make sure driver_deferred_probe_timeout is zero in
> wait_for_device_probe() to make sure all the probing is
> finished.
> 
> The downside to this solution is that kernel functionality that
> uses wait_for_device_probe(), will block until the
> driver_deferred_probe_timeout fires, regardless of if there is
> any missing dependencies.
> 
> However, the previous patch reverts the default timeout value to
> zero, so this side-effect will only affect users who specify a
> driver_deferred_probe_timeout= value as a boot argument, where
> the additional delay would be beneficial to allow modules to
> load later during boot.
> 
> Thanks to Geert for chasing down that ip_auto_config was why NFS
> was failing in this case!
> 
> Cc: "David S. Miller" <[e-mail]>
> Cc: Alexey Kuznetsov <[e-mail]>
> Cc: Hideaki YOSHIFUJI <[e-mail]>
> Cc: Jakub Kicinski <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: Rafael J. Wysocki <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Yoshihiro Shimoda <[e-mail]>
> Cc: netdev <[e-mail]>
> Cc: [e-mail]
> Reported-by: Yoshihiro Shimoda <[e-mail]>
> Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic")
> Signed-off-by: John Stultz <[e-mail]>
> ---
> * v2: Split patch, and apply it as a follow-on to setting
>       the driver_deferred_probe_timeout defalt back to zero
> ---

Thank you for the patch! This patch doesn't cause any side
effect on my environment (the value of driver_deferred_probe_timeout is 0).
So,

Tested-by: Yoshihiro Shimoda <[e-mail]>

Best regards,
Yoshihiro Shimoda

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 908ae4d7805e..5e6c00176969 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -226,6 +226,7 @@  DEFINE_SHOW_ATTRIBUTE(deferred_devs);
 
 int driver_deferred_probe_timeout;
 EXPORT_SYMBOL_GPL(driver_deferred_probe_timeout);
+static DECLARE_WAIT_QUEUE_HEAD(probe_timeout_waitqueue);
 
 static int __init deferred_probe_timeout_setup(char *str)
 {
@@ -275,6 +276,7 @@  static void deferred_probe_timeout_work_func(struct work_struct *work)
 
 	list_for_each_entry_safe(private, p, &deferred_probe_pending_list, deferred_probe)
 		dev_info(private->device, "deferred probe pending");
+	wake_up(&probe_timeout_waitqueue);
 }
 static DECLARE_DELAYED_WORK(deferred_probe_timeout_work, deferred_probe_timeout_work_func);
 
@@ -649,6 +651,9 @@  int driver_probe_done(void)
  */
 void wait_for_device_probe(void)
 {
+	/* wait for probe timeout */
+	wait_event(probe_timeout_waitqueue, !driver_deferred_probe_timeout);
+
 	/* wait for the deferred probe workqueue to finish */
 	flush_work(&deferred_probe_work);