From: Rob Herring <robh@kernel.org> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Linus Walleij <linus.walleij@linaro.org>, Alexander Graf <agraf@suse.de>, Bjorn Andersson <bjorn.andersson@linaro.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, Kevin Hilman <khilman@kernel.org>, Ulf Hansson <ulf.hansson@linaro.org>, Joerg Roedel <joro@8bytes.org>, Robin Murphy <robin.murphy@arm.com>, Mark Brown <broonie@kernel.org>, Frank Rowand <frowand.list@gmail.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, devicetree@vger.kernel.org, Architecture Mailman List <boot-architecture@lists.linaro.org>, "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v2 1/8] driver core: make deferring probe after init optional Date: Thu, 24 May 2018 15:57:54 -0500 [thread overview] Message-ID: <CAL_JsqJK69A3W=YrSfvPetM95CROOJhU1bW1rMEOGtji=t0ORQ@mail.gmail.com> (raw) In-Reply-To: <20180524190031.GB31019@kroah.com> On Thu, May 24, 2018 at 2:00 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote: >> Deferred probe will currently wait forever on dependent devices to probe, >> but sometimes a driver will never exist. It's also not always critical for >> a driver to exist. Platforms can rely on default configuration from the >> bootloader or reset defaults for things such as pinctrl and power domains. >> This is often the case with initial platform support until various drivers >> get enabled. There's at least 2 scenarios where deferred probe can render >> a platform broken. Both involve using a DT which has more devices and >> dependencies than the kernel supports. The 1st case is a driver may be >> disabled in the kernel config. The 2nd case is the kernel version may >> simply not have the dependent driver. This can happen if using a newer DT >> (provided by firmware perhaps) with a stable kernel version. >> >> Subsystems or drivers may opt-in to this behavior by calling >> driver_deferred_probe_check_init_done() instead of just returning >> -EPROBE_DEFER. They may use additional information from DT or kernel's >> config to decide whether to continue to defer probe or not. >> >> Cc: Alexander Graf <agraf@suse.de> >> Signed-off-by: Rob Herring <robh@kernel.org> >> --- >> drivers/base/dd.c | 17 +++++++++++++++++ >> include/linux/device.h | 2 ++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index c9f54089429b..d6034718da6f 100644 >> --- a/drivers/base/dd.c >> +++ b/drivers/base/dd.c >> @@ -226,6 +226,16 @@ void device_unblock_probing(void) >> driver_deferred_probe_trigger(); >> } >> >> +int driver_deferred_probe_check_init_done(struct device *dev, bool optional) >> +{ >> + if (optional && initcalls_done) { > > Wait, what's the "optional" mess here? My intent was that subsystems just always call this function and never return EPROBE_DEFER themselves. Then the driver core can make decisions as to what to do (such as the timeout added in the next patch). Or it can print common error/debug messages. So optional is a hint to allow subsystems per device control. > > The caller knows this value, so why do you need to even pass it in here? Because regardless of the value, we always stop deferring when/if we hit the timeout and the caller doesn't know about the timeout. If we get rid of it, we'd need functions for both init done and for deferred timeout. > And bool values that are not obvious are horrid. I had to go look this > up when reading the later patches that just passed "true" in this > variable as I had no idea what that meant. Perhaps inverting it and calling it "keep_deferring" would be better. However, the flag is ignored if we have timed out. > > So as-is, no, this isn't ok, sorry. > > And at the least, this needs some kerneldoc to explain it :) That part is easy enough to fix. Rob
WARNING: multiple messages have this Message-ID (diff)
From: robh@kernel.org (Rob Herring) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 1/8] driver core: make deferring probe after init optional Date: Thu, 24 May 2018 15:57:54 -0500 [thread overview] Message-ID: <CAL_JsqJK69A3W=YrSfvPetM95CROOJhU1bW1rMEOGtji=t0ORQ@mail.gmail.com> (raw) In-Reply-To: <20180524190031.GB31019@kroah.com> On Thu, May 24, 2018 at 2:00 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, May 24, 2018 at 12:50:17PM -0500, Rob Herring wrote: >> Deferred probe will currently wait forever on dependent devices to probe, >> but sometimes a driver will never exist. It's also not always critical for >> a driver to exist. Platforms can rely on default configuration from the >> bootloader or reset defaults for things such as pinctrl and power domains. >> This is often the case with initial platform support until various drivers >> get enabled. There's at least 2 scenarios where deferred probe can render >> a platform broken. Both involve using a DT which has more devices and >> dependencies than the kernel supports. The 1st case is a driver may be >> disabled in the kernel config. The 2nd case is the kernel version may >> simply not have the dependent driver. This can happen if using a newer DT >> (provided by firmware perhaps) with a stable kernel version. >> >> Subsystems or drivers may opt-in to this behavior by calling >> driver_deferred_probe_check_init_done() instead of just returning >> -EPROBE_DEFER. They may use additional information from DT or kernel's >> config to decide whether to continue to defer probe or not. >> >> Cc: Alexander Graf <agraf@suse.de> >> Signed-off-by: Rob Herring <robh@kernel.org> >> --- >> drivers/base/dd.c | 17 +++++++++++++++++ >> include/linux/device.h | 2 ++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index c9f54089429b..d6034718da6f 100644 >> --- a/drivers/base/dd.c >> +++ b/drivers/base/dd.c >> @@ -226,6 +226,16 @@ void device_unblock_probing(void) >> driver_deferred_probe_trigger(); >> } >> >> +int driver_deferred_probe_check_init_done(struct device *dev, bool optional) >> +{ >> + if (optional && initcalls_done) { > > Wait, what's the "optional" mess here? My intent was that subsystems just always call this function and never return EPROBE_DEFER themselves. Then the driver core can make decisions as to what to do (such as the timeout added in the next patch). Or it can print common error/debug messages. So optional is a hint to allow subsystems per device control. > > The caller knows this value, so why do you need to even pass it in here? Because regardless of the value, we always stop deferring when/if we hit the timeout and the caller doesn't know about the timeout. If we get rid of it, we'd need functions for both init done and for deferred timeout. > And bool values that are not obvious are horrid. I had to go look this > up when reading the later patches that just passed "true" in this > variable as I had no idea what that meant. Perhaps inverting it and calling it "keep_deferring" would be better. However, the flag is ignored if we have timed out. > > So as-is, no, this isn't ok, sorry. > > And at the least, this needs some kerneldoc to explain it :) That part is easy enough to fix. Rob
next prev parent reply other threads:[~2018-05-24 20:58 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-24 17:50 [PATCH v2 0/8] Make deferring probe forever optional Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 17:50 ` [PATCH v2 1/8] driver core: make deferring probe after init optional Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 18:18 ` Mark Brown 2018-05-24 18:18 ` Mark Brown 2018-05-24 20:25 ` Rob Herring 2018-05-24 20:25 ` Rob Herring 2018-05-25 11:47 ` Robin Murphy 2018-05-25 11:47 ` Robin Murphy 2018-05-29 5:12 ` Frank Rowand 2018-05-29 5:12 ` Frank Rowand 2018-05-29 14:46 ` Rob Herring 2018-05-29 14:46 ` Rob Herring 2018-05-29 14:46 ` Rob Herring 2018-05-24 18:56 ` Greg Kroah-Hartman 2018-05-24 18:56 ` Greg Kroah-Hartman 2018-05-24 19:42 ` Rob Herring 2018-05-24 19:42 ` Rob Herring 2018-05-24 19:00 ` Greg Kroah-Hartman 2018-05-24 19:00 ` Greg Kroah-Hartman 2018-05-24 20:57 ` Rob Herring [this message] 2018-05-24 20:57 ` Rob Herring 2018-05-25 12:20 ` Robin Murphy 2018-05-25 12:20 ` Robin Murphy 2018-05-25 17:35 ` Rob Herring 2018-05-25 17:35 ` Rob Herring 2018-05-24 22:28 ` Bjorn Andersson 2018-05-24 22:28 ` Bjorn Andersson 2018-05-24 23:47 ` Rob Herring 2018-05-24 23:47 ` Rob Herring 2018-05-24 17:50 ` [PATCH v2 2/8] driver core: add a deferred probe timeout Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 19:01 ` Greg Kroah-Hartman 2018-05-24 19:01 ` Greg Kroah-Hartman 2018-05-24 19:45 ` Rob Herring 2018-05-24 19:45 ` Rob Herring 2018-05-24 19:51 ` Greg Kroah-Hartman 2018-05-24 19:51 ` Greg Kroah-Hartman 2018-05-24 17:50 ` [PATCH v2 3/8] dt-bindings: pinctrl: add a 'pinctrl-use-default' property Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 17:50 ` [PATCH v2 4/8] arm: dts: bcm283x: mark the UART pin muxing nodes with pinctrl-use-default Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 17:50 ` [PATCH v2 5/8] pinctrl: optionally stop deferring probe at end of initcalls Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-30 7:00 ` Linus Walleij 2018-05-30 7:00 ` Linus Walleij 2018-05-30 7:00 ` Linus Walleij 2018-05-24 17:50 ` [PATCH v2 6/8] iommu: Stop " Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 17:50 ` Rob Herring [not found] ` <20180524175024.19874-1-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2018-05-24 17:50 ` [PATCH v2 7/8] iommu: Remove IOMMU_OF_DECLARE Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-24 17:50 ` Rob Herring [not found] ` <20180524175024.19874-8-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 2018-05-25 11:31 ` Will Deacon 2018-05-25 11:31 ` Will Deacon 2018-05-25 11:31 ` Will Deacon 2018-05-28 6:53 ` Marek Szyprowski 2018-05-28 6:53 ` Marek Szyprowski 2018-05-28 6:53 ` Marek Szyprowski 2018-05-24 17:50 ` [PATCH v2 8/8] PM / Domains: Stop deferring probe at the end of initcall Rob Herring 2018-05-24 17:50 ` Rob Herring 2018-05-29 11:48 ` [PATCH v2 0/8] Make deferring probe forever optional Joerg Roedel 2018-05-29 11:48 ` Joerg Roedel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAL_JsqJK69A3W=YrSfvPetM95CROOJhU1bW1rMEOGtji=t0ORQ@mail.gmail.com' \ --to=robh@kernel.org \ --cc=agraf@suse.de \ --cc=bjorn.andersson@linaro.org \ --cc=boot-architecture@lists.linaro.org \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=frowand.list@gmail.com \ --cc=gregkh@linuxfoundation.org \ --cc=joro@8bytes.org \ --cc=khilman@kernel.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=rjw@rjwysocki.net \ --cc=robin.murphy@arm.com \ --cc=ulf.hansson@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.