* [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices
@ 2020-08-31 9:31 Mika Westerberg
2020-08-31 18:13 ` Lyude Paul
2020-09-03 18:11 ` Bjorn Helgaas
0 siblings, 2 replies; 6+ messages in thread
From: Mika Westerberg @ 2020-08-31 9:31 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Kai-Heng Feng, Karol Herbst, Lyude Paul, Patrick Volkerding,
Lukas Wunner, Ben Skeggs, Mika Westerberg, linux-pci
Kai-Heng Feng reported that it takes a long time (> 1 s) to resume
Thunderbolt-connected devices from both runtime suspend and system sleep
(s2idle).
This was because some Downstream Ports that support > 5 GT/s do not also
support Data Link Layer Link Active reporting. Per PCIe r5.0 sec 6.6.1:
With a Downstream Port that supports Link speeds greater than 5.0 GT/s,
software must wait a minimum of 100 ms after Link training completes
before sending a Configuration Request to the device immediately below
that Port. Software can determine when Link training completes by
polling the Data Link Layer Link Active bit or by setting up an
associated interrupt (see Section 6.7.3.3).
Sec 7.5.3.6 requires such Ports to support DLL Link Active reporting,
but at least the Intel JHL6240 Thunderbolt 3 Bridge [8086:15c0] and
Intel JHL7540 Thunderbolt 3 Bridge [8086:15e7, 8086:15ea, 8086:15ef] do
not.
This adds a quirk for these devices that skips the the 1000 ms default
link activation delay.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206837
Link: https://lore.kernel.org/r/20200514133043.27429-1-mika.westerberg@linux.intel.com
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Hi all,
The previous version of the patch can be found below:
https://www.spinics.net/lists/linux-pci/msg97860.html
This version adds a quirk instead covering the two devices Kai-Heng Feng
reported. I added Titan Ridge DD and 2C because I think they are affected
as well.
Since the PCI IDs of these devices are now used in two places, I moved them
from TBT driver to pci_ids.h.
@Kai-Heng, if you still have access to this hardware, it would be great if
you could try this out.
drivers/pci/pci.c | 2 ++
drivers/pci/quirks.c | 23 +++++++++++++++++++++++
drivers/thunderbolt/nhi.h | 4 ----
include/linux/pci.h | 5 +++++
include/linux/pci_ids.h | 4 ++++
5 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e39c5499770f..16b61def1d46 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4674,6 +4674,8 @@ static bool pcie_wait_for_link_delay(struct pci_dev *pdev, bool active,
* case, we wait for 1000 ms + any delay requested by the caller.
*/
if (!pdev->link_active_reporting) {
+ if (active && pdev->skip_default_link_activation_delay)
+ timeout = 0;
msleep(timeout + delay);
return true;
}
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 2a589b6d6ed8..9269abb6455d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3621,6 +3621,29 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
quirk_thunderbolt_hotplug_msi);
+/*
+ * https://bugzilla.kernel.org/show_bug.cgi?id=206837
+ *
+ * Non-hotplug PCIe downstream ports of these devices do not support active
+ * link reporting but they are known to train the link within 100ms.
+ */
+static void quirk_skip_default_link_activation_delay(struct pci_dev *pdev)
+{
+ if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
+ !pdev->link_active_reporting && !pdev->is_hotplug_bridge) {
+ pci_dbg(pdev, "skipping 1000 ms default link activation delay\n");
+ pdev->skip_default_link_activation_delay = true;
+ }
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE,
+ quirk_skip_default_link_activation_delay);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE,
+ quirk_skip_default_link_activation_delay);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE,
+ quirk_skip_default_link_activation_delay);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE,
+ quirk_skip_default_link_activation_delay);
+
#ifdef CONFIG_ACPI
/*
* Apple: Shutdown Cactus Ridge Thunderbolt controller.
diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
index 80162e4b013f..c023091f6b4e 100644
--- a/drivers/thunderbolt/nhi.h
+++ b/drivers/thunderbolt/nhi.h
@@ -58,7 +58,6 @@ extern const struct tb_nhi_ops icl_nhi_ops;
#define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_NHI 0x157d
#define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_BRIDGE 0x157e
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI 0x15bf
-#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE 0x15c0
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI 0x15d2
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE 0x15d3
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI 0x15d9
@@ -66,11 +65,8 @@ extern const struct tb_nhi_ops icl_nhi_ops;
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI 0x15dc
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI 0x15dd
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI 0x15de
-#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7
#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI 0x15e8
-#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea
#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI 0x15eb
-#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15ef
#define PCI_DEVICE_ID_INTEL_ICL_NHI1 0x8a0d
#define PCI_DEVICE_ID_INTEL_ICL_NHI0 0x8a17
#define PCI_DEVICE_ID_INTEL_TGL_NHI0 0x9a1b
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 835530605c0d..c44ee4337a2a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -444,6 +444,11 @@ struct pci_dev {
unsigned int non_compliant_bars:1; /* Broken BARs; ignore them */
unsigned int is_probed:1; /* Device probing in progress */
unsigned int link_active_reporting:1;/* Device capable of reporting link active */
+ /*
+ * Skip default 1000 ms wait on ports that do not support active
+ * link reporting (link_active_reporting == 0).
+ */
+ unsigned int skip_default_link_activation_delay:1;
unsigned int no_vf_scan:1; /* Don't scan for VFs after IOV enablement */
pci_dev_flags_t dev_flags;
atomic_t enable_cnt; /* pci_enable_device has been called */
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 1ab1e24bcbce..315b555b3444 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2711,6 +2711,10 @@
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE 0x1576
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI 0x1577
#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE 0x1578
+#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE 0x15c0
+#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7
+#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea
+#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15ef
#define PCI_DEVICE_ID_INTEL_80960_RP 0x1960
#define PCI_DEVICE_ID_INTEL_QAT_C3XXX 0x19e2
#define PCI_DEVICE_ID_INTEL_QAT_C3XXX_VF 0x19e3
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices
2020-08-31 9:31 [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices Mika Westerberg
@ 2020-08-31 18:13 ` Lyude Paul
2020-09-03 18:11 ` Bjorn Helgaas
1 sibling, 0 replies; 6+ messages in thread
From: Lyude Paul @ 2020-08-31 18:13 UTC (permalink / raw)
To: Mika Westerberg, Bjorn Helgaas
Cc: Kai-Heng Feng, Karol Herbst, Patrick Volkerding, Lukas Wunner,
Ben Skeggs, linux-pci
Reviewed-by: Lyude Paul <lyude@redhat.com>
On Mon, 2020-08-31 at 12:31 +0300, Mika Westerberg wrote:
> Kai-Heng Feng reported that it takes a long time (> 1 s) to resume
> Thunderbolt-connected devices from both runtime suspend and system sleep
> (s2idle).
>
> This was because some Downstream Ports that support > 5 GT/s do not also
> support Data Link Layer Link Active reporting. Per PCIe r5.0 sec 6.6.1:
>
> With a Downstream Port that supports Link speeds greater than 5.0 GT/s,
> software must wait a minimum of 100 ms after Link training completes
> before sending a Configuration Request to the device immediately below
> that Port. Software can determine when Link training completes by
> polling the Data Link Layer Link Active bit or by setting up an
> associated interrupt (see Section 6.7.3.3).
>
> Sec 7.5.3.6 requires such Ports to support DLL Link Active reporting,
> but at least the Intel JHL6240 Thunderbolt 3 Bridge [8086:15c0] and
> Intel JHL7540 Thunderbolt 3 Bridge [8086:15e7, 8086:15ea, 8086:15ef] do
> not.
>
> This adds a quirk for these devices that skips the the 1000 ms default
> link activation delay.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206837
> Link:
> https://lore.kernel.org/r/20200514133043.27429-1-mika.westerberg@linux.intel.com
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Hi all,
>
> The previous version of the patch can be found below:
>
> https://www.spinics.net/lists/linux-pci/msg97860.html
>
> This version adds a quirk instead covering the two devices Kai-Heng Feng
> reported. I added Titan Ridge DD and 2C because I think they are affected
> as well.
>
> Since the PCI IDs of these devices are now used in two places, I moved them
> from TBT driver to pci_ids.h.
>
> @Kai-Heng, if you still have access to this hardware, it would be great if
> you could try this out.
>
> drivers/pci/pci.c | 2 ++
> drivers/pci/quirks.c | 23 +++++++++++++++++++++++
> drivers/thunderbolt/nhi.h | 4 ----
> include/linux/pci.h | 5 +++++
> include/linux/pci_ids.h | 4 ++++
> 5 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e39c5499770f..16b61def1d46 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4674,6 +4674,8 @@ static bool pcie_wait_for_link_delay(struct pci_dev
> *pdev, bool active,
> * case, we wait for 1000 ms + any delay requested by the caller.
> */
> if (!pdev->link_active_reporting) {
> + if (active && pdev->skip_default_link_activation_delay)
> + timeout = 0;
> msleep(timeout + delay);
> return true;
> }
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2a589b6d6ed8..9269abb6455d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3621,6 +3621,29 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_PORT_RIDGE,
> quirk_thunderbolt_hotplug_msi);
>
> +/*
> + * https://bugzilla.kernel.org/show_bug.cgi?id=206837
> + *
> + * Non-hotplug PCIe downstream ports of these devices do not support active
> + * link reporting but they are known to train the link within 100ms.
> + */
> +static void quirk_skip_default_link_activation_delay(struct pci_dev *pdev)
> +{
> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
> + !pdev->link_active_reporting && !pdev->is_hotplug_bridge) {
> + pci_dbg(pdev, "skipping 1000 ms default link activation
> delay\n");
> + pdev->skip_default_link_activation_delay = true;
> + }
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE,
> + quirk_skip_default_link_activation_delay);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE,
> + quirk_skip_default_link_activation_delay);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE,
> + quirk_skip_default_link_activation_delay);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE,
> + quirk_skip_default_link_activation_delay);
> +
> #ifdef CONFIG_ACPI
> /*
> * Apple: Shutdown Cactus Ridge Thunderbolt controller.
> diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
> index 80162e4b013f..c023091f6b4e 100644
> --- a/drivers/thunderbolt/nhi.h
> +++ b/drivers/thunderbolt/nhi.h
> @@ -58,7 +58,6 @@ extern const struct tb_nhi_ops icl_nhi_ops;
> #define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_NHI 0x157d
> #define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_BRIDGE 0x157e
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI 0x15bf
> -#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE 0x15c0
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI 0x15d2
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE 0x15d3
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI 0x15d9
> @@ -66,11 +65,8 @@ extern const struct tb_nhi_ops icl_nhi_ops;
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI 0x15dc
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI 0x15dd
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI 0x15de
> -#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7
> #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI 0x15e8
> -#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea
> #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI 0x15eb
> -#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15ef
> #define PCI_DEVICE_ID_INTEL_ICL_NHI1 0x8a0d
> #define PCI_DEVICE_ID_INTEL_ICL_NHI0 0x8a17
> #define PCI_DEVICE_ID_INTEL_TGL_NHI0 0x9a1b
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 835530605c0d..c44ee4337a2a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -444,6 +444,11 @@ struct pci_dev {
> unsigned int non_compliant_bars:1; /* Broken BARs; ignore them
> */
> unsigned int is_probed:1; /* Device probing in
> progress */
> unsigned int link_active_reporting:1;/* Device capable of
> reporting link active */
> + /*
> + * Skip default 1000 ms wait on ports that do not support active
> + * link reporting (link_active_reporting == 0).
> + */
> + unsigned int skip_default_link_activation_delay:1;
> unsigned int no_vf_scan:1; /* Don't scan for VFs after
> IOV enablement */
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 1ab1e24bcbce..315b555b3444 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2711,6 +2711,10 @@
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE 0x1576
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI 0x1577
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE 0x1578
> +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE 0x15c0
> +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7
> +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea
> +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15ef
> #define PCI_DEVICE_ID_INTEL_80960_RP 0x1960
> #define PCI_DEVICE_ID_INTEL_QAT_C3XXX 0x19e2
> #define PCI_DEVICE_ID_INTEL_QAT_C3XXX_VF 0x19e3
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices
2020-08-31 9:31 [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices Mika Westerberg
2020-08-31 18:13 ` Lyude Paul
@ 2020-09-03 18:11 ` Bjorn Helgaas
2020-09-07 8:43 ` Mika Westerberg
1 sibling, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2020-09-03 18:11 UTC (permalink / raw)
To: Mika Westerberg
Cc: Bjorn Helgaas, Kai-Heng Feng, Karol Herbst, Lyude Paul,
Patrick Volkerding, Lukas Wunner, Ben Skeggs, linux-pci
On Mon, Aug 31, 2020 at 12:31:47PM +0300, Mika Westerberg wrote:
> Kai-Heng Feng reported that it takes a long time (> 1 s) to resume
> Thunderbolt-connected devices from both runtime suspend and system sleep
> (s2idle).
>
> This was because some Downstream Ports that support > 5 GT/s do not also
> support Data Link Layer Link Active reporting. Per PCIe r5.0 sec 6.6.1:
>
> With a Downstream Port that supports Link speeds greater than 5.0 GT/s,
> software must wait a minimum of 100 ms after Link training completes
> before sending a Configuration Request to the device immediately below
> that Port. Software can determine when Link training completes by
> polling the Data Link Layer Link Active bit or by setting up an
> associated interrupt (see Section 6.7.3.3).
>
> Sec 7.5.3.6 requires such Ports to support DLL Link Active reporting,
> but at least the Intel JHL6240 Thunderbolt 3 Bridge [8086:15c0] and
> Intel JHL7540 Thunderbolt 3 Bridge [8086:15e7, 8086:15ea, 8086:15ef] do
> not.
Is there any erratum about this? I'm just hoping to avoid the
maintenance hassle of adding new devices to the quirk. If Intel
acknowledges this as a defect and has a plan to fix it, that would
help a lot. If they *don't* think it's a defect, then maybe they have
a hint about how we should handle this generically.
> This adds a quirk for these devices that skips the the 1000 ms default
> link activation delay.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206837
> Link: https://lore.kernel.org/r/20200514133043.27429-1-mika.westerberg@linux.intel.com
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Hi all,
>
> The previous version of the patch can be found below:
>
> https://www.spinics.net/lists/linux-pci/msg97860.html
>
> This version adds a quirk instead covering the two devices Kai-Heng Feng
> reported. I added Titan Ridge DD and 2C because I think they are affected
> as well.
>
> Since the PCI IDs of these devices are now used in two places, I moved them
> from TBT driver to pci_ids.h.
>
> @Kai-Heng, if you still have access to this hardware, it would be great if
> you could try this out.
>
> drivers/pci/pci.c | 2 ++
> drivers/pci/quirks.c | 23 +++++++++++++++++++++++
> drivers/thunderbolt/nhi.h | 4 ----
> include/linux/pci.h | 5 +++++
> include/linux/pci_ids.h | 4 ++++
> 5 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e39c5499770f..16b61def1d46 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4674,6 +4674,8 @@ static bool pcie_wait_for_link_delay(struct pci_dev *pdev, bool active,
> * case, we wait for 1000 ms + any delay requested by the caller.
> */
> if (!pdev->link_active_reporting) {
> + if (active && pdev->skip_default_link_activation_delay)
> + timeout = 0;
> msleep(timeout + delay);
> return true;
> }
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2a589b6d6ed8..9269abb6455d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3621,6 +3621,29 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PORT_RIDGE,
> quirk_thunderbolt_hotplug_msi);
>
> +/*
> + * https://bugzilla.kernel.org/show_bug.cgi?id=206837
> + *
> + * Non-hotplug PCIe downstream ports of these devices do not support active
> + * link reporting but they are known to train the link within 100ms.
> + */
> +static void quirk_skip_default_link_activation_delay(struct pci_dev *pdev)
> +{
> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM &&
> + !pdev->link_active_reporting && !pdev->is_hotplug_bridge) {
> + pci_dbg(pdev, "skipping 1000 ms default link activation delay\n");
> + pdev->skip_default_link_activation_delay = true;
> + }
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE,
> + quirk_skip_default_link_activation_delay);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE,
> + quirk_skip_default_link_activation_delay);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE,
> + quirk_skip_default_link_activation_delay);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE,
> + quirk_skip_default_link_activation_delay);
> +
> #ifdef CONFIG_ACPI
> /*
> * Apple: Shutdown Cactus Ridge Thunderbolt controller.
> diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
> index 80162e4b013f..c023091f6b4e 100644
> --- a/drivers/thunderbolt/nhi.h
> +++ b/drivers/thunderbolt/nhi.h
> @@ -58,7 +58,6 @@ extern const struct tb_nhi_ops icl_nhi_ops;
> #define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_NHI 0x157d
> #define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_BRIDGE 0x157e
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI 0x15bf
> -#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE 0x15c0
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI 0x15d2
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE 0x15d3
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI 0x15d9
> @@ -66,11 +65,8 @@ extern const struct tb_nhi_ops icl_nhi_ops;
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI 0x15dc
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI 0x15dd
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI 0x15de
> -#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7
> #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI 0x15e8
> -#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea
> #define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI 0x15eb
> -#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15ef
> #define PCI_DEVICE_ID_INTEL_ICL_NHI1 0x8a0d
> #define PCI_DEVICE_ID_INTEL_ICL_NHI0 0x8a17
> #define PCI_DEVICE_ID_INTEL_TGL_NHI0 0x9a1b
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 835530605c0d..c44ee4337a2a 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -444,6 +444,11 @@ struct pci_dev {
> unsigned int non_compliant_bars:1; /* Broken BARs; ignore them */
> unsigned int is_probed:1; /* Device probing in progress */
> unsigned int link_active_reporting:1;/* Device capable of reporting link active */
> + /*
> + * Skip default 1000 ms wait on ports that do not support active
> + * link reporting (link_active_reporting == 0).
> + */
> + unsigned int skip_default_link_activation_delay:1;
> unsigned int no_vf_scan:1; /* Don't scan for VFs after IOV enablement */
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 1ab1e24bcbce..315b555b3444 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2711,6 +2711,10 @@
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE 0x1576
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI 0x1577
> #define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE 0x1578
> +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE 0x15c0
> +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE 0x15e7
> +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE 0x15ea
> +#define PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE 0x15ef
> #define PCI_DEVICE_ID_INTEL_80960_RP 0x1960
> #define PCI_DEVICE_ID_INTEL_QAT_C3XXX 0x19e2
> #define PCI_DEVICE_ID_INTEL_QAT_C3XXX_VF 0x19e3
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices
2020-09-03 18:11 ` Bjorn Helgaas
@ 2020-09-07 8:43 ` Mika Westerberg
2020-09-10 1:00 ` Bjorn Helgaas
0 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2020-09-07 8:43 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Kai-Heng Feng, Karol Herbst, Lyude Paul,
Patrick Volkerding, Lukas Wunner, Ben Skeggs, linux-pci
Hi,
On Thu, Sep 03, 2020 at 01:11:22PM -0500, Bjorn Helgaas wrote:
> On Mon, Aug 31, 2020 at 12:31:47PM +0300, Mika Westerberg wrote:
> > Kai-Heng Feng reported that it takes a long time (> 1 s) to resume
> > Thunderbolt-connected devices from both runtime suspend and system sleep
> > (s2idle).
> >
> > This was because some Downstream Ports that support > 5 GT/s do not also
> > support Data Link Layer Link Active reporting. Per PCIe r5.0 sec 6.6.1:
> >
> > With a Downstream Port that supports Link speeds greater than 5.0 GT/s,
> > software must wait a minimum of 100 ms after Link training completes
> > before sending a Configuration Request to the device immediately below
> > that Port. Software can determine when Link training completes by
> > polling the Data Link Layer Link Active bit or by setting up an
> > associated interrupt (see Section 6.7.3.3).
> >
> > Sec 7.5.3.6 requires such Ports to support DLL Link Active reporting,
> > but at least the Intel JHL6240 Thunderbolt 3 Bridge [8086:15c0] and
> > Intel JHL7540 Thunderbolt 3 Bridge [8086:15e7, 8086:15ea, 8086:15ef] do
> > not.
>
> Is there any erratum about this? I'm just hoping to avoid the
> maintenance hassle of adding new devices to the quirk. If Intel
> acknowledges this as a defect and has a plan to fix it, that would
> help a lot. If they *don't* think it's a defect, then maybe they have
> a hint about how we should handle this generically.
I don't think there is any public documentation about these chips so
probably no errata either. I did ask our TBT HW folks about this but so
far did not get any answer.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices
2020-09-07 8:43 ` Mika Westerberg
@ 2020-09-10 1:00 ` Bjorn Helgaas
2020-09-10 14:24 ` Mika Westerberg
0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2020-09-10 1:00 UTC (permalink / raw)
To: Mika Westerberg
Cc: Bjorn Helgaas, Kai-Heng Feng, Karol Herbst, Lyude Paul,
Patrick Volkerding, Lukas Wunner, Ben Skeggs, linux-pci
On Mon, Sep 07, 2020 at 11:43:49AM +0300, Mika Westerberg wrote:
> On Thu, Sep 03, 2020 at 01:11:22PM -0500, Bjorn Helgaas wrote:
> > On Mon, Aug 31, 2020 at 12:31:47PM +0300, Mika Westerberg wrote:
> > > Kai-Heng Feng reported that it takes a long time (> 1 s) to resume
> > > Thunderbolt-connected devices from both runtime suspend and system sleep
> > > (s2idle).
> > >
> > > This was because some Downstream Ports that support > 5 GT/s do not also
> > > support Data Link Layer Link Active reporting. Per PCIe r5.0 sec 6.6.1:
> > >
> > > With a Downstream Port that supports Link speeds greater than 5.0 GT/s,
> > > software must wait a minimum of 100 ms after Link training completes
> > > before sending a Configuration Request to the device immediately below
> > > that Port. Software can determine when Link training completes by
> > > polling the Data Link Layer Link Active bit or by setting up an
> > > associated interrupt (see Section 6.7.3.3).
> > >
> > > Sec 7.5.3.6 requires such Ports to support DLL Link Active reporting,
> > > but at least the Intel JHL6240 Thunderbolt 3 Bridge [8086:15c0] and
> > > Intel JHL7540 Thunderbolt 3 Bridge [8086:15e7, 8086:15ea, 8086:15ef] do
> > > not.
> >
> > Is there any erratum about this? I'm just hoping to avoid the
> > maintenance hassle of adding new devices to the quirk. If Intel
> > acknowledges this as a defect and has a plan to fix it, that would
> > help a lot. If they *don't* think it's a defect, then maybe they have
> > a hint about how we should handle this generically.
>
> I don't think there is any public documentation about these chips so
> probably no errata either. I did ask our TBT HW folks about this but so
> far did not get any answer.
Huh. AFAICT this is a non-fatal issue -- the only problem is that
resume takes longer than it should. The fix is somewhat ugly, both
because we have to maintain a list of affected devices, and because it
clutters a generic code path that is already quite complicated.
That's all to say that I'm not very happy about this and am not in a
huge hurry to apply it. Intel is usually pretty good about following
the PCIe spec and documenting issues when they occur. For some reason
TBT seems like an exception.
I don't maintain the TBT-specific stuff, so I personally don't care
all that much about that. But this issue is plain PCIe, nothing to do
with TBT. Kai-Heng, you, and I have all spent a lot time trying to
figure this out, and it makes me sad that Intel isn't giving us any
help.
Can you please ask them again?
Bjorn
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices
2020-09-10 1:00 ` Bjorn Helgaas
@ 2020-09-10 14:24 ` Mika Westerberg
0 siblings, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2020-09-10 14:24 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Kai-Heng Feng, Karol Herbst, Lyude Paul,
Patrick Volkerding, Lukas Wunner, Ben Skeggs, linux-pci
Hi,
On Wed, Sep 09, 2020 at 08:00:26PM -0500, Bjorn Helgaas wrote:
> Can you please ask them again?
I pinged them again and this time got confirmation that it is known
issue in Alpine Ridge and Titan Ridge, and beyond. They are looking for
a firmware fix for the next controller which could then be backported to
Alpine and Titan Ridge firmwares.
So let's hold this patch for now.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-10 21:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-31 9:31 [PATCH] PCI: Add a quirk to skip 1000 ms default link activation delay on some devices Mika Westerberg
2020-08-31 18:13 ` Lyude Paul
2020-09-03 18:11 ` Bjorn Helgaas
2020-09-07 8:43 ` Mika Westerberg
2020-09-10 1:00 ` Bjorn Helgaas
2020-09-10 14:24 ` Mika Westerberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).