| Message ID | 20200422203245.83244-2-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Fixes for deferred_probe_timeout cleanup | expand |
Hi John, On Wed, Apr 22, 2020 at 10:33 PM John Stultz <john.stultz@linaro.org> wrote: > This patch addresses a regression in 5.7-rc1+ > > In commit c8c43cee29f6 ("driver core: Fix > driver_deferred_probe_check_state() logic"), we both cleaned up > the logic and also 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. > > Fixing that issue is possible, but could also introduce 30 > second delays in bootups for users who don't have any > missing dependencies, which is not ideal. > > So I think the best solution to avoid any regressions is to > revert back to a default timeout value of zero, and allow > systems that need to utilize the timeout in order for userland > to load any modules that supply misisng dependencies in the dts > to specify the timeout length via the exiting documented boot > argument. > > Thanks to Geert for chasing down that ip_auto_config was why NFS > was failing in this case! > > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Rob Herring <robh@kernel.org> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Naresh Kamboju <naresh.kamboju@linaro.org> > Cc: Basil Eljuse <Basil.Eljuse@arm.com> > Cc: Ferry Toth <fntoth@gmail.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Anders Roxell <anders.roxell@linaro.org> > Cc: netdev <netdev@vger.kernel.org> > Cc: linux-pm@vger.kernel.org > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") > Signed-off-by: John Stultz <john.stultz@linaro.org> Looks like you lost my Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> on v2. Gr{oetje,eeting}s, Geert
Hi John, On 22.04.2020 22:32, John Stultz wrote: > This patch addresses a regression in 5.7-rc1+ > > In commit c8c43cee29f6 ("driver core: Fix > driver_deferred_probe_check_state() logic"), we both cleaned up > the logic and also 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. > > Fixing that issue is possible, but could also introduce 30 > second delays in bootups for users who don't have any > missing dependencies, which is not ideal. > > So I think the best solution to avoid any regressions is to > revert back to a default timeout value of zero, and allow > systems that need to utilize the timeout in order for userland > to load any modules that supply misisng dependencies in the dts > to specify the timeout length via the exiting documented boot > argument. > > Thanks to Geert for chasing down that ip_auto_config was why NFS > was failing in this case! > > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Rob Herring <robh@kernel.org> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Naresh Kamboju <naresh.kamboju@linaro.org> > Cc: Basil Eljuse <Basil.Eljuse@arm.com> > Cc: Ferry Toth <fntoth@gmail.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Anders Roxell <anders.roxell@linaro.org> > Cc: netdev <netdev@vger.kernel.org> > Cc: linux-pm@vger.kernel.org > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") > Signed-off-by: John Stultz <john.stultz@linaro.org> Please also revert dca0b44957e5 "regulator: Use driver_deferred_probe_timeout for regulator_init_complete_work" then, because now with the default 0 timeout some regulators gets disabled during boot, before their supplies gets instantiated. This patch broke booting of Samsung Exynos5800-based Peach-Pi Chromeboot with the default multi_v7_defconfig. > --- > drivers/base/dd.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 06ec0e851fa1..908ae4d7805e 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -224,16 +224,7 @@ 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 > - */ > -int driver_deferred_probe_timeout = 30; > -#else > -/* In the case of !modules, no probe timeout needed */ > -int driver_deferred_probe_timeout = -1; > -#endif > +int driver_deferred_probe_timeout; > EXPORT_SYMBOL_GPL(driver_deferred_probe_timeout); > > static int __init deferred_probe_timeout_setup(char *str) > @@ -266,7 +257,7 @@ int driver_deferred_probe_check_state(struct device *dev) > return -ENODEV; > } > > - if (!driver_deferred_probe_timeout) { > + if (!driver_deferred_probe_timeout && initcalls_done) { > dev_WARN(dev, "deferred probe timeout, ignoring dependency"); > return -ETIMEDOUT; > } Best regards
On Wed, Apr 29, 2020 at 03:46:04PM +0200, Marek Szyprowski wrote: > On 22.04.2020 22:32, John Stultz wrote: > > Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") > > Signed-off-by: John Stultz <john.stultz@linaro.org> > Please also revert dca0b44957e5 "regulator: Use > driver_deferred_probe_timeout for regulator_init_complete_work" then, > because now with the default 0 timeout some regulators gets disabled > during boot, before their supplies gets instantiated. Yes, please - I requested this when the revert was originally proposed :(
On Wed, Apr 29, 2020 at 6:52 AM Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 29, 2020 at 03:46:04PM +0200, Marek Szyprowski wrote: > > On 22.04.2020 22:32, John Stultz wrote: > > > > Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") > > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > > Please also revert dca0b44957e5 "regulator: Use > > driver_deferred_probe_timeout for regulator_init_complete_work" then, > > because now with the default 0 timeout some regulators gets disabled > > during boot, before their supplies gets instantiated. > > Yes, please - I requested this when the revert was originally proposed :( Oh, my apologies. I misunderstood what you were suggesting earlier. Sorry for being thick headed. I'll spin up a revert here shortly. thanks -john
On Wed, Apr 22, 2020 at 08:32:43PM +0000, John Stultz wrote: > This patch addresses a regression in 5.7-rc1+ > > In commit c8c43cee29f6 ("driver core: Fix > driver_deferred_probe_check_state() logic"), we both cleaned up > the logic and also 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. > > Fixing that issue is possible, but could also introduce 30 > second delays in bootups for users who don't have any > missing dependencies, which is not ideal. > > So I think the best solution to avoid any regressions is to > revert back to a default timeout value of zero, and allow > systems that need to utilize the timeout in order for userland > to load any modules that supply misisng dependencies in the dts > to specify the timeout length via the exiting documented boot > argument. > > Thanks to Geert for chasing down that ip_auto_config was why NFS > was failing in this case! > > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Rob Herring <robh@kernel.org> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Sudeep Holla <sudeep.holla@arm.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Naresh Kamboju <naresh.kamboju@linaro.org> > Cc: Basil Eljuse <Basil.Eljuse@arm.com> > Cc: Ferry Toth <fntoth@gmail.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Anders Roxell <anders.roxell@linaro.org> > Cc: netdev <netdev@vger.kernel.org> > Cc: linux-pm@vger.kernel.org > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > drivers/base/dd.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) Sorry for being a bit late to the party, but this breaks suspend/resume support on various Tegra devices. I've only noticed now because, well, suspend/resume have been broken for other reasons for a little while and it's taken us a bit to resolve those issues. But now that those other issues have been fixed, I've started seeing an issue where after resume from suspend some of the I2C controllers are no longer working. The reason for this is that they share pins with DP AUX controllers via the pinctrl framework. The DP AUX driver registers as part of the DRM/KMS driver, which usually happens in userspace. Since the deferred probe timeout was set to 0 by default this no longer works because no pinctrl states are assigned to the I2C controller and therefore upon resume the pins cannot be configured for I2C operation. I'm also somewhat confused by this patch and a few before because they claim that they restore previous default behaviour, but that's just not true. Originally when this timeout was introduced it was -1, which meant that there was no timeout at all and hence users had to opt-in if they wanted to use a deferred probe timeout. But now after this series the default is for there to be a very short timeout, which in turn causes existing use-cases to potentially break. I'm also going to suggest here that in most cases a driver will require the resources that it asks for, so the case that Yoshihiro described and that this patch is meant to fix sounds to me like it's the odd one out rather than the other way around. But I realize that that's not very constructive. So perhaps we can find some other way for drivers to advertise that their dependencies are optional? I came up with the below patch, which restores suspend/resume on Tegra and could be used in conjunction with a patch that opts into this behaviour for the problematic driver in Yoshihiro's case to make this again work for everyone. --- >8 --- From a95f8f41b8a32dee3434db4f0515af7376d1873a Mon Sep 17 00:00:00 2001 From: Thierry Reding <treding@nvidia.com> Date: Thu, 6 Aug 2020 14:51:59 +0200 Subject: [PATCH] driver core: Do not ignore dependencies by default Many drivers do require the resources that they ask for and timing out may not always be an option. While there is a way to allow probing to continue to be deferred for some time after the system has booted, the fact that this is controlled via a command-line parameter is undesired because it require manual intervention, whereas in can be avoid in the majority of cases. Instead of requiring users to edit the kernel command-line, add a way for drivers to specify whether or not their dependencies are optional so that they can continue deferring probe indefinitely. Signed-off-by: Thierry Reding <treding@nvidia.com> --- drivers/base/dd.c | 2 +- include/linux/device/driver.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 857b0a928e8d..11e747070eae 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -279,7 +279,7 @@ int driver_deferred_probe_check_state(struct device *dev) return -ENODEV; } - if (!driver_deferred_probe_timeout && initcalls_done) { + if (dev->driver->ignore_dependencies && !driver_deferred_probe_timeout && initcalls_done) { dev_warn(dev, "deferred probe timeout, ignoring dependency\n"); return -ETIMEDOUT; } diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h index ee7ba5b5417e..6994455e8a2e 100644 --- a/include/linux/device/driver.h +++ b/include/linux/device/driver.h @@ -100,6 +100,7 @@ struct device_driver { const char *mod_name; /* used for built-in modules */ bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ + bool ignore_dependencies; /* ignores dependencies */ enum probe_type probe_type; const struct of_device_id *of_match_table;
On Thu, Aug 6, 2020 at 6:52 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Apr 22, 2020 at 08:32:43PM +0000, John Stultz wrote: > > This patch addresses a regression in 5.7-rc1+ > > > > In commit c8c43cee29f6 ("driver core: Fix > > driver_deferred_probe_check_state() logic"), we both cleaned up > > the logic and also 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. > > > > Fixing that issue is possible, but could also introduce 30 > > second delays in bootups for users who don't have any > > missing dependencies, which is not ideal. > > > > So I think the best solution to avoid any regressions is to > > revert back to a default timeout value of zero, and allow > > systems that need to utilize the timeout in order for userland > > to load any modules that supply misisng dependencies in the dts > > to specify the timeout length via the exiting documented boot > > argument. > > > > Thanks to Geert for chasing down that ip_auto_config was why NFS > > was failing in this case! > > > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> > > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > Cc: Rob Herring <robh@kernel.org> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Naresh Kamboju <naresh.kamboju@linaro.org> > > Cc: Basil Eljuse <Basil.Eljuse@arm.com> > > Cc: Ferry Toth <fntoth@gmail.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Anders Roxell <anders.roxell@linaro.org> > > Cc: netdev <netdev@vger.kernel.org> > > Cc: linux-pm@vger.kernel.org > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > --- > > drivers/base/dd.c | 13 ++----------- > > 1 file changed, 2 insertions(+), 11 deletions(-) > > Sorry for being a bit late to the party, but this breaks suspend/resume > support on various Tegra devices. I've only noticed now because, well, > suspend/resume have been broken for other reasons for a little while and > it's taken us a bit to resolve those issues. > > But now that those other issues have been fixed, I've started seeing an > issue where after resume from suspend some of the I2C controllers are no > longer working. The reason for this is that they share pins with DP AUX > controllers via the pinctrl framework. The DP AUX driver registers as > part of the DRM/KMS driver, which usually happens in userspace. Since > the deferred probe timeout was set to 0 by default this no longer works > because no pinctrl states are assigned to the I2C controller and > therefore upon resume the pins cannot be configured for I2C operation. Oof. My apologies! > I'm also somewhat confused by this patch and a few before because they > claim that they restore previous default behaviour, but that's just not > true. Originally when this timeout was introduced it was -1, which meant > that there was no timeout at all and hence users had to opt-in if they > wanted to use a deferred probe timeout. I don't think that's quite true, since the point of my original changes were to avoid troubles I was seeing with drivers not loading because once the timeout fired after init, driver loading would fail with ENODEV instead of returning EPROBE_DEFER. The logic that existed was buggy so the timeout handling didn't really work (changing the boot argument wouldn't help, because after init the logic would return ENODEV before it checked the timeout value). That said, looking at it now, I do realize the driver_deferred_probe_check_state_continue() logic in effect never returned ETIMEDOUT before was consolidated in the earlier changes, and now we've backed the default timeout to 0, old user (see bec6c0ecb243) will now get ETIMEDOUT where they wouldn't before. So would the following fix it up for you? (sorry its whitespace corrupted) diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c index c6fe7d64c913..c7448be64d07 100644 --- a/drivers/pinctrl/devicetree.c +++ b/drivers/pinctrl/devicetree.c @@ -129,9 +129,8 @@ static int dt_to_map_one_config(struct pinctrl *p, 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 && - (ret == -ENODEV)) + /* keep deferring if modules are enabled */ + if (IS_ENABLED(CONFIG_MODULES) && !allow_default && ret < 0) ret = -EPROBE_DEFER; return ret; } (you could probably argue calling driver_deferred_probe_check_state checking ret at all is silly here, since EPROBE_DEFER is the only option you want) > But now after this series the default is for there to be a very short > timeout, which in turn causes existing use-cases to potentially break. > I'm also going to suggest here that in most cases a driver will require > the resources that it asks for, so the case that Yoshihiro described and > that this patch is meant to fix sounds to me like it's the odd one out > rather than the other way around. > > But I realize that that's not very constructive. So perhaps we can find > some other way for drivers to advertise that their dependencies are > optional? I came up with the below patch, which restores suspend/resume > on Tegra and could be used in conjunction with a patch that opts into > this behaviour for the problematic driver in Yoshihiro's case to make > this again work for everyone. > > --- >8 --- > From a95f8f41b8a32dee3434db4f0515af7376d1873a Mon Sep 17 00:00:00 2001 > From: Thierry Reding <treding@nvidia.com> > Date: Thu, 6 Aug 2020 14:51:59 +0200 > Subject: [PATCH] driver core: Do not ignore dependencies by default > > Many drivers do require the resources that they ask for and timing out > may not always be an option. While there is a way to allow probing to > continue to be deferred for some time after the system has booted, the > fact that this is controlled via a command-line parameter is undesired > because it require manual intervention, whereas in can be avoid in the > majority of cases. > > Instead of requiring users to edit the kernel command-line, add a way > for drivers to specify whether or not their dependencies are optional > so that they can continue deferring probe indefinitely. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/base/dd.c | 2 +- > include/linux/device/driver.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 857b0a928e8d..11e747070eae 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -279,7 +279,7 @@ int driver_deferred_probe_check_state(struct device *dev) > return -ENODEV; > } > > - if (!driver_deferred_probe_timeout && initcalls_done) { > + if (dev->driver->ignore_dependencies && !driver_deferred_probe_timeout && initcalls_done) { > dev_warn(dev, "deferred probe timeout, ignoring dependency\n"); > return -ETIMEDOUT; > } > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index ee7ba5b5417e..6994455e8a2e 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -100,6 +100,7 @@ struct device_driver { > const char *mod_name; /* used for built-in modules */ > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > + bool ignore_dependencies; /* ignores dependencies */ > enum probe_type probe_type; > > const struct of_device_id *of_match_table; > -- > 2.27.0 > --- >8 --- > > Although, thinking about it a bit more it sounds to me like an even > better approach would be to make this part of the API where a resource > is requested. There are in fact already APIs that can request optional > resources (such as regulator_get_optional()), so I think it would make > more sense for any driver that can live without a resource to request > it with an optional flag, which in turn could then trigger this code > path for the deferred probe timeout. For anyone that really needs the > resources that they request, they really shouldn't have to jump through > hoops to get there. I do like these suggestions. I feel like the optional dt links handling is very opaque as there's no real way to tell if a dt link is truly optional or not. Having the timeout is a really subtle and implicit way of getting that optional link handling behavior. thanks -john
On Thu, Aug 06, 2020 at 07:09:16PM -0700, John Stultz wrote: > On Thu, Aug 6, 2020 at 6:52 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Wed, Apr 22, 2020 at 08:32:43PM +0000, John Stultz wrote: > > > This patch addresses a regression in 5.7-rc1+ > > > > > > In commit c8c43cee29f6 ("driver core: Fix > > > driver_deferred_probe_check_state() logic"), we both cleaned up > > > the logic and also 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. > > > > > > Fixing that issue is possible, but could also introduce 30 > > > second delays in bootups for users who don't have any > > > missing dependencies, which is not ideal. > > > > > > So I think the best solution to avoid any regressions is to > > > revert back to a default timeout value of zero, and allow > > > systems that need to utilize the timeout in order for userland > > > to load any modules that supply misisng dependencies in the dts > > > to specify the timeout length via the exiting documented boot > > > argument. > > > > > > Thanks to Geert for chasing down that ip_auto_config was why NFS > > > was failing in this case! > > > > > > Cc: "David S. Miller" <davem@davemloft.net> > > > Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> > > > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> > > > Cc: Jakub Kicinski <kuba@kernel.org> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > > Cc: Rob Herring <robh@kernel.org> > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > Cc: Robin Murphy <robin.murphy@arm.com> > > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Cc: Naresh Kamboju <naresh.kamboju@linaro.org> > > > Cc: Basil Eljuse <Basil.Eljuse@arm.com> > > > Cc: Ferry Toth <fntoth@gmail.com> > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > Cc: Anders Roxell <anders.roxell@linaro.org> > > > Cc: netdev <netdev@vger.kernel.org> > > > Cc: linux-pm@vger.kernel.org > > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") > > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > > --- > > > drivers/base/dd.c | 13 ++----------- > > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > Sorry for being a bit late to the party, but this breaks suspend/resume > > support on various Tegra devices. I've only noticed now because, well, > > suspend/resume have been broken for other reasons for a little while and > > it's taken us a bit to resolve those issues. > > > > But now that those other issues have been fixed, I've started seeing an > > issue where after resume from suspend some of the I2C controllers are no > > longer working. The reason for this is that they share pins with DP AUX > > controllers via the pinctrl framework. The DP AUX driver registers as > > part of the DRM/KMS driver, which usually happens in userspace. Since > > the deferred probe timeout was set to 0 by default this no longer works > > because no pinctrl states are assigned to the I2C controller and > > therefore upon resume the pins cannot be configured for I2C operation. > > Oof. My apologies! > > > I'm also somewhat confused by this patch and a few before because they > > claim that they restore previous default behaviour, but that's just not > > true. Originally when this timeout was introduced it was -1, which meant > > that there was no timeout at all and hence users had to opt-in if they > > wanted to use a deferred probe timeout. > > I don't think that's quite true, since the point of my original > changes were to avoid troubles I was seeing with drivers not loading > because once the timeout fired after init, driver loading would fail > with ENODEV instead of returning EPROBE_DEFER. The logic that existed > was buggy so the timeout handling didn't really work (changing the > boot argument wouldn't help, because after init the logic would return > ENODEV before it checked the timeout value). > > That said, looking at it now, I do realize the > driver_deferred_probe_check_state_continue() logic in effect never > returned ETIMEDOUT before was consolidated in the earlier changes, and > now we've backed the default timeout to 0, old user (see bec6c0ecb243) > will now get ETIMEDOUT where they wouldn't before. > > So would the following fix it up for you? (sorry its whitespace corrupted) > > diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c > index c6fe7d64c913..c7448be64d07 100644 > --- a/drivers/pinctrl/devicetree.c > +++ b/drivers/pinctrl/devicetree.c > @@ -129,9 +129,8 @@ static int dt_to_map_one_config(struct pinctrl *p, > 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 && > - (ret == -ENODEV)) > + /* keep deferring if modules are enabled */ > + if (IS_ENABLED(CONFIG_MODULES) && > !allow_default && ret < 0) > ret = -EPROBE_DEFER; > return ret; > } > > (you could probably argue calling driver_deferred_probe_check_state > checking ret at all is silly here, since EPROBE_DEFER is the only > option you want) Just by looking at it I would've said, yes, this looks like it would fix it. However, upon giving this a try, I see the same pinmux issues as I was seeing before. I'm going to have to dig a little deeper to see if I can find out why that is, because it isn't obvious to me right away. > > > But now after this series the default is for there to be a very short > > timeout, which in turn causes existing use-cases to potentially break. > > I'm also going to suggest here that in most cases a driver will require > > the resources that it asks for, so the case that Yoshihiro described and > > that this patch is meant to fix sounds to me like it's the odd one out > > rather than the other way around. > > > > But I realize that that's not very constructive. So perhaps we can find > > some other way for drivers to advertise that their dependencies are > > optional? I came up with the below patch, which restores suspend/resume > > on Tegra and could be used in conjunction with a patch that opts into > > this behaviour for the problematic driver in Yoshihiro's case to make > > this again work for everyone. > > > > --- >8 --- > > From a95f8f41b8a32dee3434db4f0515af7376d1873a Mon Sep 17 00:00:00 2001 > > From: Thierry Reding <treding@nvidia.com> > > Date: Thu, 6 Aug 2020 14:51:59 +0200 > > Subject: [PATCH] driver core: Do not ignore dependencies by default > > > > Many drivers do require the resources that they ask for and timing out > > may not always be an option. While there is a way to allow probing to > > continue to be deferred for some time after the system has booted, the > > fact that this is controlled via a command-line parameter is undesired > > because it require manual intervention, whereas in can be avoid in the > > majority of cases. > > > > Instead of requiring users to edit the kernel command-line, add a way > > for drivers to specify whether or not their dependencies are optional > > so that they can continue deferring probe indefinitely. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/base/dd.c | 2 +- > > include/linux/device/driver.h | 1 + > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > > index 857b0a928e8d..11e747070eae 100644 > > --- a/drivers/base/dd.c > > +++ b/drivers/base/dd.c > > @@ -279,7 +279,7 @@ int driver_deferred_probe_check_state(struct device *dev) > > return -ENODEV; > > } > > > > - if (!driver_deferred_probe_timeout && initcalls_done) { > > + if (dev->driver->ignore_dependencies && !driver_deferred_probe_timeout && initcalls_done) { > > dev_warn(dev, "deferred probe timeout, ignoring dependency\n"); > > return -ETIMEDOUT; > > } > > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > > index ee7ba5b5417e..6994455e8a2e 100644 > > --- a/include/linux/device/driver.h > > +++ b/include/linux/device/driver.h > > @@ -100,6 +100,7 @@ struct device_driver { > > const char *mod_name; /* used for built-in modules */ > > > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > > + bool ignore_dependencies; /* ignores dependencies */ > > enum probe_type probe_type; > > > > const struct of_device_id *of_match_table; > > -- > > 2.27.0 > > --- >8 --- > > > > Although, thinking about it a bit more it sounds to me like an even > > better approach would be to make this part of the API where a resource > > is requested. There are in fact already APIs that can request optional > > resources (such as regulator_get_optional()), so I think it would make > > more sense for any driver that can live without a resource to request > > it with an optional flag, which in turn could then trigger this code > > path for the deferred probe timeout. For anyone that really needs the > > resources that they request, they really shouldn't have to jump through > > hoops to get there. > > I do like these suggestions. I feel like the optional dt links > handling is very opaque as there's no real way to tell if a dt link is > truly optional or not. Having the timeout is a really subtle and > implicit way of getting that optional link handling behavior. Agreed. Handling this implicitly is pretty wonky and as evidenced by these two cases bound to break one way or another. Thierry
On Fri, Aug 07, 2020 at 01:02:44PM +0200, Thierry Reding wrote: > On Thu, Aug 06, 2020 at 07:09:16PM -0700, John Stultz wrote: > > On Thu, Aug 6, 2020 at 6:52 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Wed, Apr 22, 2020 at 08:32:43PM +0000, John Stultz wrote: > > > > This patch addresses a regression in 5.7-rc1+ > > > > > > > > In commit c8c43cee29f6 ("driver core: Fix > > > > driver_deferred_probe_check_state() logic"), we both cleaned up > > > > the logic and also 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. > > > > > > > > Fixing that issue is possible, but could also introduce 30 > > > > second delays in bootups for users who don't have any > > > > missing dependencies, which is not ideal. > > > > > > > > So I think the best solution to avoid any regressions is to > > > > revert back to a default timeout value of zero, and allow > > > > systems that need to utilize the timeout in order for userland > > > > to load any modules that supply misisng dependencies in the dts > > > > to specify the timeout length via the exiting documented boot > > > > argument. > > > > > > > > Thanks to Geert for chasing down that ip_auto_config was why NFS > > > > was failing in this case! > > > > > > > > Cc: "David S. Miller" <davem@davemloft.net> > > > > Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> > > > > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> > > > > Cc: Jakub Kicinski <kuba@kernel.org> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > > > > Cc: Rob Herring <robh@kernel.org> > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > Cc: Robin Murphy <robin.murphy@arm.com> > > > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > > > Cc: Sudeep Holla <sudeep.holla@arm.com> > > > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Cc: Naresh Kamboju <naresh.kamboju@linaro.org> > > > > Cc: Basil Eljuse <Basil.Eljuse@arm.com> > > > > Cc: Ferry Toth <fntoth@gmail.com> > > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > > Cc: Anders Roxell <anders.roxell@linaro.org> > > > > Cc: netdev <netdev@vger.kernel.org> > > > > Cc: linux-pm@vger.kernel.org > > > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") > > > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > > > --- > > > > drivers/base/dd.c | 13 ++----------- > > > > 1 file changed, 2 insertions(+), 11 deletions(-) > > > > > > Sorry for being a bit late to the party, but this breaks suspend/resume > > > support on various Tegra devices. I've only noticed now because, well, > > > suspend/resume have been broken for other reasons for a little while and > > > it's taken us a bit to resolve those issues. > > > > > > But now that those other issues have been fixed, I've started seeing an > > > issue where after resume from suspend some of the I2C controllers are no > > > longer working. The reason for this is that they share pins with DP AUX > > > controllers via the pinctrl framework. The DP AUX driver registers as > > > part of the DRM/KMS driver, which usually happens in userspace. Since > > > the deferred probe timeout was set to 0 by default this no longer works > > > because no pinctrl states are assigned to the I2C controller and > > > therefore upon resume the pins cannot be configured for I2C operation. > > > > Oof. My apologies! > > > > > I'm also somewhat confused by this patch and a few before because they > > > claim that they restore previous default behaviour, but that's just not > > > true. Originally when this timeout was introduced it was -1, which meant > > > that there was no timeout at all and hence users had to opt-in if they > > > wanted to use a deferred probe timeout. > > > > I don't think that's quite true, since the point of my original > > changes were to avoid troubles I was seeing with drivers not loading > > because once the timeout fired after init, driver loading would fail > > with ENODEV instead of returning EPROBE_DEFER. The logic that existed > > was buggy so the timeout handling didn't really work (changing the > > boot argument wouldn't help, because after init the logic would return > > ENODEV before it checked the timeout value). > > > > That said, looking at it now, I do realize the > > driver_deferred_probe_check_state_continue() logic in effect never > > returned ETIMEDOUT before was consolidated in the earlier changes, and > > now we've backed the default timeout to 0, old user (see bec6c0ecb243) > > will now get ETIMEDOUT where they wouldn't before. > > > > So would the following fix it up for you? (sorry its whitespace corrupted) > > > > diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c > > index c6fe7d64c913..c7448be64d07 100644 > > --- a/drivers/pinctrl/devicetree.c > > +++ b/drivers/pinctrl/devicetree.c > > @@ -129,9 +129,8 @@ static int dt_to_map_one_config(struct pinctrl *p, > > 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 && > > - (ret == -ENODEV)) > > + /* keep deferring if modules are enabled */ > > + if (IS_ENABLED(CONFIG_MODULES) && > > !allow_default && ret < 0) > > ret = -EPROBE_DEFER; > > return ret; > > } > > > > (you could probably argue calling driver_deferred_probe_check_state > > checking ret at all is silly here, since EPROBE_DEFER is the only > > option you want) > > Just by looking at it I would've said, yes, this looks like it would fix > it. However, upon giving this a try, I see the same pinmux issues as I > was seeing before. I'm going to have to dig a little deeper to see if I > can find out why that is, because it isn't obvious to me right away. Oh, nevermind. I managed to somehow mess up the device tree. With that fixed the above also restores the pinmux attachment for the I2C controller and suspend/resume works fine. Thierry
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 06ec0e851fa1..908ae4d7805e 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -224,16 +224,7 @@ 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 - */ -int driver_deferred_probe_timeout = 30; -#else -/* In the case of !modules, no probe timeout needed */ -int driver_deferred_probe_timeout = -1; -#endif +int driver_deferred_probe_timeout; EXPORT_SYMBOL_GPL(driver_deferred_probe_timeout); static int __init deferred_probe_timeout_setup(char *str) @@ -266,7 +257,7 @@ int driver_deferred_probe_check_state(struct device *dev) return -ENODEV; } - if (!driver_deferred_probe_timeout) { + if (!driver_deferred_probe_timeout && initcalls_done) { dev_WARN(dev, "deferred probe timeout, ignoring dependency"); return -ETIMEDOUT; }
This patch addresses a regression in 5.7-rc1+ In commit c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic"), we both cleaned up the logic and also 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. Fixing that issue is possible, but could also introduce 30 second delays in bootups for users who don't have any missing dependencies, which is not ideal. So I think the best solution to avoid any regressions is to revert back to a default timeout value of zero, and allow systems that need to utilize the timeout in order for userland to load any modules that supply misisng dependencies in the dts to specify the timeout length via the exiting documented boot argument. Thanks to Geert for chasing down that ip_auto_config was why NFS was failing in this case! Cc: "David S. Miller" <davem@davemloft.net> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Rob Herring <robh@kernel.org> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Sudeep Holla <sudeep.holla@arm.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Naresh Kamboju <naresh.kamboju@linaro.org> Cc: Basil Eljuse <Basil.Eljuse@arm.com> Cc: Ferry Toth <fntoth@gmail.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Anders Roxell <anders.roxell@linaro.org> Cc: netdev <netdev@vger.kernel.org> Cc: linux-pm@vger.kernel.org Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Fixes: c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state() logic") Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/base/dd.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)