[SRU] [D/E/Unstable/OEM-B] [PATCH 0/7] Make hotplugging docking station to Thunderbolt port more reliable

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU] [D/E/Unstable/OEM-B] [PATCH 0/7] Make hotplugging docking station to Thunderbolt port more reliable

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1853991

[Impact]
Sometimes USB ports and USB ethernet on Thunderbolt Docking don't work
after hotplugging.

[Fix]
Add proper link delay when PCIe devices transit out from D3cold.

[Test]
Tested with Dell WD19DC docking station on Dell XPS 9380. After applying
the fix, the USB ports on docking always work after hotplugging.

[Regression Potential]
Low. This patch series adds proper delay required by PCIe spec, and only
affects hotplug ports, i.e. Thunderbolt.

Felipe Balbi (1):
  PCI: Add support for Immediate Readiness

Keith Busch (1):
  PCI: Make link active reporting detection generic

Mika Westerberg (3):
  PCI: Make pcie_downstream_port() available outside of access.c
  PCI/PM: Add pcie_wait_for_link_delay()
  PCI/PM: Add missing link delays required by the PCIe spec

Oza Pawandeep (1):
  PCI/DPC: Clear interrupt status in interrupt handler top half

Rafael J. Wysocki (1):
  PCI: PM: Avoid skipping bus-level PM on platforms without ACPI

 drivers/pci/access.c             |   9 --
 drivers/pci/hotplug/pciehp_hpc.c |  22 +---
 drivers/pci/pci-driver.c         |  17 ++-
 drivers/pci/pci.c                | 180 +++++++++++++++++++++++++++++--
 drivers/pci/pci.h                |  10 ++
 drivers/pci/pcie/dpc.c           |   7 +-
 drivers/pci/probe.c              |   1 +
 include/linux/pci.h              |   2 +
 include/linux/suspend.h          |  26 ++++-
 include/uapi/linux/pci_regs.h    |   1 +
 kernel/power/suspend.c           |   3 +
 11 files changed, 233 insertions(+), 45 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[OEM-B] [PATCH 1/7] PCI/DPC: Clear interrupt status in interrupt handler top half

Kai-Heng Feng
From: Oza Pawandeep <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1853991

The generic IRQ handling code ensures that an interrupt handler runs with
its interrupt masked or disabled.  If the interrupt is level-triggered, the
interrupt handler must tell its device to stop asserting the interrupt
before returning.  If it doesn't, we will immediately take the interrupt
again when the handler returns and the generic code unmasks the interrupt.

The driver doesn't know whether its interrupt is edge- or level-triggered,
so it must clear its interrupt source directly in its interrupt handler.

Previously we cleared the DPC interrupt status in the bottom half, i.e., in
deferred work, which can cause an interrupt storm if the DPC interrupt
happens to be level-triggered, e.g., if we're using INTx instead of MSI.

Clear the DPC interrupt status bit in the interrupt handler, not in the
deferred work.

Signed-off-by: Oza Pawandeep <[hidden email]>
[bhelgaas: changelog]
Signed-off-by: Bjorn Helgaas <[hidden email]>
Reviewed-by: Keith Busch <[hidden email]>
(backported from commit 56abbf8ad73c89d0a4c3c84b1449ceaaabd1b8c7)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/pcie/dpc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index de937ea6d138..0292d7387454 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -126,7 +126,7 @@ static void interrupt_event_handler(struct work_struct *work)
  }
 
  pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
- PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT);
+      PCI_EXP_DPC_STATUS_TRIGGER);
 }
 
 static void dpc_rp_pio_print_tlp_header(struct device *dev,
@@ -241,7 +241,7 @@ static irqreturn_t dpc_irq(int irq, void *context)
  struct dpc_dev *dpc = (struct dpc_dev *)context;
  struct pci_dev *pdev = dpc->dev->port;
  struct device *dev = &dpc->dev->device;
- u16 status, source;
+ u16 cap = dpc->cap_pos, status, source;
 
  pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS, &status);
  pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_SOURCE_ID,
@@ -267,6 +267,9 @@ static irqreturn_t dpc_irq(int irq, void *context)
  if (reason == 3 && ext_reason == 0)
  dpc_process_rp_pio_error(dpc);
 
+ pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
+      PCI_EXP_DPC_STATUS_INTERRUPT);
+
  schedule_work(&dpc->work);
  }
  return IRQ_HANDLED;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[OEM-B] [PATCH 2/7] PCI: Add support for Immediate Readiness

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Felipe Balbi <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1853991

PCIe r4.0, sec 7.5.1.1.4 defines a new bit in the Status Register:

  Immediate Readiness – This optional bit, when Set, indicates the Function
  is guaranteed to be ready to successfully complete valid configuration
  accesses at any time following any reset that the host is capable of
  issuing Configuration Requests to this Function.

  When this bit is Set, for accesses to this Function, software is exempt
  from all requirements to delay configuration accesses following any type
  of reset, including but not limited to the timing requirements defined in
  Section 6.6.

This means that all delays after a Conventional or Function Reset can be
skipped.

This patch reads such bit and caches its value in a flag inside struct
pci_dev to be checked later if we should delay or can skip delays after a
reset.  While at that, also move the explicit msleep(100) call from
pcie_flr() and pci_af_flr() to pci_dev_wait().

Signed-off-by: Felipe Balbi <[hidden email]>
[bhelgaas: rename PCI_STATUS_IMMEDIATE to PCI_STATUS_IMM_READY]
Signed-off-by: Bjorn Helgaas <[hidden email]>
(cherry picked from commit d6112f8def514e019658bcc9b57d53acdb71ca3f)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/pci.c             | 13 ++++++++++++-
 include/linux/pci.h           |  1 +
 include/uapi/linux/pci_regs.h |  1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0a0b09663928..9316d57afc48 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -988,7 +988,7 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
  * because have already delayed for the bridge.
  */
  if (dev->runtime_d3cold) {
- if (dev->d3cold_delay)
+ if (dev->d3cold_delay && !dev->imm_ready)
  msleep(dev->d3cold_delay);
  /*
  * When powering on a bridge from D3cold, the
@@ -2694,6 +2694,7 @@ EXPORT_SYMBOL_GPL(pci_d3cold_disable);
 void pci_pm_init(struct pci_dev *dev)
 {
  int pm;
+ u16 status;
  u16 pmc;
 
  pm_runtime_forbid(&dev->dev);
@@ -2756,6 +2757,10 @@ void pci_pm_init(struct pci_dev *dev)
  /* Disable the PME# generation functionality */
  pci_pme_active(dev, false);
  }
+
+ pci_read_config_word(dev, PCI_STATUS, &status);
+ if (status & PCI_STATUS_IMM_READY)
+ dev->imm_ready = 1;
 }
 
 static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
@@ -4347,6 +4352,9 @@ int pcie_flr(struct pci_dev *dev)
 
  pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
 
+ if (dev->imm_ready)
+ return 0;
+
  /*
  * Per PCIe r4.0, sec 6.6.2, a device must complete an FLR within
  * 100ms, but may silently discard requests while the FLR is in
@@ -4388,6 +4396,9 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
 
  pci_write_config_byte(dev, pos + PCI_AF_CTRL, PCI_AF_CTRL_FLR);
 
+ if (dev->imm_ready)
+ return 0;
+
  /*
  * Per Advanced Capabilities for Conventional PCI ECN, 13 April 2006,
  * updated 27 July 2006; a device must complete an FLR within
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f324bd21c389..d2b5121bd1c5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -320,6 +320,7 @@ struct pci_dev {
  pci_power_t current_state; /* Current operating state. In ACPI,
    this is D0-D3, D0 being fully
    functional, and D3 being off. */
+ unsigned int imm_ready:1; /* Supports Immediate Readiness */
  u8 pm_cap; /* PM capability offset */
  unsigned int pme_support:5; /* Bitmask of states from which PME#
    can be generated */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e113b67f9b9c..b9c302de68b9 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -52,6 +52,7 @@
 #define  PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable */
 
 #define PCI_STATUS 0x06 /* 16 bits */
+#define  PCI_STATUS_IMM_READY 0x01 /* Immediate Readiness */
 #define  PCI_STATUS_INTERRUPT 0x08 /* Interrupt status */
 #define  PCI_STATUS_CAP_LIST 0x10 /* Support Capability List */
 #define  PCI_STATUS_66MHZ 0x20 /* Support 66 MHz PCI 2.1 bus */
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[OEM-B] [PATCH 3/7] PCI: Make link active reporting detection generic

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Keith Busch <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1853991

The spec has timing requirements when waiting for a link to become active
after a conventional reset.  Implement those hard delays when waiting for
an active link so pciehp and dpc drivers don't need to duplicate this.

For devices that don't support data link layer active reporting, wait the
fixed time recommended by the PCIe spec.

Signed-off-by: Keith Busch <[hidden email]>
[bhelgaas: changelog]
Signed-off-by: Bjorn Helgaas <[hidden email]>
Reviewed-by: Sinan Kaya <[hidden email]>
(backported from commit f0157160b359b1d263ee9d4e0a435a7ad85bbcea)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/hotplug/pciehp_hpc.c | 22 ++-------------------
 drivers/pci/pci.c                | 33 ++++++++++++++++++++++++++------
 drivers/pci/probe.c              |  1 +
 include/linux/pci.h              |  1 +
 4 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 7548b2c18641..57eb307053e6 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -217,13 +217,6 @@ int pciehp_check_link_active(struct controller *ctrl)
  return ret;
 }
 
-static void pcie_wait_link_active(struct controller *ctrl)
-{
- struct pci_dev *pdev = ctrl_dev(ctrl);
-
- pcie_wait_for_link(pdev, true);
-}
-
 static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
 {
  u32 l;
@@ -256,18 +249,9 @@ int pciehp_check_link_status(struct controller *ctrl)
  bool found;
  u16 lnk_status;
 
- /*
- * Data Link Layer Link Active Reporting must be capable for
- * hot-plug capable downstream port. But old controller might
- * not implement it. In this case, we wait for 1000 ms.
- */
- if (ctrl->link_active_reporting)
- pcie_wait_link_active(ctrl);
- else
- msleep(1000);
+ if (!pcie_wait_for_link(pdev, true))
+ return -1;
 
- /* wait 100ms before read pci conf, and try in 1s */
- msleep(100);
  found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
  PCI_DEVFN(0, 0));
 
@@ -905,8 +889,6 @@ struct controller *pcie_init(struct pcie_device *dev)
 
  /* Check if Data Link Layer Link Active Reporting is implemented */
  pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
- if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
- ctrl->link_active_reporting = 1;
 
  /* Clear all remaining event bits in Slot Status register. */
  pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9316d57afc48..bc390738a31b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4467,21 +4467,42 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
  bool ret;
  u16 lnk_status;
 
+ /*
+ * Some controllers might not implement link active reporting. In this
+ * case, we wait for 1000 + 100 ms.
+ */
+ if (!pdev->link_active_reporting) {
+ msleep(1100);
+ return true;
+ }
+
+ /*
+ * PCIe r4.0 sec 6.6.1, a component must enter LTSSM Detect within 20ms,
+ * after which we should expect an link active if the reset was
+ * successful. If so, software must wait a minimum 100ms before sending
+ * configuration requests to devices downstream this port.
+ *
+ * If the link fails to activate, either the device was physically
+ * removed or the link is permanently failed.
+ */
+ if (active)
+ msleep(20);
  for (;;) {
  pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
  ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
  if (ret == active)
- return true;
+ break;
  if (timeout <= 0)
  break;
  msleep(10);
  timeout -= 10;
  }
-
- pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n",
- active ? "set" : "cleared");
-
- return false;
+ if (active && ret)
+ msleep(100);
+ else if (ret != active)
+ pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n",
+ active ? "set" : "cleared");
+ return ret == active;
 }
 
 void pci_reset_secondary_bus(struct pci_dev *dev)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7f3a3b2a503e..29215736a52f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -703,6 +703,7 @@ static void pci_set_bus_speed(struct pci_bus *bus)
 
  pcie_capability_read_dword(bridge, PCI_EXP_LNKCAP, &linkcap);
  bus->max_bus_speed = pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS];
+ bridge->link_active_reporting = !!(linkcap & PCI_EXP_LNKCAP_DLLLARC);
 
  pcie_capability_read_word(bridge, PCI_EXP_LNKSTA, &linksta);
  pcie_update_link_speed(bus, linksta);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d2b5121bd1c5..069dc21a4b51 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -408,6 +408,7 @@ struct pci_dev {
  unsigned int has_secondary_link:1;
  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 */
  pci_dev_flags_t dev_flags;
  atomic_t enable_cnt; /* pci_enable_device has been called */
 
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[D/OEM-B][PATCH 4/7] PCI: PM: Avoid skipping bus-level PM on platforms without ACPI

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: "Rafael J. Wysocki" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1853991

There are platforms that do not call pm_set_suspend_via_firmware(),
so pm_suspend_via_firmware() returns 'false' on them, but the power
states of PCI devices (PCIe ports in particular) are changed as a
result of powering down core platform components during system-wide
suspend.  Thus the pm_suspend_via_firmware() checks in
pci_pm_suspend_noirq() and pci_pm_resume_noirq() introduced by
commit 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-
idle") are not sufficient to determine that devices left in D0
during suspend will remain in D0 during resume and so the bus-level
power management can be skipped for them.

For this reason, introduce a new global suspend flag,
PM_SUSPEND_FLAG_NO_PLATFORM, set it for suspend-to-idle only
and replace the pm_suspend_via_firmware() checks mentioned above
with checks against this flag.

Fixes: 3e26c5feed2a ("PCI: PM: Skip devices in D0 for suspend-to-idle")
Reported-by: Jon Hunter <[hidden email]>
Tested-by: Jon Hunter <[hidden email]>
Signed-off-by: Rafael J. Wysocki <[hidden email]>
Tested-by: Mika Westerberg <[hidden email]>
Reviewed-by: Mika Westerberg <[hidden email]>
(backported from commit 471a739a47aa7d582f0cdf9d392957d04632bae2)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/pci-driver.c |  8 ++++----
 include/linux/suspend.h  | 26 ++++++++++++++++++++++++--
 kernel/power/suspend.c   |  3 +++
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 38c0b2c00ce7..d1197fc97598 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -862,7 +862,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
  pci_dev->bus->self->skip_bus_pm = true;
  }
 
- if (pci_dev->skip_bus_pm && !pm_suspend_via_firmware()) {
+ if (pci_dev->skip_bus_pm && pm_suspend_no_platform()) {
  dev_dbg(dev, "PCI PM: Skipped\n");
  goto Fixup;
  }
@@ -917,10 +917,10 @@ static int pci_pm_resume_noirq(struct device *dev)
  /*
  * In the suspend-to-idle case, devices left in D0 during suspend will
  * stay in D0, so it is not necessary to restore or update their
- * configuration here and attempting to put them into D0 again may
- * confuse some firmware, so avoid doing that.
+ * configuration here and attempting to put them into D0 again is
+ * pointless, so avoid doing that.
  */
- if (!pci_dev->skip_bus_pm || pm_suspend_via_firmware())
+ if (!(pci_dev->skip_bus_pm && pm_suspend_no_platform()))
  pci_pm_default_resume_early(pci_dev);
 
  pci_fixup_device(pci_fixup_resume_early, pci_dev);
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 950b56d255ac..398bd1933943 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -209,8 +209,9 @@ extern int suspend_valid_only_mem(suspend_state_t state);
 
 extern unsigned int pm_suspend_global_flags;
 
-#define PM_SUSPEND_FLAG_FW_SUSPEND (1 << 0)
-#define PM_SUSPEND_FLAG_FW_RESUME (1 << 1)
+#define PM_SUSPEND_FLAG_FW_SUSPEND BIT(0)
+#define PM_SUSPEND_FLAG_FW_RESUME BIT(1)
+#define PM_SUSPEND_FLAG_NO_PLATFORM BIT(2)
 
 static inline void pm_suspend_clear_flags(void)
 {
@@ -227,6 +228,11 @@ static inline void pm_set_resume_via_firmware(void)
  pm_suspend_global_flags |= PM_SUSPEND_FLAG_FW_RESUME;
 }
 
+static inline void pm_set_suspend_no_platform(void)
+{
+ pm_suspend_global_flags |= PM_SUSPEND_FLAG_NO_PLATFORM;
+}
+
 static inline bool pm_suspend_via_firmware(void)
 {
  return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_SUSPEND);
@@ -237,6 +243,22 @@ static inline bool pm_resume_via_firmware(void)
  return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_FW_RESUME);
 }
 
+/**
+ * pm_suspend_no_platform - Check if platform may change device power states.
+ *
+ * To be called during system-wide power management transitions to sleep states
+ * or during the subsequent system-wide transitions back to the working state.
+ *
+ * Return 'true' if the power states of devices remain under full control of the
+ * kernel throughout the system-wide suspend and resume cycle in progress (that
+ * is, if a device is put into a certain power state during suspend, it can be
+ * expected to remain in that state during resume).
+ */
+static inline bool pm_suspend_no_platform(void)
+{
+ return !!(pm_suspend_global_flags & PM_SUSPEND_FLAG_NO_PLATFORM);
+}
+
 /* Suspend-to-idle state machnine. */
 enum s2idle_states {
  S2IDLE_STATE_NONE,      /* Not suspended/suspending. */
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 405e80219fe4..7173eb63f9cc 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -484,6 +484,9 @@ int suspend_devices_and_enter(suspend_state_t state)
 
  pm_suspend_target_state = state;
 
+ if (state == PM_SUSPEND_TO_IDLE)
+ pm_set_suspend_no_platform();
+
  error = platform_suspend_begin(state);
  if (error)
  goto Close;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[D/E] [PATCH 5/7] PCI: Make pcie_downstream_port() available outside of access.c

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1853991

pcie_downstream_port() is useful in other places where code needs to
determine whether the PCIe port is downstream so make it available outside
of access.c.

Link: https://lore.kernel.org/r/20190822085553.62697-1-mika.westerberg@...
Signed-off-by: Mika Westerberg <[hidden email]>
Signed-off-by: Bjorn Helgaas <[hidden email]>
Reviewed-by: Andy Shevchenko <[hidden email]>
(cherry picked from commit 984998e3404e9073479281dbba8af36b104e8c00)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/access.c | 9 ---------
 drivers/pci/pci.h    | 9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 544922f097c0..2fccb5762c76 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -336,15 +336,6 @@ static inline int pcie_cap_version(const struct pci_dev *dev)
  return pcie_caps_reg(dev) & PCI_EXP_FLAGS_VERS;
 }
 
-static bool pcie_downstream_port(const struct pci_dev *dev)
-{
- int type = pci_pcie_type(dev);
-
- return type == PCI_EXP_TYPE_ROOT_PORT ||
-       type == PCI_EXP_TYPE_DOWNSTREAM ||
-       type == PCI_EXP_TYPE_PCIE_BRIDGE;
-}
-
 bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 {
  int type = pci_pcie_type(dev);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d22d1b807701..a2fe6fc3cfaf 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -118,6 +118,15 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
  return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3;
 }
 
+static inline bool pcie_downstream_port(const struct pci_dev *dev)
+{
+ int type = pci_pcie_type(dev);
+
+ return type == PCI_EXP_TYPE_ROOT_PORT ||
+       type == PCI_EXP_TYPE_DOWNSTREAM ||
+       type == PCI_EXP_TYPE_PCIE_BRIDGE;
+}
+
 int pci_vpd_init(struct pci_dev *dev);
 void pci_vpd_release(struct pci_dev *dev);
 void pcie_vpd_create_sysfs_dev_files(struct pci_dev *dev);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[OEM-B] [PATCH 5/7] PCI: Make pcie_downstream_port() available outside of access.c

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1853991

pcie_downstream_port() is useful in other places where code needs to
determine whether the PCIe port is downstream so make it available outside
of access.c.

Link: https://lore.kernel.org/r/20190822085553.62697-1-mika.westerberg@...
Signed-off-by: Mika Westerberg <[hidden email]>
Signed-off-by: Bjorn Helgaas <[hidden email]>
Reviewed-by: Andy Shevchenko <[hidden email]>
(backported from commit 984998e3404e9073479281dbba8af36b104e8c00)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/access.c | 9 ---------
 drivers/pci/pci.h    | 9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 61a45bc0efc8..fbea43cd0c5f 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -701,15 +701,6 @@ static inline int pcie_cap_version(const struct pci_dev *dev)
  return pcie_caps_reg(dev) & PCI_EXP_FLAGS_VERS;
 }
 
-static bool pcie_downstream_port(const struct pci_dev *dev)
-{
- int type = pci_pcie_type(dev);
-
- return type == PCI_EXP_TYPE_ROOT_PORT ||
-       type == PCI_EXP_TYPE_DOWNSTREAM ||
-       type == PCI_EXP_TYPE_PCIE_BRIDGE;
-}
-
 bool pcie_cap_has_lnkctl(const struct pci_dev *dev)
 {
  int type = pci_pcie_type(dev);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index eccd932ecafb..a90c18982f20 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -108,6 +108,15 @@ static inline bool pci_power_manageable(struct pci_dev *pci_dev)
  return !pci_has_subordinate(pci_dev) || pci_dev->bridge_d3;
 }
 
+static inline bool pcie_downstream_port(const struct pci_dev *dev)
+{
+ int type = pci_pcie_type(dev);
+
+ return type == PCI_EXP_TYPE_ROOT_PORT ||
+       type == PCI_EXP_TYPE_DOWNSTREAM ||
+       type == PCI_EXP_TYPE_PCIE_BRIDGE;
+}
+
 struct pci_vpd_ops {
  ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
  ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[D/E/Unstable/OEM-B] [PATCH 6/7] PCI/PM: Add pcie_wait_for_link_delay()

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1853991

Add pcie_wait_for_link_delay().  Similar to pcie_wait_for_link() but allows
passing custom activation delay in milliseconds.

Link: https://lore.kernel.org/r/20191112091617.70282-2-mika.westerberg@...
Signed-off-by: Mika Westerberg <[hidden email]>
Signed-off-by: Bjorn Helgaas <[hidden email]>
Reviewed-by: Rafael J. Wysocki <[hidden email]>
(cherry picked from commit 4827d63891b6a839dac49c6ab62e61c4b011c4f2 linux-next)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/pci.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bc390738a31b..cd31d8ee209a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4454,14 +4454,17 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
 
  return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS);
 }
+
 /**
- * pcie_wait_for_link - Wait until link is active or inactive
+ * pcie_wait_for_link_delay - Wait until link is active or inactive
  * @pdev: Bridge device
  * @active: waiting for active or inactive?
+ * @delay: Delay to wait after link has become active (in ms)
  *
  * Use this to wait till link becomes active or inactive.
  */
-bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
+static bool pcie_wait_for_link_delay(struct pci_dev *pdev, bool active,
+     int delay)
 {
  int timeout = 1000;
  bool ret;
@@ -4498,13 +4501,25 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
  timeout -= 10;
  }
  if (active && ret)
- msleep(100);
+ msleep(delay);
  else if (ret != active)
  pci_info(pdev, "Data Link Layer Link Active not %s in 1000 msec\n",
  active ? "set" : "cleared");
  return ret == active;
 }
 
+/**
+ * pcie_wait_for_link - Wait until link is active or inactive
+ * @pdev: Bridge device
+ * @active: waiting for active or inactive?
+ *
+ * Use this to wait till link becomes active or inactive.
+ */
+bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
+{
+ return pcie_wait_for_link_delay(pdev, active, 100);
+}
+
 void pci_reset_secondary_bus(struct pci_dev *dev)
 {
  u16 ctrl;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[D/E/Unstable/OEM-B] [PATCH 7/7] PCI/PM: Add missing link delays required by the PCIe spec

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Mika Westerberg <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1853991

Currently Linux does not follow PCIe spec regarding the required delays
after reset. A concrete example is a Thunderbolt add-in-card that consists
of a PCIe switch and two PCIe endpoints:

  +-1b.0-[01-6b]----00.0-[02-6b]--+-00.0-[03]----00.0 TBT controller
                                  +-01.0-[04-36]-- DS hotplug port
                                  +-02.0-[37]----00.0 xHCI controller
                                  \-04.0-[38-6b]-- DS hotplug port

The root port (1b.0) and the PCIe switch downstream ports are all PCIe Gen3
so they support 8GT/s link speeds.

We wait for the PCIe hierarchy to enter D3cold (runtime):

  pcieport 0000:00:1b.0: power state changed by ACPI to D3cold

When it wakes up from D3cold, according to the PCIe 5.0 section 5.8 the
PCIe switch is put to reset and its power is re-applied. This means that we
must follow the rules in PCIe 5.0 section 6.6.1.

For the PCIe Gen3 ports we are dealing with here, the following applies:

  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).

Translating this into the above topology we would need to do this (DLLLA
stands for Data Link Layer Link Active):

  0000:00:1b.0: wait for 100 ms after DLLLA is set before access to 0000:01:00.0
  0000:02:00.0: wait for 100 ms after DLLLA is set before access to 0000:03:00.0
  0000:02:02.0: wait for 100 ms after DLLLA is set before access to 0000:37:00.0

I've instrumented the kernel with some additional logging so we can see the
actual delays performed:

  pcieport 0000:00:1b.0: power state changed by ACPI to D0
  pcieport 0000:00:1b.0: waiting for D3cold delay of 100 ms
  pcieport 0000:00:1b.0: waiting for D3hot delay of 10 ms
  pcieport 0000:02:01.0: waiting for D3hot delay of 10 ms
  pcieport 0000:02:04.0: waiting for D3hot delay of 10 ms

For the switch upstream port (01:00.0 reachable through 00:1b.0 root port)
we wait for 100 ms but not taking into account the DLLLA requirement. We
then wait 10 ms for D3hot -> D0 transition of the root port and the two
downstream hotplug ports. This means that we deviate from what the spec
requires.

Performing the same check for system sleep (s2idle) transitions it turns
out to be even worse. None of the mandatory delays are performed. If this
would be S3 instead of s2idle then according to PCI FW spec 3.2 section
4.6.8. there is a specific _DSM that allows the OS to skip the delays but
this platform does not provide the _DSM and does not go to S3 anyway so no
firmware is involved that could already handle these delays.

On this particular platform these delays are not actually needed because
there is an additional delay as part of the ACPI power resource that is
used to turn on power to the hierarchy but since that additional delay is
not required by any of standards (PCIe, ACPI) it is not present in the
Intel Ice Lake, for example where missing the mandatory delays causes
pciehp to start tearing down the stack too early (links are not yet
trained). Below is an example how it looks like when this happens:

  pcieport 0000:83:04.0: pciehp: Slot(4): Card not present
  pcieport 0000:87:04.0: PME# disabled
  pcieport 0000:83:04.0: pciehp: pciehp_unconfigure_device: domain:bus:dev = 0000:86:00
  pcieport 0000:86:00.0: Refused to change power state, currently in D3
  pcieport 0000:86:00.0: restoring config space at offset 0x3c (was 0xffffffff, writing 0x201ff)
  pcieport 0000:86:00.0: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
  ...

There is also one reported case (see the bugzilla link below) where the
missing delay causes xHCI on a Titan Ridge controller fail to runtime
resume when USB-C dock is plugged. This does not involve pciehp but instead
the PCI core fails to runtime resume the xHCI device:

  pcieport 0000:04:02.0: restoring config space at offset 0xc (was 0x10000, writing 0x10020)
  pcieport 0000:04:02.0: restoring config space at offset 0x4 (was 0x100000, writing 0x100406)
  xhci_hcd 0000:39:00.0: Refused to change power state, currently in D3
  xhci_hcd 0000:39:00.0: restoring config space at offset 0x3c (was 0xffffffff, writing 0x1ff)
  xhci_hcd 0000:39:00.0: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
  ...

Add a new function pci_bridge_wait_for_secondary_bus() that is called on
PCI core resume and runtime resume paths accordingly if the bridge entered
D3cold (and thus went through reset).

This is second attempt to add the missing delays. The previous solution in
c2bf1fc212f7 ("PCI: Add missing link delays required by the PCIe spec") was
reverted because of two issues it caused:

  1. One system become unresponsive after S3 resume due to PME service
     spinning in pcie_pme_work_fn(). The root port in question reports that
     the xHCI sent PME but the xHCI device itself does not have PME status
     set. The PME status bit is never cleared in the root port resulting
     the indefinite loop in pcie_pme_work_fn().

  2. Slows down resume if the root/downstream port does not support Data
     Link Layer Active Reporting because pcie_wait_for_link_delay() waits
     1100 ms in that case.

This version should avoid the above issues because we restrict the delay to
happen only if the port went into D3cold.

Link: https://lore.kernel.org/linux-pci/SL2P216MB01878BBCD75F21D882AEEA2880C60@.../
Link: https://bugzilla.kernel.org/show_bug.cgi?id=203885
Link: https://lore.kernel.org/r/20191112091617.70282-3-mika.westerberg@...
Reported-by: Kai-Heng Feng <[hidden email]>
Tested-by: Kai-Heng Feng <[hidden email]>
Signed-off-by: Mika Westerberg <[hidden email]>
Signed-off-by: Bjorn Helgaas <[hidden email]>
(backported from commit ad9001f2f41198784b0423646450ba2cb24793a3 linux-next)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/pci/pci-driver.c |  11 +++-
 drivers/pci/pci.c        | 121 ++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci.h        |   1 +
 3 files changed, 130 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index d1197fc97598..e11e958d4759 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -902,6 +902,8 @@ static int pci_pm_resume_noirq(struct device *dev)
  struct pci_dev *pci_dev = to_pci_dev(dev);
  struct device_driver *drv = dev->driver;
  int error = 0;
+ pci_power_t prev_state = pci_dev->current_state;
+ bool skip_bus_pm = pci_dev->skip_bus_pm;
 
  if (dev_pm_may_skip_resume(dev))
  return 0;
@@ -920,11 +922,14 @@ static int pci_pm_resume_noirq(struct device *dev)
  * configuration here and attempting to put them into D0 again is
  * pointless, so avoid doing that.
  */
- if (!(pci_dev->skip_bus_pm && pm_suspend_no_platform()))
+ if (!(skip_bus_pm && pm_suspend_no_platform()))
  pci_pm_default_resume_early(pci_dev);
 
  pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
+ if (!skip_bus_pm && prev_state == PCI_D3cold)
+ pci_bridge_wait_for_secondary_bus(pci_dev);
+
  if (pci_has_legacy_pm_support(pci_dev))
  return pci_legacy_resume_early(dev);
 
@@ -1335,6 +1340,7 @@ static int pci_pm_runtime_resume(struct device *dev)
  int rc = 0;
  struct pci_dev *pci_dev = to_pci_dev(dev);
  const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+ pci_power_t prev_state = pci_dev->current_state;
 
  /*
  * Restoring config space is necessary even if the device is not bound
@@ -1350,6 +1356,9 @@ static int pci_pm_runtime_resume(struct device *dev)
  pci_enable_wake(pci_dev, PCI_D0, false);
  pci_fixup_device(pci_fixup_resume, pci_dev);
 
+ if (prev_state == PCI_D3cold)
+ pci_bridge_wait_for_secondary_bus(pci_dev);
+
  if (pm && pm->runtime_resume)
  rc = pm->runtime_resume(dev);
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index cd31d8ee209a..a52cdf2895c4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -988,8 +988,6 @@ static void __pci_start_power_transition(struct pci_dev *dev, pci_power_t state)
  * because have already delayed for the bridge.
  */
  if (dev->runtime_d3cold) {
- if (dev->d3cold_delay && !dev->imm_ready)
- msleep(dev->d3cold_delay);
  /*
  * When powering on a bridge from D3cold, the
  * whole hierarchy may be powered on into
@@ -4520,6 +4518,125 @@ bool pcie_wait_for_link(struct pci_dev *pdev, bool active)
  return pcie_wait_for_link_delay(pdev, active, 100);
 }
 
+/*
+ * Find maximum D3cold delay required by all the devices on the bus.  The
+ * spec says 100 ms, but firmware can lower it and we allow drivers to
+ * increase it as well.
+ *
+ * Called with @pci_bus_sem locked for reading.
+ */
+static int pci_bus_max_d3cold_delay(const struct pci_bus *bus)
+{
+ const struct pci_dev *pdev;
+ int min_delay = 100;
+ int max_delay = 0;
+
+ list_for_each_entry(pdev, &bus->devices, bus_list) {
+ if (pdev->d3cold_delay < min_delay)
+ min_delay = pdev->d3cold_delay;
+ if (pdev->d3cold_delay > max_delay)
+ max_delay = pdev->d3cold_delay;
+ }
+
+ return max(min_delay, max_delay);
+}
+
+/**
+ * pci_bridge_wait_for_secondary_bus - Wait for secondary bus to be accessible
+ * @dev: PCI bridge
+ *
+ * Handle necessary delays before access to the devices on the secondary
+ * side of the bridge are permitted after D3cold to D0 transition.
+ *
+ * For PCIe this means the delays in PCIe 5.0 section 6.6.1. For
+ * conventional PCI it means Tpvrh + Trhfa specified in PCI 3.0 section
+ * 4.3.2.
+ */
+void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev)
+{
+ struct pci_dev *child;
+ int delay;
+
+ if (pci_dev_is_disconnected(dev))
+ return;
+
+ if (!pci_is_bridge(dev) || !dev->bridge_d3)
+ return;
+
+ down_read(&pci_bus_sem);
+
+ /*
+ * We only deal with devices that are present currently on the bus.
+ * For any hot-added devices the access delay is handled in pciehp
+ * board_added(). In case of ACPI hotplug the firmware is expected
+ * to configure the devices before OS is notified.
+ */
+ if (!dev->subordinate || list_empty(&dev->subordinate->devices)) {
+ up_read(&pci_bus_sem);
+ return;
+ }
+
+ /* Take d3cold_delay requirements into account */
+ delay = pci_bus_max_d3cold_delay(dev->subordinate);
+ if (!delay) {
+ up_read(&pci_bus_sem);
+ return;
+ }
+
+ child = list_first_entry(&dev->subordinate->devices, struct pci_dev,
+ bus_list);
+ up_read(&pci_bus_sem);
+
+ /*
+ * Conventional PCI and PCI-X we need to wait Tpvrh + Trhfa before
+ * accessing the device after reset (that is 1000 ms + 100 ms). In
+ * practice this should not be needed because we don't do power
+ * management for them (see pci_bridge_d3_possible()).
+ */
+ if (!pci_is_pcie(dev)) {
+ pci_dbg(dev, "waiting %d ms for secondary bus\n", 1000 + delay);
+ msleep(1000 + delay);
+ return;
+ }
+
+ /*
+ * For PCIe downstream and root ports that do not support speeds
+ * greater than 5 GT/s need to wait minimum 100 ms. For higher
+ * speeds (gen3) we need to wait first for the data link layer to
+ * become active.
+ *
+ * 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).
+ *
+ * Therefore we wait for 100 ms and check for the device presence.
+ * If it is still not present give it an additional 100 ms.
+ */
+ if (!pcie_downstream_port(dev))
+ return;
+
+ 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 */
+ return;
+ }
+ }
+
+ if (!pci_device_is_present(child)) {
+ pci_dbg(child, "waiting additional %d ms to become accessible\n", delay);
+ msleep(delay);
+ }
+}
+
 void pci_reset_secondary_bus(struct pci_dev *dev)
 {
  u16 ctrl;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a90c18982f20..9e36f752e2d6 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -87,6 +87,7 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev);
 void pci_free_cap_save_buffers(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
+void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
 {
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [SRU] [D/E/Unstable/OEM-B] [PATCH 0/7] Make hotplugging docking station to Thunderbolt port more reliable

Thadeu Lima de Souza Cascardo-3
In reply to this post by Kai-Heng Feng
On Tue, Nov 26, 2019 at 04:51:42PM +0800, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1853991
>
> [Impact]
> Sometimes USB ports and USB ethernet on Thunderbolt Docking don't work
> after hotplugging.
>
> [Fix]
> Add proper link delay when PCIe devices transit out from D3cold.
>
> [Test]
> Tested with Dell WD19DC docking station on Dell XPS 9380. After applying
> the fix, the USB ports on docking always work after hotplugging.
>
> [Regression Potential]
> Low. This patch series adds proper delay required by PCIe spec, and only
> affects hotplug ports, i.e. Thunderbolt.

That doesn't seem like a correct statement. The last commit mentions a previous
attempt was reverted because of regressions. And it's backported from
linux-next.

By reading the code, it seems it could at least cause some delays during
resume, but I would like to see this better tested to show that's likely the
most that could happen.

Could you test other systems and look for problems during suspend-resume tests?

Thanks.
Cascardo.

>
> Felipe Balbi (1):
>   PCI: Add support for Immediate Readiness
>
> Keith Busch (1):
>   PCI: Make link active reporting detection generic
>
> Mika Westerberg (3):
>   PCI: Make pcie_downstream_port() available outside of access.c
>   PCI/PM: Add pcie_wait_for_link_delay()
>   PCI/PM: Add missing link delays required by the PCIe spec
>
> Oza Pawandeep (1):
>   PCI/DPC: Clear interrupt status in interrupt handler top half
>
> Rafael J. Wysocki (1):
>   PCI: PM: Avoid skipping bus-level PM on platforms without ACPI
>
>  drivers/pci/access.c             |   9 --
>  drivers/pci/hotplug/pciehp_hpc.c |  22 +---
>  drivers/pci/pci-driver.c         |  17 ++-
>  drivers/pci/pci.c                | 180 +++++++++++++++++++++++++++++--
>  drivers/pci/pci.h                |  10 ++
>  drivers/pci/pcie/dpc.c           |   7 +-
>  drivers/pci/probe.c              |   1 +
>  include/linux/pci.h              |   2 +
>  include/linux/suspend.h          |  26 ++++-
>  include/uapi/linux/pci_regs.h    |   1 +
>  kernel/power/suspend.c           |   3 +
>  11 files changed, 233 insertions(+), 45 deletions(-)
>
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NAK [D/E]: [SRU] [D/E/Unstable/OEM-B] [PATCH 0/7] Make hotplugging docking station to Thunderbolt port more reliable

Kai-Heng Feng


> On Nov 26, 2019, at 17:18, Thadeu Lima de Souza Cascardo <[hidden email]> wrote:
>
> On Tue, Nov 26, 2019 at 04:51:42PM +0800, Kai-Heng Feng wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1853991
>>
>> [Impact]
>> Sometimes USB ports and USB ethernet on Thunderbolt Docking don't work
>> after hotplugging.
>>
>> [Fix]
>> Add proper link delay when PCIe devices transit out from D3cold.
>>
>> [Test]
>> Tested with Dell WD19DC docking station on Dell XPS 9380. After applying
>> the fix, the USB ports on docking always work after hotplugging.
>>
>> [Regression Potential]
>> Low. This patch series adds proper delay required by PCIe spec, and only
>> affects hotplug ports, i.e. Thunderbolt.
>
> That doesn't seem like a correct statement. The last commit mentions a previous
> attempt was reverted because of regressions. And it's backported from
> linux-next.

The patch author asked the user once had the regression to test it, and the result is no regression found.

> By reading the code, it seems it could at least cause some delays during
> resume, but I would like to see this better tested to show that's likely the
> most that could happen.

For systems use s2idle the delay won't be introduced because of direct-complete optimization.
For systems that use S3 there can be a delay, but at least I don't feel the delay on my tests.

>
> Could you test other systems and look for problems during suspend-resume tests?

I don't find any problems on the three systems I've tested but I do understand changing PCI core implies higher risk.
As this issue isn't that severe (the docking will work if it's plugged again), I think we can skip D/E and focus on Focal.

Kai-Heng

>
> Thanks.
> Cascardo.
>
>>
>> Felipe Balbi (1):
>>  PCI: Add support for Immediate Readiness
>>
>> Keith Busch (1):
>>  PCI: Make link active reporting detection generic
>>
>> Mika Westerberg (3):
>>  PCI: Make pcie_downstream_port() available outside of access.c
>>  PCI/PM: Add pcie_wait_for_link_delay()
>>  PCI/PM: Add missing link delays required by the PCIe spec
>>
>> Oza Pawandeep (1):
>>  PCI/DPC: Clear interrupt status in interrupt handler top half
>>
>> Rafael J. Wysocki (1):
>>  PCI: PM: Avoid skipping bus-level PM on platforms without ACPI
>>
>> drivers/pci/access.c             |   9 --
>> drivers/pci/hotplug/pciehp_hpc.c |  22 +---
>> drivers/pci/pci-driver.c         |  17 ++-
>> drivers/pci/pci.c                | 180 +++++++++++++++++++++++++++++--
>> drivers/pci/pci.h                |  10 ++
>> drivers/pci/pcie/dpc.c           |   7 +-
>> drivers/pci/probe.c              |   1 +
>> include/linux/pci.h              |   2 +
>> include/linux/suspend.h          |  26 ++++-
>> include/uapi/linux/pci_regs.h    |   1 +
>> kernel/power/suspend.c           |   3 +
>> 11 files changed, 233 insertions(+), 45 deletions(-)
>>
>> --
>> 2.17.1
>>
>>
>> --
>> kernel-team mailing list
>> [hidden email]
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED[Unstable]: [SRU] [D/E/Unstable/OEM-B] [PATCH 0/7] Make hotplugging docking station to Thunderbolt port more reliable

Seth Forshee
In reply to this post by Kai-Heng Feng
On Tue, Nov 26, 2019 at 04:51:42PM +0800, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1853991
>
> [Impact]
> Sometimes USB ports and USB ethernet on Thunderbolt Docking don't work
> after hotplugging.
>
> [Fix]
> Add proper link delay when PCIe devices transit out from D3cold.
>
> [Test]
> Tested with Dell WD19DC docking station on Dell XPS 9380. After applying
> the fix, the USB ports on docking always work after hotplugging.
>
> [Regression Potential]
> Low. This patch series adds proper delay required by PCIe spec, and only
> affects hotplug ports, i.e. Thunderbolt.

Applied to unstable/master, thanks!

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team