[PATCH v3] driver core: Don't set a deferred probe timeout if modules are disabled

[PATCH v3] driver core: Don't set a deferred probe timeout if modules are disabled

[Javier Martinez Canillas]

There is no point to schedule the workqueue 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 skip this and avoid queuing work that is not needed.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v3:
- Just skip setting the deferred_probe_timeout parameter when modules
  are disabled (Andrew Halaney).

 drivers/base/dd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 85152537dbf1..48a45860d2bb 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -266,6 +266,13 @@ static int __init deferred_probe_timeout_setup(char *str)
 {
 	int timeout;
 
+	/*
+	 * If loadable modules support is disabled, there is no point to
+	 * set a timeout for the deferred probe and schedule a workqueue.
+	 */
+	if (!IS_ENABLED(CONFIG_MODULES))
+		return 1;
+
 	if (!kstrtoint(str, 10, &timeout))
 		driver_deferred_probe_timeout = timeout;
 	return 1;
-- 
2.43.2

Re: [PATCH v3] driver core: Don't set a deferred probe timeout if modules are disabled

[Andrew Halaney]

On Wed, Feb 28, 2024 at 12:09:02AM +0100, Javier Martinez Canillas wrote:
> There is no point to schedule the workqueue 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 skip this and avoid queuing work that is not needed.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>

> ---
> 
> Changes in v3:
> - Just skip setting the deferred_probe_timeout parameter when modules
>   are disabled (Andrew Halaney).
> 
>  drivers/base/dd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 85152537dbf1..48a45860d2bb 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -266,6 +266,13 @@ static int __init deferred_probe_timeout_setup(char *str)
>  {
>  	int timeout;
>  
> +	/*
> +	 * If loadable modules support is disabled, there is no point to
> +	 * set a timeout for the deferred probe and schedule a workqueue.
> +	 */
> +	if (!IS_ENABLED(CONFIG_MODULES))
> +		return 1;
> +
>  	if (!kstrtoint(str, 10, &timeout))
>  		driver_deferred_probe_timeout = timeout;
>  	return 1;
> -- 
> 2.43.2
> 

Re: [PATCH v3] driver core: Don't set a deferred probe timeout if modules are disabled

[Greg Kroah-Hartman]

On Wed, Feb 28, 2024 at 12:09:02AM +0100, Javier Martinez Canillas wrote:
> There is no point to schedule the workqueue 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 skip this and avoid queuing work that is not needed.

As the option is already there to set the timeout to 0, why confuse
things by trying to tie this to if modules are enabled or not?  And even
if you do want to do that, where did you now document this new system
behavior?

thanks,

greg k-h

Re: [PATCH v3] driver core: Don't set a deferred probe timeout if modules are disabled

[Javier Martinez Canillas]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Wed, Feb 28, 2024 at 12:09:02AM +0100, Javier Martinez Canillas wrote:
>> There is no point to schedule the workqueue 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 skip this and avoid queuing work that is not needed.
>
> As the option is already there to set the timeout to 0, why confuse
> things by trying to tie this to if modules are enabled or not?  And even

Because the timeout is already tied to whether modules are enabled or not [0]:

#ifdef CONFIG_MODULES
static int driver_deferred_probe_timeout = 10;
#else
static int driver_deferred_probe_timeout;
#endif

static int __init deferred_probe_timeout_setup(char *str)
{
	int timeout;

	if (!kstrtoint(str, 10, &timeout))
		driver_deferred_probe_timeout = timeout;
	return 1;
}
__setup("deferred_probe_timeout=", deferred_probe_timeout_setup);

Since that's already the case, it makes no sense to allow this option to
be set when modules is not enabled IMO.

That will just cause to schedule a workqueue for no good reasons, since it
happens at late_initcall() after all the initcall functions for built-in
drivers have already been executed.

[0]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/dd.c#L259

> if you do want to do that, where did you now document this new system
> behavior?
>

I thought about it but the current behaviour is also not documented [1],
in fact most of the help text for the deferred_probe_timeout= option is
still from the time when it was introduced _only_ as a debugging option
by 25b4e70dcce9 ("driver core: allow stopping deferred probe after init").

Later, c8c43cee29f6 ("driver core: Fix driver_deferred_probe_check_state()
logic") and e2cec7d68537 ("driver core: Set deferred_probe_timeout to a
longer default if CONFIG_MODULES is set"), tied the behavior and default
timeout to whether the modules were enabled or not. And those two commits
did not document the behavioural changes in the kernel option help either.

I can update the kernel-parameters.txt in the next iteration if you agree
with the change.

[1]: https://elixir.bootlin.com/linux/v6.8-rc7/source/Documentation/admin-guide/kernel-parameters.txt#L1035

> thanks,
>
> greg k-h
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat