| Message ID | 20230425064751.24951-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v5] PCI/PM: Shorten pci_bridge_wait_for_secondary_bus() wait time for slow links | expand |
On Tue, Apr 25, 2023 at 09:47:51AM +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 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> Reviewed-by: Lukas Wunner <lukas@wunner.de> This completes the patches queued up on pci/reset for v6.4, so would ideally be added to them (if at all possible at this point in the cycle). Thanks, Lukas
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);
Hi Bjorn, On Tue, Apr 25, 2023 at 09:47:51AM +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 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> Any comments on this? If not can you consider taking this for v6.5? Thanks! > --- > 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); > -- > 2.39.2
Hi again, On Wed, May 24, 2023 at 03:05:46PM +0300, Mika Westerberg wrote: > Hi Bjorn, > > On Tue, Apr 25, 2023 at 09:47:51AM +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 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> > > Any comments on this? If not can you consider taking this for v6.5? > Thanks! A gentle reminder :)
On Tue, Apr 25, 2023 at 09:47:51AM +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 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> Applied with acks from Lukas and Sathy to pci/pm for v6.5, thanks! > --- > 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); > -- > 2.39.2 >
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);
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> --- 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(-)