PCI: Fixes and cleanups for native PCIe and ACPI hotplug

When hot-adding a PCIe switch the way we currently distribute resources does not always work well because devices connected to the switch might need to have their MMIO resources aligned to something else than the default 1 MB boundary. For example Intel Gigabit ET2 quad port server adapter includes PCIe switch leading to 4 x GbE NIC devices that want to have their MMIO resources aligned to 2 MB boundary instead.

The current resource distribution code does not take this alignment into account and might try to add too much resources for the extension hotplug bridge(s). The resulting bridge window is too big which makes the resource assignment operation fail, and we are left with a bridge window with minimal amount (1 MB) of MMIO space.

Here is what happens when an Intel Gigabit ET2 quad port server adapter is hot-added:

pci 0000:39:00.0: BAR 14: assigned [mem 0x53300000-0x6a0fffff]

                                          ^^^^^^^^^^
  pci 0000:3a:01.0: BAR 14: assigned [mem 0x53400000-0x547fffff]
                                          ^^^^^^^^^^

The above shows that the downstream bridge (3a:01.0) window is aligned to 2 MB instead of 1 MB as is the upstream bridge (39:00.0) window. The remaining MMIO space (0x15a00000) is assigned to the hotplug bridge (3a:04.0) but it fails:

pci 0000:3a:04.0: BAR 14: no space for [mem size 0x15a00000] pci 0000:3a:04.0: BAR 14: failed to assign [mem size 0x15a00000]

The MMIO resource is calculated as follows:

start = 0x54800000
end = 0x54800000 + 0x15a00000 - 1 = 0x6a1fffff

This results bridge window [mem 0x54800000 - 0x6a1fffff] and it ends after the upstream bridge window [mem 0x53300000-0x6a0fffff] explaining the above failure. Because of this Linux falls back to the default allocation of 1 MB as can be seen from 'lspci' output:

39:00.0 Memory behind bridge: 53300000-6a0fffff [size=366M]
3a:01.0 Memory behind bridge: 53400000-547fffff [size=20M] 3a:04.0 Memory behind bridge: 53300000-533fffff [size=1M]

The hotplug bridge 3a:04.0 only occupies 1 MB MMIO window which is clearly not enough for extending the PCIe topology later if more devices are to be hot-added.

Fix this by substracting properly aligned non-hotplug downstream bridge window size from the remaining resources used for extension. After this change the resource allocation looks like:

39:00.0 Memory behind bridge: 53300000-6a0fffff [size=366M]

    3a:01.0 Memory behind bridge: 53400000-547fffff [size=20M]
    3a:04.0 Memory behind bridge: 54800000-6a0fffff [size=345M]

This matches the expectation. All the extra MMIO resource space (345 MB) is allocated to the extension hotplug bridge (3a:04.0).

Fixes 1a5767725cec ("PCI: Distribute available resources to hotplug-capable bridges")

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Cc [email protected]

---
 drivers/pci/setup-bus.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

drivers/pci/setup-bus.c +40 -1

index 072784f55ea5..eb3059fb7f63 100644
@@ -1878,6 +1878,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 	resource_size_t available_mmio, resource_size_t available_mmio_pref)
 {
 	resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+	resource_size_t io_start, mmio_start, mmio_pref_start;
 	unsigned int normal_bridges = 0, hotplug_bridges = 0;
 	struct resource *io_res, *mmio_res, *mmio_pref_res;
 	struct pci_dev *dev, *bridge = bus->self;

@@ -1942,11 +1943,16 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			remaining_mmio_pref -= resource_size(res);
 	}
 
+	io_start = io_res->start;
+	mmio_start = mmio_res->start;
+	mmio_pref_start = mmio_pref_res->start;
+
 	/*
 	 * Go over devices on this bus and distribute the remaining
 	 * resource space between hotplug bridges.
 	 */
 	for_each_pci_bridge(dev, bus) {
+		resource_size_t align;
 		struct pci_bus *b;
 
 		b = dev->subordinate;

@@ -1964,7 +1970,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 				available_io, available_mmio,
 				available_mmio_pref);
 		} else if (dev->is_hotplug_bridge) {
-			resource_size_t align, io, mmio, mmio_pref;
+			resource_size_t io, mmio, mmio_pref;
 
 			/*
 			 * Distribute available extra resources equally

@@ -1977,11 +1983,13 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			io = div64_ul(available_io, hotplug_bridges);
 			io = min(ALIGN(io, align), remaining_io);
 			remaining_io -= io;
+			io_start += io;
 
 			align = pci_resource_alignment(bridge, mmio_res);
 			mmio = div64_ul(available_mmio, hotplug_bridges);
 			mmio = min(ALIGN(mmio, align), remaining_mmio);
 			remaining_mmio -= mmio;
+			mmio_start += mmio;
 
 			align = pci_resource_alignment(bridge, mmio_pref_res);
 			mmio_pref = div64_ul(available_mmio_pref,

@@ -1989,9 +1997,40 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			mmio_pref = min(ALIGN(mmio_pref, align),
 					remaining_mmio_pref);
 			remaining_mmio_pref -= mmio_pref;
+			mmio_pref_start += mmio_pref;
 
 			pci_bus_distribute_available_resources(b, add_list, io,
 							       mmio, mmio_pref);
+		} else {
+			/*
+			 * For normal bridges, track start of the parent
+			 * bridge window to make sure we align the
+			 * remaining space which is distributed to the
+			 * hotplug bridges properly.
+			 */
+			resource_size_t aligned;
+			struct resource *res;
+
+			res = &dev->resource[PCI_BRIDGE_RESOURCES + 0];
+			io_start += resource_size(res);
+			aligned = ALIGN(io_start,
+					pci_resource_alignment(dev, res));
+			if (aligned > io_start)
+				remaining_io -= aligned - io_start;
+
+			res = &dev->resource[PCI_BRIDGE_RESOURCES + 1];
+			mmio_start += resource_size(res);
+			aligned = ALIGN(mmio_start,
+					pci_resource_alignment(dev, res));
+			if (aligned > mmio_start)
+				remaining_mmio -= aligned - mmio_start;
+
+			res = &dev->resource[PCI_BRIDGE_RESOURCES + 2];
+			mmio_pref_start += resource_size(res);
+			aligned = ALIGN(mmio_pref_start,
+					pci_resource_alignment(dev, res));
+			if (aligned > mmio_pref_start)
+				remaining_mmio_pref -= aligned - mmio_pref_start;
 		}
 	}
 }

Signature · 6 lines

2.16.3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 54 lines · Mika Westerberg

When hot-adding a PCIe switch the way we currently distribute resources does not always work well because devices connected to the switch might need to have their MMIO resources aligned to something else than the default 1 MB boundary. For example Intel Gigabit ET2 quad port server adapter includes PCIe switch leading to 4 x GbE NIC devices that want to have their MMIO resources aligned to 2 MB boundary instead.

The current resource distribution code does not take this alignment into account and might try to add too much resources for the extension hotplug bridge(s). The resulting bridge window is too big which makes the resource assignment operation fail, and we are left with a bridge window with minimal amount (1 MB) of MMIO space.

Here is what happens when an Intel Gigabit ET2 quad port server adapter is hot-added:

pci 0000:39:00.0: BAR 14: assigned [mem 0x53300000-0x6a0fffff]

                                          ^^^^^^^^^^
  pci 0000:3a:01.0: BAR 14: assigned [mem 0x53400000-0x547fffff]
                                          ^^^^^^^^^^

The above shows that the downstream bridge (3a:01.0) window is aligned to 2 MB instead of 1 MB as is the upstream bridge (39:00.0) window. The remaining MMIO space (0x15a00000) is assigned to the hotplug bridge (3a:04.0) but it fails:

pci 0000:3a:04.0: BAR 14: no space for [mem size 0x15a00000] pci 0000:3a:04.0: BAR 14: failed to assign [mem size 0x15a00000]

The MMIO resource is calculated as follows:

start = 0x54800000
end = 0x54800000 + 0x15a00000 - 1 = 0x6a1fffff

This results bridge window [mem 0x54800000 - 0x6a1fffff] and it ends after the upstream bridge window [mem 0x53300000-0x6a0fffff] explaining the above failure. Because of this Linux falls back to the default allocation of 1 MB as can be seen from 'lspci' output:

39:00.0 Memory behind bridge: 53300000-6a0fffff [size=366M]
3a:01.0 Memory behind bridge: 53400000-547fffff [size=20M] 3a:04.0 Memory behind bridge: 53300000-533fffff [size=1M]

The hotplug bridge 3a:04.0 only occupies 1 MB MMIO window which is clearly not enough for extending the PCIe topology later if more devices are to be hot-added.

Fix this by substracting properly aligned non-hotplug downstream bridge window size from the remaining resources used for extension. After this change the resource allocation looks like:

39:00.0 Memory behind bridge: 53300000-6a0fffff [size=366M]

    3a:01.0 Memory behind bridge: 53400000-547fffff [size=20M]
    3a:04.0 Memory behind bridge: 54800000-6a0fffff [size=345M]

Quoted 2 lines

This matches the expectation. All the extra MMIO resource space (345 MB) is allocated to the extension hotplug bridge (3a:04.0).

Sorry, I've spent a lot of time trying to trace through this code, and I'm still hopelessly confused. Can you post the complete "lspci -vv" output and the dmesg log (including the hot-add event) somewhere and include a URL to it?

I think I understand the problem you're solving:

- You have 366M, 1M-aligned, available for things on bus 3a
- You assign 20M, 2M-aligned to 3a:01.0
- This leaves 346M for other things on bus 3a, but it's not all contiguous because the 20M is in the middle.
- The remaining 346M might be 1M on one side and 345M on the other (and there are many other possibilities, e.g., 3M + 343M, 5M + 341M, ..., 345M + 1M).
- The current code tries to assign all 346M to 3a:04.0, which fails because that space is not contiguous, so it falls back to allocating 1M, which works but is insufficient for future hot-adds.

Obviously this patch makes *this* situation work: it assigns 345M to 3a:04.0 and (I assume) leaves the 1M unused. But I haven't been able to convince myself that this patch works *in general*.

For example, what if we assigned the 20M from the end of the 366M window instead of the beginning, so the 345M piece is below the 20M and there's 1M left above it? That is legal and should work, but I suspect this patch would ignore the 345M piece and again assign 1M to 3a:04.0.

Or what if there are several hotplug bridges on bus 3a? This example has two, but there could be many more.

Or what if there are normal bridges as well as hotplug bridges on bus 3a? Or if they're in arbitrary orders?

Quoted 5 lines

Fixes 1a5767725cec ("PCI: Distribute available resources to hotplug-capable bridges")

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Cc [email protected]

Given my confusion about this, I doubt this satisfies the stable kernel "obviously correct" rule.

s/substracting/subtracting/ above

Quoted 55 lines · Bjorn Helgaas

Quoted 54 lines · Mika Westerberg

When hot-adding a PCIe switch the way we currently distribute resources does not always work well because devices connected to the switch might need to have their MMIO resources aligned to something else than the default 1 MB boundary. For example Intel Gigabit ET2 quad port server adapter includes PCIe switch leading to 4 x GbE NIC devices that want to have their MMIO resources aligned to 2 MB boundary instead.

The current resource distribution code does not take this alignment into account and might try to add too much resources for the extension hotplug bridge(s). The resulting bridge window is too big which makes the resource assignment operation fail, and we are left with a bridge window with minimal amount (1 MB) of MMIO space.

Here is what happens when an Intel Gigabit ET2 quad port server adapter is hot-added:

pci 0000:39:00.0: BAR 14: assigned [mem 0x53300000-0x6a0fffff]

                                          ^^^^^^^^^^
  pci 0000:3a:01.0: BAR 14: assigned [mem 0x53400000-0x547fffff]
                                          ^^^^^^^^^^

The above shows that the downstream bridge (3a:01.0) window is aligned to 2 MB instead of 1 MB as is the upstream bridge (39:00.0) window. The remaining MMIO space (0x15a00000) is assigned to the hotplug bridge (3a:04.0) but it fails:

pci 0000:3a:04.0: BAR 14: no space for [mem size 0x15a00000] pci 0000:3a:04.0: BAR 14: failed to assign [mem size 0x15a00000]

The MMIO resource is calculated as follows:

start = 0x54800000
end = 0x54800000 + 0x15a00000 - 1 = 0x6a1fffff

This results bridge window [mem 0x54800000 - 0x6a1fffff] and it ends after the upstream bridge window [mem 0x53300000-0x6a0fffff] explaining the above failure. Because of this Linux falls back to the default allocation of 1 MB as can be seen from 'lspci' output:

39:00.0 Memory behind bridge: 53300000-6a0fffff [size=366M]
3a:01.0 Memory behind bridge: 53400000-547fffff [size=20M] 3a:04.0 Memory behind bridge: 53300000-533fffff [size=1M]

The hotplug bridge 3a:04.0 only occupies 1 MB MMIO window which is clearly not enough for extending the PCIe topology later if more devices are to be hot-added.

Fix this by substracting properly aligned non-hotplug downstream bridge window size from the remaining resources used for extension. After this change the resource allocation looks like:

39:00.0 Memory behind bridge: 53300000-6a0fffff [size=366M]

    3a:01.0 Memory behind bridge: 53400000-547fffff [size=20M]
    3a:04.0 Memory behind bridge: 54800000-6a0fffff [size=345M]

Quoted 7 lines

Quoted 2 lines

This matches the expectation. All the extra MMIO resource space (345 MB) is allocated to the extension hotplug bridge (3a:04.0).

Sorry, I've spent a lot of time trying to trace through this code, and I'm still hopelessly confused. Can you post the complete "lspci -vv" output and the dmesg log (including the hot-add event) somewhere and include a URL to it?

I sent you the logs and lspci output both with and without this patch when I connect a full chain of 6 Thunderbolt devices where 3 of them include those NICs with 4 ethernet ports. The resulting topology includes total of 6 + 3 + 1 PCIe switches.

Quoted 13 lines

I think I understand the problem you're solving:

- You have 366M, 1M-aligned, available for things on bus 3a
- You assign 20M, 2M-aligned to 3a:01.0
- This leaves 346M for other things on bus 3a, but it's not all contiguous because the 20M is in the middle.
- The remaining 346M might be 1M on one side and 345M on the other (and there are many other possibilities, e.g., 3M + 343M, 5M + 341M, ..., 345M + 1M).
- The current code tries to assign all 346M to 3a:04.0, which fails because that space is not contiguous, so it falls back to allocating 1M, which works but is insufficient for future hot-adds.

My understanding is that the 20M is aligned to 2M so we need to take that into account when we distribute the remaining space which makes it 345 instead of 346 which it would be without the alignment.

Quoted 3 lines

Obviously this patch makes *this* situation work: it assigns 345M to 3a:04.0 and (I assume) leaves the 1M unused. But I haven't been able to convince myself that this patch works *in general*.

I've tested this patch with full chain of devices with all my three Intel Gigabit ET2 quad port server adapters connected there along with other devices and the issue does not happen.

Quoted 5 lines

For example, what if we assigned the 20M from the end of the 366M window instead of the beginning, so the 345M piece is below the 20M and there's 1M left above it? That is legal and should work, but I suspect this patch would ignore the 345M piece and again assign 1M to 3a:04.0.

It should work so that it first allocates resources for the non-hotplug bridges and after that everything else is put to hotplug bridges.

Quoted 3 lines

Or what if there are several hotplug bridges on bus 3a? This example has two, but there could be many more.

Quoted 2 lines

Or what if there are normal bridges as well as hotplug bridges on bus 3a? Or if they're in arbitrary orders?

Thunderbolt host router with two ports has such configuration where there are two hotplug ports and two normal ports (there could be more) and it is hot-added as well. At least that works. With the other arbitrary scenarios, it is hard to say without actually testing it on a real hardware.

Also I'm fine dropping this patch altogether and just file a kernel bugzilla with this information attached. Maybe someone else can provide a better fix eventually. This is not really common situation anyway because typically you have only PCIe endpoints included in a Thunderbolt device (not PCIe switches with a bunch of endpoints connected). Furthermore, I tried the same in Windows and it does not handle it properly either ;-)

This is where it gets hard for me -- I'm not really comfortable if we have to convince ourselves that code is correct by testing every scenario. It's a lot better if we can convince ourselves by reasoning about what the code does. That's not very reliable either, but if we understand the code, we at least have a hope of being able to fix the bugs we missed in our reasoning.

After suspend/resume cycle the Presence Detect or Data Link Layer Status Changed bits might be set and if we don't clear them those events will not fire anymore and nothing happens for instance when a device is now hot-unplugged.

Fix this by clearing those bits in a newly introduced function pcie_reenable_notification(). This should be fine because immediately after we check if the adapter is still present by reading directly from the status register.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Cc [email protected]

---
 drivers/pci/hotplug/pciehp.h      |  2 +-
 drivers/pci/hotplug/pciehp_core.c |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c  | 13 ++++++++++++-
 3 files changed, 14 insertions(+), 3 deletions(-)

drivers/pci/hotplug/pciehp.h +1 -1

index 88e917c9120f..5f892065585e 100644
@@ -121,7 +121,7 @@ struct controller *pcie_init(struct pcie_device *dev);
 int pcie_init_notification(struct controller *ctrl);
 int pciehp_enable_slot(struct slot *p_slot);
 int pciehp_disable_slot(struct slot *p_slot);
-void pcie_enable_notification(struct controller *ctrl);
+void pcie_reenable_notification(struct controller *ctrl);
 int pciehp_power_on_slot(struct slot *slot);
 void pciehp_power_off_slot(struct slot *slot);
 void pciehp_get_power_status(struct slot *slot, u8 *status);

drivers/pci/hotplug/pciehp_core.c +1 -1

index 332b723ff9e6..44a6a63802d5 100644
@@ -283,7 +283,7 @@ static int pciehp_resume(struct pcie_device *dev)
 	ctrl = get_service_data(dev);
 
 	/* reinitialize the chipset's event detection logic */
-	pcie_enable_notification(ctrl);
+	pcie_reenable_notification(ctrl);
 
 	slot = ctrl->slot;
 

drivers/pci/hotplug/pciehp_hpc.c +12 -1

index 18a42f8f5dc5..98ea75aa32c7 100644
@@ -659,7 +659,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
 	return handled;
 }
 
-void pcie_enable_notification(struct controller *ctrl)
+static void pcie_enable_notification(struct controller *ctrl)
 {
 	u16 cmd, mask;
 

@@ -697,6 +697,17 @@ void pcie_enable_notification(struct controller *ctrl)
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, cmd);
 }
 
+void pcie_reenable_notification(struct controller *ctrl)
+{
+	/*
+	 * Clear both Presence and Data Link Layer Changed to make sure
+	 * those events still fire after we have re-enabled them.
+	 */
+	pcie_capability_write_word(ctrl->pcie->port, PCI_EXP_SLTSTA,
+				   PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
+	pcie_enable_notification(ctrl);
+}
+
 static void pcie_disable_notification(struct controller *ctrl)
 {
 	u16 mask;

Signature · 6 lines

2.16.3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Mika,

Quoted 10 lines · Mika Westerberg

After suspend/resume cycle the Presence Detect or Data Link Layer Status Changed bits might be set and if we don't clear them those events will not fire anymore and nothing happens for instance when a device is now hot-unplugged.

Fix this by clearing those bits in a newly introduced function pcie_reenable_notification(). This should be fine because immediately after we check if the adapter is still present by reading directly from the status register.

Quoted 4 lines

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Cc [email protected]

I want to understand why we need to handle this differently between the boot-time probe path and the resume path.

This patch clears PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the resume path:

pciehp_resume
pcie_reenable_notification
# clear PDC DLLSC
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA

We used to clear PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the probe path:

pciehp_probe
pcie_init
# clear PDC ABP PFD MRLSC CC DLSCC
pcie_init_notification
pciehp_request_irq
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA
pciehp_get_power_status

db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") changed the probe path so we don't clear PCI_EXP_SLTSTA_PDC there anymore. This was so we wouldn't miss a PDC interrupt that happened before probe.

Why can't we handle probe and resume the same way? They look quite similar.

You say this patch should be fine because we read SLTSTA immediately after clearing PDC and DLLSC. But we also read SLTSTA in the probe path, so I'm not sure why we need to clear PDC and DLLSC for resume but not for probe.

Quoted 51 lines · Bjorn Helgaas

Hi Mika,

Quoted 10 lines · Mika Westerberg

After suspend/resume cycle the Presence Detect or Data Link Layer Status Changed bits might be set and if we don't clear them those events will not fire anymore and nothing happens for instance when a device is now hot-unplugged.

Fix this by clearing those bits in a newly introduced function pcie_reenable_notification(). This should be fine because immediately after we check if the adapter is still present by reading directly from the status register.

Quoted 4 lines

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Cc [email protected]

I want to understand why we need to handle this differently between the boot-time probe path and the resume path.

This patch clears PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the resume path:

pciehp_resume
pcie_reenable_notification
# clear PDC DLLSC
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA

We used to clear PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the probe path:

pciehp_probe
pcie_init
# clear PDC ABP PFD MRLSC CC DLSCC
pcie_init_notification
pciehp_request_irq
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA
pciehp_get_power_status

db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") changed the probe path so we don't clear PCI_EXP_SLTSTA_PDC there anymore. This was so we wouldn't miss a PDC interrupt that happened before probe.

Quoted 7 lines

Why can't we handle probe and resume the same way? They look quite similar.

You say this patch should be fine because we read SLTSTA immediately after clearing PDC and DLLSC. But we also read SLTSTA in the probe path, so I'm not sure why we need to clear PDC and DLLSC for resume but not for probe.

On probe path we read status but here is what it does:

	pcie_init_notification()
	...
        pciehp_get_adapter_status(slot, &occupied);
	...
        if (occupied && pciehp_force) {
                mutex_lock(&slot->hotplug_lock);
                pciehp_enable_slot(slot);
                mutex_unlock(&slot->hotplug_lock);
        }

If you don't have "pciehp.pciehp_force=1" in your kernel command line you miss the fact that the card is already there. Obviously you can't expect ordinary users to pass random command line options to get their already connected device detected by Linux.

So the reason why in probe we don't clear PDC is that once the interrupt is unmasked, you get an interrupt and the card gets detected properly.

On resume path we already check whether the card is there or not and handle it accordingly. However, if we don't clear PDC and DLLSC bits we will never get hotplug interrupt again.

Now, you ask why can't we handle probe and resume the same way? I think we can if we could get rid of that pciehp_force thing but it seems to fix an issue on some HP hardware. See 9e5858244926 ("pciehp: don't enable slot unless forced").

Quoted 72 lines · Mika Westerberg

Quoted 51 lines · Bjorn Helgaas

Hi Mika,

Quoted 10 lines · Mika Westerberg

After suspend/resume cycle the Presence Detect or Data Link Layer Status Changed bits might be set and if we don't clear them those events will not fire anymore and nothing happens for instance when a device is now hot-unplugged.

Fix this by clearing those bits in a newly introduced function pcie_reenable_notification(). This should be fine because immediately after we check if the adapter is still present by reading directly from the status register.

Quoted 4 lines

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Cc [email protected]

I want to understand why we need to handle this differently between the boot-time probe path and the resume path.

This patch clears PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the resume path:

pciehp_resume
pcie_reenable_notification
# clear PDC DLLSC
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA

We used to clear PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the probe path:

pciehp_probe
pcie_init
# clear PDC ABP PFD MRLSC CC DLSCC
pcie_init_notification
pciehp_request_irq
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA
pciehp_get_power_status

db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") changed the probe path so we don't clear PCI_EXP_SLTSTA_PDC there anymore. This was so we wouldn't miss a PDC interrupt that happened before probe.

Quoted 7 lines

Why can't we handle probe and resume the same way? They look quite similar.

You say this patch should be fine because we read SLTSTA immediately after clearing PDC and DLLSC. But we also read SLTSTA in the probe path, so I'm not sure why we need to clear PDC and DLLSC for resume but not for probe.

On probe path we read status but here is what it does:

	pcie_init_notification()
	...
        pciehp_get_adapter_status(slot, &occupied);
	...
        if (occupied && pciehp_force) {
                mutex_lock(&slot->hotplug_lock);
                pciehp_enable_slot(slot);
                mutex_unlock(&slot->hotplug_lock);
        }

Quoted 4 lines

If you don't have "pciehp.pciehp_force=1" in your kernel command line you miss the fact that the card is already there. Obviously you can't expect ordinary users to pass random command line options to get their already connected device detected by Linux.

Yeah, definitely not, that's really ugly.

Quoted 11 lines

So the reason why in probe we don't clear PDC is that once the interrupt is unmasked, you get an interrupt and the card gets detected properly.

On resume path we already check whether the card is there or not and handle it accordingly. However, if we don't clear PDC and DLLSC bits we will never get hotplug interrupt again.

Now, you ask why can't we handle probe and resume the same way? I think we can if we could get rid of that pciehp_force thing but it seems to fix an issue on some HP hardware. See 9e5858244926 ("pciehp: don't enable slot unless forced").

Ugh. That's really ancient history. I would *love* to get rid of pciehp.pciehp_force. There's not much detail in 9e5858244926, but maybe with some digging we can figure out something.

I'd rather do the digging now and try to simplify this area instead of adding another tweak.

Quoted 92 lines · Bjorn Helgaas

Quoted 72 lines · Mika Westerberg

Quoted 51 lines · Bjorn Helgaas

Hi Mika,

Quoted 10 lines · Mika Westerberg

After suspend/resume cycle the Presence Detect or Data Link Layer Status Changed bits might be set and if we don't clear them those events will not fire anymore and nothing happens for instance when a device is now hot-unplugged.

Fix this by clearing those bits in a newly introduced function pcie_reenable_notification(). This should be fine because immediately after we check if the adapter is still present by reading directly from the status register.

Quoted 4 lines

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Cc [email protected]

I want to understand why we need to handle this differently between the boot-time probe path and the resume path.

This patch clears PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the resume path:

pciehp_resume
pcie_reenable_notification
# clear PDC DLLSC
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA

We used to clear PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the probe path:

pciehp_probe
pcie_init
# clear PDC ABP PFD MRLSC CC DLSCC
pcie_init_notification
pciehp_request_irq
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA
pciehp_get_power_status

db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") changed the probe path so we don't clear PCI_EXP_SLTSTA_PDC there anymore. This was so we wouldn't miss a PDC interrupt that happened before probe.

Quoted 7 lines

Why can't we handle probe and resume the same way? They look quite similar.

You say this patch should be fine because we read SLTSTA immediately after clearing PDC and DLLSC. But we also read SLTSTA in the probe path, so I'm not sure why we need to clear PDC and DLLSC for resume but not for probe.

On probe path we read status but here is what it does:

	pcie_init_notification()
	...
        pciehp_get_adapter_status(slot, &occupied);
	...
        if (occupied && pciehp_force) {
                mutex_lock(&slot->hotplug_lock);
                pciehp_enable_slot(slot);
                mutex_unlock(&slot->hotplug_lock);
        }

Quoted 4 lines

If you don't have "pciehp.pciehp_force=1" in your kernel command line you miss the fact that the card is already there. Obviously you can't expect ordinary users to pass random command line options to get their already connected device detected by Linux.

Yeah, definitely not, that's really ugly.

Quoted 11 lines

So the reason why in probe we don't clear PDC is that once the interrupt is unmasked, you get an interrupt and the card gets detected properly.

On resume path we already check whether the card is there or not and handle it accordingly. However, if we don't clear PDC and DLLSC bits we will never get hotplug interrupt again.

Now, you ask why can't we handle probe and resume the same way? I think we can if we could get rid of that pciehp_force thing but it seems to fix an issue on some HP hardware. See 9e5858244926 ("pciehp: don't enable slot unless forced").

Quoted 6 lines

Ugh. That's really ancient history. I would *love* to get rid of pciehp.pciehp_force. There's not much detail in 9e5858244926, but maybe with some digging we can figure out something.

I'd rather do the digging now and try to simplify this area instead of adding another tweak.

I did some digging but unfortunately it is still not clear what issue 9e5858244926 is fixing. There is no bugzilla link and I was not able to find any discussion around this either.

However, I think currently pciehp_force=1 does not actually do what it was intended to do. Reason is that portdrv core already asks BIOS whether native PCIe is allowed and if not, it does not set PCIE_PORT_SERVICE_HP and that prevents the whole device to be created. So even if you specify pciehp_force=1 in the command line, it does not bypass the BIOS setting.

Furthermore we have had similar check in pciehp_resume() but it was removed with 87683e22c646 ("PCI: pciehp: Always implement resume, regardless of pciehp_force param") because it broke resume.

If I understand correctly you want me to change the driver so that I remove pciehp_force check from probe. Then I can revert db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") and that makes both probe path and resume path similar (when this patch is included). Is that correct? I can do that in the next version of the patch series.

If you think we can remove pciehp_force, that would be awesome. This should be a separate patch all by itself, of course, and include your reasoning above.

I would also love to revert db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") because I'm not convinced that trying to handle interrupts that happened before binding the driver makes sense. It *would* make sense to me to enable interrupts, clear the "changed" status bits so future changes will cause interrupts, and check the "state" status bits and act on them.

Quoted 108 lines · Mika Westerberg

Quoted 85 lines · Bjorn Helgaas

Quoted 65 lines · Mika Westerberg

Quoted 44 lines · Bjorn Helgaas

Quoted 9 lines · Mika Westerberg

After suspend/resume cycle the Presence Detect or Data Link Layer Status Changed bits might be set and if we don't clear them those events will not fire anymore and nothing happens for instance when a device is now hot-unplugged.

Fix this by clearing those bits in a newly introduced function pcie_reenable_notification(). This should be fine because immediately after we check if the adapter is still present by reading directly from the status register.

I want to understand why we need to handle this differently between the boot-time probe path and the resume path.

This patch clears PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the resume path:

pciehp_resume
pcie_reenable_notification
# clear PDC DLLSC
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA

We used to clear PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the probe path:

pciehp_probe
pcie_init
# clear PDC ABP PFD MRLSC CC DLSCC
pcie_init_notification
pciehp_request_irq
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA
pciehp_get_power_status

db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") changed the probe path so we don't clear PCI_EXP_SLTSTA_PDC there anymore. This was so we wouldn't miss a PDC interrupt that happened before probe.

Quoted 7 lines

Why can't we handle probe and resume the same way? They look quite similar.

You say this patch should be fine because we read SLTSTA immediately after clearing PDC and DLLSC. But we also read SLTSTA in the probe path, so I'm not sure why we need to clear PDC and DLLSC for resume but not for probe.

On probe path we read status but here is what it does:

	pcie_init_notification()
	...
        pciehp_get_adapter_status(slot, &occupied);
	...
        if (occupied && pciehp_force) {
                mutex_lock(&slot->hotplug_lock);
                pciehp_enable_slot(slot);
                mutex_unlock(&slot->hotplug_lock);
        }

Quoted 4 lines

If you don't have "pciehp.pciehp_force=1" in your kernel command line you miss the fact that the card is already there. Obviously you can't expect ordinary users to pass random command line options to get their already connected device detected by Linux.

Yeah, definitely not, that's really ugly.

Quoted 11 lines

So the reason why in probe we don't clear PDC is that once the interrupt is unmasked, you get an interrupt and the card gets detected properly.

On resume path we already check whether the card is there or not and handle it accordingly. However, if we don't clear PDC and DLLSC bits we will never get hotplug interrupt again.

Now, you ask why can't we handle probe and resume the same way? I think we can if we could get rid of that pciehp_force thing but it seems to fix an issue on some HP hardware. See 9e5858244926 ("pciehp: don't enable slot unless forced").

Quoted 6 lines

Ugh. That's really ancient history. I would *love* to get rid of pciehp.pciehp_force. There's not much detail in 9e5858244926, but maybe with some digging we can figure out something.

I'd rather do the digging now and try to simplify this area instead of adding another tweak.

I did some digging but unfortunately it is still not clear what issue 9e5858244926 is fixing. There is no bugzilla link and I was not able to find any discussion around this either.

However, I think currently pciehp_force=1 does not actually do what it was intended to do. Reason is that portdrv core already asks BIOS whether native PCIe is allowed and if not, it does not set PCIE_PORT_SERVICE_HP and that prevents the whole device to be created. So even if you specify pciehp_force=1 in the command line, it does not bypass the BIOS setting.

Furthermore we have had similar check in pciehp_resume() but it was removed with 87683e22c646 ("PCI: pciehp: Always implement resume, regardless of pciehp_force param") because it broke resume.

Quoted 6 lines

If I understand correctly you want me to change the driver so that I remove pciehp_force check from probe. Then I can revert db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") and that makes both probe path and resume path similar (when this patch is included). Is that correct? I can do that in the next version of the patch series.

If you think we can remove pciehp_force, that would be awesome. This should be a separate patch all by itself, of course, and include your reasoning above.

I would also love to revert db63d40017a5 ("PCI: pciehp: Do not clear Presence Detect Changed during initialization") because I'm not convinced that trying to handle interrupts that happened before binding the driver makes sense. It *would* make sense to me to enable interrupts, clear the "changed" status bits so future changes will cause interrupts, and check the "state" status bits and act on them.

JFYI, I've added the below patch to my upcoming series which reworks pciehp event handling and adds runtime PM support:
https://github.com/l1k/linux/commits/pciehp_runpm_v2

The patch removes pciehp_force and reverts db63d40017a5 as requested.

Quoted 9 lines · Mika Westerberg

After suspend/resume cycle the Presence Detect or Data Link Layer Status Changed bits might be set and if we don't clear them those events will not fire anymore and nothing happens for instance when a device is now hot-unplugged.

Fix this by clearing those bits in a newly introduced function pcie_reenable_notification(). This should be fine because immediately after we check if the adapter is still present by reading directly from the status register.

I want to understand why we need to handle this differently between the boot-time probe path and the resume path.

This patch clears PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the resume path:

pciehp_resume
pcie_reenable_notification
# clear PDC DLLSC
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA

From that point on, slot events are enabled, but they're not received

Quoted 17 lines · Bjorn Helgaas

Quoted 9 lines · Mika Westerberg

After suspend/resume cycle the Presence Detect or Data Link Layer Status Changed bits might be set and if we don't clear them those events will not fire anymore and nothing happens for instance when a device is now hot-unplugged.

Fix this by clearing those bits in a newly introduced function pcie_reenable_notification(). This should be fine because immediately after we check if the adapter is still present by reading directly from the status register.

I want to understand why we need to handle this differently between the boot-time probe path and the resume path.

This patch clears PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC in the resume path:

Quoted 7 lines

pciehp_resume
pcie_reenable_notification
# clear PDC DLLSC
pcie_enable_notification
# set DLLSCE ABPE/PDCE HPIE CCIE
pciehp_get_adapter_status
# read PCI_EXP_SLTSTA

The Slot Control register is already written during the ->resume_noirq phase via:

pci_pm_resume_noirq
pci_pm_default_resume_early
pci_restore_state
pci_restore_pcie_state

Quoted 1 line

From that point on, slot events are enabled, but they're not received

because interrupts are disabled until the ->resume_early phase commences.

My guess is that if the hotplug port uses edge-triggered MSI/MSI-X, an interrupt may have already been sent during ->resume_noirq, but was lost because interrupts were disabled. Clearing the Slot Status register thus acknowledges any lost interrupts.

If this theory is correct, it should probably be included in the commit message and/or a code comment so that it's clear what's going on.

Quoted 4 lines · Mika Westerberg

When a system is using native PCIe hotplug for Thunderbolt and the controller is not enabled for full RTD3 (runtime D3) it will be only present in the system when there is a device connected. This pretty much follows the BIOS assisted hotplug behaviour.

This is a tangent, but what exactly does "controller is not enabled for full RTD3 (runtime D3)" mean? The way that's worded sounds like it *could* be a setting in a PCI config register, but I suspect it's really something in Linux, e.g., some bit in struct pci_dev or device?

Quoted 8 lines

Thunderbolt host router integrated PCIe switch has two additional PCIe downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI (USB 3 host controller) respectively. These downstream bridges are not marked being hotplug capable. Reason for that is to preserve resources. Otherwise the OS would distribute remaining resources between all downstream bridges making these two bridges consume precious resources of the actual hotplug bridges.

Quoted 5 lines

Now, because these two bridges are not marked being hotplug capable the OS will not enable hotplug interrupt for them either and will not receive interrupt when devices behind them are hot-added. Solution to this is that the BIOS sends ACPI Notify() to the root port let the OS know it needs to rescan for added and/or removed devices.

We're building stuff based on "is_hotplug_bridge", but I'm not sure that bit is really useful.

set_pcie_hotplug_bridge() sets it for downstream ports with PCI_EXP_SLTCAP_HPC, which is easy enough to understand.

In acpiphp, check_hotplug_bridge() sets in some scenario that I can't quite figure out. I assume it's based on something in the namespace.

But it seems like the whole point of this patch is that even if a bridge doesn't have "is_hotplug_bridge" set, ACPI hotplug can hot-add devices below it.

So I'm not sure what "is_hotplug_bridge" is really telling us. Is it just a hint about how many resources we want to assign to the bridge, i.e., we assign only a few when it's not set and more if it is set?

Quoted 12 lines

Here is how the mechanism is supposed to work when a Thunderbolt endpoint is connected to one of the ports. In case of a standard USB-C device only the xHCI is hot-added otherwise steps are the same.

1. Initially there is only the PCIe root port that is controlled by the pciehp driver

00:1b.0 (Hotplug+) --

2. Then we get native PCIe hotplug interrupt and once it is handled the topology looks as following

Quoted 5 lines

00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --

                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 --

3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and

By "not marked as hotplug capable", I guess you mean PCI_EXP_SLTCAP_HPC is not set, right?

Quoted 52 lines

they don't have anything behind them currently. Bridge 02:01.0 is hotplug capable and used for extending the topology. At this point the required PCIe devices are enabled and ACPI Notify() is sent to the root port. The resulting topology is expected to look like

00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller

                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 -- xHCI host controller

However, the current ACPI hotplug implementation scans the whole 00:1b.0 hotplug slot and everything behind it regardless whether native PCIe is used or not, and it expects that the BIOS has configured bridge resources upfront. If that's not the case it assigns resources using minimal allocation (everything currently found just barely fit) preventing future extension. In addition to that, if there is another native PCIe hotplug going on we may find the new PCIe switch only partially ready (all links are not fully trained yet) confusing pciehp when it finally starts to enumerate for new devices.

To make this work better with the native PCIe hotplug driver (pciehp), we let it handle all slot management and resource allocation for hotplug bridges and restrict ACPI hotplug to non-hotplug bridges.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Cc [email protected]

---
 drivers/pci/hotplug/acpiphp.h      |  1 +
 drivers/pci/hotplug/acpiphp_glue.c | 73 ++++++++++++++++++++++++++++++--------
 2 files changed, 59 insertions(+), 15 deletions(-)

drivers/pci/hotplug/acpiphp.h +1 -0

index e438a2d734f2..8dcfd623ef1d 100644
@@ -158,6 +158,7 @@ struct acpiphp_attention_info
 
 #define SLOT_ENABLED		(0x00000001)
 #define SLOT_IS_GOING_AWAY	(0x00000002)
+#define SLOT_IS_NATIVE		(0x00000004)
 
 /* function flags */
 

drivers/pci/hotplug/acpiphp_glue.c +0 -0

index b45b375c0e6c..5efa21cdddc9 100644
@@ -282,6 +282,9 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
 	slot->device = device;
 	INIT_LIST_HEAD(&slot->funcs);
 

drivers/pci/hotplug/acpiphp_glue.c static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data, · line 286

Quoted 2 lines

drivers/pci/hotplug/acpiphp_glue.c +2 -0

+	if (pdev && pciehp_is_native(pdev))
+		slot->flags |= SLOT_IS_NATIVE;

If I understand correctly, pciehp_is_native() checks root->osc_control_set, so for everything under the same host bridge (PNP0A03 device), it gives the same answer.

If so, the name "SLOT_IS_NATIVE" seems a little misleading because there are very often several slots under the same host bridge, and they will be either all native or all non-native.

But I must be missing something because it seems like the whole point of this patch is to treat part of the Thunderbolt host router as native and another part as non-native. Help, I'm confused :)

Quoted 10 lines · Bjorn Helgaas

Quoted 4 lines · Mika Westerberg

When a system is using native PCIe hotplug for Thunderbolt and the controller is not enabled for full RTD3 (runtime D3) it will be only present in the system when there is a device connected. This pretty much follows the BIOS assisted hotplug behaviour.

This is a tangent, but what exactly does "controller is not enabled for full RTD3 (runtime D3)" mean? The way that's worded sounds like it *could* be a setting in a PCI config register, but I suspect it's really something in Linux, e.g., some bit in struct pci_dev or device?

It means that the controller can to into D3 runtime. With BIOS assisted mode and native PCIe mode, the controller is hot-removed when there is nothing connected. In RTD3 mode it is there always and the OS expected to put it into D3 when idle using standard PCI PM registers.

Quoted 20 lines

Quoted 8 lines

Thunderbolt host router integrated PCIe switch has two additional PCIe downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI (USB 3 host controller) respectively. These downstream bridges are not marked being hotplug capable. Reason for that is to preserve resources. Otherwise the OS would distribute remaining resources between all downstream bridges making these two bridges consume precious resources of the actual hotplug bridges.

Quoted 5 lines

Now, because these two bridges are not marked being hotplug capable the OS will not enable hotplug interrupt for them either and will not receive interrupt when devices behind them are hot-added. Solution to this is that the BIOS sends ACPI Notify() to the root port let the OS know it needs to rescan for added and/or removed devices.

We're building stuff based on "is_hotplug_bridge", but I'm not sure that bit is really useful.

set_pcie_hotplug_bridge() sets it for downstream ports with PCI_EXP_SLTCAP_HPC, which is easy enough to understand.

Quoted 6 lines

In acpiphp, check_hotplug_bridge() sets in some scenario that I can't quite figure out. I assume it's based on something in the namespace.

But it seems like the whole point of this patch is that even if a bridge doesn't have "is_hotplug_bridge" set, ACPI hotplug can hot-add devices below it.

Correct.

Quoted 3 lines

So I'm not sure what "is_hotplug_bridge" is really telling us. Is it just a hint about how many resources we want to assign to the bridge, i.e., we assign only a few when it's not set and more if it is set?

Good question. I did not invent the flag so hard to say. I've been using it because IIRC you prefer it.

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data, · line 297

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +1 -1

@@ -291,7 +294,7 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
         * expose slots to user space in those cases.
         */
        if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
-           && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
+           && !(slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge)) {

Quoted 16 lines

Quoted 16 lines

Here is how the mechanism is supposed to work when a Thunderbolt endpoint is connected to one of the ports. In case of a standard USB-C device only the xHCI is hot-added otherwise steps are the same.

1. Initially there is only the PCIe root port that is controlled by the pciehp driver

00:1b.0 (Hotplug+) --

2. Then we get native PCIe hotplug interrupt and once it is handled the topology looks as following

00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --

                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 --

Quoted 4 lines

Quoted 1 line

3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and

By "not marked as hotplug capable", I guess you mean PCI_EXP_SLTCAP_HPC is not set, right?

Right.

Quoted 22 lines

Quoted 22 lines

they don't have anything behind them currently. Bridge 02:01.0 is hotplug capable and used for extending the topology. At this point the required PCIe devices are enabled and ACPI Notify() is sent to the root port. The resulting topology is expected to look like

00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller

                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 -- xHCI host controller

However, the current ACPI hotplug implementation scans the whole 00:1b.0 hotplug slot and everything behind it regardless whether native PCIe is used or not, and it expects that the BIOS has configured bridge resources upfront. If that's not the case it assigns resources using minimal allocation (everything currently found just barely fit) preventing future extension. In addition to that, if there is another native PCIe hotplug going on we may find the new PCIe switch only partially ready (all links are not fully trained yet) confusing pciehp when it finally starts to enumerate for new devices.

To make this work better with the native PCIe hotplug driver (pciehp), we let it handle all slot management and resource allocation for hotplug bridges and restrict ACPI hotplug to non-hotplug bridges.

Quoted 12 lines · Mika Westerberg

Quoted 10 lines · Bjorn Helgaas

Quoted 4 lines · Mika Westerberg

When a system is using native PCIe hotplug for Thunderbolt and the controller is not enabled for full RTD3 (runtime D3) it will be only present in the system when there is a device connected. This pretty much follows the BIOS assisted hotplug behaviour.

This is a tangent, but what exactly does "controller is not enabled for full RTD3 (runtime D3)" mean? The way that's worded sounds like it *could* be a setting in a PCI config register, but I suspect it's really something in Linux, e.g., some bit in struct pci_dev or device?

Quoted 4 lines

It means that the controller can to into D3 runtime. With BIOS assisted mode and native PCIe mode, the controller is hot-removed when there is nothing connected. In RTD3 mode it is there always and the OS expected to put it into D3 when idle using standard PCI PM registers.

How do I tell if a controller is enabled for runtime D3?

I'm still struggling to figure out exactly how this runtime D3 part is relevant to this patch. It might be behavior that happens in this particular scenario, but I'm not sure yet that it is a required part of the picture.

I think you're trying to separate enumeration handled by pciehp from enumeration handled by acpiphp, and runtime D3 sounds like an orthogonal issue.

Quoted 33 lines

Quoted 20 lines

Quoted 8 lines

Thunderbolt host router integrated PCIe switch has two additional PCIe downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI (USB 3 host controller) respectively. These downstream bridges are not marked being hotplug capable. Reason for that is to preserve resources. Otherwise the OS would distribute remaining resources between all downstream bridges making these two bridges consume precious resources of the actual hotplug bridges.

Quoted 5 lines

Now, because these two bridges are not marked being hotplug capable the OS will not enable hotplug interrupt for them either and will not receive interrupt when devices behind them are hot-added. Solution to this is that the BIOS sends ACPI Notify() to the root port let the OS know it needs to rescan for added and/or removed devices.

We're building stuff based on "is_hotplug_bridge", but I'm not sure that bit is really useful.

set_pcie_hotplug_bridge() sets it for downstream ports with PCI_EXP_SLTCAP_HPC, which is easy enough to understand.

Quoted 6 lines

In acpiphp, check_hotplug_bridge() sets in some scenario that I can't quite figure out. I assume it's based on something in the namespace.

But it seems like the whole point of this patch is that even if a bridge doesn't have "is_hotplug_bridge" set, ACPI hotplug can hot-add devices below it.

Correct.

Quoted 3 lines

So I'm not sure what "is_hotplug_bridge" is really telling us. Is it just a hint about how many resources we want to assign to the bridge, i.e., we assign only a few when it's not set and more if it is set?

Quoted 2 lines

Good question. I did not invent the flag so hard to say. I've been using it because IIRC you prefer it.

It's not a question of whether I prefer it. It's only useful if it means something specific and we agree on what that is.

So what do you think it means?

In this code:

Quoted 6 lines

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data, · line 297

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +1 -1

@@ -291,7 +294,7 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
         * expose slots to user space in those cases.
         */
        if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
-           && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
+           && !(slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge)) {

I *think* this part:

  slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge

means "this bridge supports hotplug but it is handled by pciehp, and acpiphp should not expose the slot to user space".

If that's the case, maybe we should rework pciehp_is_native(bridge) so instead of answering the question "does the OS have permission to control PCIe hotplug in the hierarchy containing <bridge>?", it could answer the specific question "does pciehp handle hotplug for <bridge>?", e.g., something along these lines:

  bool pciehp_is_native(struct pci_dev *bridge)
  {
  #ifdef CONFIG_HOTPLUG_PCI_PCIE
    u32 slot_cap;
    struct pci_host_bridge *host;

    if (!pci_is_pcie(bridge))
      return false;

    pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
    if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
      return false;

    if (pcie_ports_native)
      return true;

    host = pci_find_host_bridge(bridge->bus);
    return host->native_hotplug;
  #else
    return false;

#endif
}

Then your test for whether acpiphp should expose the slot to user space could be:

- && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
+ && !pciehp_is_native(pdev)) {

We could also use pciehp_is_native() directly in get_port_device_capability(), which is essentially the condition that allows pciehp to claim the device.

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Here is how the mechanism is supposed to work when a Thunderbolt endpoint is connected to one of the ports. In case of a standard USB-C device only the xHCI is hot-added otherwise steps are the same.

1. Initially there is only the PCIe root port that is controlled by the pciehp driver

00:1b.0 (Hotplug+) --

2. Then we get native PCIe hotplug interrupt and once it is handled the topology looks as following

00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --

                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 --

Quoted 29 lines

Quoted 4 lines

Quoted 1 line

3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and

By "not marked as hotplug capable", I guess you mean PCI_EXP_SLTCAP_HPC is not set, right?

Right.

Quoted 22 lines

Quoted 22 lines

they don't have anything behind them currently. Bridge 02:01.0 is hotplug capable and used for extending the topology. At this point the required PCIe devices are enabled and ACPI Notify() is sent to the root port. The resulting topology is expected to look like

00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller

                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 -- xHCI host controller

However, the current ACPI hotplug implementation scans the whole 00:1b.0 hotplug slot and everything behind it regardless whether native PCIe is used or not, and it expects that the BIOS has configured bridge resources upfront. If that's not the case it assigns resources using minimal allocation (everything currently found just barely fit) preventing future extension. In addition to that, if there is another native PCIe hotplug going on we may find the new PCIe switch only partially ready (all links are not fully trained yet) confusing pciehp when it finally starts to enumerate for new devices.

To make this work better with the native PCIe hotplug driver (pciehp), we let it handle all slot management and resource allocation for hotplug bridges and restrict ACPI hotplug to non-hotplug bridges.

I *think* the point of this patch is that:

If X is a bridge, and pciehp manages hotplug on X, i.e., X has PCI_EXP_SLTCAP_HPC set and the OS owns PCIe hotplug in this hierarchy per _OSC, acpiphp should not enumerate devices on X's secondary bus.

Furthermore, acpiphp should avoid this enumeration no matter where X is in the hierarchy. So if the Notify() goes to a bridge Y far above X, acpiphp should look at every existing bridge in the hierarchy below Y. If the bridge is managed by pciehp, acpiphp should ignore it. Otherwise, acpiphp should scan for new devices below it.

This enumeration by acpiphp is recursive, and it has to avoid the pciehp-managed bridges at every level. Your changes to enable_slot() check for SLOT_IS_NATIVE at the top level, but I don't see how they avoid pciehp-managed bridges at lower levels that may be scanned by pci_scan_bridge().

On Fri, May 04, 2018 at 07:04:20PM -0500, Bjorn Helgaas wrote:
...

Quoted 11 lines

In this code:

Quoted 6 lines

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data, · line 297

Quoted 6 lines

drivers/pci/hotplug/acpiphp_glue.c +1 -1

@@ -291,7 +294,7 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
         * expose slots to user space in those cases.
         */
        if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
-           && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
+           && !(slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge)) {

I *think* this part:

Quoted 4 lines

  slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge

means "this bridge supports hotplug but it is handled by pciehp, and acpiphp should not expose the slot to user space".

Yes, I specifically added SLOT_IS_NATIVE flag there so we can get rid of one condition. Note that:

  pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev)

and

  slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge

are equivalent.

There are no functional changes in that hunk.

Quoted 26 lines

If that's the case, maybe we should rework pciehp_is_native(bridge) so instead of answering the question "does the OS have permission to control PCIe hotplug in the hierarchy containing <bridge>?", it could answer the specific question "does pciehp handle hotplug for <bridge>?", e.g., something along these lines:

  bool pciehp_is_native(struct pci_dev *bridge)
  {
  #ifdef CONFIG_HOTPLUG_PCI_PCIE
    u32 slot_cap;
    struct pci_host_bridge *host;

    if (!pci_is_pcie(bridge))
      return false;

    pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
    if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
      return false;

    if (pcie_ports_native)
      return true;

    host = pci_find_host_bridge(bridge->bus);
    return host->native_hotplug;
  #else
    return false;

Quoted 5 lines

#endif
}

Then your test for whether acpiphp should expose the slot to user space could be:

[That test was there already before this patch.]

Quoted 6 lines

- && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
+ && !pciehp_is_native(pdev)) {

We could also use pciehp_is_native() directly in get_port_device_capability(), which is essentially the condition that allows pciehp to claim the device.

Fine, I agree it makes sense.

However, I intended *this* patch to be *fix* and thus did not want to start reworking things too much. Especially code that is not related to the issue at all (pciehp_is_native()).

There is a special case where there is only single bridge on the bus. In that case we just assign all resources to it. Currently this is done as a part of the resource distribution loop but it does not have to be there, and moving it outside actually improves readability.

While there we can add hotplug_bridges == 1 && normal_bridges == 0 to the same block because they are dealt the same way.

Suggested-by Bjorn Helgaas <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

---
 drivers/pci/setup-bus.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

drivers/pci/setup-bus.c +0 -0

index eb3059fb7f63..a1b63c7745d0 100644

drivers/pci/setup-bus.c +10 -11

+		dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
+		if (dev->subordinate) {
+			pci_bus_distribute_available_resources(dev->subordinate,
+				add_list, available_io, available_mmio,
+				available_mmio_pref);
+		}
+		return;
+	}
+
 	/*
 	 * Go over devices on this bus and distribute the remaining
 	 * resource space between hotplug bridges.

@@ -1959,17 +1975,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 		if (!b)
 			continue;
 
-		if (!hotplug_bridges && normal_bridges == 1) {
-			/*
-			 * There is only one bridge on the bus (upstream
-			 * port) so it gets all available resources
-			 * which it can then distribute to the possible
-			 * hotplug bridges below.
-			 */
-			pci_bus_distribute_available_resources(b, add_list,
-				available_io, available_mmio,
-				available_mmio_pref);
-		} else if (dev->is_hotplug_bridge) {
+		if (dev->is_hotplug_bridge) {
 			resource_size_t io, mmio, mmio_pref;
 
 			/*

Signature · 6 lines

2.16.3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

drivers/pci/setup-bus.c static void pci_bus_distribute_available_resources(struct pci_bus *bus, · line 1956

Quoted 7 lines

drivers/pci/setup-bus.c +7 -0

+	/*
+	 * There is only one bridge on the bus so it gets all available
+	 * resources which it can then distribute to the possible
+	 * hotplug bridges below.
+	 */
+	if ((hotplug_bridges == 0 && normal_bridges == 1) ||
+	    (hotplug_bridges == 1 && normal_bridges == 0)) {

  if (hotplug_bridges + normal_bridges == 1) {

Don't repost just for this; I can fold it in if you agree.