* [PATCH 1/7] PCI: Do not allocate more buses than available in parent
2017-09-26 14:17 [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
@ 2017-09-26 14:17 ` Mika Westerberg
2017-09-26 14:17 ` [PATCH 2/7] PCI: Introduce pcie_upstream_port() Mika Westerberg
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2017-09-26 14:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ashok Raj, Keith Busch, Rafael J . Wysocki, Lukas Wunner,
Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
Mika Westerberg, linux-pci, linux-kernel
One can ask more buses to be reserved for hotplug bridges by passing
pci=hpbussize=N in the kernel command line. However, if the parent bus
does not have enough bus space available we still create child bus with
the requested number of subordinate buses.
In the example below hpbussize is set to one more than we have available
buses in the root port:
pci 0000:07:00.0: [8086:1578] type 01 class 0x060400
pci 0000:07:00.0: calling pci_fixup_transparent_bridge+0x0/0x20
pci 0000:07:00.0: supports D1 D2
pci 0000:07:00.0: PME# supported from D0 D1 D2 D3hot D3cold
pci 0000:07:00.0: PME# disabled
pci 0000:07:00.0: scanning [bus 00-00] behind bridge, pass 0
pci 0000:07:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
pci 0000:07:00.0: scanning [bus 00-00] behind bridge, pass 1
pci_bus 0000:08: busn_res: can not insert [bus 08-ff] under [bus 07-3f] (conflicts with (null) [bus 07-3f])
pci_bus 0000:08: scanning bus
...
pci_bus 0000:0a: bus scan returning with max=40
pci_bus 0000:0a: busn_res: [bus 0a-ff] end is updated to 40
pci_bus 0000:0a: [bus 0a-40] partially hidden behind bridge 0000:07 [bus 07-3f]
pci_bus 0000:08: bus scan returning with max=40
pci_bus 0000:08: busn_res: [bus 08-ff] end is updated to 40
Instead of allowing this we limit the subordinate number to be less than
or equal the maximum subordinate number allocated for the parent bus (if
it has any).
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/pci/probe.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ff94b69738a8..f285cd74088e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1076,7 +1076,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
child = pci_add_new_bus(bus, dev, max+1);
if (!child)
goto out;
- pci_bus_insert_busn_res(child, max+1, 0xff);
+ pci_bus_insert_busn_res(child, max+1,
+ bus->busn_res.end);
}
max++;
buses = (buses & 0xff000000)
@@ -2433,6 +2434,10 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
if (bus->self && bus->self->is_hotplug_bridge && pci_hotplug_bus_size) {
if (max - bus->busn_res.start < pci_hotplug_bus_size - 1)
max = bus->busn_res.start + pci_hotplug_bus_size - 1;
+
+ /* Do not allocate more buses than we have room left */
+ if (max > bus->busn_res.end)
+ max = bus->busn_res.end;
}
/*
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 2/7] PCI: Introduce pcie_upstream_port()
2017-09-26 14:17 [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
2017-09-26 14:17 ` [PATCH 1/7] PCI: Do not allocate more buses than available in parent Mika Westerberg
@ 2017-09-26 14:17 ` Mika Westerberg
2017-09-26 14:17 ` [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports Mika Westerberg
` (5 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2017-09-26 14:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ashok Raj, Keith Busch, Rafael J . Wysocki, Lukas Wunner,
Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
Mika Westerberg, linux-pci, linux-kernel
This helper allows to determine whether the PCI device is PCIe upstream
port.
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
include/linux/pci.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f68c58a93dd0..4397692be538 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2042,6 +2042,17 @@ static inline int pci_pcie_type(const struct pci_dev *dev)
return (pcie_caps_reg(dev) & PCI_EXP_FLAGS_TYPE) >> 4;
}
+/**
+ * pcie_upstream_port - is the PCI device PCIe upstream port
+ * @dev: PCI device
+ */
+static inline bool pcie_upstream_port(struct pci_dev *dev)
+{
+ if (!pci_is_pcie(dev))
+ return false;
+ return pci_pcie_type(dev) == PCI_EXP_TYPE_UPSTREAM;
+}
+
static inline struct pci_dev *pcie_find_root_port(struct pci_dev *dev)
{
while (1) {
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports
2017-09-26 14:17 [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
2017-09-26 14:17 ` [PATCH 1/7] PCI: Do not allocate more buses than available in parent Mika Westerberg
2017-09-26 14:17 ` [PATCH 2/7] PCI: Introduce pcie_upstream_port() Mika Westerberg
@ 2017-09-26 14:17 ` Mika Westerberg
2017-10-11 23:32 ` Bjorn Helgaas
2017-09-26 14:17 ` [PATCH 4/7] PCI: Distribute available resources " Mika Westerberg
` (4 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Mika Westerberg @ 2017-09-26 14:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ashok Raj, Keith Busch, Rafael J . Wysocki, Lukas Wunner,
Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
Mika Westerberg, linux-pci, linux-kernel
System BIOS sometimes allocates extra bus space for hotplug capable PCIe
root/downstream ports. This space is needed if the device plugged to the
port will have more hotplug capable downstream ports. A good example of
this is Thunderbolt. Each Thunderbolt device contains a PCIe switch and
one or more hotplug capable PCIe downstream ports where the daisy chain
can be extended.
Currently Linux only allocates minimal bus space to make sure all the
enumerated devices barely fit there. The BIOS reserved extra space is
not taken into consideration at all. Because of this we run out of bus
space pretty quickly when more PCIe devices are attached to hotplug
downstream ports in order to extend the chain.
This modifies PCI core so that we distribute the available BIOS
allocated bus space equally between hotplug capable PCIe downstream
ports to make sure there is enough bus space for extending the
hierarchy later on.
While there update kernel docs of the affected functions.
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/pci/hotplug-pci.c | 13 +++++-
drivers/pci/probe.c | 114 ++++++++++++++++++++++++++++++++++++++--------
include/linux/pci.h | 19 ++++++--
3 files changed, 123 insertions(+), 23 deletions(-)
diff --git a/drivers/pci/hotplug-pci.c b/drivers/pci/hotplug-pci.c
index c68366cee6b7..deb06fe22b26 100644
--- a/drivers/pci/hotplug-pci.c
+++ b/drivers/pci/hotplug-pci.c
@@ -8,6 +8,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
{
struct pci_bus *parent = dev->bus;
int pass, busnr, start = parent->busn_res.start;
+ unsigned int available_buses = 0;
int end = parent->busn_res.end;
for (busnr = start; busnr <= end; busnr++) {
@@ -19,8 +20,18 @@ int pci_hp_add_bridge(struct pci_dev *dev)
pci_name(dev));
return -1;
}
+
+ /*
+ * In case of PCIe the hierarchy can be extended through hotplug
+ * capable downstream ports. Distribute the available bus
+ * numbers between them to make extending the chain possible.
+ */
+ if (pci_is_pcie(dev))
+ available_buses = end - busnr;
+
for (pass = 0; pass < 2; pass++)
- busnr = pci_scan_bridge(parent, dev, busnr, pass);
+ busnr = pci_scan_bridge_extend(parent, dev, busnr,
+ available_buses, pass);
if (!dev->subordinate)
return -1;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index f285cd74088e..ae0bf3c807f5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -960,6 +960,17 @@ static void pci_enable_crs(struct pci_dev *pdev)
}
/*
+ * pci_scan_bridge_extend() - Scan buses behind a bridge
+ * @bus: Parent bus the bridge is on
+ * @dev: Bridge itself
+ * @max: Starting subordinate number of buses behind this bridge
+ * @available_buses: Total number of buses available for this bridge and
+ * the devices below. After the minimal bus space has
+ * been allocated the remaining buses will be
+ * distributed equally between hotplug capable bridges.
+ * @pass: Either %0 (scan already configured bridges) or %1 (scan bridges
+ * that need to be reconfigured.
+ *
* If it's a bridge, configure it and scan the bus behind it.
* For CardBus bridges, we don't scan behind as the devices will
* be handled by the bridge driver itself.
@@ -969,7 +980,8 @@ static void pci_enable_crs(struct pci_dev *pdev)
* them, we proceed to assigning numbers to the remaining buses in
* order to avoid overlaps between old and new bus numbers.
*/
-int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
+int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, int max,
+ unsigned int available_buses, int pass)
{
struct pci_bus *child;
int is_cardbus = (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS);
@@ -1080,6 +1092,9 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
bus->busn_res.end);
}
max++;
+ if (available_buses)
+ available_buses--;
+
buses = (buses & 0xff000000)
| ((unsigned int)(child->primary) << 0)
| ((unsigned int)(child->busn_res.start) << 8)
@@ -1101,7 +1116,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
if (!is_cardbus) {
child->bridge_ctl = bctl;
- max = pci_scan_child_bus(child);
+ max = pci_scan_child_bus_extend(child, available_buses);
} else {
/*
* For CardBus bridges, we leave 4 bus numbers
@@ -1169,7 +1184,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
return max;
}
-EXPORT_SYMBOL(pci_scan_bridge);
+EXPORT_SYMBOL(pci_scan_bridge_extend);
/*
* Read interrupt line and base address registers.
@@ -2397,9 +2412,24 @@ void __weak pcibios_fixup_bus(struct pci_bus *bus)
/* nothing to do, expected to be removed in the future */
}
-unsigned int pci_scan_child_bus(struct pci_bus *bus)
+/**
+ * pci_scan_child_bus_extend() - Scan devices below a bus
+ * @bus: Bus to scan for devices
+ * @available_buses: Total number of buses available (%0 does not try to
+ * extend beyond the minimal)
+ *
+ * Scans devices below @bus including subordinate buses. Returns new
+ * subordinate number including all the found devices. Passing
+ * @available_buses causes the remaining bus space to be distributed
+ * equally between hotplug capable bridges to allow future extension of
+ * the hierarchy.
+ */
+unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
+ unsigned int available_buses)
{
- unsigned int devfn, pass, max = bus->busn_res.start;
+ unsigned int used_buses, hotplug_bridges = 0;
+ unsigned int start = bus->busn_res.start;
+ unsigned int devfn, cmax, max = start;
struct pci_dev *dev;
dev_dbg(&bus->dev, "scanning bus\n");
@@ -2409,7 +2439,8 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
pci_scan_slot(bus, devfn);
/* Reserve buses for SR-IOV capability. */
- max += pci_iov_bus_range(bus);
+ used_buses = pci_iov_bus_range(bus);
+ max += used_buses;
/*
* After performing arch-dependent fixup of the bus, look behind
@@ -2421,23 +2452,68 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
bus->is_added = 1;
}
- for (pass = 0; pass < 2; pass++)
- list_for_each_entry(dev, &bus->devices, bus_list) {
- if (pci_is_bridge(dev))
- max = pci_scan_bridge(bus, dev, max, pass);
+ /*
+ * First pass. Bridges that are already configured. We don't touch
+ * these unless they are misconfigured (which we will do in second
+ * pass).
+ */
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ if (pci_is_bridge(dev)) {
+ /*
+ * Calculate how many hotplug bridges there are on
+ * this bus. We will distribute the additional
+ * available buses between them.
+ */
+ if (dev->is_hotplug_bridge)
+ hotplug_bridges++;
+
+ cmax = max;
+ max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
+ used_buses += cmax - max;
+ }
+ }
+
+ /* Second pass. Bridges that need to be configured. */
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ if (pci_is_bridge(dev)) {
+ unsigned int buses = 0;
+
+ if (pcie_upstream_port(dev)) {
+ /* Upstream port gets all available buses */
+ buses = available_buses;
+ } else if (dev->is_hotplug_bridge) {
+ /*
+ * Distribute the extra buses between
+ * hotplug bridges if any.
+ */
+ buses = available_buses / hotplug_bridges;
+ buses = min(buses, available_buses - used_buses);
+ }
+
+ cmax = max;
+ max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
+ used_buses += max - cmax;
}
+ }
/*
* Make sure a hotplug bridge has at least the minimum requested
- * number of buses.
+ * number of buses but allow it to grow up to the maximum available
+ * bus number of there is room.
*/
- if (bus->self && bus->self->is_hotplug_bridge && pci_hotplug_bus_size) {
- if (max - bus->busn_res.start < pci_hotplug_bus_size - 1)
- max = bus->busn_res.start + pci_hotplug_bus_size - 1;
-
- /* Do not allocate more buses than we have room left */
- if (max > bus->busn_res.end)
- max = bus->busn_res.end;
+ if (bus->self && bus->self->is_hotplug_bridge) {
+ used_buses = max_t(unsigned int, available_buses,
+ pci_hotplug_bus_size - 1);
+ if (max - start < used_buses) {
+ max = start + used_buses;
+
+ /* Do not allocate more buses than we have room left */
+ if (max > bus->busn_res.end)
+ max = bus->busn_res.end;
+
+ dev_dbg(&bus->dev, "%pR extended by %#02x\n",
+ &bus->busn_res, max - start);
+ }
}
/*
@@ -2450,7 +2526,7 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
dev_dbg(&bus->dev, "bus scan returning with max=%02x\n", max);
return max;
}
-EXPORT_SYMBOL_GPL(pci_scan_child_bus);
+EXPORT_SYMBOL_GPL(pci_scan_child_bus_extend);
/**
* pcibios_root_bridge_prepare - Platform-specific host bridge setup.
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4397692be538..c9b34c2de0fb 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -909,7 +909,14 @@ static inline void pci_dev_assign_slot(struct pci_dev *dev) { }
int pci_scan_slot(struct pci_bus *bus, int devfn);
struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn);
void pci_device_add(struct pci_dev *dev, struct pci_bus *bus);
-unsigned int pci_scan_child_bus(struct pci_bus *bus);
+unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
+ unsigned int available_buses);
+
+static inline unsigned int pci_scan_child_bus(struct pci_bus *bus)
+{
+ return pci_scan_child_bus_extend(bus, 0);
+}
+
void pci_bus_add_device(struct pci_dev *dev);
void pci_read_bridge_bases(struct pci_bus *child);
struct resource *pci_find_parent_resource(const struct pci_dev *dev,
@@ -1292,8 +1299,14 @@ int pci_add_dynid(struct pci_driver *drv,
unsigned long driver_data);
const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
struct pci_dev *dev);
-int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
- int pass);
+int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, int max,
+ unsigned int available_buses, int pass);
+
+static inline int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev,
+ int max, int pass)
+{
+ return pci_scan_bridge_extend(bus, dev, max, 0, pass);
+}
void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
void *userdata);
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports
2017-09-26 14:17 ` [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports Mika Westerberg
@ 2017-10-11 23:32 ` Bjorn Helgaas
2017-10-12 9:50 ` David Laight
2017-10-12 12:47 ` Mika Westerberg
0 siblings, 2 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2017-10-11 23:32 UTC (permalink / raw)
To: Mika Westerberg
Cc: Bjorn Helgaas, Ashok Raj, Keith Busch, Rafael J . Wysocki,
Lukas Wunner, Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
linux-pci, linux-kernel
On Tue, Sep 26, 2017 at 05:17:16PM +0300, Mika Westerberg wrote:
> System BIOS sometimes allocates extra bus space for hotplug capable PCIe
> root/downstream ports. This space is needed if the device plugged to the
> port will have more hotplug capable downstream ports. A good example of
> this is Thunderbolt. Each Thunderbolt device contains a PCIe switch and
> one or more hotplug capable PCIe downstream ports where the daisy chain
> can be extended.
>
> Currently Linux only allocates minimal bus space to make sure all the
> enumerated devices barely fit there. The BIOS reserved extra space is
> not taken into consideration at all. Because of this we run out of bus
> space pretty quickly when more PCIe devices are attached to hotplug
> downstream ports in order to extend the chain.
>
> This modifies PCI core so that we distribute the available BIOS
> allocated bus space equally between hotplug capable PCIe downstream
> ports to make sure there is enough bus space for extending the
> hierarchy later on.
I think this makes sense in general. It's a fairly complicated patch,
so my comments here are just a first pass.
Why do you limit it to PCIe? Isn't it conceivable that one could
hot-add a conventional PCI card that contained a bridge leading to
another hotplug slot? E.g., a PCI card with PCMCIA slot or something
on it?
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> drivers/pci/hotplug-pci.c | 13 +++++-
> drivers/pci/probe.c | 114 ++++++++++++++++++++++++++++++++++++++--------
> include/linux/pci.h | 19 ++++++--
> 3 files changed, 123 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/pci/hotplug-pci.c b/drivers/pci/hotplug-pci.c
> index c68366cee6b7..deb06fe22b26 100644
> --- a/drivers/pci/hotplug-pci.c
> +++ b/drivers/pci/hotplug-pci.c
> @@ -8,6 +8,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
> {
> struct pci_bus *parent = dev->bus;
> int pass, busnr, start = parent->busn_res.start;
> + unsigned int available_buses = 0;
> int end = parent->busn_res.end;
>
> for (busnr = start; busnr <= end; busnr++) {
> @@ -19,8 +20,18 @@ int pci_hp_add_bridge(struct pci_dev *dev)
> pci_name(dev));
> return -1;
> }
> +
> + /*
> + * In case of PCIe the hierarchy can be extended through hotplug
> + * capable downstream ports. Distribute the available bus
> + * numbers between them to make extending the chain possible.
> + */
> + if (pci_is_pcie(dev))
> + available_buses = end - busnr;
> +
> for (pass = 0; pass < 2; pass++)
> - busnr = pci_scan_bridge(parent, dev, busnr, pass);
> + busnr = pci_scan_bridge_extend(parent, dev, busnr,
> + available_buses, pass);
> if (!dev->subordinate)
> return -1;
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index f285cd74088e..ae0bf3c807f5 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -960,6 +960,17 @@ static void pci_enable_crs(struct pci_dev *pdev)
> }
>
> /*
> + * pci_scan_bridge_extend() - Scan buses behind a bridge
> + * @bus: Parent bus the bridge is on
> + * @dev: Bridge itself
> + * @max: Starting subordinate number of buses behind this bridge
> + * @available_buses: Total number of buses available for this bridge and
> + * the devices below. After the minimal bus space has
> + * been allocated the remaining buses will be
> + * distributed equally between hotplug capable bridges.
> + * @pass: Either %0 (scan already configured bridges) or %1 (scan bridges
> + * that need to be reconfigured.
> + *
> * If it's a bridge, configure it and scan the bus behind it.
> * For CardBus bridges, we don't scan behind as the devices will
> * be handled by the bridge driver itself.
> @@ -969,7 +980,8 @@ static void pci_enable_crs(struct pci_dev *pdev)
> * them, we proceed to assigning numbers to the remaining buses in
> * order to avoid overlaps between old and new bus numbers.
> */
> -int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> +int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, int max,
> + unsigned int available_buses, int pass)
> {
> struct pci_bus *child;
> int is_cardbus = (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS);
> @@ -1080,6 +1092,9 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> bus->busn_res.end);
> }
> max++;
> + if (available_buses)
> + available_buses--;
> +
> buses = (buses & 0xff000000)
> | ((unsigned int)(child->primary) << 0)
> | ((unsigned int)(child->busn_res.start) << 8)
> @@ -1101,7 +1116,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
>
> if (!is_cardbus) {
> child->bridge_ctl = bctl;
> - max = pci_scan_child_bus(child);
> + max = pci_scan_child_bus_extend(child, available_buses);
> } else {
> /*
> * For CardBus bridges, we leave 4 bus numbers
> @@ -1169,7 +1184,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
>
> return max;
> }
> -EXPORT_SYMBOL(pci_scan_bridge);
> +EXPORT_SYMBOL(pci_scan_bridge_extend);
>
> /*
> * Read interrupt line and base address registers.
> @@ -2397,9 +2412,24 @@ void __weak pcibios_fixup_bus(struct pci_bus *bus)
> /* nothing to do, expected to be removed in the future */
> }
>
> -unsigned int pci_scan_child_bus(struct pci_bus *bus)
> +/**
> + * pci_scan_child_bus_extend() - Scan devices below a bus
> + * @bus: Bus to scan for devices
> + * @available_buses: Total number of buses available (%0 does not try to
> + * extend beyond the minimal)
What does "%0" mean? Is that kernel-doc for something? 0?
> + * Scans devices below @bus including subordinate buses. Returns new
> + * subordinate number including all the found devices. Passing
> + * @available_buses causes the remaining bus space to be distributed
> + * equally between hotplug capable bridges to allow future extension of
> + * the hierarchy.
> + */
> +unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> + unsigned int available_buses)
> {
> - unsigned int devfn, pass, max = bus->busn_res.start;
> + unsigned int used_buses, hotplug_bridges = 0;
> + unsigned int start = bus->busn_res.start;
> + unsigned int devfn, cmax, max = start;
> struct pci_dev *dev;
>
> dev_dbg(&bus->dev, "scanning bus\n");
> @@ -2409,7 +2439,8 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
> pci_scan_slot(bus, devfn);
>
> /* Reserve buses for SR-IOV capability. */
> - max += pci_iov_bus_range(bus);
> + used_buses = pci_iov_bus_range(bus);
> + max += used_buses;
>
> /*
> * After performing arch-dependent fixup of the bus, look behind
> @@ -2421,23 +2452,68 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
> bus->is_added = 1;
> }
>
> - for (pass = 0; pass < 2; pass++)
> - list_for_each_entry(dev, &bus->devices, bus_list) {
> - if (pci_is_bridge(dev))
> - max = pci_scan_bridge(bus, dev, max, pass);
Thanks for getting rid of this "for (pass = 0; ...)" loop. Much nicer
to just have it spelled out. It might even be worth pulling this
looping change into a separate patch to simplify *this* patch a little
bit.
I'd be happy if you did the same for the similar loop in
pci_hp_add_bridge().
> + /*
> + * First pass. Bridges that are already configured. We don't touch
> + * these unless they are misconfigured (which we will do in second
> + * pass).
> + */
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + if (pci_is_bridge(dev)) {
> + /*
> + * Calculate how many hotplug bridges there are on
> + * this bus. We will distribute the additional
> + * available buses between them.
> + */
> + if (dev->is_hotplug_bridge)
> + hotplug_bridges++;
Maybe pull this out into a third list traversal, since it's not
related to the "scan bridge" functionality?
> +
> + cmax = max;
> + max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
> + used_buses += cmax - max;
> + }
> + }
I'm trying to relate the "Bridges that are already configured" comment
to this code. I don't see any test here for whether the bridge is
already configured. Oh, I see -- the last parameter (0) to
pci_scan_bridge_extend() means this is the first pass, and it
basically just checks for secondary and subordinate being set.
I thought "first pass" was related to the list_for_each_entry() loop
here, but it's actually related to the internals of
pci_scan_bridge_extend(). I think maybe rewording the comment can
address this.
> + /* Second pass. Bridges that need to be configured. */
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + if (pci_is_bridge(dev)) {
> + unsigned int buses = 0;
> +
> + if (pcie_upstream_port(dev)) {
> + /* Upstream port gets all available buses */
> + buses = available_buses;
I guess this relies on the assumption that there can only be one
upstream port on a bus? Is that true? Typically a switch only has a
function 0 upstream port, but something niggles at me like the spec
admits the possibility of a switch with multiple functions of upstream
ports? I don't know where that is right now (or if it exists), but
I'll try to find it.
> + } else if (dev->is_hotplug_bridge) {
> + /*
> + * Distribute the extra buses between
> + * hotplug bridges if any.
> + */
> + buses = available_buses / hotplug_bridges;
> + buses = min(buses, available_buses - used_buses);
> + }
> +
> + cmax = max;
> + max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
> + used_buses += max - cmax;
> }
> + }
>
> /*
> * Make sure a hotplug bridge has at least the minimum requested
> - * number of buses.
> + * number of buses but allow it to grow up to the maximum available
> + * bus number of there is room.
> */
> - if (bus->self && bus->self->is_hotplug_bridge && pci_hotplug_bus_size) {
> - if (max - bus->busn_res.start < pci_hotplug_bus_size - 1)
> - max = bus->busn_res.start + pci_hotplug_bus_size - 1;
> -
> - /* Do not allocate more buses than we have room left */
> - if (max > bus->busn_res.end)
> - max = bus->busn_res.end;
> + if (bus->self && bus->self->is_hotplug_bridge) {
> + used_buses = max_t(unsigned int, available_buses,
> + pci_hotplug_bus_size - 1);
> + if (max - start < used_buses) {
> + max = start + used_buses;
> +
> + /* Do not allocate more buses than we have room left */
> + if (max > bus->busn_res.end)
> + max = bus->busn_res.end;
> +
> + dev_dbg(&bus->dev, "%pR extended by %#02x\n",
> + &bus->busn_res, max - start);
> + }
> }
>
> /*
> @@ -2450,7 +2526,7 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
> dev_dbg(&bus->dev, "bus scan returning with max=%02x\n", max);
> return max;
> }
> -EXPORT_SYMBOL_GPL(pci_scan_child_bus);
> +EXPORT_SYMBOL_GPL(pci_scan_child_bus_extend);
Does this need to be exported? The only callers I see are
pci_scan_bridge_extend() (already in the same module) and
pci_scan_child_bus() (now an inline in linux/pci.h).
I'd rather move pci_scan_child_bus() back to probe.c and make
pci_scan_child_bus_extend() static.
Same with pci_scan_bridge_extend(), although that looks harder because
it's called from hotplug-pci.c. Really, I'm not sure hotplug-pci.c
deserves to exist. I think the whole file (one function) could be
folded into pci/probe.c (as a separate patch all by itself).
> /**
> * pcibios_root_bridge_prepare - Platform-specific host bridge setup.
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4397692be538..c9b34c2de0fb 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -909,7 +909,14 @@ static inline void pci_dev_assign_slot(struct pci_dev *dev) { }
> int pci_scan_slot(struct pci_bus *bus, int devfn);
> struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn);
> void pci_device_add(struct pci_dev *dev, struct pci_bus *bus);
> -unsigned int pci_scan_child_bus(struct pci_bus *bus);
> +unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> + unsigned int available_buses);
> +
> +static inline unsigned int pci_scan_child_bus(struct pci_bus *bus)
> +{
> + return pci_scan_child_bus_extend(bus, 0);
> +}
> +
> void pci_bus_add_device(struct pci_dev *dev);
> void pci_read_bridge_bases(struct pci_bus *child);
> struct resource *pci_find_parent_resource(const struct pci_dev *dev,
> @@ -1292,8 +1299,14 @@ int pci_add_dynid(struct pci_driver *drv,
> unsigned long driver_data);
> const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
> struct pci_dev *dev);
> -int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
> - int pass);
> +int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, int max,
> + unsigned int available_buses, int pass);
> +
> +static inline int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev,
> + int max, int pass)
> +{
> + return pci_scan_bridge_extend(bus, dev, max, 0, pass);
> +}
>
> void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> void *userdata);
> --
> 2.14.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports
2017-10-11 23:32 ` Bjorn Helgaas
@ 2017-10-12 9:50 ` David Laight
2017-10-12 12:47 ` Mika Westerberg
1 sibling, 0 replies; 14+ messages in thread
From: David Laight @ 2017-10-12 9:50 UTC (permalink / raw)
To: 'Bjorn Helgaas', Mika Westerberg
Cc: Bjorn Helgaas, Ashok Raj, Keith Busch, Rafael J . Wysocki,
Lukas Wunner, Michael Jamet, Yehezkel Bernat,
Mario.Limonciello@dell.com, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
From: Bjorn Helgaas
> Sent: 12 October 2017 00:33
> Why do you limit it to PCIe? Isn't it conceivable that one could
> hot-add a conventional PCI card that contained a bridge leading to
> another hotplug slot? E.g., a PCI card with PCMCIA slot or something
> on it?
There are (maybe were!) cardbus cards that contained PCI bridges to
connect to expansion chassis.
Bus enumeration was always a problem unless they were connected
at boot time.
David
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports
2017-10-11 23:32 ` Bjorn Helgaas
2017-10-12 9:50 ` David Laight
@ 2017-10-12 12:47 ` Mika Westerberg
2017-10-12 18:32 ` Bjorn Helgaas
1 sibling, 1 reply; 14+ messages in thread
From: Mika Westerberg @ 2017-10-12 12:47 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Ashok Raj, Keith Busch, Rafael J . Wysocki,
Lukas Wunner, Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
linux-pci, linux-kernel
On Wed, Oct 11, 2017 at 06:32:43PM -0500, Bjorn Helgaas wrote:
> On Tue, Sep 26, 2017 at 05:17:16PM +0300, Mika Westerberg wrote:
> > System BIOS sometimes allocates extra bus space for hotplug capable PCIe
> > root/downstream ports. This space is needed if the device plugged to the
> > port will have more hotplug capable downstream ports. A good example of
> > this is Thunderbolt. Each Thunderbolt device contains a PCIe switch and
> > one or more hotplug capable PCIe downstream ports where the daisy chain
> > can be extended.
> >
> > Currently Linux only allocates minimal bus space to make sure all the
> > enumerated devices barely fit there. The BIOS reserved extra space is
> > not taken into consideration at all. Because of this we run out of bus
> > space pretty quickly when more PCIe devices are attached to hotplug
> > downstream ports in order to extend the chain.
> >
> > This modifies PCI core so that we distribute the available BIOS
> > allocated bus space equally between hotplug capable PCIe downstream
> > ports to make sure there is enough bus space for extending the
> > hierarchy later on.
>
> I think this makes sense in general. It's a fairly complicated patch,
> so my comments here are just a first pass.
Thanks for the comments!
> Why do you limit it to PCIe? Isn't it conceivable that one could
> hot-add a conventional PCI card that contained a bridge leading to
> another hotplug slot? E.g., a PCI card with PCMCIA slot or something
> on it?
I guess this could be generalized to such configurations but I wanted to
restrict this with PCIe for a couple of reasons. Firstly, I'm able to
actually test this ;-) Second, the rules in PCIe are quite simple,
whenever you have hotplug bridge (downstream port with a hotplug
capability set) you distribute the available bus space with it. With a
conventional PCI it is not so clear (at least to me).
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > drivers/pci/hotplug-pci.c | 13 +++++-
> > drivers/pci/probe.c | 114 ++++++++++++++++++++++++++++++++++++++--------
> > include/linux/pci.h | 19 ++++++--
> > 3 files changed, 123 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/pci/hotplug-pci.c b/drivers/pci/hotplug-pci.c
> > index c68366cee6b7..deb06fe22b26 100644
> > --- a/drivers/pci/hotplug-pci.c
> > +++ b/drivers/pci/hotplug-pci.c
> > @@ -8,6 +8,7 @@ int pci_hp_add_bridge(struct pci_dev *dev)
> > {
> > struct pci_bus *parent = dev->bus;
> > int pass, busnr, start = parent->busn_res.start;
> > + unsigned int available_buses = 0;
> > int end = parent->busn_res.end;
> >
> > for (busnr = start; busnr <= end; busnr++) {
> > @@ -19,8 +20,18 @@ int pci_hp_add_bridge(struct pci_dev *dev)
> > pci_name(dev));
> > return -1;
> > }
> > +
> > + /*
> > + * In case of PCIe the hierarchy can be extended through hotplug
> > + * capable downstream ports. Distribute the available bus
> > + * numbers between them to make extending the chain possible.
> > + */
> > + if (pci_is_pcie(dev))
> > + available_buses = end - busnr;
> > +
> > for (pass = 0; pass < 2; pass++)
> > - busnr = pci_scan_bridge(parent, dev, busnr, pass);
> > + busnr = pci_scan_bridge_extend(parent, dev, busnr,
> > + available_buses, pass);
> > if (!dev->subordinate)
> > return -1;
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index f285cd74088e..ae0bf3c807f5 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -960,6 +960,17 @@ static void pci_enable_crs(struct pci_dev *pdev)
> > }
> >
> > /*
> > + * pci_scan_bridge_extend() - Scan buses behind a bridge
> > + * @bus: Parent bus the bridge is on
> > + * @dev: Bridge itself
> > + * @max: Starting subordinate number of buses behind this bridge
> > + * @available_buses: Total number of buses available for this bridge and
> > + * the devices below. After the minimal bus space has
> > + * been allocated the remaining buses will be
> > + * distributed equally between hotplug capable bridges.
> > + * @pass: Either %0 (scan already configured bridges) or %1 (scan bridges
> > + * that need to be reconfigured.
> > + *
> > * If it's a bridge, configure it and scan the bus behind it.
> > * For CardBus bridges, we don't scan behind as the devices will
> > * be handled by the bridge driver itself.
> > @@ -969,7 +980,8 @@ static void pci_enable_crs(struct pci_dev *pdev)
> > * them, we proceed to assigning numbers to the remaining buses in
> > * order to avoid overlaps between old and new bus numbers.
> > */
> > -int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> > +int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev, int max,
> > + unsigned int available_buses, int pass)
> > {
> > struct pci_bus *child;
> > int is_cardbus = (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS);
> > @@ -1080,6 +1092,9 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> > bus->busn_res.end);
> > }
> > max++;
> > + if (available_buses)
> > + available_buses--;
> > +
> > buses = (buses & 0xff000000)
> > | ((unsigned int)(child->primary) << 0)
> > | ((unsigned int)(child->busn_res.start) << 8)
> > @@ -1101,7 +1116,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> >
> > if (!is_cardbus) {
> > child->bridge_ctl = bctl;
> > - max = pci_scan_child_bus(child);
> > + max = pci_scan_child_bus_extend(child, available_buses);
> > } else {
> > /*
> > * For CardBus bridges, we leave 4 bus numbers
> > @@ -1169,7 +1184,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass)
> >
> > return max;
> > }
> > -EXPORT_SYMBOL(pci_scan_bridge);
> > +EXPORT_SYMBOL(pci_scan_bridge_extend);
> >
> > /*
> > * Read interrupt line and base address registers.
> > @@ -2397,9 +2412,24 @@ void __weak pcibios_fixup_bus(struct pci_bus *bus)
> > /* nothing to do, expected to be removed in the future */
> > }
> >
> > -unsigned int pci_scan_child_bus(struct pci_bus *bus)
> > +/**
> > + * pci_scan_child_bus_extend() - Scan devices below a bus
> > + * @bus: Bus to scan for devices
> > + * @available_buses: Total number of buses available (%0 does not try to
> > + * extend beyond the minimal)
>
> What does "%0" mean? Is that kernel-doc for something? 0?
"%" means constant in kernel-doc comments. I've been using it with
numbers as well as with NULL and so on.
> > + * Scans devices below @bus including subordinate buses. Returns new
> > + * subordinate number including all the found devices. Passing
> > + * @available_buses causes the remaining bus space to be distributed
> > + * equally between hotplug capable bridges to allow future extension of
> > + * the hierarchy.
> > + */
> > +unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> > + unsigned int available_buses)
> > {
> > - unsigned int devfn, pass, max = bus->busn_res.start;
> > + unsigned int used_buses, hotplug_bridges = 0;
> > + unsigned int start = bus->busn_res.start;
> > + unsigned int devfn, cmax, max = start;
> > struct pci_dev *dev;
> >
> > dev_dbg(&bus->dev, "scanning bus\n");
> > @@ -2409,7 +2439,8 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
> > pci_scan_slot(bus, devfn);
> >
> > /* Reserve buses for SR-IOV capability. */
> > - max += pci_iov_bus_range(bus);
> > + used_buses = pci_iov_bus_range(bus);
> > + max += used_buses;
> >
> > /*
> > * After performing arch-dependent fixup of the bus, look behind
> > @@ -2421,23 +2452,68 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
> > bus->is_added = 1;
> > }
> >
> > - for (pass = 0; pass < 2; pass++)
> > - list_for_each_entry(dev, &bus->devices, bus_list) {
> > - if (pci_is_bridge(dev))
> > - max = pci_scan_bridge(bus, dev, max, pass);
>
> Thanks for getting rid of this "for (pass = 0; ...)" loop. Much nicer
> to just have it spelled out. It might even be worth pulling this
> looping change into a separate patch to simplify *this* patch a little
> bit.
OK, I'll make a separate patch of it.
> I'd be happy if you did the same for the similar loop in
> pci_hp_add_bridge().
Sure, I'll do the same for pci_hp_add_bridge().
> > + /*
> > + * First pass. Bridges that are already configured. We don't touch
> > + * these unless they are misconfigured (which we will do in second
> > + * pass).
> > + */
> > + list_for_each_entry(dev, &bus->devices, bus_list) {
> > + if (pci_is_bridge(dev)) {
> > + /*
> > + * Calculate how many hotplug bridges there are on
> > + * this bus. We will distribute the additional
> > + * available buses between them.
> > + */
> > + if (dev->is_hotplug_bridge)
> > + hotplug_bridges++;
>
> Maybe pull this out into a third list traversal, since it's not
> related to the "scan bridge" functionality?
OK.
> > +
> > + cmax = max;
> > + max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
> > + used_buses += cmax - max;
> > + }
> > + }
>
> I'm trying to relate the "Bridges that are already configured" comment
> to this code. I don't see any test here for whether the bridge is
> already configured. Oh, I see -- the last parameter (0) to
> pci_scan_bridge_extend() means this is the first pass, and it
> basically just checks for secondary and subordinate being set.
> I thought "first pass" was related to the list_for_each_entry() loop
> here, but it's actually related to the internals of
> pci_scan_bridge_extend(). I think maybe rewording the comment can
> address this.
I'll update the comment.
> > + /* Second pass. Bridges that need to be configured. */
> > + list_for_each_entry(dev, &bus->devices, bus_list) {
> > + if (pci_is_bridge(dev)) {
> > + unsigned int buses = 0;
> > +
> > + if (pcie_upstream_port(dev)) {
> > + /* Upstream port gets all available buses */
> > + buses = available_buses;
>
> I guess this relies on the assumption that there can only be one
> upstream port on a bus? Is that true? Typically a switch only has a
> function 0 upstream port, but something niggles at me like the spec
> admits the possibility of a switch with multiple functions of upstream
> ports? I don't know where that is right now (or if it exists), but
> I'll try to find it.
My understanding is that there can be only one upstream port on a bus.
That said I looked at the spec again and there is this in chapter 7.3.1
of PCIe 3.1 spec:
Switches, and components wishing to incorporate more than eight
Functions at their Upstream Port, are permitted to implement one or
more “virtual switches” represented by multiple Type 1 (PCI-PCI
Bridge) Configuration Space headers as illustrated in Figure 7-2.
These virtual switches serve to allow fan-out beyond eight Functions.
Since Switch Downstream Ports are permitted to appear on any Device
Number, in this case all address information fields (Bus, Device, and
Function Numbers) must be completely decoded to access the correct
register. Any Configuration Request targeting an unimplemented Bus,
Device, or Function must return a Completion with Unsupported Request
Completion Status.
Not sure what it actually means, though. A "virtual switch" to me says
it is a switch with one upstream port and multiple downstream ports,
just like normal switch. Is this what you meant? Do you understand it so
that there can be multiple upstream ports connected to a bus?
> > + } else if (dev->is_hotplug_bridge) {
> > + /*
> > + * Distribute the extra buses between
> > + * hotplug bridges if any.
> > + */
> > + buses = available_buses / hotplug_bridges;
> > + buses = min(buses, available_buses - used_buses);
> > + }
> > +
> > + cmax = max;
> > + max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
> > + used_buses += max - cmax;
> > }
> > + }
> >
> > /*
> > * Make sure a hotplug bridge has at least the minimum requested
> > - * number of buses.
> > + * number of buses but allow it to grow up to the maximum available
> > + * bus number of there is room.
> > */
> > - if (bus->self && bus->self->is_hotplug_bridge && pci_hotplug_bus_size) {
> > - if (max - bus->busn_res.start < pci_hotplug_bus_size - 1)
> > - max = bus->busn_res.start + pci_hotplug_bus_size - 1;
> > -
> > - /* Do not allocate more buses than we have room left */
> > - if (max > bus->busn_res.end)
> > - max = bus->busn_res.end;
> > + if (bus->self && bus->self->is_hotplug_bridge) {
> > + used_buses = max_t(unsigned int, available_buses,
> > + pci_hotplug_bus_size - 1);
> > + if (max - start < used_buses) {
> > + max = start + used_buses;
> > +
> > + /* Do not allocate more buses than we have room left */
> > + if (max > bus->busn_res.end)
> > + max = bus->busn_res.end;
> > +
> > + dev_dbg(&bus->dev, "%pR extended by %#02x\n",
> > + &bus->busn_res, max - start);
> > + }
> > }
> >
> > /*
> > @@ -2450,7 +2526,7 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
> > dev_dbg(&bus->dev, "bus scan returning with max=%02x\n", max);
> > return max;
> > }
> > -EXPORT_SYMBOL_GPL(pci_scan_child_bus);
> > +EXPORT_SYMBOL_GPL(pci_scan_child_bus_extend);
>
> Does this need to be exported? The only callers I see are
> pci_scan_bridge_extend() (already in the same module) and
> pci_scan_child_bus() (now an inline in linux/pci.h).
>
> I'd rather move pci_scan_child_bus() back to probe.c and make
> pci_scan_child_bus_extend() static.
OK.
> Same with pci_scan_bridge_extend(), although that looks harder because
> it's called from hotplug-pci.c. Really, I'm not sure hotplug-pci.c
> deserves to exist. I think the whole file (one function) could be
> folded into pci/probe.c (as a separate patch all by itself).
No problem, I'll move it to drivers/pci/probe.c and make
pci_scan_bridge_extend() static instead.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports
2017-10-12 12:47 ` Mika Westerberg
@ 2017-10-12 18:32 ` Bjorn Helgaas
2017-10-13 10:26 ` Mika Westerberg
0 siblings, 1 reply; 14+ messages in thread
From: Bjorn Helgaas @ 2017-10-12 18:32 UTC (permalink / raw)
To: Mika Westerberg
Cc: Bjorn Helgaas, Ashok Raj, Keith Busch, Rafael J . Wysocki,
Lukas Wunner, Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
linux-pci, linux-kernel
On Thu, Oct 12, 2017 at 03:47:03PM +0300, Mika Westerberg wrote:
> On Wed, Oct 11, 2017 at 06:32:43PM -0500, Bjorn Helgaas wrote:
> > On Tue, Sep 26, 2017 at 05:17:16PM +0300, Mika Westerberg wrote:
> > > System BIOS sometimes allocates extra bus space for hotplug capable PCIe
> > > root/downstream ports. This space is needed if the device plugged to the
> > > port will have more hotplug capable downstream ports. A good example of
> > > this is Thunderbolt. Each Thunderbolt device contains a PCIe switch and
> > > one or more hotplug capable PCIe downstream ports where the daisy chain
> > > can be extended.
> > >
> > > Currently Linux only allocates minimal bus space to make sure all the
> > > enumerated devices barely fit there. The BIOS reserved extra space is
> > > not taken into consideration at all. Because of this we run out of bus
> > > space pretty quickly when more PCIe devices are attached to hotplug
> > > downstream ports in order to extend the chain.
> > >
> > > This modifies PCI core so that we distribute the available BIOS
> > > allocated bus space equally between hotplug capable PCIe downstream
> > > ports to make sure there is enough bus space for extending the
> > > hierarchy later on.
> >
> > I think this makes sense in general. It's a fairly complicated patch,
> > so my comments here are just a first pass.
>
> Thanks for the comments!
>
> > Why do you limit it to PCIe? Isn't it conceivable that one could
> > hot-add a conventional PCI card that contained a bridge leading to
> > another hotplug slot? E.g., a PCI card with PCMCIA slot or something
> > on it?
>
> I guess this could be generalized to such configurations but I wanted to
> restrict this with PCIe for a couple of reasons. Firstly, I'm able to
> actually test this ;-) Second, the rules in PCIe are quite simple,
> whenever you have hotplug bridge (downstream port with a hotplug
> capability set) you distribute the available bus space with it. With a
> conventional PCI it is not so clear (at least to me).
You're testing dev->is_hotplug_bridge, which I think is the right
approach. It happens that we currently only set that for PCIe bridges
with PCI_EXP_SLTCAP_HPC and for some ACPI cases (and a quirk for one
device). But in principle we could and probably should set it if we
can identify a conventional PCI hotplug bridge. So my suggestion is
to just remove the explicit PCIe tests.
> > > + /* Second pass. Bridges that need to be configured. */
> > > + list_for_each_entry(dev, &bus->devices, bus_list) {
> > > + if (pci_is_bridge(dev)) {
> > > + unsigned int buses = 0;
> > > +
> > > + if (pcie_upstream_port(dev)) {
> > > + /* Upstream port gets all available buses */
> > > + buses = available_buses;
> >
> > I guess this relies on the assumption that there can only be one
> > upstream port on a bus? Is that true? Typically a switch only has a
> > function 0 upstream port, but something niggles at me like the spec
> > admits the possibility of a switch with multiple functions of upstream
> > ports? I don't know where that is right now (or if it exists), but
> > I'll try to find it.
>
> My understanding is that there can be only one upstream port on a bus.
> That said I looked at the spec again and there is this in chapter 7.3.1
> of PCIe 3.1 spec:
>
> Switches, and components wishing to incorporate more than eight
> Functions at their Upstream Port, are permitted to implement one or
> more “virtual switches” represented by multiple Type 1 (PCI-PCI
> Bridge) Configuration Space headers as illustrated in Figure 7-2.
> These virtual switches serve to allow fan-out beyond eight Functions.
> Since Switch Downstream Ports are permitted to appear on any Device
> Number, in this case all address information fields (Bus, Device, and
> Function Numbers) must be completely decoded to access the correct
> register. Any Configuration Request targeting an unimplemented Bus,
> Device, or Function must return a Completion with Unsupported Request
> Completion Status.
>
> Not sure what it actually means, though. A "virtual switch" to me says
> it is a switch with one upstream port and multiple downstream ports,
> just like normal switch. Is this what you meant? Do you understand it so
> that there can be multiple upstream ports connected to a bus?
I agree with you; I think that section is just saying that if a
component needs more then eight functions, it can incorporate a
switch, so it could have one upstream port, one internal logical bus,
up to 32 * 8 = 256 downstream ports on that logical bus, and 8
endpoints below each downstream port. Of course, there wouldn't be
enough bus number space for all that. But I don't think this is
talking about a multifunction switch upstream port.
Anyway, I think you're right that there can only be one upstream port
on a bus, because an upstream port contains link management stuff, and
it wouldn't make sense to have two upstream ports trying to manage the
same end of a single link.
But I would really like to remove the PCIe-specific nature of this
test somehow so it could work on a conventional PCI topology.
Bjorn
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports
2017-10-12 18:32 ` Bjorn Helgaas
@ 2017-10-13 10:26 ` Mika Westerberg
0 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2017-10-13 10:26 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Ashok Raj, Keith Busch, Rafael J . Wysocki,
Lukas Wunner, Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
linux-pci, linux-kernel
On Thu, Oct 12, 2017 at 01:32:23PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 12, 2017 at 03:47:03PM +0300, Mika Westerberg wrote:
> > On Wed, Oct 11, 2017 at 06:32:43PM -0500, Bjorn Helgaas wrote:
> > > On Tue, Sep 26, 2017 at 05:17:16PM +0300, Mika Westerberg wrote:
> > > > System BIOS sometimes allocates extra bus space for hotplug capable PCIe
> > > > root/downstream ports. This space is needed if the device plugged to the
> > > > port will have more hotplug capable downstream ports. A good example of
> > > > this is Thunderbolt. Each Thunderbolt device contains a PCIe switch and
> > > > one or more hotplug capable PCIe downstream ports where the daisy chain
> > > > can be extended.
> > > >
> > > > Currently Linux only allocates minimal bus space to make sure all the
> > > > enumerated devices barely fit there. The BIOS reserved extra space is
> > > > not taken into consideration at all. Because of this we run out of bus
> > > > space pretty quickly when more PCIe devices are attached to hotplug
> > > > downstream ports in order to extend the chain.
> > > >
> > > > This modifies PCI core so that we distribute the available BIOS
> > > > allocated bus space equally between hotplug capable PCIe downstream
> > > > ports to make sure there is enough bus space for extending the
> > > > hierarchy later on.
> > >
> > > I think this makes sense in general. It's a fairly complicated patch,
> > > so my comments here are just a first pass.
> >
> > Thanks for the comments!
> >
> > > Why do you limit it to PCIe? Isn't it conceivable that one could
> > > hot-add a conventional PCI card that contained a bridge leading to
> > > another hotplug slot? E.g., a PCI card with PCMCIA slot or something
> > > on it?
> >
> > I guess this could be generalized to such configurations but I wanted to
> > restrict this with PCIe for a couple of reasons. Firstly, I'm able to
> > actually test this ;-) Second, the rules in PCIe are quite simple,
> > whenever you have hotplug bridge (downstream port with a hotplug
> > capability set) you distribute the available bus space with it. With a
> > conventional PCI it is not so clear (at least to me).
>
> You're testing dev->is_hotplug_bridge, which I think is the right
> approach. It happens that we currently only set that for PCIe bridges
> with PCI_EXP_SLTCAP_HPC and for some ACPI cases (and a quirk for one
> device). But in principle we could and probably should set it if we
> can identify a conventional PCI hotplug bridge. So my suggestion is
> to just remove the explicit PCIe tests.
Fair enough :)
> > > > + /* Second pass. Bridges that need to be configured. */
> > > > + list_for_each_entry(dev, &bus->devices, bus_list) {
> > > > + if (pci_is_bridge(dev)) {
> > > > + unsigned int buses = 0;
> > > > +
> > > > + if (pcie_upstream_port(dev)) {
> > > > + /* Upstream port gets all available buses */
> > > > + buses = available_buses;
> > >
> > > I guess this relies on the assumption that there can only be one
> > > upstream port on a bus? Is that true? Typically a switch only has a
> > > function 0 upstream port, but something niggles at me like the spec
> > > admits the possibility of a switch with multiple functions of upstream
> > > ports? I don't know where that is right now (or if it exists), but
> > > I'll try to find it.
> >
> > My understanding is that there can be only one upstream port on a bus.
> > That said I looked at the spec again and there is this in chapter 7.3.1
> > of PCIe 3.1 spec:
> >
> > Switches, and components wishing to incorporate more than eight
> > Functions at their Upstream Port, are permitted to implement one or
> > more “virtual switches” represented by multiple Type 1 (PCI-PCI
> > Bridge) Configuration Space headers as illustrated in Figure 7-2.
> > These virtual switches serve to allow fan-out beyond eight Functions.
> > Since Switch Downstream Ports are permitted to appear on any Device
> > Number, in this case all address information fields (Bus, Device, and
> > Function Numbers) must be completely decoded to access the correct
> > register. Any Configuration Request targeting an unimplemented Bus,
> > Device, or Function must return a Completion with Unsupported Request
> > Completion Status.
> >
> > Not sure what it actually means, though. A "virtual switch" to me says
> > it is a switch with one upstream port and multiple downstream ports,
> > just like normal switch. Is this what you meant? Do you understand it so
> > that there can be multiple upstream ports connected to a bus?
>
> I agree with you; I think that section is just saying that if a
> component needs more then eight functions, it can incorporate a
> switch, so it could have one upstream port, one internal logical bus,
> up to 32 * 8 = 256 downstream ports on that logical bus, and 8
> endpoints below each downstream port. Of course, there wouldn't be
> enough bus number space for all that. But I don't think this is
> talking about a multifunction switch upstream port.
>
> Anyway, I think you're right that there can only be one upstream port
> on a bus, because an upstream port contains link management stuff, and
> it wouldn't make sense to have two upstream ports trying to manage the
> same end of a single link.
>
> But I would really like to remove the PCIe-specific nature of this
> test somehow so it could work on a conventional PCI topology.
I think we can change the test to check if the bus has only one
non-hotplug bridge and assing resources to it then instead of explictly
checking for PCIe upstream port.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/7] PCI: Distribute available resources to hotplug capable PCIe downstream ports
2017-09-26 14:17 [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
` (2 preceding siblings ...)
2017-09-26 14:17 ` [PATCH 3/7] PCI: Distribute available buses to hotplug capable PCIe downstream ports Mika Westerberg
@ 2017-09-26 14:17 ` Mika Westerberg
2017-09-26 14:17 ` [PATCH 5/7] PCI: pciehp: Fix race condition handling surprise link down Mika Westerberg
` (3 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2017-09-26 14:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ashok Raj, Keith Busch, Rafael J . Wysocki, Lukas Wunner,
Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
Mika Westerberg, linux-pci, linux-kernel
The same problem that we have with bus space, applies to other resources
as well. Linux only allocates the minimal amount of resources so that
the devices currently present barely fit there. This prevents extending
the chain later on because the resource windows allocated for hotplug
downstream ports are too small.
Here we follow what we already did for bus number and assign all
available extra resources to hotplug capable PCIe downstream ports. This
makes it possible to extend the hierarchy later.
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/pci/setup-bus.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 169 insertions(+)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 958da7db9033..5df6cbcfbf54 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1853,6 +1853,167 @@ void __init pci_assign_unassigned_resources(void)
}
}
+static void extend_bridge_window(struct pci_dev *bridge, struct resource *res,
+ struct list_head *add_list, resource_size_t available)
+{
+ struct pci_dev_resource *dev_res;
+
+ if (res->parent)
+ return;
+
+ if (resource_size(res) >= available)
+ return;
+
+ dev_res = res_to_dev_res(add_list, res);
+ if (!dev_res)
+ return;
+
+ /* Is there room to extend the window */
+ if (available - resource_size(res) <= dev_res->add_size)
+ return;
+
+ dev_res->add_size = available - resource_size(res);
+ dev_dbg(&bridge->dev, "bridge window %pR extended by %pa\n", res,
+ &dev_res->add_size);
+}
+
+static void pci_bus_distribute_available_resources(struct pci_bus *bus,
+ struct list_head *add_list, resource_size_t available_io,
+ resource_size_t available_mmio, resource_size_t available_mmio_pref)
+{
+ resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ struct resource *io_res, *mmio_res, *mmio_pref_res;
+ struct pci_dev *dev, *bridge = bus->self;
+ unsigned int hotplug_bridges = 0;
+
+ io_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 0];
+ mmio_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 1];
+ mmio_pref_res = &bridge->resource[PCI_BRIDGE_RESOURCES + 2];
+
+ /*
+ * Update additional resource list (add_list) to fill all the
+ * extra resource space available for this port except the space
+ * calculated in __pci_bus_size_bridges() which covers all the
+ * devices currently connected to the port and below.
+ */
+ extend_bridge_window(bridge, io_res, add_list, available_io);
+ extend_bridge_window(bridge, mmio_res, add_list, available_mmio);
+ extend_bridge_window(bridge, mmio_pref_res, add_list,
+ available_mmio_pref);
+
+ /*
+ * Calculate the total amount of extra resource space we can
+ * pass the to bridges below this one. This is basically the
+ * extra space substracted by the minimal required space for the
+ * non-hotplug bridges.
+ */
+ remaining_io = available_io;
+ remaining_mmio = available_mmio;
+ remaining_mmio_pref = available_mmio_pref;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ const struct resource *res;
+
+ if (!pci_is_bridge(dev))
+ continue;
+
+ /* Keep track how many hotplug bridges this bus has */
+ if (dev->is_hotplug_bridge) {
+ hotplug_bridges++;
+ } else {
+ /*
+ * Reduce the available resource space by what
+ * the bridge and devices below it occupy.
+ */
+ res = &dev->resource[PCI_BRIDGE_RESOURCES + 0];
+ if (!res->parent && available_io > resource_size(res))
+ remaining_io -= resource_size(res);
+
+ res = &dev->resource[PCI_BRIDGE_RESOURCES + 1];
+ if (!res->parent && available_mmio > resource_size(res))
+ remaining_mmio -= resource_size(res);
+
+ res = &dev->resource[PCI_BRIDGE_RESOURCES + 2];
+ if (!res->parent &&
+ available_mmio_pref > resource_size(res))
+ remaining_mmio_pref -= resource_size(res);
+ }
+ }
+
+ /*
+ * Go over devices on this bus and distribute the remaining
+ * resource space between hotplug bridges.
+ */
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ struct pci_bus *b;
+
+ b = dev->subordinate;
+ if (!b || !pci_is_bridge(dev))
+ continue;
+
+ if (pcie_upstream_port(dev)) {
+ /*
+ * Upstream port gets all resources directly
+ * from the downstream port.
+ */
+ pci_bus_distribute_available_resources(b, add_list,
+ available_io, available_mmio,
+ available_mmio_pref);
+ } else if (dev->is_hotplug_bridge) {
+ resource_size_t align, io, mmio, mmio_pref;
+
+ /*
+ * Distribute available extra resources equally
+ * between hotplug capable downstream ports
+ * taking alignment into account.
+ *
+ * Here hotplug_bridges is always != 0.
+ */
+ align = pci_resource_alignment(bridge, io_res);
+ io = div64_ul(available_io, hotplug_bridges);
+ io = min(ALIGN(io, align), remaining_io);
+ remaining_io -= 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;
+
+ align = pci_resource_alignment(bridge, mmio_pref_res);
+ mmio_pref = div64_ul(available_mmio_pref,
+ hotplug_bridges);
+ mmio_pref = min(ALIGN(mmio_pref, align),
+ remaining_mmio_pref);
+ remaining_mmio_pref -= mmio_pref;
+
+ pci_bus_distribute_available_resources(b, add_list, io,
+ mmio, mmio_pref);
+ }
+ }
+}
+
+static void
+pci_bridge_distribute_available_resources(struct pci_dev *bridge,
+ struct list_head *add_list)
+{
+ resource_size_t available_io, available_mmio, available_mmio_pref;
+ const struct resource *res;
+
+ if (!bridge->is_hotplug_bridge)
+ return;
+
+ /* Take the initial extra resources from the hotplug port */
+ res = &bridge->resource[PCI_BRIDGE_RESOURCES + 0];
+ available_io = resource_size(res);
+ res = &bridge->resource[PCI_BRIDGE_RESOURCES + 1];
+ available_mmio = resource_size(res);
+ res = &bridge->resource[PCI_BRIDGE_RESOURCES + 2];
+ available_mmio_pref = resource_size(res);
+
+ pci_bus_distribute_available_resources(bridge->subordinate,
+ add_list, available_io, available_mmio, available_mmio_pref);
+}
+
void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
{
struct pci_bus *parent = bridge->subordinate;
@@ -1867,6 +2028,14 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
again:
__pci_bus_size_bridges(parent, &add_list);
+
+ /*
+ * Distribute remaining resources (if any) equally between
+ * hotplug bridges below. This makes it possible to extend the
+ * hierarchy later without running out of resources.
+ */
+ pci_bridge_distribute_available_resources(bridge, &add_list);
+
__pci_bridge_assign_resources(bridge, &add_list, &fail_head);
BUG_ON(!list_empty(&add_list));
tried_times++;
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 5/7] PCI: pciehp: Fix race condition handling surprise link down
2017-09-26 14:17 [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
` (3 preceding siblings ...)
2017-09-26 14:17 ` [PATCH 4/7] PCI: Distribute available resources " Mika Westerberg
@ 2017-09-26 14:17 ` Mika Westerberg
2017-09-26 14:17 ` [PATCH 6/7] PCI: pciehp: Do not clear Presence Detect Changed during initialization Mika Westerberg
` (2 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2017-09-26 14:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ashok Raj, Keith Busch, Rafael J . Wysocki, Lukas Wunner,
Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
Mika Westerberg, linux-pci, linux-kernel
A surprise link down may retrain very quickly causing the same slot
generate a link up event before handling the link down event completes.
Since the link is active, the power off work queued from the first link
down will cause a second down event when power is disabled. However, the
link up event sets the slot state to POWERON_STATE before the event to
handle this is enqueued, making the second down event believe it needs
to do something.
This creates constant link up and down event cycle.
To prevent this it is better to handle each event at the time in order
it occurred, so change the driver to use ordered workqueue instead.
A normal device hotplug triggers two events (presense detect and link
up) that are already handled properly in the driver but we currently log
an error if we find an existing device in the slot. Since this is not an
error change the log level to be debug instead to avoid scaring users.
This is based on the original work by Ashok Raj.
Link: https://patchwork.kernel.org/patch/9469023
Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/pci/hotplug/pciehp_ctrl.c | 7 ++++---
drivers/pci/hotplug/pciehp_hpc.c | 2 +-
drivers/pci/hotplug/pciehp_pci.c | 6 +++++-
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index ec0b4c11ccd9..83f3d4af3677 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -113,10 +113,11 @@ static int board_added(struct slot *p_slot)
retval = pciehp_configure_device(p_slot);
if (retval) {
- ctrl_err(ctrl, "Cannot add device at %04x:%02x:00\n",
- pci_domain_nr(parent), parent->number);
- if (retval != -EEXIST)
+ if (retval != -EEXIST) {
+ ctrl_err(ctrl, "Cannot add device at %04x:%02x:00\n",
+ pci_domain_nr(parent), parent->number);
goto err_exit;
+ }
}
pciehp_green_led_on(p_slot);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index e5d5ce9e3010..83c93f9da65a 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -795,7 +795,7 @@ static int pcie_init_slot(struct controller *ctrl)
if (!slot)
return -ENOMEM;
- slot->wq = alloc_workqueue("pciehp-%u", 0, 0, PSN(ctrl));
+ slot->wq = alloc_ordered_workqueue("pciehp-%u", 0, PSN(ctrl));
if (!slot->wq)
goto abort;
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 19f30a9f461d..b31702f76149 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -46,7 +46,11 @@ int pciehp_configure_device(struct slot *p_slot)
dev = pci_get_slot(parent, PCI_DEVFN(0, 0));
if (dev) {
- ctrl_err(ctrl, "Device %s already exists at %04x:%02x:00, cannot hot-add\n",
+ /*
+ * The device is already there. Either configured by the
+ * boot firmware or a previous hotplug event.
+ */
+ ctrl_dbg(ctrl, "Device %s already exists at %04x:%02x:00, skipping hot-add\n",
pci_name(dev), pci_domain_nr(parent), parent->number);
pci_dev_put(dev);
ret = -EEXIST;
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 6/7] PCI: pciehp: Do not clear Presence Detect Changed during initialization
2017-09-26 14:17 [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
` (4 preceding siblings ...)
2017-09-26 14:17 ` [PATCH 5/7] PCI: pciehp: Fix race condition handling surprise link down Mika Westerberg
@ 2017-09-26 14:17 ` Mika Westerberg
2017-09-26 14:17 ` [PATCH 7/7] PCI: pciehp: Check that the device is really present before touching it Mika Westerberg
2017-10-09 8:47 ` [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
7 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2017-09-26 14:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ashok Raj, Keith Busch, Rafael J . Wysocki, Lukas Wunner,
Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
Mika Westerberg, linux-pci, linux-kernel
It is possible that the hotplug event has already happened before the
driver is attached to a PCIe hotplug downstream port. If we just clear
the status we never get the hotplug interrupt and thus the event will be
missed.
To make sure that does not happen, we leave Presence Detect Changed bit
untouched during initialization. Then once the event is unmasked we get
an interrupt and handle the hotplug event properly.
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/pci/hotplug/pciehp_hpc.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 83c93f9da65a..bc1622aa7a05 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -862,11 +862,16 @@ struct controller *pcie_init(struct pcie_device *dev)
if (link_cap & PCI_EXP_LNKCAP_DLLLARC)
ctrl->link_active_reporting = 1;
- /* Clear all remaining event bits in Slot Status register */
+ /*
+ * Clear all remaining event bits in Slot Status register except
+ * Presence Detect Changed. We want to make sure possible
+ * hotplug event is triggered when the interrupt is unmasked so
+ * that we don't lose that event.
+ */
pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
- PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
- PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
+ PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_CC |
+ PCI_EXP_SLTSTA_DLLSC);
ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c LLActRep%c\n",
(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 7/7] PCI: pciehp: Check that the device is really present before touching it
2017-09-26 14:17 [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
` (5 preceding siblings ...)
2017-09-26 14:17 ` [PATCH 6/7] PCI: pciehp: Do not clear Presence Detect Changed during initialization Mika Westerberg
@ 2017-09-26 14:17 ` Mika Westerberg
2017-10-09 8:47 ` [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
7 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2017-09-26 14:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ashok Raj, Keith Busch, Rafael J . Wysocki, Lukas Wunner,
Michael Jamet, Yehezkel Bernat, Mario.Limonciello,
Mika Westerberg, linux-pci, linux-kernel
During surprise hot-unplug the device is not there anymore. When that
happens we read 0xffffffff from the registers and pciehp_unconfigure_device()
inadvertently thinks the device is a display device because bridge
control register returns 0xff refusing to remove it:
pciehp 0000:00:1c.0:pcie004: Slot(0): Link Down
pciehp 0000:00:1c.0:pcie004: Slot(0): Card present
pciehp 0000:00:1c.0:pcie004: Cannot remove display device 0000:01:00.0
This causes the hotplug functionality to leave the hierarcy untouched
preventing further hotplug operations.
To fix this verify presence of a device by calling pci_device_is_present()
for it before we touch it any further.
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/pci/hotplug/pciehp_pci.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index b31702f76149..2a3a62393ba9 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -101,8 +101,14 @@ int pciehp_unconfigure_device(struct slot *p_slot)
*/
list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
bus_list) {
+ bool present;
+
pci_dev_get(dev);
- if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE && presence) {
+
+ /* Check if the device is really there anymore */
+ present = presence ? pci_device_is_present(dev) : false;
+
+ if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE && present) {
pci_read_config_byte(dev, PCI_BRIDGE_CONTROL, &bctl);
if (bctl & PCI_BRIDGE_CTL_VGA) {
ctrl_err(ctrl,
@@ -113,7 +119,7 @@ int pciehp_unconfigure_device(struct slot *p_slot)
break;
}
}
- if (!presence) {
+ if (!present) {
pci_dev_set_disconnected(dev, NULL);
if (pci_has_subordinate(dev))
pci_walk_bus(dev->subordinate,
@@ -124,7 +130,7 @@ int pciehp_unconfigure_device(struct slot *p_slot)
* Ensure that no new Requests will be generated from
* the device.
*/
- if (presence) {
+ if (present) {
pci_read_config_word(dev, PCI_COMMAND, &command);
command &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_SERR);
command |= PCI_COMMAND_INTX_DISABLE;
--
2.14.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 0/7] PCI: Improvements for native PCIe hotplug
2017-09-26 14:17 [PATCH 0/7] PCI: Improvements for native PCIe hotplug Mika Westerberg
` (6 preceding siblings ...)
2017-09-26 14:17 ` [PATCH 7/7] PCI: pciehp: Check that the device is really present before touching it Mika Westerberg
@ 2017-10-09 8:47 ` Mika Westerberg
7 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2017-10-09 8:47 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Ashok Raj, Keith Busch, Rafael J . Wysocki, Lukas Wunner,
Michael Jamet, Yehezkel Bernat, Mario.Limonciello, linux-pci,
linux-kernel
On Tue, Sep 26, 2017 at 05:17:13PM +0300, Mika Westerberg wrote:
> Hi,
>
> Currently when plugging PCIe device using native PCIe hotplug Linux PCI
> core tries to allocate bus space and resources so that the newly enumerated
> topology barely fits there. Now, if the PCIe topology that was just plugged
> in has more PCIe hotplug ports we will run out of bus space and resources
> pretty quickly. There is a workaround for this by passing pci=hpbussize=N
> in the kernel command line but it runs to the same situation after next
> hotplug.
Hi Bjorn,
Do you have any comments regarding this series? I think I need to respin
this on top of Andy's for_each_pci_bridge() patch but if there are other
comments I can address them at the same time.
Thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread