Messages in this thread Patch in this message | | | Date | Fri, 28 Aug 2020 14:01:54 +0200 | From | Thierry Reding <> | Subject | Re: [RFC][PATCH] pinctrl: Rework driver_deferred_probe_check_state() evaluation since default timeout has changed |
| |
On Sat, Aug 08, 2020 at 04:35:12AM +0000, John Stultz wrote: > In commit bec6c0ecb243 ("pinctrl: Remove use of > driver_deferred_probe_check_state_continue()"), we removed the > use of driver_deferred_probe_check_state_continue() which > effectively never returned -ETIMED_OUT, with the > driver_deferred_probe_check_state() function that had been > reworked to properly return ETIMED_OUT when the deferred probe > timeout expired. Along with that change, we set the default > timeout to 30 seconds. > > However, since moving the timeout to 30 seconds caused some > issues for some users with optional dt links, we set the > default timeout back to zero - see commit ce68929f07de ("driver > core: Revert default driver_deferred_probe_timeout value to 0") > > This in essence changed the behavior of the pinctrl's usage > of driver_deferred_probe_check_state(), as it now would return > ETIMED_OUT by default. Thierry reported this caused problems with > resume on tegra platforms. > > Thus this patch tweaks the pinctrl logic so that it behaves as > before. If modules are enabled, we'll only return EPROBE_DEFERRED > while we're missing drivers linked in the DT. > > Cc: linux-pm@vger.kernel.org > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Thierry Reding <treding@nvidia.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Liam Girdwood <lgirdwood@gmail.com> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Cc: Saravana Kannan <saravanak@google.com> > Cc: Todd Kjos <tkjos@google.com> > Cc: Len Brown <len.brown@intel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Rob Herring <robh@kernel.org> > Fixes: bec6c0ecb243 ("pinctrl: Remove use of driver_deferred_probe_check_state_continue()") > Fixes: ce68929f07de ("driver core: Revert default driver_deferred_probe_timeout value to 0") > Reported-by: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > drivers/pinctrl/devicetree.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c > index c6fe7d64c913..09ddf567ccb4 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 = -EPROBE_DEFER; > return ret; > }
I posted almost exactly the same patch a couple of days ago since I hadn't noticed this:
https://patchwork.ozlabs.org/project/linux-gpio/patch/20200825143348.1358679-1-thierry.reding@gmail.com/
I like that slightly better because it keeps the "ret < 0" condition, which I think is perhaps a bit more future-proof. Thinking about it, I'm not sure your version above is entirely correct. For example if the call to driver_deferred_probe_check_state() were to ever return 0, we might still be returning -EPROBE_DEFER here.
That's not something that happens currently, but I suspect that these implications will be easy to overlook.
Actually... I think it might be best to just bring back (albeit perhaps in a modified form) driver_deferred_probe_check_state_continue() because we're now basically doing exactly what that was supposed to do: special- casing the case where we do want to continue returning -EPROBE_DEFER in some special cases.
Thierry [unhandled content-type:application/pgp-signature] | |