Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221114104333.3695531-1-javierm@redhat.com>
In-Reply-To: <20221114104333.3695531-1-javierm@redhat.com>
From:   John Stultz <jstultz@google.com>
Date:   Tue, 15 Nov 2022 01:33:40 -0800
Message-ID: <CANDhNCrSO_kByOCEHUyrdnbmaypt+u2Fw86iASDAjwbTNU3j3Q@mail.gmail.com>
Subject: Re: [PATCH] driver core: Disable driver deferred probe timeout by default
To:     Javier Martinez Canillas <javierm@redhat.com>
Cc:     linux-kernel@vger.kernel.org,
        Saravana Kannan <saravanak@google.com>,
        Peter Robinson <pbrobinson@redhat.com>,
        Steev Klimaszewski <steev@kali.org>,
        Rob Herring <robh@kernel.org>,
        Sergio Lopez Pascual <slp@redhat.com>,
        Enric Balletbo i Serra <eballetbo@redhat.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 14, 2022 at 2:43 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> The driver_deferred_probe_timeout value has a long story. It was first set
> to -1 when it was introduced by commit 25b4e70dcce9 ("driver core: allow
> stopping deferred probe after init"), meaning that the driver core would
> defer the probe forever unless a subsystem would opt-in by checking if the
> initcalls where done using the driver_deferred_probe_check_state() helper,
> or if a timeout was explicitly set with a "deferred_probe_timeout" param.

Indeed, it is a long history, I appreciate your archaeology here and
I'm sorry for my part in adding complexity to it.

That said, I don't believe the above is quite right (but the logic was
quite confusing, so it's easy to get wrong - and it's late here so
forgive me if I muck up my own explanation).

As I mentioned in c8c43cee29f6 ("driver core: Fix
driver_deferred_probe_check_state() logic"):
"
    driver_deferred_probe_check_state() has some uninituitive behavior.

    * From boot to late_initcall, it returns -EPROBE_DEFER

    * From late_initcall to the deferred_probe_timeout (if set)
      it returns -ENODEV

    * If the deferred_probe_timeout it set, after it fires, it
      returns -ETIMEDOUT

    This is a bit confusing, as its useful to have the function
    return -EPROBE_DEFER while the timeout is still running. This
    behavior has resulted in the somwhat duplicative
    driver_deferred_probe_check_state_continue() function being
    added.
"

Looking at the code before this change, at late_initcall time, we'd
run deferred_probe_initcall():
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/dd.c?id=e94f62b7140fa3da4c69a685b2e73ef52dd32c51#n321
Which sets:  initcalls_done = true;

Then in __driver_deferred_probe_check_state():
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/dd.c?id=e94f62b7140fa3da4c69a685b2e73ef52dd32c51#n238

We check initcalls_done, and after its set, we stop returning
-EPROBE_DEFER. If deferred_probe_timeout was not set, it would then
return 0.

But then in driver_deferred_probe_check_state()
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/dd.c?id=e94f62b7140fa3da4c69a685b2e73ef52dd32c51#n262

When we see the 0 back from from __driver_deferred_probe_check_state()
we'll return -ENODEV.

So I don't think it's true that before the change the driver core
would defer the probe forever when there was no timeout.

The problem we had at the time was that there were dts optional links
(like iommus that may be in the dts but may not have a kernel driver),
so if there is no module to load, we don't want to defer probing
forever. The cutoff point was originally set to late_initcall time
with a timeout option to extend it further.  However, with Android,
modules often wouldn't get a chance to load until much after
late_initcall time.

But setting the timeout didn't actually help to extend the probe time,
because it would return -ENODEV after late_initcall. -  That's what my
patch was trying to address.

I also tried to move the default to 30 so we'd not need to set a boot
parameter to get modules to load on a normal boot, but that caused
some trouble w/ with NFS root systems and the fix there would mean
that they'd have to wait 30 seconds before the rootfs would mount.

> Only the power domain, IOMMU and MDIO subsystems currently opt-in to check
> if the initcalls have completed with driver_deferred_probe_check_state().
>
> Commit c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state()
> logic") then changed the driver_deferred_probe_check_state() helper logic,
> to take into account whether modules have been enabled or not and also to
> return -EPROBE_DEFER if the probe deferred timeout was still running.
>
> Then in commit e2cec7d68537 ("driver core: Set deferred_probe_timeout to a
> longer default if CONFIG_MODULES is set"), the timeout was increased to 30
> seconds if modules are enabled. Because seems that some of the subsystems
> that were opt-in to not return -EPROBE_DEFER after the initcall where done
> could still have dependencies whose drivers were built as a module.
>
> This commit did a fundamental change to how probe deferral worked though,
> since now the default was not to attempt probing for drivers indefinitely
> but instead it would timeout after 30 seconds unless a different timeout
> was set using the "deferred_probe_timeout" parameter.
>
> The behavior was changed even mere with commit ce68929f07de ("driver core:
> Revert default driver_deferred_probe_timeout value to 0"), since the value
> was set to 0 by default. Meaning that the probe deferral would be disabled
> after the initcalls where done. Unless a timeout was set in the cmdline.
>
> Notice that the commit said that it was reverting the default value to 0,
> but this was never 0. The default was -1 at the beginning and then changed
> to 30 in a later commit.

Hrm.  So -1 sort of changed meaning after my initial
change(c8c43cee29f6), because after that it did indeed change to being
indefinite EPROBE_DEFER.
For !modules systems, it was ok to use -1 before, because the check
here would catch it:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/dd.c?id=ce68929f07de#n255

But for modular systems, probing forever would break systems that have
optional links in their DTS with no matching drivers, as the dependent
drivers will never load.

So setting the default to 0 was done to match the earlier behavior
(with probing ending at late_initcall time).

> This default value of 0 was reverted again by commit f516d01b9df2 ("Revert
> "driver core: Set default deferred_probe_timeout back to 0."") and set to
> 10 seconds instead. Which was still less than the 30 seconds that was set
> at some point to allow systems with drivers built as modules and loaded by
> user-land to probe drivers that were previously deferred.
>
> The 10 seconds timeout isn't enough for the mentioned systems, for example
> general purpose distributions attempt to build all the possible drivers as
> a module to keep the Linux kernel image small. But that means that in very
> likely that the probe deferral mechanism will timeout and drivers won't be
> probed correctly.
>
> So let's change the default again to -1 as it was at the beginning. That's
> how probe deferral always worked. In fact, it could even be that users can
> load modules manually after the system has already booted so it is better
> to not assume when it can be safe to just timeout instead of probe defer.

So I worry setting it to -1 by default will cause regressions on
systems with optional dts links.
But maybe I'm still missing something?

thanks
-john