Re: [PATCH v5] PCI/PM: Shorten pci_bridge_wait_for_secondary_bus() wait time for slow links

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 4/24/23 11:47 PM, Mika Westerberg wrote:
> With slow links (<= 5GT/s) active link reporting is not mandatory, so if
> a device is disconnected during system sleep we might end up waiting for
> it to respond for ~60s slowing down resume time. PCIe spec r6.0 sec
> 6.6.1 mandates that the system software must wait for at least 1s before
> it can determine the device as broken device so use the minimum
> requirement for slow links and bail out if we do not get reply within
> 1s. However, if the port supports active link reporting we can continue
> the wait following what we do with the fast links.
> 
> This should make system resume time faster for slow links as well while
> still following the PCIe spec.
> 
> While there move the PCI_RESET_WAIT constant into pci.c because it is
> not used outside of that file anymore.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> Hi all,
> 
> The previous version of the patch can be found here:
> 
>   https://lore.kernel.org/linux-pci/20230418072808.10431-1-mika.westerberg@linux.intel.com/
> 
> Changes from the previous version:
> 
>   * Observe the mandatory 1s delay before looking at the active link
>     reporting as suggesteed by Lukas.
> 
>  drivers/pci/pci.c | 49 +++++++++++++++++++++++++++++++++++------------
>  drivers/pci/pci.h |  7 -------
>  2 files changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0b4f3b08f780..6bc0eeeb37fa 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -64,6 +64,13 @@ struct pci_pme_device {
>  
>  #define PME_TIMEOUT 1000 /* How long between PME checks */
>  
> +/*
> + * Following exit from Conventional Reset, devices must be ready within 1 sec
> + * (PCIe r6.0 sec 6.6.1).  A D3cold to D0 transition implies a Conventional
> + * Reset (PCIe r6.0 sec 5.8).
> + */
> +#define PCI_RESET_WAIT 1000 /* msec */
> +
>  /*
>   * Devices may extend the 1 sec period through Request Retry Status
>   * completions (PCIe r6.0 sec 2.3.1).  The spec does not provide an upper
> @@ -5012,11 +5019,9 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>  	 *
>  	 * However, 100 ms is the minimum and the PCIe spec says the
>  	 * software must allow at least 1s before it can determine that the
> -	 * device that did not respond is a broken device. There is
> -	 * evidence that 100 ms is not always enough, for example certain
> -	 * Titan Ridge xHCI controller does not always respond to
> -	 * configuration requests if we only wait for 100 ms (see
> -	 * https://bugzilla.kernel.org/show_bug.cgi?id=203885).
> +	 * device that did not respond is a broken device. Also device can
> +	 * take longer than that to respond if it indicates so through Request
> +	 * Retry Status completions.
>  	 *
>  	 * Therefore we wait for 100 ms and check for the device presence
>  	 * until the timeout expires.
> @@ -5025,16 +5030,36 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
>  		return 0;
>  
>  	if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
> +		u16 status;
> +
>  		pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
>  		msleep(delay);
> -	} else {
> -		pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
> -			delay);
> -		if (!pcie_wait_for_link_delay(dev, true, delay)) {
> -			/* Did not train, no need to wait any further */
> -			pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n");
> +
> +		if (!pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay))
> +			return 0;
> +
> +		/*
> +		 * If the port supports active link reporting we now check
> +		 * whether the link is active and if not bail out early with
> +		 * the assumption that the device is not present anymore.
> +		 */
> +		if (!dev->link_active_reporting)
>  			return -ENOTTY;
> -		}
> +
> +		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
> +		if (!(status & PCI_EXP_LNKSTA_DLLLA))
> +			return -ENOTTY;
> +
> +		return pci_dev_wait(child, reset_type,
> +				    PCIE_RESET_READY_POLL_MS - PCI_RESET_WAIT);
> +	}
> +
> +	pci_dbg(dev, "waiting %d ms for downstream link, after activation\n",
> +		delay);
> +	if (!pcie_wait_for_link_delay(dev, true, delay)) {
> +		/* Did not train, no need to wait any further */
> +		pci_info(dev, "Data Link Layer Link Active not set in 1000 msec\n");
> +		return -ENOTTY;
>  	}
>  
>  	return pci_dev_wait(child, reset_type,
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 67005a0ee381..42ce4c6e738f 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -64,13 +64,6 @@ struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
>  #define PCI_PM_D3HOT_WAIT       10	/* msec */
>  #define PCI_PM_D3COLD_WAIT      100	/* msec */
>  
> -/*
> - * Following exit from Conventional Reset, devices must be ready within 1 sec
> - * (PCIe r6.0 sec 6.6.1).  A D3cold to D0 transition implies a Conventional
> - * Reset (PCIe r6.0 sec 5.8).
> - */
> -#define PCI_RESET_WAIT		1000	/* msec */
> -
>  void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
>  void pci_refresh_power_state(struct pci_dev *dev);
>  int pci_power_up(struct pci_dev *dev);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer