Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 16 Nov 2022 13:05:00 -0600
From:   Andrew Halaney <ahalaney@redhat.com>
To:     Javier Martinez Canillas <javierm@redhat.com>
Cc:     linux-kernel@vger.kernel.org,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        Saravana Kannan <saravanak@google.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        linux-arm-msm@vger.kernel.org, John Stultz <jstultz@google.com>,
        Peter Robinson <pbrobinson@redhat.com>,
        Steev Klimaszewski <steev@kali.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Enric Balletbo i Serra <eballetbo@redhat.com>,
        Bjorn Andersson <andersson@kernel.org>,
        Brian Masney <bmasney@redhat.com>,
        Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v2 3/4] driver core: Add fw_devlink.timeout param to stop
 waiting for devlinks
Message-ID: <20221116190500.m4jyuimxo5tqhjdy@halaney-x13s>
References: <20221116115348.517599-1-javierm@redhat.com>
 <20221116120159.519908-1-javierm@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221116120159.519908-1-javierm@redhat.com>
Precedence: bulk

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 <javierm@redhat.com>

I like this idea and it looks good as far as I can tell.

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

> ---
> 
> (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
>