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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Fri, Apr 21, 2023 at 10:51:14PM +0200, Lukas Wunner wrote:
> On Tue, Apr 18, 2023 at 10:28:08AM +0300, 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 brokine device so use the minimum
>                                  ^^^^^^^
> 				 broken
> 
> 
> > @@ -5027,14 +5032,29 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
> >  	if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
> >  		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");
> > -			return -ENOTTY;
> > +
> > +		/*
> > +		 * 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) {
> > +			u16 status;
> > +
> > +			pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
> > +			if (!(status & PCI_EXP_LNKSTA_DLLLA))
> > +				return -ENOTTY;
> >  		}
> > +
> > +		return pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay);
> > +	}
> 
> So above in the Gen1/Gen2 case (<= 5 GT/s), a delay of 100 msec is afforded
> and if the link isn't up by then, the function returns an error.
> 
> Doesn't that violate PCIe r6.0.1 sec 6.6.1 that states:
> 
>  "system software must allow at least 1.0 s following exit from a
>   Conventional Reset of a device, before determining that the device
>   is broken if it fails to return a Successful Completion status for
>   a valid Configuration Request.  This period is independent of how
>   quickly Link training completes."

Yes, it does :( Missed that last sentence.

> I think what we can do here is:
> 
> 		if (!pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay))
> 			return 0;
> 
> 		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);
> 
> In other words, if link active reporting is unsupported, we can only
> afford the 1 second prescribed by the spec and that's it.  If the
> subordinate device is still inaccessible after that, reset recovery
> failed.
> 
> If link active reporting is supported and the link is up, then we know
> the device is accessible but may need more time.  In that case the
> full 60 seconds are afforded.
> 
> Does that make sense?

Yes, it does, thanks! I will send an updated version with this (and the
typo) fixed after the merge window closes.