This series is a v2 of patch "driver core: Disable driver deferred probe
timeout by default" [0] but using a slightly different approach after the
feedback I got on v1.
The problem with v1 was that just disabling the probe deferral timeout by
default would cause a regression for drivers that may want to probe even
if their (optional) dependencies are not present yet.
But this was achieved by timing out the probe deferral mechanism, which
calls fw_devlink_drivers_done() in its work queue function handler. There
is not reason to tie the two though, it should be possible to relax the
device links to allow drivers to probe even if their optional suppliers
are not present, while still keep the probe deferral mechanism enabled
so that drivers that have required dependencies are still able to defer
their probe.
This series decouple the two operations by adding a fw_devlink.timeout=
command line parameter. That way, the probe deferral timeout can be set
to -1 by default, reverting to the previous behaviour while still allow
drivers to probe with optional dependencies missing.
Patch #1 is just a cleanup that makes the driver_deferred_probe_timeout
variable static since isn't used outside of its compilation unit.
Patch #2 disables the deferred probe mechanism after late_initcall if
modules are disable. Since there is no point to schedule the timer in
that case.
Patch #3 adds the new "fw_devlink.timeout=" cmdline param, that can be
used to set a timeout for the device links enforcing. The semantics are
quite similar to the existing "deferred_probe_timeout=" cmdline param.
Patch #4 then changes the default value for the probe deferral timeout,
to just disable it by default and make the probe deferral mechanism to
revert to the behaviour that had before. That is, to just try to probe
the drivers indefinitely. But the device link enforcing timeout is set
to 10 seconds, to keep the existing expectations for drivers that want
to probe even if their optional dependencies are not present.
I have tested on my HP X2 Chromebook and the DRM driver that was failing
to probe before now works without any cmdline parameters. I also tested
with different combinations of device links and deferred probe timeouts.
[0]: https://lore.kernel.org/lkml/[email protected]/T/#t
Best regards,
Javier
Changes in v2:
- Mention in the commit messsage the specific machine and drivers that
are affected by the issue (Greg).
- Double check the commit message for accuracy (John).
- Add a second workqueue to timeout the devlink enforcing and allow
drivers to probe even without their optional dependencies available.
Javier Martinez Canillas (4):
driver core: Make driver_deferred_probe_timeout a static variable
driver core: Set deferred probe timeout to 0 if modules are disabled
driver core: Add fw_devlink.timeout param to stop waiting for devlinks
driver core: Disable driver deferred probe timeout by default
.../admin-guide/kernel-parameters.txt | 7 +++
drivers/base/dd.c | 48 +++++++++++++++----
include/linux/device/driver.h | 1 -
3 files changed, 47 insertions(+), 9 deletions(-)
--
2.38.1
Currently, the probe deferral timeout does two things:
1) Call to fw_devlink_drivers_done() to relax the device dependencies and
allow drivers to be probed if these dependencies are optional.
2) Disable the probe deferral mechanism so that drivers will fail to probe
if the required dependencies are not present, instead of adding them to
the deferred probe pending list.
But there is no need to couple these two, for example the probe deferral
can be used even when the device links are disable (i.e: fw_devlink=off).
So let's add a separate fw_devlink.timeout command line parameter to allow
relaxing the device links and prevent drivers to wait for these to probe.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
(no changes since v1)
.../admin-guide/kernel-parameters.txt | 7 ++++
drivers/base/dd.c | 38 ++++++++++++++++++-
2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a465d5242774..38138a44d5ed 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1581,6 +1581,13 @@
dependencies. This only applies for fw_devlink=on|rpm.
Format: <bool>
+ fw_devlink.timeout=
+ [KNL] Debugging option to set a timeout in seconds for
+ drivers to give up waiting on dependencies and to probe
+ these are optional. A timeout of 0 will timeout at the
+ end of initcalls. If the time out hasn't expired, it'll
+ be restarted by each successful driver registration.
+
gamecon.map[2|3]=
[HW,JOY] Multisystem joystick and NES/SNES/PSX pad
support via parallel port (up to 5 devices per port)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1e8f1afeac98..ea448df94d24 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -261,6 +261,7 @@ static int driver_deferred_probe_timeout = 10;
#else
static int driver_deferred_probe_timeout;
#endif
+static int fw_devlink_timeout = -1;
static int __init deferred_probe_timeout_setup(char *str)
{
@@ -272,6 +273,16 @@ static int __init deferred_probe_timeout_setup(char *str)
}
__setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
+static int __init fw_devlink_timeout_setup(char *str)
+{
+ int timeout;
+
+ if (!kstrtoint(str, 10, &timeout))
+ fw_devlink_timeout = timeout;
+ return 1;
+}
+__setup("fw_devlink.timeout=", fw_devlink_timeout_setup);
+
/**
* driver_deferred_probe_check_state() - Check deferred probe state
* @dev: device to check
@@ -318,6 +329,15 @@ static void deferred_probe_timeout_work_func(struct work_struct *work)
}
static DECLARE_DELAYED_WORK(deferred_probe_timeout_work, deferred_probe_timeout_work_func);
+static void fw_devlink_timeout_work_func(struct work_struct *work)
+{
+ fw_devlink_drivers_done();
+
+ driver_deferred_probe_trigger();
+ flush_work(&deferred_probe_work);
+}
+static DECLARE_DELAYED_WORK(fw_devlink_timeout_work, fw_devlink_timeout_work_func);
+
void deferred_probe_extend_timeout(void)
{
/*
@@ -330,6 +350,13 @@ void deferred_probe_extend_timeout(void)
pr_debug("Extended deferred probe timeout by %d secs\n",
driver_deferred_probe_timeout);
}
+
+ if (cancel_delayed_work(&fw_devlink_timeout_work)) {
+ schedule_delayed_work(&fw_devlink_timeout_work,
+ fw_devlink_timeout * HZ);
+ pr_debug("Extended fw_devlink timeout by %d secs\n",
+ fw_devlink_timeout);
+ }
}
/**
@@ -352,9 +379,12 @@ static int deferred_probe_initcall(void)
if (!IS_ENABLED(CONFIG_MODULES)) {
driver_deferred_probe_timeout = 0;
- fw_devlink_drivers_done();
+ fw_devlink_timeout = 0;
}
+ if (!fw_devlink_timeout)
+ fw_devlink_drivers_done();
+
/*
* Trigger deferred probe again, this time we won't defer anything
* that is optional
@@ -366,6 +396,12 @@ static int deferred_probe_initcall(void)
schedule_delayed_work(&deferred_probe_timeout_work,
driver_deferred_probe_timeout * HZ);
}
+
+ if (fw_devlink_timeout > 0) {
+ schedule_delayed_work(&fw_devlink_timeout_work,
+ fw_devlink_timeout * HZ);
+ }
+
return 0;
}
late_initcall(deferred_probe_initcall);
--
2.38.1
The driver_deferred_probe_timeout value has a long history. It was first
set to -1 when 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.
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 work 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 to timeout after 30 seconds, unless a different timeout is set
using the "deferred_probe_timeout" command line parameter.
The behavior was changed even more 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.
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
later by user-land to probe drivers that were still in the deferred list.
The 10 seconds timeout isn't enough in some cases, for example the Fedora
kernel builds as much drivers as possible as modules. And this leads to an
Snapdragon SC7180 based HP X2 Chromebook to not have display, due the DRM
driver failing to probe if CONFIG_ARM_SMMU=y and CONFIG_SC_GPUCC_7180=m.
So let's change the default again to -1 as it was at the beginning. That's
how probe deferral always worked. The kernel should try to avoid guessing
when it should be safe to give up on deferred drivers to be probed.
The reason why the default "deferred_probe_timeout" was changed from -1 to
the other values was to allow drivers that have only optional dependencies
to probe even if the suppliers are not available.
But now there is a "fw_devlink.timeout" parameter to timeout the links and
allow drivers to probe even when the dependencies are not present. Let's
set the default for that timeout to 10 seconds, to give the same behaviour
as expected by these driver with optional device links.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
Changes in v2:
- Mention in the commit messsage the specific machine and drivers that
are affected by the issue (Greg).
- Double check the commit message for accuracy (John).
- Add a second workqueue to timeout the devlink enforcing and allow
drivers to probe even without their optional dependencies available.
drivers/base/dd.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index ea448df94d24..5f18cd497850 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -256,12 +256,8 @@ static int deferred_devs_show(struct seq_file *s, void *data)
}
DEFINE_SHOW_ATTRIBUTE(deferred_devs);
-#ifdef CONFIG_MODULES
-static int driver_deferred_probe_timeout = 10;
-#else
-static int driver_deferred_probe_timeout;
-#endif
-static int fw_devlink_timeout = -1;
+static int driver_deferred_probe_timeout = -1;
+static int fw_devlink_timeout = 10;
static int __init deferred_probe_timeout_setup(char *str)
{
--
2.38.1
There is no point to schedule the work queue to timeout the deferred probe
if all the initcalls are done and modules are not enabled. The default for
this case is already 0 but can be overridden by the deferred_probe_timeout
parameter. Let's just disable to avoid queuing a work that is not needed.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
(no changes since v1)
drivers/base/dd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 040b4060f903..1e8f1afeac98 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -350,8 +350,10 @@ static int deferred_probe_initcall(void)
flush_work(&deferred_probe_work);
initcalls_done = true;
- if (!IS_ENABLED(CONFIG_MODULES))
+ if (!IS_ENABLED(CONFIG_MODULES)) {
+ driver_deferred_probe_timeout = 0;
fw_devlink_drivers_done();
+ }
/*
* Trigger deferred probe again, this time we won't defer anything
--
2.38.1
Hi--
On 11/16/22 04:01, Javier Martinez Canillas wrote:
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a465d5242774..38138a44d5ed 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1581,6 +1581,13 @@
> dependencies. This only applies for fw_devlink=on|rpm.
> Format: <bool>
>
> + fw_devlink.timeout=
> + [KNL] Debugging option to set a timeout in seconds for
> + drivers to give up waiting on dependencies and to probe
> + these are optional. A timeout of 0 will timeout at the
> + end of initcalls. If the time out hasn't expired, it'll
timeout
> + be restarted by each successful driver registration.
--
~Randy
On Wed, Nov 16, 2022 at 01:00:43PM +0100, Javier Martinez Canillas wrote:
> There is no point to schedule the work queue to timeout the deferred probe
> if all the initcalls are done and modules are not enabled. The default for
> this case is already 0 but can be overridden by the deferred_probe_timeout
> parameter. Let's just disable to avoid queuing a work that is not needed.
>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/base/dd.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 040b4060f903..1e8f1afeac98 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -350,8 +350,10 @@ static int deferred_probe_initcall(void)
> flush_work(&deferred_probe_work);
> initcalls_done = true;
>
> - if (!IS_ENABLED(CONFIG_MODULES))
> + if (!IS_ENABLED(CONFIG_MODULES)) {
> + driver_deferred_probe_timeout = 0;
> fw_devlink_drivers_done();
> + }
>
Potentially a stupid suggestion, but would it make sense to take out
the ability to actually set that param if !CONFIG_MODULES? Then
driver_deferred_probe_timeout would be the default value already.
> /*
> * Trigger deferred probe again, this time we won't defer anything
> --
> 2.38.1
>
Hello Andrew,
Thanks for your feedback.
On 11/16/22 17:39, Andrew Halaney wrote:
> On Wed, Nov 16, 2022 at 01:00:43PM +0100, Javier Martinez Canillas wrote:
>> There is no point to schedule the work queue to timeout the deferred probe
>> if all the initcalls are done and modules are not enabled. The default for
>> this case is already 0 but can be overridden by the deferred_probe_timeout
>> parameter. Let's just disable to avoid queuing a work that is not needed.
>>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>> ---
>>
>> (no changes since v1)
>>
>> drivers/base/dd.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 040b4060f903..1e8f1afeac98 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -350,8 +350,10 @@ static int deferred_probe_initcall(void)
>> flush_work(&deferred_probe_work);
>> initcalls_done = true;
>>
>> - if (!IS_ENABLED(CONFIG_MODULES))
>> + if (!IS_ENABLED(CONFIG_MODULES)) {
>> + driver_deferred_probe_timeout = 0;
>> fw_devlink_drivers_done();
>> + }
>>
>
> Potentially a stupid suggestion, but would it make sense to take out
> the ability to actually set that param if !CONFIG_MODULES? Then
> driver_deferred_probe_timeout would be the default value already.
>
Yes, I think it makes sense. I will do that in the next iteration.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On Wed, Nov 16, 2022 at 01:01:59PM +0100, Javier Martinez Canillas wrote:
> Currently, the probe deferral timeout does two things:
>
> 1) Call to fw_devlink_drivers_done() to relax the device dependencies and
> allow drivers to be probed if these dependencies are optional.
>
> 2) Disable the probe deferral mechanism so that drivers will fail to probe
> if the required dependencies are not present, instead of adding them to
> the deferred probe pending list.
>
> But there is no need to couple these two, for example the probe deferral
> can be used even when the device links are disable (i.e: fw_devlink=off).
>
> So let's add a separate fw_devlink.timeout command line parameter to allow
> relaxing the device links and prevent drivers to wait for these to probe.
>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
I like this idea and it looks good as far as I can tell.
Acked-by: Andrew Halaney <[email protected]>
> ---
>
> (no changes since v1)
>
> .../admin-guide/kernel-parameters.txt | 7 ++++
> drivers/base/dd.c | 38 ++++++++++++++++++-
> 2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a465d5242774..38138a44d5ed 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1581,6 +1581,13 @@
> dependencies. This only applies for fw_devlink=on|rpm.
> Format: <bool>
>
> + fw_devlink.timeout=
> + [KNL] Debugging option to set a timeout in seconds for
> + drivers to give up waiting on dependencies and to probe
> + these are optional. A timeout of 0 will timeout at the
> + end of initcalls. If the time out hasn't expired, it'll
> + be restarted by each successful driver registration.
> +
> gamecon.map[2|3]=
> [HW,JOY] Multisystem joystick and NES/SNES/PSX pad
> support via parallel port (up to 5 devices per port)
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 1e8f1afeac98..ea448df94d24 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -261,6 +261,7 @@ static int driver_deferred_probe_timeout = 10;
> #else
> static int driver_deferred_probe_timeout;
> #endif
> +static int fw_devlink_timeout = -1;
>
> static int __init deferred_probe_timeout_setup(char *str)
> {
> @@ -272,6 +273,16 @@ static int __init deferred_probe_timeout_setup(char *str)
> }
> __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>
> +static int __init fw_devlink_timeout_setup(char *str)
> +{
> + int timeout;
> +
> + if (!kstrtoint(str, 10, &timeout))
> + fw_devlink_timeout = timeout;
> + return 1;
> +}
> +__setup("fw_devlink.timeout=", fw_devlink_timeout_setup);
> +
> /**
> * driver_deferred_probe_check_state() - Check deferred probe state
> * @dev: device to check
> @@ -318,6 +329,15 @@ static void deferred_probe_timeout_work_func(struct work_struct *work)
> }
> static DECLARE_DELAYED_WORK(deferred_probe_timeout_work, deferred_probe_timeout_work_func);
>
> +static void fw_devlink_timeout_work_func(struct work_struct *work)
> +{
> + fw_devlink_drivers_done();
> +
> + driver_deferred_probe_trigger();
> + flush_work(&deferred_probe_work);
> +}
> +static DECLARE_DELAYED_WORK(fw_devlink_timeout_work, fw_devlink_timeout_work_func);
> +
> void deferred_probe_extend_timeout(void)
> {
> /*
> @@ -330,6 +350,13 @@ void deferred_probe_extend_timeout(void)
> pr_debug("Extended deferred probe timeout by %d secs\n",
> driver_deferred_probe_timeout);
> }
> +
> + if (cancel_delayed_work(&fw_devlink_timeout_work)) {
> + schedule_delayed_work(&fw_devlink_timeout_work,
> + fw_devlink_timeout * HZ);
> + pr_debug("Extended fw_devlink timeout by %d secs\n",
> + fw_devlink_timeout);
> + }
> }
>
> /**
> @@ -352,9 +379,12 @@ static int deferred_probe_initcall(void)
>
> if (!IS_ENABLED(CONFIG_MODULES)) {
> driver_deferred_probe_timeout = 0;
> - fw_devlink_drivers_done();
> + fw_devlink_timeout = 0;
> }
>
> + if (!fw_devlink_timeout)
> + fw_devlink_drivers_done();
> +
> /*
> * Trigger deferred probe again, this time we won't defer anything
> * that is optional
> @@ -366,6 +396,12 @@ static int deferred_probe_initcall(void)
> schedule_delayed_work(&deferred_probe_timeout_work,
> driver_deferred_probe_timeout * HZ);
> }
> +
> + if (fw_devlink_timeout > 0) {
> + schedule_delayed_work(&fw_devlink_timeout_work,
> + fw_devlink_timeout * HZ);
> + }
> +
> return 0;
> }
> late_initcall(deferred_probe_initcall);
> --
> 2.38.1
>
On Wed, Nov 16, 2022 at 01:02:36PM +0100, Javier Martinez Canillas wrote:
> The driver_deferred_probe_timeout value has a long history. It was first
> set to -1 when 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.
This or statement here sounds like you either opt-in, or the timeout
affects you (at least that's how I read it).
A subsystem has to opt-in to get either result by using
driver_deferred_probe_check_state()!
>
> 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 work 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
s/where/were/
> 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 to timeout after 30 seconds, unless a different timeout is set
> using the "deferred_probe_timeout" command line parameter.
>
> The behavior was changed even more 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.
>
> 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
> later by user-land to probe drivers that were still in the deferred list.
>
> The 10 seconds timeout isn't enough in some cases, for example the Fedora
> kernel builds as much drivers as possible as modules. And this leads to an
> Snapdragon SC7180 based HP X2 Chromebook to not have display, due the DRM
> driver failing to probe if CONFIG_ARM_SMMU=y and CONFIG_SC_GPUCC_7180=m.
>
> So let's change the default again to -1 as it was at the beginning. That's
> how probe deferral always worked. The kernel should try to avoid guessing
> when it should be safe to give up on deferred drivers to be probed.
>
> The reason why the default "deferred_probe_timeout" was changed from -1 to
> the other values was to allow drivers that have only optional dependencies
> to probe even if the suppliers are not available.
>
> But now there is a "fw_devlink.timeout" parameter to timeout the links and
> allow drivers to probe even when the dependencies are not present. Let's
> set the default for that timeout to 10 seconds, to give the same behaviour
> as expected by these driver with optional device links.
>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
This sounds like a reasonable solution to me:
Acked-by: Andrew Halaney <[email protected]>
> ---
>
> Changes in v2:
> - Mention in the commit messsage the specific machine and drivers that
> are affected by the issue (Greg).
> - Double check the commit message for accuracy (John).
> - Add a second workqueue to timeout the devlink enforcing and allow
> drivers to probe even without their optional dependencies available.
>
> drivers/base/dd.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index ea448df94d24..5f18cd497850 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -256,12 +256,8 @@ static int deferred_devs_show(struct seq_file *s, void *data)
> }
> DEFINE_SHOW_ATTRIBUTE(deferred_devs);
>
> -#ifdef CONFIG_MODULES
> -static int driver_deferred_probe_timeout = 10;
> -#else
> -static int driver_deferred_probe_timeout;
> -#endif
> -static int fw_devlink_timeout = -1;
> +static int driver_deferred_probe_timeout = -1;
> +static int fw_devlink_timeout = 10;
>
> static int __init deferred_probe_timeout_setup(char *str)
> {
> --
> 2.38.1
>
On Wed, Nov 16, 2022 at 01:01:59PM +0100, Javier Martinez Canillas wrote:
> Currently, the probe deferral timeout does two things:
>
> 1) Call to fw_devlink_drivers_done() to relax the device dependencies and
> allow drivers to be probed if these dependencies are optional.
>
> 2) Disable the probe deferral mechanism so that drivers will fail to probe
> if the required dependencies are not present, instead of adding them to
> the deferred probe pending list.
>
> But there is no need to couple these two, for example the probe deferral
> can be used even when the device links are disable (i.e: fw_devlink=off).
>
> So let's add a separate fw_devlink.timeout command line parameter to allow
> relaxing the device links and prevent drivers to wait for these to probe.
>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
>
> (no changes since v1)
>
> .../admin-guide/kernel-parameters.txt | 7 ++++
> drivers/base/dd.c | 38 ++++++++++++++++++-
> 2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a465d5242774..38138a44d5ed 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1581,6 +1581,13 @@
> dependencies. This only applies for fw_devlink=on|rpm.
> Format: <bool>
>
> + fw_devlink.timeout=
Just thought about this, but I think this should be called
fw_devlink_timeout. Generally the $MODULE.$PARAM syntax is reserved for
things that can be specificed with module_param().
The advantage is if you accidentally type say fw_devlink_timeut=10 the
kernel logs will indicate it has no clue what that means. Including the
"." makes the kernel assume that maybe a future module name fw_devlink
will be loaded, and at that time will see if that module has the
parameter mentioned. A little thing but I think work changing in v3.
Thanks,
Andrew
Hello Andrew,
On 11/17/22 16:19, Andrew Halaney wrote:
> On Wed, Nov 16, 2022 at 01:01:59PM +0100, Javier Martinez Canillas wrote:
>> Currently, the probe deferral timeout does two things:
>>
>> 1) Call to fw_devlink_drivers_done() to relax the device dependencies and
>> allow drivers to be probed if these dependencies are optional.
>>
>> 2) Disable the probe deferral mechanism so that drivers will fail to probe
>> if the required dependencies are not present, instead of adding them to
>> the deferred probe pending list.
>>
>> But there is no need to couple these two, for example the probe deferral
>> can be used even when the device links are disable (i.e: fw_devlink=off).
>>
>> So let's add a separate fw_devlink.timeout command line parameter to allow
>> relaxing the device links and prevent drivers to wait for these to probe.
>>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>> ---
>>
>> (no changes since v1)
>>
>> .../admin-guide/kernel-parameters.txt | 7 ++++
>> drivers/base/dd.c | 38 ++++++++++++++++++-
>> 2 files changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index a465d5242774..38138a44d5ed 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1581,6 +1581,13 @@
>> dependencies. This only applies for fw_devlink=on|rpm.
>> Format: <bool>
>>
>> + fw_devlink.timeout=
>
> Just thought about this, but I think this should be called
> fw_devlink_timeout. Generally the $MODULE.$PARAM syntax is reserved for
> things that can be specificed with module_param().
>
> The advantage is if you accidentally type say fw_devlink_timeut=10 the
> kernel logs will indicate it has no clue what that means. Including the
> "." makes the kernel assume that maybe a future module name fw_devlink
> will be loaded, and at that time will see if that module has the
> parameter mentioned. A little thing but I think work changing in v3.
>
I was actually on the fence on this one but the reason why I did the .timeout
was that the other fw_devlink param [0] is defined as fw_devlink.strict=<bool>
so I wanted to keep this one consistent with that.
https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
On Wed, Nov 16, 2022 at 4:02 AM Javier Martinez Canillas
<[email protected]> wrote:
>
> Currently, the probe deferral timeout does two things:
>
> 1) Call to fw_devlink_drivers_done() to relax the device dependencies and
> allow drivers to be probed if these dependencies are optional.
>
> 2) Disable the probe deferral mechanism so that drivers will fail to probe
> if the required dependencies are not present, instead of adding them to
> the deferred probe pending list.
>
> But there is no need to couple these two, for example the probe deferral
> can be used even when the device links are disable (i.e: fw_devlink=off).
>
> So let's add a separate fw_devlink.timeout command line parameter to allow
> relaxing the device links and prevent drivers to wait for these to probe.
I'm probably being dim, but it's not immediately clear from this
description *why* this is useful. Maybe add some words on the tangible
benefit of splitting this up?
I'd also push a little bit back on why we need to split this into a
separate boot option. Since it's not obvious as to when a user would
want to use fw_devlink.timeout vs probe_deferral_timeout.
The extra complexity of remembering which timeout is for what might
become a burden to users and developers.
>
> + fw_devlink.timeout=
> + [KNL] Debugging option to set a timeout in seconds for
> + drivers to give up waiting on dependencies and to probe
> + these are optional. A timeout of 0 will timeout at the
> + end of initcalls. If the time out hasn't expired, it'll
> + be restarted by each successful driver registration.
> +
This sounds pretty close to like the deferred_probe_timeout option.
I'd suggest some words to make the distinction more clear.
thanks
-john
Hello John,
On 11/17/22 20:07, John Stultz wrote:
> On Wed, Nov 16, 2022 at 4:02 AM Javier Martinez Canillas
> <[email protected]> wrote:
>>
>> Currently, the probe deferral timeout does two things:
>>
>> 1) Call to fw_devlink_drivers_done() to relax the device dependencies and
>> allow drivers to be probed if these dependencies are optional.
>>
>> 2) Disable the probe deferral mechanism so that drivers will fail to probe
>> if the required dependencies are not present, instead of adding them to
>> the deferred probe pending list.
>>
>> But there is no need to couple these two, for example the probe deferral
>> can be used even when the device links are disable (i.e: fw_devlink=off).
>>
>> So let's add a separate fw_devlink.timeout command line parameter to allow
>> relaxing the device links and prevent drivers to wait for these to probe.
>
> I'm probably being dim, but it's not immediately clear from this
> description *why* this is useful. Maybe add some words on the tangible
> benefit of splitting this up?
>
Thanks for your feedback. You are right that I need to better explain
the why / goal of this patch. But basically is that it would be good
to allow timeout waiting for the optional links while still allow the
non-optional links to keep make the drivers to keep deferring.
I can make a better job at explaining the why in the next iteration.
> I'd also push a little bit back on why we need to split this into a
> separate boot option. Since it's not obvious as to when a user would
> want to use fw_devlink.timeout vs probe_deferral_timeout.
> The extra complexity of remembering which timeout is for what might
> become a burden to users and developers.
>
>>
>> + fw_devlink.timeout=
>> + [KNL] Debugging option to set a timeout in seconds for
>> + drivers to give up waiting on dependencies and to probe
>> + these are optional. A timeout of 0 will timeout at the
>> + end of initcalls. If the time out hasn't expired, it'll
>> + be restarted by each successful driver registration.
>> +
>
> This sounds pretty close to like the deferred_probe_timeout option.
> I'd suggest some words to make the distinction more clear.
>
Yeah, I can think how to better explain this... but it's similar because
there is some overlapping between the two really, but are not exactly the
same even though we are tying the two and folding the disable of the two
under the same timeout.
I'll be OK to drop this parameter btw, and just keep it as an internal var
if it's fine to just always use the default 10 seconds or whatever timeout
it is decided.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat