[PATCH v2 0/5] PCI: Fixes for native PCIe and ACPI hotplug

[PATCH v2 0/5] PCI: Fixes for native PCIe and ACPI hotplug

[Mika Westerberg]

Hi,

When Thunderbolt controller is configured to be in native enumeration mode
it actually includes two slightly different modes. First there is non-RTD3
mode where the Thunderbolt host controller is only present when there is a
device connected. The second one is RTD3 mode where the controller is
always present.

In non-RTD3 mode the Thunderbolt host controller (NHI) and USB host (xHCI)
controller are not hotplugged using native PCIe hotplug but instead they
will be hotplugged via BIOS triggered ACPI Notify() to the root port. This
is done to preserve resources since the NHI and xHCI only need 1 MB of MMIO
space and no additional buses. Currently Linux does not support this very
well and ends up failing the hotplug in one way or another. More detailed
explanation is in changelog of patch [4/5].

This series fixes this issue and in addition includes fixes for few other
issues found during testing on a system that has Thunderbolt controller in
non-RTD3 native PCIe enumeration mode. However, the fixes here are not in
any way Thunderbolt specific and should be applicable to other systems as
well.

The previous version of the patch series can be found here:

  https://www.spinics.net/lists/linux-acpi/msg80607.html

Changes from the previous version:

  - Drop 'cmax - max ?: 1' and use similar construct than we use in second
    pass loop in patch [1/5].
  - Drop unnecessary parentheses in patch [1/5].
  - Added Rafael's tag to patches [2-5/5].

[ I'm sending this updated patch series now because my vacation starts
  tomorrow and I'll be away whole next week. If any further changes are
  needed, I'll do those when I'm back. ]

Mika Westerberg (5):
  PCI: Make sure all bridges reserve at least one bus number
  PCI: Take bridge window alignment into account when distributing resources
  PCI: pciehp: Clear Presence Detect and Data Link Layer Status Changed on resume
  ACPI / hotplug / PCI: Do not scan all bridges when native PCIe hotplug is used
  ACPI / hotplug / PCI: Mark stale PCI devices disconnected

 drivers/pci/hotplug/acpiphp.h      |  1 +
 drivers/pci/hotplug/acpiphp_glue.c | 78 ++++++++++++++++++++++++++++++--------
 drivers/pci/hotplug/pciehp.h       |  2 +-
 drivers/pci/hotplug/pciehp_core.c  |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c   | 13 ++++++-
 drivers/pci/probe.c                | 11 ++++--
 drivers/pci/setup-bus.c            | 41 +++++++++++++++++++-
 7 files changed, 126 insertions(+), 22 deletions(-)

-- 
2.15.1

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

[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 <mario.limonciello@dell.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/probe.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ef5377438a1e..6cefd47556e3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2561,7 +2561,10 @@ 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++;
+		if (cmax - max > 1)
+			used_buses += cmax - max - 1;
 	}
 
 	/* Scan bridges that need to be reconfigured */
@@ -2584,12 +2587,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;
 	}
 
 	/*
-- 
2.15.1

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

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

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 <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/setup-bus.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 3cce29a069e6..f1e6172734f0 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -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

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

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

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: stable@vger.kernel.org
---
 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(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 636ed8f4b869..5abf864eae35 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -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);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 332b723ff9e6..44a6a63802d5 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -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;
 
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 18a42f8f5dc5..98ea75aa32c7 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -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

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

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

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 <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/hotplug/acpiphp.h      |  1 +
 drivers/pci/hotplug/acpiphp_glue.c | 73 ++++++++++++++++++++++++++++++--------
 2 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index e438a2d734f2..8dcfd623ef1d 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -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 */
 
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b45b375c0e6c..5efa21cdddc9 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -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

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

[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 <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/hotplug/acpiphp_glue.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 5efa21cdddc9..0aef35ee665a 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -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);
-- 
2.15.1

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

[Rafael J. Wysocki]

On Thu, Feb 15, 2018 at 3:39 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> 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 <mario.limonciello@dell.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/probe.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ef5377438a1e..6cefd47556e3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2561,7 +2561,10 @@ 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++;
> +               if (cmax - max > 1)
> +                       used_buses += cmax - max - 1;
>         }
>
>         /* Scan bridges that need to be reconfigured */
> @@ -2584,12 +2587,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;
>         }
>
>         /*
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bjorn Helgaas]

On Thu, Feb 15, 2018 at 05:39:55PM +0300, Mika Westerberg wrote:
> 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 <mario.limonciello@dell.com>

Is there a bugzilla or email URL we can include here?

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/probe.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ef5377438a1e..6cefd47556e3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2561,7 +2561,10 @@ 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++;
> +		if (cmax - max > 1)
> +			used_buses += cmax - max - 1;
>  	}
>  
>  	/* Scan bridges that need to be reconfigured */
> @@ -2584,12 +2587,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;
>  	}
>  
>  	/*
> -- 
> 2.15.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mario.Limonciello]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]


On Feb 22, 2018 18:27, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Feb 15, 2018 at 05:39:55PM +0300, Mika Westerberg wrote:
> > 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 <mario.limonciello@dell.com>
>
> Is there a bugzilla or email URL we can include here?

Sorry it was private communication that I believe Mika is referring to in Reported-By here.

You can remove the tag if you think it's inappropriate for this commit.

>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/probe.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index ef5377438a1e..6cefd47556e3 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2561,7 +2561,10 @@ 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++;
> > + if (cmax - max > 1)
> > + used_buses += cmax - max - 1;
> >  }
> >
> >  /* Scan bridges that need to be reconfigured */
> > @@ -2584,12 +2587,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;
> >  }
> >
> >  /*
> > --
> > 2.15.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: Type: text/html, Size: 4752 bytes --]

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

[Bjorn Helgaas]

On Fri, Feb 23, 2018 at 12:52:18AM +0000, Mario.Limonciello@dell.com wrote:
> On Feb 22, 2018 18:27, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Feb 15, 2018 at 05:39:55PM +0300, Mika Westerberg wrote:
> > > 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 <mario.limonciello@dell.com>
> >
> > Is there a bugzilla or email URL we can include here?
> 
> Sorry it was private communication that I believe Mika is referring to in Reported-By here.
> 
> You can remove the tag if you think it's inappropriate for this commit.

I like to give credit whenever possible, so I wouldn't necessarily
remove the Reported-by.

But it would be very useful to also have a URL to something with more
details, e.g., what sort of failure the user would observe, dmesg
logs, PCI topology, etc.

This information can certainly be redacted to remove any proprietary
things that can't be made public.  Usually that isn't of interest to
structural issues like this anyway.

> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/pci/probe.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index ef5377438a1e..6cefd47556e3 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -2561,7 +2561,10 @@ 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++;
> > > + if (cmax - max > 1)
> > > + used_buses += cmax - max - 1;
> > >  }
> > >
> > >  /* Scan bridges that need to be reconfigured */
> > > @@ -2584,12 +2587,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;
> > >  }
> > >
> > >  /*
> > > --
> > > 2.15.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Mario.Limonciello]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Friday, February 23, 2018 11:26 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: bhelgaas@google.com; rjw@rjwysocki.net;
> andriy.shevchenko@linux.intel.com; lenb@kernel.org; michael.jamet@intel.c=
om;
> yehezkel.bernat@intel.com; linux-acpi@vger.kernel.org; linux-
> pci@vger.kernel.org; mika.westerberg@linux.intel.com
> Subject: Re: [PATCH v2 1/5] PCI: Make sure all bridges reserve at least o=
ne bus
> number
>=20
> On Fri, Feb 23, 2018 at 12:52:18AM +0000, Mario.Limonciello@dell.com wrot=
e:
> > On Feb 22, 2018 18:27, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Feb 15, 2018 at 05:39:55PM +0300, Mika Westerberg wrote:
> > > > When distributing extra buses between hotplug bridges we need to ma=
ke
> > > > sure each bridge reserve at least one bus number, even if there is
> > > > currently nothing connected to it. For instance ACPI hotplug may br=
ing
> > > > 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 bu=
s
> > > > number for them upfront before distributing buses for hotplug bridg=
es.
> > > >
> > > > Fixes: 1c02ea810065 ("PCI: Distribute available buses to hotplug-ca=
pable
> bridges")
> > > > Reported-by: Mario Limonciello <mario.limonciello@dell.com>
> > >
> > > Is there a bugzilla or email URL we can include here?
> >
> > Sorry it was private communication that I believe Mika is referring to =
in Reported-
> By here.
> >
> > You can remove the tag if you think it's inappropriate for this commit.
>=20
> I like to give credit whenever possible, so I wouldn't necessarily
> remove the Reported-by.
>=20
> But it would be very useful to also have a URL to something with more
> details, e.g., what sort of failure the user would observe, dmesg
> logs, PCI topology, etc.
>=20
> This information can certainly be redacted to remove any proprietary
> things that can't be made public.  Usually that isn't of interest to
> structural issues like this anyway.

It was a result of a very long communication chain, but I think I pulled
out the relevant details here if you would like a URL to include:
https://gist.github.com/superm1/100a2ed20449f684ebb84c392e35dbed

Thank you.

>=20
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > ---
> > > >  drivers/pci/probe.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index ef5377438a1e..6cefd47556e3 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -2561,7 +2561,10 @@ static unsigned int
> pci_scan_child_bus_extend(struct pci_bus *bus,
> > > >  for_each_pci_bridge(dev, bus) {
> > > >  cmax =3D max;
> > > >  max =3D pci_scan_bridge_extend(bus, dev, max, 0, 0);
> > > > - used_buses +=3D cmax - max;
> > > > + /* Reserve one bus for each bridge */
> > > > + used_buses++;
> > > > + if (cmax - max > 1)
> > > > + used_buses +=3D cmax - max - 1;
> > > >  }
> > > >
> > > >  /* Scan bridges that need to be reconfigured */
> > > > @@ -2584,12 +2587,14 @@ static unsigned int
> pci_scan_child_bus_extend(struct pci_bus *bus,
> > > >  * bridges if any.
> > > >  */
> > > >  buses =3D available_buses / hotplug_bridges;
> > > > - buses =3D min(buses, available_buses - used_buses);
> > > > + buses =3D min(buses, available_buses - used_buses + 1);
> > > >  }
> > > >
> > > >  cmax =3D max;
> > > >  max =3D pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
> > > > - used_buses +=3D max - cmax;
> > > > + /* One bus is already accounted so don't add it again */
> > > > + if (max - cmax > 1)
> > > > + used_buses +=3D max - cmax - 1;
> > > >  }
> > > >
> > > >  /*
> > > > --
> > > > 2.15.1
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-acp=
i" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

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

[Mika Westerberg]

On Fri, Feb 23, 2018 at 07:06:59PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> > Sent: Friday, February 23, 2018 11:26 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: bhelgaas@google.com; rjw@rjwysocki.net;
> > andriy.shevchenko@linux.intel.com; lenb@kernel.org; michael.jamet@intel.com;
> > yehezkel.bernat@intel.com; linux-acpi@vger.kernel.org; linux-
> > pci@vger.kernel.org; mika.westerberg@linux.intel.com
> > Subject: Re: [PATCH v2 1/5] PCI: Make sure all bridges reserve at least one bus
> > number
> > 
> > On Fri, Feb 23, 2018 at 12:52:18AM +0000, Mario.Limonciello@dell.com wrote:
> > > On Feb 22, 2018 18:27, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Thu, Feb 15, 2018 at 05:39:55PM +0300, Mika Westerberg wrote:
> > > > > 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 <mario.limonciello@dell.com>
> > > >
> > > > Is there a bugzilla or email URL we can include here?
> > >
> > > Sorry it was private communication that I believe Mika is referring to in Reported-
> > By here.
> > >
> > > You can remove the tag if you think it's inappropriate for this commit.
> > 
> > I like to give credit whenever possible, so I wouldn't necessarily
> > remove the Reported-by.
> > 
> > But it would be very useful to also have a URL to something with more
> > details, e.g., what sort of failure the user would observe, dmesg
> > logs, PCI topology, etc.
> > 
> > This information can certainly be redacted to remove any proprietary
> > things that can't be made public.  Usually that isn't of interest to
> > structural issues like this anyway.
> 
> It was a result of a very long communication chain, but I think I pulled
> out the relevant details here if you would like a URL to include:
> https://gist.github.com/superm1/100a2ed20449f684ebb84c392e35dbed

Thanks Mario.

I'll update changelog to include the above information as well.