Messages in this thread | | | From | John Stultz <> | Date | Tue, 18 Feb 2020 17:11:00 -0800 | Subject | Re: [PATCH v3 1/2] driver core: Rework logic in __driver_deferred_probe_check_state to allow EPROBE_DEFER to be returned for longer |
| |
On Tue, Feb 18, 2020 at 4:19 PM John Stultz <john.stultz@linaro.org> wrote: > > On Tue, Feb 18, 2020 at 4:06 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wednesday, February 19, 2020, John Stultz <john.stultz@linaro.org> wrote: > >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c > >> index b25bcab2a26b..9d916a7b56a6 100644 > >> --- a/drivers/base/dd.c > >> +++ b/drivers/base/dd.c > >> @@ -237,13 +237,12 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup); > >> > >> static int __driver_deferred_probe_check_state(struct device *dev) > >> { > >> - if (!initcalls_done) > >> - return -EPROBE_DEFER; > > > > > > Why to touch this? Can't you simple add a new condition here 'if (deferred_probe_timeout > 0)'... ? > > I think that might work. I'll give it a spin later tonight and double check it. > > The main thing I wanted to do is fix the logic hole in the current > code where after initcalls_done=true but before deferred_probe_timeout > has expired we just fall through and return 0, which results in an > ENODEV being returned from the calling function.
So on IRC Bjorn sort of clarified a point I think Rob was trying to make on the earlier iteration of this patch, that it seems like Thierry's patch here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=62a6bc3a1e4f4ee9ae0076fa295f9af1c3725ce3 *seems* to be trying to address the exact same issue, and maybe we should just have the genpd code use that instead?
The main question though, is why do we need both? As mentioned above, the existing logic in __driver_deferred_probe_check_state() seems wrong: Until late_initcall it returns EPROBE_DEFER, then after initcalls_done==true returns 0 (in which case the caller then translates to ENODEV), until the timeout expires which it then returns ETIMEDOUT.
I suspect what is really wanted is EPROBE_DEFER -> (0) ENODEV (when timeout is not set) or EPROBE_DEFER -> ETIMEOUT (when the timeout is set), instead of the two state transitions it currently makes.
So I still think my patch is needed, but I also suspect a better fix would be to kill driver_deferred_probe_check_state() and just replace its usage with driver_deferred_probe_check_state_continue(). Or am I still missing something?
thanks -john
| |