driver core: Rework logic in __driver_deferred_probe_check_state to allow EPROBE_DEFER to be returned for longer
Subsystems (2)
PATCH v3 1/2 driver core: Rework logic in __driver_deferred_probe_check_state to allow EPROBE_DEFER to be returned for longer
Due to commit e01afc3250255 ("PM / Domains: Stop deferring probe at the end of initcall"), along with commit 25b4e70dcce9 ("driver core: allow stopping deferred probe after init") after late_initcall, drivers will stop getting EPROBE_DEFER, and instead see an error causing the driver to fail to load.
That change causes trouble when trying to use many clk drivers as modules, as the clk modules may not load until much later after init has started. If a dependent driver loads and gets an error instead of EPROBE_DEFER, it won't try to reload later when the dependency is met, and will thus fail to load.
This patch reworks some of the logic in __driver_deferred_probe_check_state() so that if the deferred_probe_timeout value is set, we will return EPROBE_DEFER until that timeout expires, which may be after initcalls_done is set to true.
Specifically, on db845c, this change (when combined with booting using deferred_probe_timeout=30) allows us to set SDM_GPUCC_845, QCOM_CLK_RPMH and COMMON_CLK_QCOM as modules and get a working system, where as without it the display will fail to load.
---
v2:
* Add calls to driver_deferred_probe_trigger() after the two minute timeout, as suggested by Bjorn
* Minor whitespace cleanups
* Switch to 30 second timeout to match what the regulator code is doing as suggested by Rob.
v3:
* Rework to reuse existing deferred_probe_timeout value, suggested by Rob
* Dropped Fixes: tags as Rob requested (Not my hill to die on :)
--- drivers/base/dd.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/base/dd.c +3 -4
@@ -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 ; - -if (!deferred_probe_timeout) { +if (initcalls_done && !deferred_probe_timeout) {dev_WARN (dev,"deferred probe timeout, ignoring dependency" );return -ETIMEDOUT ; } +if (!initcalls_done || deferred_probe_timeout >0 ) +return -EPROBE_DEFER ;return 0 ; }
Signature · 1 line
2.17.1
PATCH v3 2/2 driver core: Make deferred_probe_timeout global so it can be shared
This patch, suggested by Rob, allows deferred_probe_timeout to be global so other substems can use it.
This also sets the default to 30 instead of -1 (no timeout) and modifies the regulator code to make use of it instead of its hard-coded 30 second interval.
In the case that deferred_probe_timeout is manually set to -1, we preserve the regulator's hard coded 30 second interval (just to be cautious this doesn't change behavior in that case).
Feedback would be apprecaited!
--- drivers/base/dd.c | 4 +++- drivers/regulator/core.c | 12 ++++++++---- include/linux/device/driver.h | 1 + 3 files changed, 12 insertions(+), 5 deletions(-)
drivers/base/dd.c +3 -1
@@ -224,7 +224,9 @@ static int deferred_devs_show(struct seq_file *s, void *data) }DEFINE_SHOW_ATTRIBUTE (deferred_devs); -static int deferred_probe_timeout =-1 ; +int deferred_probe_timeout =30 ; +EXPORT_SYMBOL_GPL (deferred_probe_timeout); +static int __init deferred_probe_timeout_setup (char *str) {int timeout;
drivers/regulator/core.c +8 -4
@@ -5757,6 +5757,11 @@ static DECLARE_DELAYED_WORK(regulator_init_complete_work,static int __init regulator_init_complete (void ) { +int delay = deferred_probe_timeout; + +/* preserve 30 second interval if deferred_probe_timeout=-1 */ +if (delay <0 ) + delay =30 ;/* * Since DT doesn't provide an idiomatic mechanism for * enabling full constraints and since it's much more natural
@@ -5767,18 +5772,17 @@ static int __init regulator_init_complete(void) has_full_constraints = true;/* -* We punt completion for an arbitrary amount of time since +* We punt completion for deferred_probe_timeout seconds since * systems like distros will load many drivers from userspace * so consumers might not always be ready yet, this is * particularly an issue with laptops where this might bounce * the display off then on. Ideally we'd get a notification * from userspace when this happens but we don't so just wait * a bit and hope we waited long enough. It'd be better if -* we'd only do this on systems that need it, and a kernel -* command line option might be useful. +* we'd only do this on systems that need it. */ schedule_delayed_work (®ulator_init_complete_work, -msecs_to_jiffies (30000 )); + delay *HZ );return 0 ; }
include/linux/device/driver.h +1 -0
@@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) }#endif +extern int deferred_probe_timeout;void driver_deferred_probe_add (struct device *dev);int driver_deferred_probe_check_state (struct device *dev);int driver_deferred_probe_check_state_continue (struct device *dev);
Signature · 1 line
2.17.1
Greg Kroah-Hartman
maintainerRe: [PATCH v3 2/2] driver core: Make deferred_probe_timeout global so it can be shared
include/linux/device/driver.h driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) · line 240
Quoted 8 lines · John Stultz
include/linux/device/driver.h +1
-0@@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) }#endif +extern int deferred_probe_timeout;void driver_deferred_probe_add (struct device *dev);
If this is going to be global now, can you rename it to "driver_defferred_probe_timeout" to make it more in line with the other exported symbols here?
thanks,
greg k-h
Re: [PATCH v3 2/2] driver core: Make deferred_probe_timeout global so it can be shared
Quoted 7 lines · Greg Kroah-Hartman
Quoted 6 lines · John Stultz
include/linux/device/driver.h +0
-0@@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) }#endif
Quoted 5 lines
include/linux/device/driver.h driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) · line 240
Quoted 2 lines
include/linux/device/driver.h +1
-0+extern int deferred_probe_timeout;void driver_deferred_probe_add (struct device *dev);If this is going to be global now, can you rename it to "
driver_defferred_probe_timeout" to make it more in line with the other
or driver_deferred_probe_timeout please.
Quoted 1 line
exported symbols here?
Signature · 1 line
~Randy
Greg Kroah-Hartman
maintainerRe: [PATCH v3 2/2] driver core: Make deferred_probe_timeout global so it can be shared
Quoted 8 lines · Randy Dunlap
Quoted 7 lines · Greg Kroah-Hartman
Quoted 6 lines · John Stultz
include/linux/device/driver.h +0
-0@@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) }#endif
Quoted 7 lines
Quoted 5 lines
include/linux/device/driver.h driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev) · line 240
Quoted 2 lines
include/linux/device/driver.h +1
-0+extern int deferred_probe_timeout;void driver_deferred_probe_add (struct device *dev);If this is going to be global now, can you rename it to "
driver_defferred_probe_timeout" to make it more in line with the otheror
driver_deferred_probe_timeoutplease.
Yes, that's spelt better :)
Mark Brown
maintainerRe: [PATCH v3 2/2] driver core: Make deferred_probe_timeout global so it can be shared
In reply to John StultzQuoted 6 lines · John Stultz
This patch, suggested by Rob, allows
deferred_probe_timeoutto be global so other substems can use it.This also sets the default to 30 instead of -1 (no timeout) and modifies the regulator code to make use of it instead of its hard-coded 30 second interval.
This is at least two patches, one adding the new feature and the other adding a user of that feature.
drivers/regulator/core.c static int __init regulator_init_complete(void) · line 5778
Quoted 9 lines
drivers/regulator/core.c +1
-1@@ -5767,18 +5772,17 @@ static int __init regulator_init_complete(void) has_full_constraints = true;/* -* We punt completion for an arbitrary amount of time since +* We punt completion for deferred_probe_timeout seconds since * systems like distros will load many drivers from userspace * so consumers might not always be ready yet, this is * particularly an issue with laptops where this might bounce
While I don't see it doing any harm I'm not 100% convinced by this change - we're not really doing anything directly to do with deferred probe here, we're shutting off regulators that remain unused late in boot but even then they'll still be available for use. It feels a bit unclear and the way you've adapted the code to always have a timeout even if the deferred probe timeout gets changed feels a bit off. If nothing else this comment needs more of an update than you've done.
Re: [PATCH v3 2/2] driver core: Make deferred_probe_timeout global so it can be shared
Quoted 11 lines · Mark Brown
Quoted 6 lines · John Stultz
This patch, suggested by Rob, allows
deferred_probe_timeoutto be global so other substems can use it.This also sets the default to 30 instead of -1 (no timeout) and modifies the regulator code to make use of it instead of its hard-coded 30 second interval.
This is at least two patches, one adding the new feature and the other adding a user of that feature.
Yea, agreed. I combined it here just to get input at this point without flooding folks with a ton of small patches.
Quoted 17 lines
drivers/regulator/core.c static int __init regulator_init_complete(void) · line 5778
Quoted 9 lines
drivers/regulator/core.c +1
-1@@ -5767,18 +5772,17 @@ static int __init regulator_init_complete(void) has_full_constraints = true;/* -* We punt completion for an arbitrary amount of time since +* We punt completion for deferred_probe_timeout seconds since * systems like distros will load many drivers from userspace * so consumers might not always be ready yet, this is * particularly an issue with laptops where this might bounce While I don't see it doing any harm I'm not 100% convinced by this change - we're not really doing anything directly to do with deferred probe here, we're shutting off regulators that remain unused late in boot but even then they'll still be available for use. It feels a bit unclear and the way you've adapted the code to always have a timeout even if the deferred probe timeout gets changed feels a bit off. If nothing else this comment needs more of an update than you've done.
This was mostly spurred by Rob's suggestion, as it seemed both subsystems were doing similar timeouts that may need to be customized (likely around the same amount of time). I agree it doesn't quite map perfectly, but I imagine it would be useful to align those in some way.
I'll likely take a swing first at getting the driver core bits to make sense, and then I'll come and revisit this shared timeout issue.
thanks
-john
Quoted 19 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.That change causes trouble when trying to use many clk drivers as modules, as the clk modules may not load until much later after init has started. If a dependent driver loads and gets an error instead of
EPROBE_DEFER, it won't try to reload later when the dependency is met, and will thus fail to load.This patch reworks some of the logic in
__driver_deferred_probe_check_state()so that if thedeferred_probe_timeoutvalue is set, we will returnEPROBE_DEFERuntil that timeout expires, which may be afterinitcalls_doneis set to true.
Quoted 4 lines
Specifically, on db845c, this change (when combined with booting using deferred_probe_timeout=30) allows us to set
SDM_GPUCC_845,QCOM_CLK_RPMHandCOMMON_CLK_QCOMas modules and get a working system, where as without it the display will fail to load.
I would change the default for deferred_probe_timeout to 30 and then regulator code can rely on that. Curious, why 30 sec is fine now when you originally had 2 min? I'd just pick what you think is best. I doubt Mark had any extensive experiments to come up with 30sec.
Quoted 51 lines
Cc Rob Herring <[email protected]>Cc "Rafael J. Wysocki" <[email protected]>Cc Kevin Hilman <[email protected]>Cc Ulf Hansson <[email protected]>Cc Pavel Machek <[email protected]>Cc Len Brown <[email protected]>Cc Todd Kjos <[email protected]>Cc Bjorn Andersson <[email protected]>Cc Liam Girdwood <[email protected]>Cc Mark Brown <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Signed-off-by John Stultz <[email protected]>---
v2:
* Add calls todriver_deferred_probe_trigger()after the two minute timeout, as suggested by Bjorn
* Minor whitespace cleanups
* Switch to 30 second timeout to match what the regulator code is doing as suggested by Rob.
v3:
* Rework to reuse existingdeferred_probe_timeoutvalue, suggested by Rob
* Dropped Fixes: tags as Rob requested (Not my hill to die on :)--- drivers/base/dd.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)drivers/base/dd.c +3
-5@@ -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 ; - -if (!deferred_probe_timeout) { +if (initcalls_done && !deferred_probe_timeout) {dev_WARN (dev,"deferred probe timeout, ignoring dependency" );return -ETIMEDOUT ; } +if (!initcalls_done || deferred_probe_timeout >0 ) +return -EPROBE_DEFER ;return 0 ; } --2.17.1
Quoted 21 lines · Rob Herring
Quoted 19 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.That change causes trouble when trying to use many clk drivers as modules, as the clk modules may not load until much later after init has started. If a dependent driver loads and gets an error instead of
EPROBE_DEFER, it won't try to reload later when the dependency is met, and will thus fail to load.This patch reworks some of the logic in
__driver_deferred_probe_check_state()so that if thedeferred_probe_timeoutvalue is set, we will returnEPROBE_DEFERuntil that timeout expires, which may be afterinitcalls_doneis set to true.
Quoted 7 lines
Quoted 4 lines
Specifically, on db845c, this change (when combined with booting using deferred_probe_timeout=30) allows us to set
SDM_GPUCC_845,QCOM_CLK_RPMHandCOMMON_CLK_QCOMas modules and get a working system, where as without it the display will fail to load.I would change the default for
deferred_probe_timeoutto 30 and then regulator code can rely on that.
That is exactly what I do in the following patch! Let me know if you have further suggestions for it.
Quoted 3 lines
Curious, why 30 sec is fine now when
you originally had 2 min? I'd just pick what you think is best. I doubt Mark had any extensive experiments to come up with 30sec.
I had two minutes initially as, due to other delays I still have to chase, booting all the way to UI on the db845c can sometimes take almost a minute. So it was just a rough safety estimate. But in v2, I tested with the 30 second time used by the regulator code, and that seemed to work ok.
As long as 30 seconds is working well, I'm ok with it for now (and it can be overrided via boot arg). Though from past experience with enterprise distros running on servers with tons of disks (which might take many minutes to initialize), I'd suspect its likely some environments may need much longer.
thanks
-john
Quoted 28 lines · John Stultz
Quoted 26 lines · Rob Herring
Quoted 23 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.That change causes trouble when trying to use many clk drivers as modules, as the clk modules may not load until much later after init has started. If a dependent driver loads and gets an error instead of
EPROBE_DEFER, it won't try to reload later when the dependency is met, and will thus fail to load.This patch reworks some of the logic in
__driver_deferred_probe_check_state()so that if thedeferred_probe_timeoutvalue is set, we will returnEPROBE_DEFERuntil that timeout expires, which may be afterinitcalls_doneis set to true.Specifically, on db845c, this change (when combined with booting using deferred_probe_timeout=30) allows us to set
SDM_GPUCC_845,QCOM_CLK_RPMHandCOMMON_CLK_QCOMas modules and get a working system, where as without it the display will fail to load.
Quoted 5 lines
Quoted 2 lines
I would change the default for
deferred_probe_timeoutto 30 and then regulator code can rely on that.That is exactly what I do in the following patch! Let me know if you have further suggestions for it.
Indeed.
Between the above comment and this comment in patch 2:
/* preserve 30 second interval if deferred_probe_timeout=-1 */
...I was confused.
Quoted 10 lines
Quoted 3 lines
Curious, why 30 sec is fine now when
you originally had 2 min? I'd just pick what you think is best. I doubt Mark had any extensive experiments to come up with 30sec.I had two minutes initially as, due to other delays I still have to chase, booting all the way to UI on the db845c can sometimes take almost a minute. So it was just a rough safety estimate. But in v2, I tested with the 30 second time used by the regulator code, and that seemed to work ok.
Quoted 5 lines
As long as 30 seconds is working well, I'm ok with it for now (and it can be overrided via boot arg). Though from past experience with enterprise distros running on servers with tons of disks (which might take many minutes to initialize), I'd suspect its likely some environments may need much longer.
Those systems aren't going to have a pinctrl or clock driver sitting in an encrypted RAID partition either. :)
Rob
Mark Brown
maintainerQuoted 10 lines · Rob Herring
Quoted 5 lines · John Stultz
As long as 30 seconds is working well, I'm ok with it for now (and it can be overrided via boot arg). Though from past experience with enterprise distros running on servers with tons of disks (which might take many minutes to initialize), I'd suspect its likely some environments may need much longer.
Those systems aren't going to have a pinctrl or clock driver sitting in an encrypted RAID partition either. :)
There speaks the voice of hope and optimism.
Quoted 36 lines · Rob Herring
Quoted 28 lines · John Stultz
Quoted 26 lines · Rob Herring
Quoted 23 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.That change causes trouble when trying to use many clk drivers as modules, as the clk modules may not load until much later after init has started. If a dependent driver loads and gets an error instead of
EPROBE_DEFER, it won't try to reload later when the dependency is met, and will thus fail to load.This patch reworks some of the logic in
__driver_deferred_probe_check_state()so that if thedeferred_probe_timeoutvalue is set, we will returnEPROBE_DEFERuntil that timeout expires, which may be afterinitcalls_doneis set to true.Specifically, on db845c, this change (when combined with booting using deferred_probe_timeout=30) allows us to set
SDM_GPUCC_845,QCOM_CLK_RPMHandCOMMON_CLK_QCOMas modules and get a working system, where as without it the display will fail to load.Quoted 5 lines
Quoted 2 lines
I would change the default for
deferred_probe_timeoutto 30 and then regulator code can rely on that.That is exactly what I do in the following patch! Let me know if you have further suggestions for it.
Quoted 7 lines
Indeed.
Between the above comment and this comment in patch 2:
/* preserve 30 second interval if deferred_probe_timeout=-1 */...I was confused.
Sorry. I added that out of an abundance of caution to avoid breaking things if someone booted specifying deferred_probe_timeout=-1 as a boot argument, since that would cause the regulator timeout to likely never expire.
Quoted 10 lines
Quoted 10 lines
Quoted 3 lines
Curious, why 30 sec is fine now when
you originally had 2 min? I'd just pick what you think is best. I doubt Mark had any extensive experiments to come up with 30sec.I had two minutes initially as, due to other delays I still have to chase, booting all the way to UI on the db845c can sometimes take almost a minute. So it was just a rough safety estimate. But in v2, I tested with the 30 second time used by the regulator code, and that seemed to work ok.
Quoted 8 lines
Quoted 5 lines
As long as 30 seconds is working well, I'm ok with it for now (and it can be overrided via boot arg). Though from past experience with enterprise distros running on servers with tons of disks (which might take many minutes to initialize), I'd suspect its likely some environments may need much longer.
Those systems aren't going to have a pinctrl or clock driver sitting in an encrypted RAID partition either. :)
Fair enough. Not today.. but it's always only a matter of time between "that's ridiculous!" and "oh, we need that!" :)
thanks
-john
Mark Brown
maintainerQuoted 11 lines · Rob Herring
Quoted 4 lines · John Stultz
Specifically, on db845c, this change (when combined with booting using deferred_probe_timeout=30) allows us to set
SDM_GPUCC_845,QCOM_CLK_RPMHandCOMMON_CLK_QCOMas modules and get a working system, where as without it the display will fail to load.I would change the default for
deferred_probe_timeoutto 30 and then regulator code can rely on that. Curious, why 30 sec is fine now when you originally had 2 min? I'd just pick what you think is best. I doubt Mark had any extensive experiments to come up with 30sec.
Sort of - I've spent a bunch of time looking at the sorts of devices where this is applicable for regulators and 30s is wildly excessive for the use case. I didn't specifically measure anything at the time I did the change though, even longer should work just as well.
That feature in the regulator framework is targetted quite narrowly at things we really don't want to glitch out during boot if we can avoid it like the display, people tend to make efforts to ensure that they come up quickly during boot anyway so we're not expecting to worry about the full boot time for bigger systems. The expectation is that most devices will cope fine with having the power turned off for a period and if the user can't see it happening then it doesn't *really* matter.
A message that is not in the archive (sent off-list or not yet received): CAHp75VcPL7DYp9hjgMu+d=CE=g+V7ZxT9ZyXX-OjEW_JQ4m_nA@mail.gmail.com
Quoted 7 lines · Andy Shevchenko
Quoted 6 lines · John Stultz
drivers/base/dd.c +0
-0@@ -237,13 +237,12 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
Quoted 7 lines
drivers/base/dd.c __setup("deferred_probe_timeout=", deferred_probe_timeout_setup); · line 241
Quoted 4 lines
drivers/base/dd.c +0
-2static 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.
thanks
-john
Quoted 18 lines · John Stultz
Quoted 7 lines · Andy Shevchenko
Quoted 6 lines · John Stultz
drivers/base/dd.c +0
-0@@ -237,13 +237,12 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);Quoted 7 lines
drivers/base/dd.c __setup("deferred_probe_timeout=", deferred_probe_timeout_setup); · line 241
Quoted 4 lines
drivers/base/dd.c +0
-2static 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)'... ?
Quoted 6 lines
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_timeouthas 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
Quoted 28 lines · John Stultz
Quoted 18 lines · John Stultz
Quoted 7 lines · Andy Shevchenko
Quoted 6 lines · John Stultz
drivers/base/dd.c +0
-0@@ -237,13 +237,12 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);Quoted 7 lines
drivers/base/dd.c __setup("deferred_probe_timeout=", deferred_probe_timeout_setup); · line 241
Quoted 4 lines
drivers/base/dd.c +0
-2static 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)'... ?Quoted 6 lines
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_timeouthas expired we just fall through and return 0, which results in an ENODEV being returned from the calling function.
Quoted 6 lines
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?
Looking at it some more, I think the change to the pinctrl code there breaks the case I care about (pinctrl described in DT and no driver on a system that previously worked without pinctrl). Maybe if I set the timeout to 0 it will still work (which would be fine).
Quoted 10 lines
The main question though, is why do we need both? As mentioned above, the existing logic in
__driver_deferred_probe_check_state()seems
wrong: Untillate_initcallit returnsEPROBE_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) orEPROBE_DEFER-> ETIMEOUT (when the timeout is set), instead of the two state transitions it currently makes.
Yes. There's never any reason to return 0. It should be one of 3 errnos. If we're moving to always having a timeout, then maybe ENODEV isn't even needed. I guess it's a stronger "we're done with init and there's never going to be another driver" which maybe we should do for !CONFIG_MODULES.
Quoted 4 lines
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 withdriver_deferred_probe_check_state_continue(). Or am I still missing something?
I think those should be merged. They now do almost the same thing. Only in the timeout==-1 case do they differ.
The original intent was that driver_deferred_probe_check_state() simply returned what state we're in and the caller would decide what to do with that. IOW, each caller could implement their own policy possibly based on other information. Pinctrl factored in a DT hint. IOMMU relied on everything was built-in.
The one complication which I mentioned already is with consoles. A timeout (and dependencies in modules) there doesn't work. You have to probe and register the console before init is done.
Rob
Quoted 59 lines · Rob Herring
Quoted 28 lines · John Stultz
Quoted 18 lines · John Stultz
Quoted 7 lines · Andy Shevchenko
Quoted 6 lines · John Stultz
drivers/base/dd.c +0
-0@@ -237,13 +237,12 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);Quoted 7 lines
drivers/base/dd.c __setup("deferred_probe_timeout=", deferred_probe_timeout_setup); · line 241
Quoted 4 lines
drivers/base/dd.c +0
-2static 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)'... ?Quoted 6 lines
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_timeouthas expired we just fall through and return 0, which results in an ENODEV being returned from the calling function.Quoted 6 lines
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?Looking at it some more, I think the change to the pinctrl code there breaks the case I care about (pinctrl described in DT and no driver on a system that previously worked without pinctrl). Maybe if I set the timeout to 0 it will still work (which would be fine).
Quoted 10 lines
The main question though, is why do we need both? As mentioned above, the existing logic in
__driver_deferred_probe_check_state()seems
wrong: Untillate_initcallit returnsEPROBE_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) orEPROBE_DEFER-> ETIMEOUT (when the timeout is set), instead of the two state transitions it currently makes.Yes. There's never any reason to return 0. It should be one of 3 errnos. If we're moving to always having a timeout, then maybe ENODEV isn't even needed. I guess it's a stronger "we're done with init and there's never going to be another driver" which maybe we should do for !CONFIG_MODULES.
Quoted 7 lines
Quoted 4 lines
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 withdriver_deferred_probe_check_state_continue(). Or am I still missing something?I think those should be merged. They now do almost the same thing. Only in the timeout==-1 case do they differ.
Well.. almost.. in addition to that different after late_initcall but before the timeout driver_deferred_probe_check_state() will return ENODEV, where as driver_deferred_probe_check_state_continue() returns EPROBE_DEFER until the timeout happens.
Quoted 5 lines
The original intent was that
driver_deferred_probe_check_state()simply returned what state we're in and the caller would decide what to do with that. IOW, each caller could implement their own policy possibly based on other information. Pinctrl factored in a DT hint. IOMMU relied on everything was built-in.
I'm not super opinionated here, having the subsystem opt in and decide what to do with the check_state() return value seems sane. But I suspect that we can consolidate the two cases down and simplify some of the logic here.
I'll take another spin on this.
thanks
-john
Apologies in advance for replying to this one email but discussing the points raised in all the other replies. I'm not cc'ed in this thread and replying to each email individually is a pain.
Quoted 5 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.
Both of those patches were the best solution at that point in time. But the kernel has changed a lot since then. Power domain and IOMMU drivers can work as modules now. We have of_devlink and sync_state().
So, while a delay might have been the ideal solution back then, I think we need to work towards removing arbitrary timeouts instead of making the timeout mechanism more elaborate.
I think driver_deferred_probe_check_state() logic should boiled down to something like:
int driver_deferred_probe_check_state(struct device *dev)
{
/* No modules and init done, deferred probes are pointless from
* now. */
if (!defined(CONFIG_MODULES) && initcall_done)
return -ENODEV;
/* If modules and of_devlink then you clean want dependencies to
* be enforced.
*/
if (defined(CONFIG_MODULES) && of_devlink)
return -EPROBE_DEFER;
/* Whatever other complexity (including timeouts) we want to
* add. Hopefully none - we can discuss in this thread. */
if (.....)
return -Exxxx;
/* When in doubt, allow probe deferral. */
return -EPROBE_DEFER;
}Rob, for the original use case those two patches were added for, do they need to support CONFIG_MODULES?
Quoted 5 lines
That change causes trouble when trying to use many clk drivers as modules, as the clk modules may not load until much later after init has started. If a dependent driver loads and gets an error instead of
EPROBE_DEFER, it won't try to reload later when the dependency is met, and will thus fail to load.
Once we add of_devlink support for power-domains, you won't even hit the genpd error path if you have of_devlink enabled. I believe in the case you are testing DB 845c, of_devlink is enabled?
If of_devlink is enabled, the devices depending on the unprobed power domains would be deferred without even calling the driver's probe() function.
Adding power-domain support to of_devlink is a 2 line change. I'll send it out soon.
Also, regulator_init_complete() can be replaced by a sync_state() based implementation. I have a downstream implementation that works. But it's definitely not upstream ready. I plan to rewrite it and send it upstream at some point, but it's fairly straightforward if anyone else want to implement it. My main point being, we shouldn't have to make the timeout logic more complex (or even need one) for regulator clean up either.
Quoted 3 lines · Rob Herring
The one complication which I mentioned already is with consoles. A timeout (and dependencies in modules) there doesn't work. You have to probe and register the console before init is done.
Rob,
I've seen you say this a couple of times before. But I don't think this is true any more. With of_devlink enabled I've booted hardware countless times with the console device probing after userspace comes up. The only limitation for console drivers is that they need to be built-in if they need to support earlycon. If you don't care to support earlycon (very useful for bringup debugging), I think the console driver can even be a module. I don't think even of_devlink needs to be enabled technically if you load the modules in the right order.
Thanks,
Saravana
Quoted 17 lines · Saravana Kannan
Quoted 5 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.Both of those patches were the best solution at that point in time. But the kernel has changed a lot since then. Power domain and IOMMU drivers can work as modules now. We have
of_devlinkandsync_state().So, while a delay might have been the ideal solution back then, I think we need to work towards removing arbitrary timeouts instead of making the timeout mechanism more elaborate.
I think
driver_deferred_probe_check_state()logic should boiled down to something like:
...
Quoted 10 lines
Once we add
of_devlinksupport for power-domains, you won't even hit the genpd error path if you haveof_devlinkenabled. I believe in the case you are testing DB 845c,of_devlinkis enabled?If
of_devlinkis enabled, the devices depending on the unprobed power domains would be deferred without even calling the driver'sprobe()function.Adding power-domain support to
of_devlinkis a 2 line change. I'll send it out soon.
...
Pardon me for not knowing the OF and devlink stuff in particular, but have you had a chance to test your changes on some (rather complex) ACPI enabled platforms?
Would it have any complications there?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 33 lines · Andy Shevchenko
Quoted 17 lines · Saravana Kannan
Quoted 5 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.Both of those patches were the best solution at that point in time. But the kernel has changed a lot since then. Power domain and IOMMU drivers can work as modules now. We have
of_devlinkandsync_state().So, while a delay might have been the ideal solution back then, I think we need to work towards removing arbitrary timeouts instead of making the timeout mechanism more elaborate.
I think
driver_deferred_probe_check_state()logic should boiled down to something like:...
Quoted 10 lines
Once we add
of_devlinksupport for power-domains, you won't even hit the genpd error path if you haveof_devlinkenabled. I believe in the case you are testing DB 845c,of_devlinkis enabled?If
of_devlinkis enabled, the devices depending on the unprobed power domains would be deferred without even calling the driver'sprobe()function.Adding power-domain support to
of_devlinkis a 2 line change. I'll send it out soon.
Quoted 6 lines
...
Pardon me for not knowing the OF and devlink stuff in particular, but have you had a chance to test your changes on some (rather complex) ACPI enabled platforms?
Would it have any complications there?
Hi Andy,
I'm not sure which changes you are referring to. So I'll try to answer all possibilities :)
If you are referring to the pseudo code proposal, it's mostly narrowing down the conditions under which the timeout will matter.
1. It's ignoring the timeout stuff if modules are not supported and all initcall levels are done.
2. If modules are enabled and of_devlink is enabled, it's making sure the timeout doesn't apply.
(1) seems like a straightforward assumption. (2) is functionally a NOP for ACPI based platforms. So I don't think we'd have ACPI specific complications here.
If you were referring to the "of_devlink support for power-domains" that also won't have any impact on ACPI platforms because right not it only runs for OF based systems. So again a NOP for ACPI.
Hope that answers your questions.
Thanks,
Saravana
Quoted 12 lines · Saravana Kannan
Apologies in advance for replying to this one email but discussing the points raised in all the other replies. I'm not cc'ed in this thread and replying to each email individually is a pain.
Quoted 5 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.
Quoted 7 lines
Both of those patches were the best solution at that point in time. But the kernel has changed a lot since then. Power domain and IOMMU drivers can work as modules now. We have
of_devlinkandsync_state().So, while a delay might have been the ideal solution back then, I think we need to work towards removing arbitrary timeouts instead of making the timeout mechanism more elaborate.
If you don't have some way to say all the dependencies that can be resolved have been resolved already, how do you get away from a timeout? Nothing has changed in that respect.
If a dtb+kernel works, updating just the dtb with new dependencies should not break things.
Quoted 16 lines
I think
driver_deferred_probe_check_state()logic should boiled down to something like:int
driver_deferred_probe_check_state(struct device *dev){ /* No modules and init done, deferred probes are pointless from * now. */ if (!defined(CONFIG_MODULES) && initcall_done) return -ENODEV; /* If modules and of_devlink then you clean want dependencies to * be enforced. */ if (defined(CONFIG_MODULES) && of_devlink) return -EPROBE_DEFER;
Quoted 8 lines
/* Whatever other complexity (including timeouts) we want to
* add. Hopefully none - we can discuss in this thread. */
if (.....)
return -Exxxx;
/* When in doubt, allow probe deferral. */
return -EPROBE_DEFER;
}Mostly makes sense to me. I think using CONFIG_MODULES is good.
However, there is the case in pinctrl that we have a DT flag 'pinctrl-use-default' and we stop deferring at the end of initcalls if set. With the above, there's no way to detect that. I'm pretty sure that's broken now with of_devlink and maybe from Thierry's change too.
Quoted 2 lines
Rob, for the original use case those two patches were added for, do they need to support
CONFIG_MODULES?
At the time since the subsystems involved were not typically modules so using CONFIG_MODULES didn't really matter. As you said, that's changed now.
Quoted 31 lines
Quoted 5 lines
That change causes trouble when trying to use many clk drivers as modules, as the clk modules may not load until much later after init has started. If a dependent driver loads and gets an error instead of
EPROBE_DEFER, it won't try to reload later when the dependency is met, and will thus fail to load.Once we add
of_devlinksupport for power-domains, you won't even hit the genpd error path if you haveof_devlinkenabled. I believe in the case you are testing DB 845c,of_devlinkis enabled?If
of_devlinkis enabled, the devices depending on the unprobed power domains would be deferred without even calling the driver'sprobe()function.Adding power-domain support to
of_devlinkis a 2 line change. I'll send it out soon.Also,
regulator_init_complete()can be replaced by async_state()based implementation. I have a downstream implementation that works. But it's definitely not upstream ready. I plan to rewrite it and send it upstream at some point, but it's fairly straightforward if anyone else want to implement it. My main point being, we shouldn't have to make the timeout logic more complex (or even need one) for regulator clean up either.Quoted 3 lines · Rob Herring
The one complication which I mentioned already is with consoles. A timeout (and dependencies in modules) there doesn't work. You have to probe and register the console before init is done.
Rob,
Quoted 8 lines
I've seen you say this a couple of times before. But I don't think this is true any more. With
of_devlinkenabled I've booted hardware countless times with the console device probing after userspace comes up. The only limitation for console drivers is that they need to be built-in if they need to support earlycon. If you don't care to support earlycon (very useful for bringup debugging), I think the console driver can even be a module. I don't think evenof_devlinkneeds to be enabled technically if you load the modules in the right order.
Every serial driver has to be built-in to enable console support. That's not because of earlycon. It's been that way for as long as I've worked on linux. Now of course, a driver could be built-in and still probe after userspace starts, but in my testing with the timeout that didn't work. I don't see how of_devlink changes that.
It could depend on what userspace you have. Certainly, booting with 'console=ttyS0 init=/bin/sh' would not work for example. What I probably tested with was a busybox based rootfs. What are you testing with? Android?
Rob
Quoted 18 lines · Rob Herring
Quoted 16 lines · Saravana Kannan
Apologies in advance for replying to this one email but discussing the points raised in all the other replies. I'm not cc'ed in this thread and replying to each email individually is a pain.
Quoted 5 lines · John Stultz
Due to commit
e01afc3250255("PM / Domains: Stop deferring probe at the end of initcall"), along with commit25b4e70dcce9("driver core: allow stopping deferred probe after init") afterlate_initcall, drivers will stop gettingEPROBE_DEFER, and instead see an error causing the driver to fail to load.Both of those patches were the best solution at that point in time. But the kernel has changed a lot since then. Power domain and IOMMU drivers can work as modules now. We have
of_devlinkandsync_state().
Quoted 7 lines
Quoted 3 lines
So, while a delay might have been the ideal solution back then, I think we need to work towards removing arbitrary timeouts instead of making the timeout mechanism more elaborate.
If you don't have some way to say all the dependencies that can be resolved have been resolved already, how do you get away from a timeout? Nothing has changed in that respect.
Right, we can't get away from timeout if we need to support CONFIG_MODULES AND mix-n-matched dtc+kernel versions.
But hopefully we can simplify the timeout logic by reducing the configurations it needs to support (because other checks take care of those configurations).
Quoted 2 lines
If a dtb+kernel works, updating just the dtb with new dependencies should not break things.
I'm not sold on that policy (I agree newer kernel + old dtb shouldn't break), but that's a discussion for another time.
Quoted 22 lines
Quoted 22 lines
I think
driver_deferred_probe_check_state()logic should boiled down to something like:int
driver_deferred_probe_check_state(struct device *dev){ /* No modules and init done, deferred probes are pointless from * now. */ if (!defined(CONFIG_MODULES) && initcall_done) return -ENODEV; /* If modules and of_devlink then you clean want dependencies to * be enforced. */ if (defined(CONFIG_MODULES) && of_devlink) return -EPROBE_DEFER; /* Whatever other complexity (including timeouts) we want to * add. Hopefully none - we can discuss in this thread. */ if (.....) return -Exxxx;
Quoted 5 lines
Quoted 3 lines
/* When in doubt, allow probe deferral. */ return -EPROBE_DEFER; }Mostly makes sense to me. I think using
CONFIG_MODULESis good.
Good to know. I'll see if John wants to do that. If not, I'll get around to it.
Quoted 3 lines
However, there is the case in pinctrl that we have a DT flag 'pinctrl-use-default' and we stop deferring at the end of initcalls if set.
Ugh! That code hurts my head! Mainly because the driver_deferred_probe_check_state[_continue]() function names are so similar and confusing. And their implementation is also a bit twisty (like using triple negatives in a sentence). But I also noticed there is no user of pinctrl-use-default in the upstream kernel. So, whatever.
Quoted 2 lines
With the above, there's no way to detect that. I'm pretty sure that's broken now with
of_devlinkand maybe from Thierry's change too.
of_devlink doesn't parse pinctrl yet. I have some simple changes downstream for that which just parses -0, -1, -2 and -3 -- I'll get around to upstreaming those sometime. However, adding support for pinctrl-use-default is not hard. But I'd rather not do it unless someone actually needs to use that along with of_devlink enabled and asks for it in LKML.
Quoted 47 lines
Quoted 2 lines
Rob, for the original use case those two patches were added for, do they need to support
CONFIG_MODULES?At the time since the subsystems involved were not typically modules so using
CONFIG_MODULESdidn't really matter. As you said, that's changed now.Quoted 31 lines
Quoted 5 lines
That change causes trouble when trying to use many clk drivers as modules, as the clk modules may not load until much later after init has started. If a dependent driver loads and gets an error instead of
EPROBE_DEFER, it won't try to reload later when the dependency is met, and will thus fail to load.Once we add
of_devlinksupport for power-domains, you won't even hit the genpd error path if you haveof_devlinkenabled. I believe in the case you are testing DB 845c,of_devlinkis enabled?If
of_devlinkis enabled, the devices depending on the unprobed power domains would be deferred without even calling the driver'sprobe()function.Adding power-domain support to
of_devlinkis a 2 line change. I'll send it out soon.Also,
regulator_init_complete()can be replaced by async_state()based implementation. I have a downstream implementation that works. But it's definitely not upstream ready. I plan to rewrite it and send it upstream at some point, but it's fairly straightforward if anyone else want to implement it. My main point being, we shouldn't have to make the timeout logic more complex (or even need one) for regulator clean up either.Quoted 3 lines · Rob Herring
The one complication which I mentioned already is with consoles. A timeout (and dependencies in modules) there doesn't work. You have to probe and register the console before init is done.
Rob,
Quoted 8 lines
I've seen you say this a couple of times before. But I don't think this is true any more. With
of_devlinkenabled I've booted hardware countless times with the console device probing after userspace comes up. The only limitation for console drivers is that they need to be built-in if they need to support earlycon. If you don't care to support earlycon (very useful for bringup debugging), I think the console driver can even be a module. I don't think evenof_devlinkneeds to be enabled technically if you load the modules in the right order.
Quoted 7 lines
Every serial driver has to be built-in to enable console support. That's not because of earlycon. It's been that way for as long as I've worked on linux. Now of course, a driver could be built-in and still probe after userspace starts, but in my testing with the timeout that didn't work.
I don't see how
of_devlinkchanges that.
of_devlink sometimes helps because it avoids running probe() functions with poor error handling (Eg: returning -Esomethingelse when they should return -EPROBE_DEFER). Also, I did say "I don't think even of_devlink needs to be enabled..."
Quoted 1 line
It could depend on what userspace you have.
Right, and because of that I think we should say the following going forward:
"Every serial driver has to be built-in to enable console support, but it can still probe after userspace starts"
Instead of:
"You have to probe and register the console before init is done".
Because the former statement gives a clearer picture and doesn't discourage people from trying to modularize their platforms more thoroughly :)
Quoted 3 lines
Certainly, booting with 'console=ttyS0 init=/bin/sh' would not work for example. What I probably tested with was a busybox based rootfs. What are you testing with? Android?
Yeah, with Android. Happen to know why busybox (or whatever other shell) might not work? Is it because they exit immediately if console device is not available instead of continuing to run the scripts?
-Saravana