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: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Saravana Kannan <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Pavel Machek <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Rob Herring <[email protected]>
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 <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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;
}
--
2.17.1
On Sat, Aug 8, 2020 at 6:35 AM John Stultz <[email protected]> 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: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Rob Herring <[email protected]>
> 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 <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
This looks sensible to me, but I'm not the best with the device core.
It'd be good to have some Greg or Rafael stamp on this.
Can we have some comments? Reviewed-by?
Yours,
Linus Walleij
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: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Rob Herring <[email protected]>
> 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 <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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/[email protected]/
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
On Fri, Aug 28, 2020 at 5:02 AM Thierry Reding <[email protected]> wrote:
>
> 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: [email protected]
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Linus Walleij <[email protected]>
> > Cc: Thierry Reding <[email protected]>
> > Cc: Mark Brown <[email protected]>
> > Cc: Liam Girdwood <[email protected]>
> > Cc: Bjorn Andersson <[email protected]>
> > Cc: Saravana Kannan <[email protected]>
> > Cc: Todd Kjos <[email protected]>
> > Cc: Len Brown <[email protected]>
> > Cc: Pavel Machek <[email protected]>
> > Cc: Ulf Hansson <[email protected]>
> > Cc: Kevin Hilman <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > 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/[email protected]/
>
> 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.
Yea. I agree that your patch is preferred.
> 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.
Is it likely that the special case situation is common? It seems
having the driver do the special casing in its code (even better, with
an explanation for why that driver needs it) will be better then
having multiple similar hooks that don't have clear explanations that
drivers are likely to mix up.
thanks
-john
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: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Rob Herring <[email protected]>
> 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 <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> drivers/pinctrl/devicetree.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On Sat, Aug 8, 2020 at 6:35 AM John Stultz <[email protected]> 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: [email protected]
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Pavel Machek <[email protected]>
> Cc: Ulf Hansson <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Rob Herring <[email protected]>
> 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 <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
As noted I applied Thierry's similar patch.
Yours,
Linus Walleij