PCI: Fixes for native PCIe and ACPI hotplug

---
 drivers/pci/probe.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/pci/probe.c +6 -3

index ef5377438a1e..3e91d2191a7d 100644
@@ -2561,7 +2561,8 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 	for_each_pci_bridge(dev, bus) {
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
-		used_buses += cmax - max;
+		/* Reserve one bus for each bridge */
+		used_buses += cmax - max ?: 1;
 	}
 
 	/* Scan bridges that need to be reconfigured */

@@ -2584,12 +2585,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 			 * bridges if any.
 			 */
 			buses = available_buses / hotplug_bridges;
-			buses = min(buses, available_buses - used_buses);
+			buses = min(buses, available_buses - (used_buses - 1));
 		}
 
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
-		used_buses += max - cmax;
+		/* One bus is already accounted so don't add it again */
+		if (max - cmax > 1)
+			used_buses += (max - cmax) - 1;
 	}
 
 	/*

Signature · 1 line

2.15.1

Rafael J. Wysocki

maintainer

2018-02-13 17:43 UTC #2 lore raw

Re: [PATCH 1/5] PCI: Make sure all bridges reserve at least one bus number

Quoted 11 lines · Mika Westerberg

When distributing extra buses between hotplug bridges we need to make sure each bridge reserve at least one bus number, even if there is currently nothing connected to it. For instance ACPI hotplug may bring in additional devices to non-hotplug bridges later on. To make sure we don't run out of bus numbers for non-hotplug bridges reserve one bus number for them upfront before distributing buses for hotplug bridges.
Fixes 1c02ea810065 ("PCI: Distribute available buses to hotplug-capable bridges")
Reported-by Mario Limonciello <[email protected]>
Signed-off-by Mika Westerberg <[email protected]>
Cc [email protected]

Just a few notation-related nits below.

Quoted 9 lines

---
 drivers/pci/probe.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/pci/probe.c +0 -0

index ef5377438a1e..3e91d2191a7d 100644

drivers/pci/probe.c static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, · line 2565

Quoted 6 lines

drivers/pci/probe.c +2 -1

@@ -2561,7 +2561,8 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
        for_each_pci_bridge(dev, bus) {
                cmax = max;
                max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
-               used_buses += cmax - max;
+               /* Reserve one bus for each bridge */
+               used_buses += cmax - max ?: 1;

I would write this as

used_buses++;
if (cmax - max > 1)

        used_buses += cmax - max - 1;

to make this and the new code below look analogous.

drivers/pci/probe.c static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, · line 2588

Quoted 9 lines

drivers/pci/probe.c +1 -1

        }
 
        /* Scan bridges that need to be reconfigured */

@@ -2584,12 +2585,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
                         * bridges if any.
                         */
                        buses = available_buses / hotplug_bridges;
-                       buses = min(buses, available_buses - used_buses);
+                       buses = min(buses, available_buses - (used_buses - 1));

You can write the inner expression as "available_buses - used_buses + 1" (without the extra parens).

drivers/pci/probe.c static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, · line 2595

Quoted 8 lines

drivers/pci/probe.c +3 -1

                }
 
                cmax = max;
                max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
-               used_buses += max - cmax;
+               /* One bus is already accounted so don't add it again */
+               if (max - cmax > 1)
+                       used_buses += (max - cmax) - 1;

The parens on the right-hand side are not necessary here AFAICS.

Quoted 4 lines

drivers/pci/probe.c +0 -1

Mika Westerberg

author

2018-02-14 09:47 UTC #3 lore raw

Re: [PATCH 1/5] PCI: Make sure all bridges reserve at least one bus number

Quoted 9 lines · Rafael J. Wysocki

Quoted 7 lines · Mika Westerberg
When distributing extra buses between hotplug bridges we need to make sure each bridge reserve at least one bus number, even if there is currently nothing connected to it. For instance ACPI hotplug may bring in additional devices to non-hotplug bridges later on. To make sure we don't run out of bus numbers for non-hotplug bridges reserve one bus number for them upfront before distributing buses for hotplug bridges.

Quoted 6 lines

Quoted 4 lines
Fixes 1c02ea810065 ("PCI: Distribute available buses to hotplug-capable bridges")
Reported-by Mario Limonciello <[email protected]>
Signed-off-by Mika Westerberg <[email protected]>
Cc [email protected]
Just a few notation-related nits below.

Thanks for the comments! I'll update the patch accordingly.

Mika Westerberg

author

2018-02-13 16:31 UTC #4 lore raw

PATCH 2/5 PCI: Take bridge window alignment into account when distributing resources

When connecting a Thunderbolt endpoint which itself has internal PCIe switch (not the integrated one) the way we currently distribute resources does not always work well because devices below non-hotplug bridges might need to have their MMIO resources aligned to something else than 1 MB boundary. As an example connecting a server grade 4-port GbE add in card adds another PCIe switch leading to GbE devices that want to have their MMIO resources aligned to 2 MB boundary instead.

Current resource distribution code does not take this alignment into account and might try to add too much resources for the extension hotplug bridges. The resulting bridge window is then too big which makes Linux to re-allocate minimal amount of resources, making future extension impossible.

Make this work better by substracting properly aligned non-hotplug downstream bridge window size from the remaining resources.

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

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

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

index 3cce29a069e6..f1e6172734f0 100644
@@ -1882,6 +1882,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;

@@ -1946,11 +1947,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;

@@ -1968,7 +1974,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

@@ -1981,11 +1987,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,

@@ -1993,9 +2001,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 · 1 line

2.15.1

Rafael J. Wysocki

maintainer Reviewed-by on v1 2/5

2018-02-13 17:45 UTC #5 lore raw

Re: [PATCH 2/5] PCI: Take bridge window alignment into account when distributing resources

Quoted 14 lines · Mika Westerberg

When connecting a Thunderbolt endpoint which itself has internal PCIe switch (not the integrated one) the way we currently distribute resources does not always work well because devices below non-hotplug bridges might need to have their MMIO resources aligned to something else than 1 MB boundary. As an example connecting a server grade 4-port GbE add in card adds another PCIe switch leading to GbE devices that want to have their MMIO resources aligned to 2 MB boundary instead.
Current resource distribution code does not take this alignment into account and might try to add too much resources for the extension hotplug bridges. The resulting bridge window is then too big which makes Linux to re-allocate minimal amount of resources, making future extension impossible.

Quoted 6 lines

Make this work better by substracting properly aligned non-hotplug downstream bridge window size from the remaining resources.
Fixes 1a5767725cec ("PCI: Distribute available resources to hotplug-capable bridges")
Signed-off-by Mika Westerberg <[email protected]>
Cc [email protected]

Quoted 104 lines

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

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

index 3cce29a069e6..f1e6172734f0 100644
@@ -1882,6 +1882,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;

@@ -1946,11 +1947,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;

@@ -1968,7 +1974,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

@@ -1981,11 +1987,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,

@@ -1993,9 +2001,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;
                }
        }
 }
--

2.15.1

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

Mika Westerberg

author

2018-02-13 16:31 UTC #6 lore raw

PATCH 3/5 PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume

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.

---
 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 636ed8f4b869..5abf864eae35 100644
@@ -120,7 +120,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 · 1 line

2.15.1

Rafael J. Wysocki

maintainer Reviewed-by on v1 3/5

2018-02-13 17:46 UTC #7 lore raw

Re: [PATCH 3/5] PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume

Quoted 5 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.

Quoted 7 lines

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]>
Cc [email protected]

Quoted 70 lines

---
 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 636ed8f4b869..5abf864eae35 100644
@@ -120,7 +120,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 -2

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;
--

2.15.1

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

Mika Westerberg

author

2018-02-13 16:32 UTC #8 lore raw

PATCH 4/5 ACPI / hotplug / PCI: Do not scan all bridges when native PCIe hotplug is used

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.

Thunderbolt host controller integrated PCIe switch has two additional PCIe downstream ports that lead to NHI (Thunderbolt host controller) and xHCI (USB 3 host controller) respectively. These downstream ports are not marked being hotplug capable. Reason for that is to preserve resources. Otherwise the OS would distribute remaining resources between all downstream ports making these two ports consume too much resources they don't really need (both NHI and xHCI only need 1 MB of MMIO space, and no extra buses).

Now, because these ports are not marked being hotplug capable the OS will not enable hotplug interrupt for them either and will not receive interrupt when devices below them are hotplugged. Solution to this is that the BIOS sends ACPI Notify() for the root port to notify the OS it needs to rescan for added devices.

However, the current ACPI hotplug implementation scans the whole slot 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 new devices.

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

---
 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 +58 ~~-15~~

102 lines not on this page · raw message

Signature · 1 line

2.15.1

Rafael J. Wysocki

maintainer Reviewed-by on v1 4/5

2018-02-13 17:51 UTC #9 lore raw

Re: [PATCH 4/5] ACPI / hotplug / PCI: Do not scan all bridges when native PCIe hotplug is used

Quoted 29 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.
Thunderbolt host controller integrated PCIe switch has two additional PCIe downstream ports that lead to NHI (Thunderbolt host controller) and xHCI (USB 3 host controller) respectively. These downstream ports are not marked being hotplug capable. Reason for that is to preserve resources. Otherwise the OS would distribute remaining resources between all downstream ports making these two ports consume too much resources they don't really need (both NHI and xHCI only need 1 MB of MMIO space, and no extra buses).
Now, because these ports are not marked being hotplug capable the OS will not enable hotplug interrupt for them either and will not receive interrupt when devices below them are hotplugged. Solution to this is that the BIOS sends ACPI Notify() for the root port to notify the OS it needs to rescan for added devices.
However, the current ACPI hotplug implementation scans the whole slot 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 new devices.

Quoted 6 lines

To make this work better with the native PCIe hotplug driver (PCIehp), we let it to 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]>
Cc [email protected]

Quoted 134 lines

---
 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 +58 ~~-16~~

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);
 
+       if (pdev && pciehp_is_native(pdev))
+               slot->flags |= SLOT_IS_NATIVE;
+
        list_add_tail(&slot->node, &bridge->slots);
 
        /*

@@ -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)) {
                unsigned long long sun;
                int retval;

@@ -430,6 +433,29 @@ static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
        return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
 }
 
+static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
+{
+       struct pci_bus *bus = bridge->subordinate;
+       struct pci_dev *dev;
+       int max;
+
+       if (!bus)
+               return;
+
+       max = bus->busn_res.start;
+       /* Scan already configured non-hotplug bridges */
+       for_each_pci_bridge(dev, bus) {
+               if (!dev->is_hotplug_bridge)
+                       max = pci_scan_bridge(bus, dev, max, 0);
+       }
+
+       /* Scan non-hotplug bridges that need to be reconfigured */
+       for_each_pci_bridge(dev, bus) {
+               if (!dev->is_hotplug_bridge)
+                       max = pci_scan_bridge(bus, dev, max, 1);
+       }
+}
+
 /**
  * enable_slot - enable, configure a slot
  * @slot: slot to be enabled

@@ -442,25 +468,42 @@ static void enable_slot(struct acpiphp_slot *slot)
        struct pci_dev *dev;
        struct pci_bus *bus = slot->bus;
        struct acpiphp_func *func;
-       int max, pass;
-       LIST_HEAD(add_list);
 
-       acpiphp_rescan_slot(slot);
-       max = acpiphp_max_busnr(bus);
-       for (pass = 0; pass < 2; pass++) {
+       if (slot->flags & SLOT_IS_NATIVE) {
+               /*
+                * If native PCIe hotplug is used, it will take care of
+                * hotplug slot management and resource allocation for
+                * hotplug bridges. However, ACPI hotplug may still be
+                * used for non-hotplug bridges to bring in additional
+                * devices such as Thunderbolt host controller.
+                */
                for_each_pci_bridge(dev, bus) {
-                       if (PCI_SLOT(dev->devfn) != slot->device)
-                               continue;
-
-                       max = pci_scan_bridge(bus, dev, max, pass);
-                       if (pass && dev->subordinate) {
-                               check_hotplug_bridge(slot, dev);
-                               pcibios_resource_survey_bus(dev->subordinate);
-                               __pci_bus_size_bridges(dev->subordinate, &add_list);
+                       if (PCI_SLOT(dev->devfn) == slot->device)
+                               acpiphp_native_scan_bridge(dev);
+               }
+               pci_assign_unassigned_bridge_resources(bus->self);
+       } else {
+               LIST_HEAD(add_list);
+               int max, pass;
+
+               acpiphp_rescan_slot(slot);
+               max = acpiphp_max_busnr(bus);
+               for (pass = 0; pass < 2; pass++) {
+                       for_each_pci_bridge(dev, bus) {
+                               if (PCI_SLOT(dev->devfn) != slot->device)
+                                       continue;
+
+                               max = pci_scan_bridge(bus, dev, max, pass);
+                               if (pass && dev->subordinate) {
+                                       check_hotplug_bridge(slot, dev);
+                                       pcibios_resource_survey_bus(dev->subordinate);
+                                       __pci_bus_size_bridges(dev->subordinate,
+                                                              &add_list);
+                               }
                        }
                }
+               __pci_bus_assign_resources(bus, &add_list, NULL);
        }
-       __pci_bus_assign_resources(bus, &add_list, NULL);
 
        acpiphp_sanitize_bus(bus);
        pcie_bus_configure_settings(bus);
--

2.15.1

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

Mika Westerberg

author

2018-02-13 16:32 UTC #10 lore raw

PATCH 5/5 ACPI / hotplug / PCI: Mark stale PCI devices disconnected

Following PCIehp mark the unplugged PCI devices disconnected. This makes sure PCI core code leaves the now missing hardware registers alone.

---
 drivers/pci/hotplug/acpiphp_glue.c | 5 +++++
 1 file changed, 5 insertions(+)

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

index 5efa21cdddc9..0aef35ee665a 100644
@@ -651,6 +651,11 @@ static void trim_stale_devices(struct pci_dev *dev)
 		alive = pci_device_is_present(dev);
 
 	if (!alive) {
+		pci_dev_set_disconnected(dev, NULL);
+		if (pci_has_subordinate(dev))
+			pci_walk_bus(dev->subordinate, pci_dev_set_disconnected,
+				     NULL);
+
 		pci_stop_and_remove_bus_device(dev);
 		if (adev)
 			acpi_bus_trim(adev);

Signature · 1 line

2.15.1

Rafael J. Wysocki

maintainer Reviewed-by on v1 5/5

2018-02-13 17:51 UTC #11 lore raw

Re: [PATCH 5/5] ACPI / hotplug / PCI: Mark stale PCI devices disconnected

Quoted 4 lines · Mika Westerberg

Following PCIehp mark the unplugged PCI devices disconnected. This makes sure PCI core code leaves the now missing hardware registers alone.
Signed-off-by Mika Westerberg <[email protected]>