Re: [PATCH] pci: allow user specifiy a reset wait timeout

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 7 Feb 2025, Keith Busch wrote:

> From: Keith Busch <kbusch@kernel.org>
> 
> The spec does not provide any upper limit to how long a device may
> return Request Retry Status. It just says "Some devices require a
> lengthy self-initialization sequence to complete". The kernel
> arbitrarily chose 60 seconds since that really ought to be enough. But
> there are devices where this turns out not to be enough.
> 
> Since any timeout choice would be arbitrary, and 60 seconds is generally
> more than enough for the majority of hardware, let's make this a
> parameter so an admin can adjust it specifically to their needs if the
> default timeout isn't appropriate.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 3 +++
>  drivers/pci/pci.c                               | 6 +++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index fb8752b42ec85..1aed555ef8b40 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4843,6 +4843,9 @@
>  
>  				Note: this may remove isolation between devices
>  				and may put more devices in an IOMMU group.
> +		reset_wait=nn	The number of milliseconds to wait after a
> +				reset while seeing Request Retry Status.
> +				Default is 60000 (1 minute).
>  		force_floating	[S390] Force usage of floating interrupts.
>  		nomio		[S390] Do not use MIO instructions.
>  		norid		[S390] ignore the RID field and force use of
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a37..20817dd5ebba7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -75,7 +75,8 @@ struct pci_pme_device {
>   * limit, but 60 sec ought to be enough for any device to become
>   * responsive.
>   */
> -#define PCIE_RESET_READY_POLL_MS 60000 /* msec */
> +#define PCIE_RESET_READY_POLL_MS pci_reset_ready_wait
> +unsigned long pci_reset_ready_wait = 60000; /* msec */

I don't think masking variables with defines like that is a good idea.

I also suggest you put the unit as a postfix to the variable name.

>  static void pci_dev_d3_sleep(struct pci_dev *dev)
>  {
> @@ -6841,6 +6842,9 @@ static int __init pci_setup(char *str)
>  				disable_acs_redir_param = str + 18;
>  			} else if (!strncmp(str, "config_acs=", 11)) {
>  				config_acs_param = str + 11;
> +			} else if (!strncmp(str, "reset_wait=", 11)) {
> +				pci_reset_ready_wait =
> +					simple_strtoul(str + 11, &str, 0);
>  			} else {
>  				pr_err("PCI: Unknown option `%s'\n", str);
>  			}
> 

-- 
 i.