[PATCH v4 0/4] PCI: Patch series to support Thunderbolt without any BIOS support
1. Based on 5.1-rc5 so should apply cleanly to latest version 2. The comment clean-up patch is now last 3. Kernel parameters are no longer renamed Sorry - last time I misunderstood or read over the requirements too quickly. I hope this time everything is as requested. Please let me know if there is anything else to change. It is still not as thoroughly checked over as my PATCH v2 series for small typos, but I cannot find any problems. All patches pass the scripts/checkpatch.pl perfectly with no warnings. Thank you for your continued patience and understanding. Nicholas Johnson (4): PCI: Consider alignment of hot-added bridges when distributing available resources PCI: Fix serious bug when sizing bridges with additional size PCI: Modify kernel parameters to differentiate between MMIO and MMIO_PREF sizes PCI: Cleanup block comments in setup-bus.c to meet kernel style guidelines .../admin-guide/kernel-parameters.txt | 5 +- drivers/pci/pci.c | 12 +- drivers/pci/setup-bus.c | 504 +- include/linux/pci.h | 3 +- 4 files changed, 278 insertions(+), 246 deletions(-) -- 2.20.1
[PATCH v4 1/4] PCI: Consider alignment of hot-added bridges when distributing available resources
Rewrite pci_bus_distribute_available_resources to better handle bridges
with different resource alignment requirements. Pass more details
arguments recursively to track the resource start and end addresses
relative to the initial hotplug bridge. This is especially useful for
Thunderbolt with native PCI enumeration, enabling external graphics
cards and other devices with bridge alignment higher than 0x10
bytes.
Change extend_bridge_window to resize the actual resource, rather than
using add_list and dev_res->add_size. If an additional resource entry
exists for the given resource, zero out the add_size field to avoid it
interfering. Because add_size is considered optional when allocating,
using add_size could cause issues in some cases, because successful
resource distribution requires sizes to be guaranteed. Such cases
include hot-adding nested hotplug bridges in one enumeration, and
potentially others which are yet to be encountered.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 203 ++--
1 file changed, 110 insertions(+), 93 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ec44a0f3a..a1ca8a11f 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1815,34 +1815,48 @@ 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 list_head *add_list, resource_size_t new_size)
{
struct pci_dev_resource *dev_res;
+ resource_size_t add_size;
if (res->parent)
return;
- if (resource_size(res) >= available)
- return;
-
- dev_res = res_to_dev_res(add_list, res);
- if (!dev_res)
- return;
+ /*
+* Resources requested using add_size in additional resource lists are
+* considered optional when allocated. Guaranteed size of allocation
+* is required to guarantee successful resource distribution. Hence,
+* the size of the actual resource must be adjusted.
+*/
+ if (new_size >= resource_size(res)) {
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
+ &add_size);
+ } else {
+ add_size = resource_size(res) - new_size;
+ pci_dbg(bridge, "bridge window %pR shrunken by %pa\n", res,
+ &add_size);
+ }
- /* Is there room to extend the window? */
- if (available - resource_size(res) <= dev_res->add_size)
- return;
+ res->end = res->start + new_size - 1;
- dev_res->add_size = available - resource_size(res);
- pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
- &dev_res->add_size);
+ /*
+* If a list entry exists, we need to remove any additional size
+* requested because that could interfere with the alignment and
+* sizing done when distributing resources, causing resources to
+* fail to allocate later on.
+*/
+ dev_res = res_to_dev_res(add_list, res);
+ if (dev_res)
+ dev_res->add_size = 0;
}
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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1852,25 +1866,32 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window. A single bridge
+* might not be able to occupy the whole parent region if the alignment
+* differs - for example, an external GPU at the end of a Thunderbolt
+* daisy chain.
*/
- extend_bridge_window(bridge, io_res, add_list, available_io);
- extend_br
[PATCH v4 2/4] PCI: Fix serious bug when sizing bridges with additional size
Change find_free_bus_resource function to not skip assigned resources
with non-null parent.
Add checks in pbus_size_io and pbus_size_mem to return success if
resource returned from find_free_bus_resource is already allocated.
This avoids pbus_size_io and pbus_size_mem returning error code to
__pci_bus_size_bridges when a resource has been successfully assigned in
a previous pass. This fixes the existing behaviour where space for a
resource could be reserved multiple times in different parent bridge
windows. This also greatly reduces the number of failed BAR messages in
dmesg when Linux assigns resources.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 28 +++-
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index a1ca8a11f..5b9ee9945 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -752,11 +752,17 @@ static void pci_bridge_check_ranges(struct pci_bus *bus)
}
}
-/* Helper function for sizing routines: find first available
- bus resource of a given type. Note: we intentionally skip
- the bus resources which have already been assigned (that is,
- have non-NULL parent resource). */
-static struct resource *find_free_bus_resource(struct pci_bus *bus,
+/*
+ * Helper function for sizing routines: find first bus resource of a given
+ * type. Note: we do not skip the bus resources which have already been
+ * assigned (r->parent != NULL). This is because a resource that is already
+ * assigned (nothing more to be done) will be indistinguishable from one that
+ * failed due to lack of space if we skip assigned resources. If the caller
+ * function cannot tell the difference then it might try to place the
+ * resources in a different window, doubling up on resources or causing
+ * unforeseeable issues.
+ */
+static struct resource *find_bus_resource_of_type(struct pci_bus *bus,
unsigned long type_mask, unsigned long type)
{
int i;
@@ -765,7 +771,7 @@ static struct resource *find_free_bus_resource(struct
pci_bus *bus,
pci_bus_for_each_resource(bus, r, i) {
if (r == &ioport_resource || r == &iomem_resource)
continue;
- if (r && (r->flags & type_mask) == type && !r->parent)
+ if (r && (r->flags & type_mask) == type)
return r;
}
return NULL;
@@ -863,14 +869,16 @@ static void pbus_size_io(struct pci_bus *bus,
resource_size_t min_size,
resource_size_t add_size, struct list_head *realloc_head)
{
struct pci_dev *dev;
- struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
- IORESOURCE_IO);
+ struct resource *b_res = find_bus_resource_of_type(bus, IORESOURCE_IO,
+ IORESOURCE_IO);
resource_size_t size = 0, size0 = 0, size1 = 0;
resource_size_t children_add_size = 0;
resource_size_t min_align, align;
if (!b_res)
return;
+ if (b_res->parent)
+ return;
min_align = window_alignment(bus, IORESOURCE_IO);
list_for_each_entry(dev, &bus->devices, bus_list) {
@@ -975,7 +983,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long
mask,
resource_size_t min_align, align, size, size0, size1;
resource_size_t aligns[18]; /* Alignments from 1Mb to 128Gb */
int order, max_order;
- struct resource *b_res = find_free_bus_resource(bus,
+ struct resource *b_res = find_bus_resource_of_type(bus,
mask | IORESOURCE_PREFETCH, type);
resource_size_t children_add_size = 0;
resource_size_t children_add_align = 0;
@@ -983,6 +991,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long
mask,
if (!b_res)
return -ENOSPC;
+ if (b_res->parent)
+ return 0;
memset(aligns, 0, sizeof(aligns));
max_order = 0;
--
2.20.1
[PATCH v4 4/4] PCI: Cleanup block comments in setup-bus.c to meet kernel style guidelines
Change block comments to accepted style with asterisks on each line.
Justify block comments to 80-character limit to reduce the number of
lines where possible.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 248 +++-
1 file changed, 120 insertions(+), 128 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 95149165a..bdce5b734 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -51,11 +51,9 @@ static void free_list(struct list_head *head)
/**
* add_to_list() - add a new resource tracker to the list
* @head: Head of the list
- * @dev: device corresponding to which the resource
- * belongs
+ * @dev: device corresponding to which the resource belongs
* @res: The resource to be tracked
- * @add_size: additional size to be optionally added
- * to the resource
+ * @add_size: additional size to be optionally added to the resource
*/
static int add_to_list(struct list_head *head,
struct pci_dev *dev, struct resource *res,
@@ -158,7 +156,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct
list_head *head)
tmp->res = r;
tmp->dev = dev;
- /* fallback is smallest one or list is empty*/
+ /* fallback is smallest one or list is empty */
n = head;
list_for_each_entry(dev_res, head, list) {
resource_size_t align;
@@ -171,7 +169,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct
list_head *head)
break;
}
}
- /* Insert it just before n*/
+ /* Insert it just before n */
list_add_tail(&tmp->list, n);
}
}
@@ -181,7 +179,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
{
u16 class = dev->class >> 8;
- /* Don't touch classless devices or host bridges or ioapics. */
+ /* Don't touch classless devices or host bridges or ioapics */
if (class == PCI_CLASS_NOT_DEFINED || class == PCI_CLASS_BRIDGE_HOST)
return;
@@ -208,12 +206,10 @@ static inline void reset_resource(struct resource *res)
*
* @realloc_head : head of the list tracking requests requiring additional
* resources
- * @head : head of the list tracking requests with allocated
- * resources
+ * @head : head of the list tracking requests with allocated resources
*
- * Walk through each element of the realloc_head and try to procure
- * additional resources for the element, provided the element
- * is in the head list.
+ * Walk through each element of the realloc_head and try to procure additional
+ * resources for the element, provided the element is in the head list.
*/
static void reassign_resources_sorted(struct list_head *realloc_head,
struct list_head *head)
@@ -315,10 +311,9 @@ static unsigned long pci_fail_res_type_mask(struct
list_head *fail_head)
mask |= fail_res->flags;
/*
-* one pref failed resource will set IORESOURCE_MEM,
-* as we can allocate pref in non-pref range.
-* Will release all assigned non-pref sibling resources
-* according to that bit.
+* one pref failed resource will set IORESOURCE_MEM, as we can allocate
+* pref in non-pref range. Will release all assigned non-pref sibling
+* resources according to that bit.
*/
return mask & (IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH);
}
@@ -351,25 +346,24 @@ static void __assign_resources_sorted(struct list_head
*head,
struct list_head *fail_head)
{
/*
-* Should not assign requested resources at first.
-* they could be adjacent, so later reassign can not reallocate
-* them one by one in parent resource window.
-* Try to assign requested + add_size at beginning
-* if could do that, could get out early.
-* if could not do that, we still try to assign requested at first,
-*then try to reassign add_size for some resources.
+* Should not assign requested resources at first. They could be
+* adjacent, so later reassign can not reallocate them one by one in
+* parent resource window.
+*
+* Try to assign requested + add_size at beginning. If could do that,
+* could get out early. If could not do that, we still try to assign
+* requested at first, then try to reassign add_size for some resources.
*
* Separate three resource type checking if we need to release
* assigned resource after requested + add_size try.
-* 1. if there is io port assign fail, will release assigned
-* io port.
-*
[PATCH v4 3/4] PCI: Modify kernel parameters to differentiate between MMIO and MMIO_PREF sizes
Add kernel parameter pci=hpmemprefsize=nn[KMG] to control MMIO_PREF
size for PCI hotplug bridges.
Change behaviour of pci=hpmemsize=nn[KMG] to only control MMIO size,
rather than controlling both MMIO and MMIO_PREF sizes.
Update kernel-parameters documentation to reflect the above changes.
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 5 +++-
drivers/pci/pci.c | 12 ++---
drivers/pci/setup-bus.c | 25 +++
include/linux/pci.h | 3 ++-
4 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 2b8ee90bb..f33b590c2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3361,7 +3361,10 @@
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
hpmemsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's memory window.
+ reserved for hotplug bridge's MMIO window.
+ Default size is 2 megabytes.
+ hpmemprefsize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO_PREF window.
Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7c1b362f5..e0502831f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,10 +85,12 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6245,7 +6247,11 @@ static int __init pci_setup(char *str)
} else if (!strncmp(str, "hpiosize=", 9)) {
pci_hotplug_io_size = memparse(str + 9, &str);
} else if (!strncmp(str, "hpmemsize=", 10)) {
- pci_hotplug_mem_size = memparse(str + 10, &str);
+ pci_hotplug_mmio_size =
+ memparse(str + 10, &str);
+ } else if (!strncmp(str, "hpmemprefsize=", 14)) {
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 14, &str);
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
simple_strtoul(str + 10, &str, 0);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 5b9ee9945..95149165a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1187,7 +1187,8 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
{
struct pci_dev *dev;
unsigned long mask, prefmask, type2 = 0, type3 = 0;
- resource_size_t additional_mem_size = 0, additional_io_size = 0;
+ resource_size_t additional_io_size = 0, additional_mmio_size = 0,
+ additional_mmio_pref_size = 0;
struct resource *b_res;
int ret;
@@ -1221,7 +1222,8 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
pci_bridge_check_ranges(bus);
if (bus->self->is_hotplug_bridge) {
additional_io_size = pci_hotplug_io_size;
- additional_mem_size = pci_hotplug_mem_size;
+ additional_mmio_size = pci_hotplug_mmio_size;
+ additional_mmio_pref_size = pci_hotplug_mmio_pref_size;
}
/* Fall through */
default:
@@ -1239,9 +1241,9 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
if (b_res[2].flags & IORESOURCE_MEM_64) {
prefmask |= IORESOURCE_MEM_64;
[PATCH 1/2] PCI: fix serious bug when sizing bridges with additional size
Background: I discovered a bug while preparing my next patch for Thunderbolt native PCI enumeration. This bugfix is vital for that next Thunderbolt patch. They are related and could be put together but I am presenting them as separate patches. Nature of problem: On boot, the PCI bridges are handled in the function pci_assign_unassigned_root_bus_resources(). This function makes multiple passes / attempts to assign all of the resources. It calls __pci_bus_size_bridges() which tries to assign required resources, plus additional requested space. It makes calls to pbus_size_io() and pbus_size_mem() which return zero on success. However, these functions misleadingly return non-zero if the resource is already assigned. The fallout: If the first attempt to allocate resources on a bridge succeeds in 64-bit MMIO_PREF, but fails in 32-bit MMIO, then a second attempt is made. Then, the __pbus_size_mem() returns non-zero for MMIO_PREF because the resource is already assigned. That makes __pci_bus_size_bridges() think there is no space (ENOSPC) and it attempts to assign the sum of the MMIO_PREF and MMIO sizes into a 32-bit window. This means that it tries to request the sum of the the MMIO and MMIO_PREF size into the MMIO window. In the best case, the MMIO size is equal to the MMIO_PREF size and we get double the window in MMIO, and the correct size in MMIO_PREF, and nothing bad happens. In a worse case, doubling the size makes it fail due to lack of space in the miniscule MMIO 32-bit address space. In that case, we might be able to reduce the requested size. In the worst case, we have requested for a vast amount of MMIO_PREF, and a small amount of MMIO. The kernel tries to assign the sum of the sizes of the two types in the MMIO window, leaving us with MMIO_PREF assigned and MMIO unassigned due to the requested size being too large to assign. In this case, there is nothing we can do about it. If a massive additional MMIO_PREF and a small non-zero amount of additional MMIO are needed, then we cannot reduce the requested MMIO_PREF to work around the bug. The cause: pbus_size_io() and pbus_size_mem() both call find_free_bus_resource() to identify which bridge window of the bus meets the criteria. This function explicitly skips resources with a parent, inevitably returning NULL. The functions pbus_size_io() and pbus_size_mem() return -ENOSPC if find_free_bus_resource() returns NULL, meaning an assigned resource is interpreted as having been failed to assign, potentially causing it to try to allocate it again in another window. The solution: my proposed patch renames find_free_bus_resource() to find_bus_resource_of_type() and modifies it to not skip resources with r->parent. The name change is because returning an assigned resource makes the resource potentially not "free". The calling functions, pbus_size_io() and pbus_size_mem() have checks added for b_res->parent and they return 0 (success) in this case. This is because a resource with a parent is already assigned and should be interpreted as a success, not a failure. Testing: I have tested my proposed patch and it appears to work flawlessly. It has the added benefit of slashing the number of attempts taken to assign a given BAR, meaning a much cleaner dmesg. In fact, in my configuration, it has gone from very messy to mainly IO BAR failures. There is nothing that can be done about IO BARs because of the 16-bit hardware limitation, meaning the amount of available space cannot be increased. All that I am left with is a few "failed to assign [mem size]" very early in boot, before those BARs succeed in the next attempt. After the small initial set of failures for MEM, there are no more "failed to assign" whatsoever for non-IO BARs. Remarks: I fixed some other block comments to comply with kernel code styling standards. I had to modify the one block comment to reflect the changes, and checkpatch.pl got really angry and yelled at me. So I figured I may as well fix the lot. Signed-off-by: Nicholas Johnson --- drivers/pci/setup-bus.c | 82 + 1 file changed, 51 insertions(+), 31 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index a8be1a90f..c7e0a5e2b 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -563,17 +563,19 @@ void pci_setup_cardbus(struct pci_bus *bus) } EXPORT_SYMBOL(pci_setup_cardbus); -/* Initialize bridges with base/limit values we have collected. - PCI-to-PCI Bridge Architecture Specification rev. 1.1 (1998) - requires that if there is no I/O ports or memory behind the - bridge, corresponding range must be turned off by writing base - value greater than limit to the bridge's base/limit registers. - - Note: care must be taken when updating I/O base/limit registers - of bridges which support 32-bit I/O. This update requires two - config space writes, so it's quite possible that an I/O window of - the
[PATCH 2/2] PCI: modify kernel parameters to differentiate between MMIO and MMIO_PREF sizes
Depends:
commit a1140e7bcb10ff96c192ee200e6cbf832f27158e
("PCI: fix serious bug when sizing bridges with additional size")
Background: I have come to find that the kernel parameters for reserving
resources for the hotplug bridges are not useful for Thunderbolt with
native PCI enumeration. You can only increase the size so far before it
fails to allocate the 32-bit MMIO windows.
Nature of problem: pci=hpmemsize=nnM is parsed from drivers/pci/pci.c
and used in drivers/pci/setup-bus.c. It overwrites pci_hotplug_mem_size
which has a default value set by DEFAULT_HOTPLUG_MEM_SIZE. When used,
this value is used to request additional space for MMIO and MMIO_PREF in
equal amounts.
The fallout: if you increase the value of pci=hpmemsize=nnM to be too
large to fit into the 32-bit address space, the MMIO window fails to
allocate and has zero-sized BARs. In the case of Thunderbolt, this means
the NHI does not have the resources needed to function, and the
thunderbolt.ko driver fails to probe the device. All Thunderbolt
functionality is lost. Even if the NHI worked, any Thunderbolt device
containing endpoints with 32-bit BARs (most of them) would fail to
initialise properly. The worst case happens if some resources end up
allocating with non-zero size, but too small. With some AMD Radeon
external GPUs, if certain combinations of BARs are assigned / failed to
assign, it can cause BUG_ON from arch/x86/mm/pat.c due to the buggy
amdgpu.ko driver trying to use a zero-sized BAR, instead of failing
gracefully. This more or less renders the system unusable.
The cause: because the kernel parameters do not allow the user to
specify the MMIO and MMIO_PREF hotplug bridge additional sizes
separately, they have no choice but to give unrealistic sizes that will
fail, or will be forced to make unpleasant compromises, resulting in
having to tolerate unstable or unpredictable operation.
The solution: My proposed patch depreciates
pci=hpmemsize=nnM,hpiosize=nn but allows them to work without any change
in user-visible functionality, with a KERN_WARNING message when either
are used. It adds the new parameters
pci=hp_io_size=nn,hp_mmio_size=nnM,hp_mmio_pref_size=nnM. If any of the
new kernel parameters are used, all of the newly-depreciated ones will
be overridden. This will allow for a grace period to allow people to
change to the new kernel parameters without unpleasant disruption. At a
time deemed appropriate by kernel developers, the old parameters can be
easily removed without the need to rework any code.
My testing: This works very well. I have not encountered any problems
and I am happily allocating 128M/64G under each Thunderbolt port with
nocrs. The success is dependent on my previous bug patch, which I
decided to separate from this feature patch. The old functionality still
works the same, and is overridden by the new commands if they are used.
Remarks: The printk at the end of pci_setup() in drivers/pci/pci.c has a
strange backquote ('`', under the tilde on the keyboard key) in the
format. I am not sure if it is a typo, or deliberate, and whether it
should change or if my KERN_WARNING logs need to use that symbol. Also,
the scripts/checkpatch.pl berates me for using printk, but the
alternative, pci_warn(), requires a pci_dev to work, and this message
does not pertain to a particular device.
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 21 --
drivers/pci/pci.c | 39 +--
drivers/pci/setup-bus.c | 26 +++--
include/linux/pci.h | 3 +-
4 files changed, 69 insertions(+), 20 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index b799bcf67..284752ff7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3327,12 +3327,27 @@
the default.
off: Turn ECRC off
on: Turn ECRC on.
- hpiosize=nn[KMG]The fixed amount of bus space which is
+
+ hpiosize=nn[KMG]Depreciated. Overridden by hp_io_size
+ value if any of hp_io_size, hp_mmio_size
+ or hp_mmio_pref_size are used. This sets
+ hp_io_size to the given value if not
+ overridden.
+
+ hpmemsize=nn[KMG] Depreciated. Overridden if any of
+ hp_io_size, hp_mmio_size or
+ hp_mmio_pref_size are used. This sets
+ hp_mmio_size and hp_mmio_pref_size to
+ the given value if not overridden.
+ hp_io_size=nn[KMG] The fixed amount of bus space which is
Re: [PATCH 1/1] amdgpu: use MMIO to init atombios if device is Thunderbolt / USB4 attached
On Mon, Mar 15, 2021 at 11:05:21AM -0400, Alex Deucher wrote:
> On Mon, Mar 15, 2021 at 4:04 AM Nicholas Johnson
> wrote:
> >
> > When using some Thunderbolt hosts using BIOS-assisted PCI enumeration
> > with IO BAR assigned, we get an atombios timeout, such as:
> >
> > [drm:atom_op_jump [amdgpu]] *ERROR* atombios stuck in loop for more than
> > 20secs aborting
> > [drm:amdgpu_atom_execute_table_locked [amdgpu]] *ERROR* atombios stuck
> > executing B456 (len 304, WS 4, PS 0) @ 0xB51B
> > [drm:amdgpu_atom_execute_table_locked [amdgpu]] *ERROR* atombios stuck
> > executing B104 (len 183, WS 0, PS 8) @ 0xB17E
> > amdgpu :08:00.0: amdgpu: gpu post error!
> > amdgpu :08:00.0: amdgpu: Fatal error during GPU init
> > amdgpu: probe of :08:00.0 failed with error -22
> >
> > A workaround is to use MMIO to access ATOMBIOS when device is
> > Thunderbolt / USB4 attached.
>
> Missing your signed-off-by. Also, we can just remove the legacy IO
> callbacks altogether. They are leftover from radeon and not required
> at all on amdgpu.
Sorry, it's been a while; I forgot "-s". And I like your patch much
better. I look forward to the day when all new PCIe devices only have
64-bit MMIO_PREF BARs.
Thanks for looking at this! If you are still doing work on surprise
removal / driver unloading for Thunderbolt, then I am happy to do
testing for you. Removal of DRM devices in Linux is the main sore point
for me, and I would love to see it through.
Regards,
Nicholas Johnson
>
> Alex
>
>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 8 ++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> > index 86add0f4e..5d16ec10d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
> > @@ -1999,11 +1999,15 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
> > atom_card_info->reg_read = cail_reg_read;
> > atom_card_info->reg_write = cail_reg_write;
> > /* needed for iio ops */
> > - if (adev->rio_mem) {
> > + if (adev->rio_mem && !pci_is_thunderbolt_attached(adev->pdev)) {
> > atom_card_info->ioreg_read = cail_ioreg_read;
> > atom_card_info->ioreg_write = cail_ioreg_write;
> > } else {
> > - DRM_DEBUG("PCI I/O BAR is not found. Using MMIO to access
> > ATOM BIOS\n");
> > + if (pci_is_thunderbolt_attached(adev->pdev))
> > + DRM_DEBUG("Device is attached via Thunderbolt /
> > USB4. Using MMIO to access ATOM BIOS\n");
> > + else
> > + DRM_DEBUG("PCI I/O BAR is not found. Using MMIO to
> > access ATOM BIOS\n");
> > +
> > atom_card_info->ioreg_read = cail_reg_read;
> > atom_card_info->ioreg_write = cail_reg_write;
> > }
> > --
> > 2.30.2
> >
> > ___
> > amd-gfx mailing list
> > amd-...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/1] amdgpu: use MMIO to init atombios if device is Thunderbolt / USB4 attached
When using some Thunderbolt hosts using BIOS-assisted PCI enumeration
with IO BAR assigned, we get an atombios timeout, such as:
[drm:atom_op_jump [amdgpu]] *ERROR* atombios stuck in loop for more than 20secs
aborting
[drm:amdgpu_atom_execute_table_locked [amdgpu]] *ERROR* atombios stuck
executing B456 (len 304, WS 4, PS 0) @ 0xB51B
[drm:amdgpu_atom_execute_table_locked [amdgpu]] *ERROR* atombios stuck
executing B104 (len 183, WS 0, PS 8) @ 0xB17E
amdgpu :08:00.0: amdgpu: gpu post error!
amdgpu :08:00.0: amdgpu: Fatal error during GPU init
amdgpu: probe of :08:00.0 failed with error -22
A workaround is to use MMIO to access ATOMBIOS when device is
Thunderbolt / USB4 attached.
---
drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
index 86add0f4e..5d16ec10d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c
@@ -1999,11 +1999,15 @@ int amdgpu_atombios_init(struct amdgpu_device *adev)
atom_card_info->reg_read = cail_reg_read;
atom_card_info->reg_write = cail_reg_write;
/* needed for iio ops */
- if (adev->rio_mem) {
+ if (adev->rio_mem && !pci_is_thunderbolt_attached(adev->pdev)) {
atom_card_info->ioreg_read = cail_ioreg_read;
atom_card_info->ioreg_write = cail_ioreg_write;
} else {
- DRM_DEBUG("PCI I/O BAR is not found. Using MMIO to access ATOM
BIOS\n");
+ if (pci_is_thunderbolt_attached(adev->pdev))
+ DRM_DEBUG("Device is attached via Thunderbolt / USB4.
Using MMIO to access ATOM BIOS\n");
+ else
+ DRM_DEBUG("PCI I/O BAR is not found. Using MMIO to
access ATOM BIOS\n");
+
atom_card_info->ioreg_read = cail_reg_read;
atom_card_info->ioreg_write = cail_reg_write;
}
--
2.30.2
[PATCH 0/1] Init atombios timeout when amdgpu is Thunderbolt / USB4 when IO BAR is assigned
Hi all, I am not certain why this happens, but when IO bar is assigned on Thunderbolt, the amdgpu init fails: [drm:atom_op_jump [amdgpu]] *ERROR* atombios stuck in loop for more than 20secs aborting [drm:amdgpu_atom_execute_table_locked [amdgpu]] *ERROR* atombios stuck executing B456 (len 304, WS 4, PS 0) @ 0xB51B [drm:amdgpu_atom_execute_table_locked [amdgpu]] *ERROR* atombios stuck executing B104 (len 183, WS 0, PS 8) @ 0xB17E amdgpu :08:00.0: amdgpu: gpu post error! amdgpu :08:00.0: amdgpu: Fatal error during GPU init amdgpu: probe of :08:00.0 failed with error -22 It seems to happen mostly when BIOS-assisted PCI enumeration is used (older Thunderbolt systems). I cannot rule it out with native enumeration, but generally native enumeration works, as the IO BAR is not assigned, due to limited IO resources. This patch is a simple fix against v5.12-rc2. I have experienced this issue for a long time, and have finally decided to do something about it. I do not see a downside to using MMIO, which is required to be assigned. Kind regards, Nicholas Johnson Nicholas Johnson (1): amdgpu: use MMIO to init atombios if device is Thunderbolt / USB4 attached drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) -- 2.30.2
Re: [PATCH 0/1] Fiji GPU audio register timeout when in BACO state
On Thu, Apr 30, 2020 at 05:14:56PM +0200, Takashi Iwai wrote:
> On Wed, 29 Apr 2020 18:19:57 +0200,
> Alex Deucher wrote:
> >
> > On Wed, Apr 29, 2020 at 12:05 PM Takashi Iwai wrote:
> > > Well, but the code path there is the runtime PM resume of the audio
> > > device and it means that GPU must have been runtime-resumed again
> > > beforehand via the device link. So, it should have worked from the
> > > beginning but in reality not -- that is, apparently some inconsistency
> > > is found in the initial attempt of the runtime resume...
> >
> > Yeah, it should be covered, but I wonder if there is something in the
> > ELD update sequence that needs to call pm_runtime_get_sync()? The ELD
> > sequence on AMD GPUs doesn't work the same as on other vendors. The
> > GPU driver has a backdoor into the HDA device's verbs to set update
> > the audio state rather than doing it via an ELD buffer update. We
> > still update the ELD buffer for consistency. Maybe when the GPU
> > driver sets the audio state at monitor detection time that triggers an
> > interrupt or something on the HDA side which races with the CPU and
> > the power down of the GPU. That still seems unlikely though since the
> > runtime pm on the GPU side defaults to a 5 second suspend timer.
>
> I'm not sure whether it's the race between runtime suspend of GPU vs
> runtime resume of audio. My wild guess is rather that it's the timing
> GPU notifies to the audio; then the audio driver notifies to
> user-space and user-space opens the stream, which in turn invokes the
> runtime resume of GPU. But in GPU side, it's still under processing,
> so it proceeds before the GPU finishes its initialization job.
>
> Nicholas, could you try the patch below and see whether the problem
> still appears? The patch artificially delays the notification and ELD
> update for 300msec. If this works, it means the timing problem.
The bug still occurred after applying the patch.
But you were absolutely correct - it just needed to be increased to
3000ms - then the bug stopped.
Now the question is, what do we do now that we know this?
Also, are you still interested in the contents of the ELD# files? I can
dump them all into a file at some specific moment in time which you
request, if needed.
Thanks.
Regards, Nicholas
>
>
> thanks,
>
> Takashi
>
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -767,6 +767,7 @@ static void check_presence_and_report(struct hda_codec
> *codec, hda_nid_t nid,
> if (pin_idx < 0)
> return;
> mutex_lock(&spec->pcm_lock);
> + get_pin(spec, pin_idx)->repoll_count = 1;
> hdmi_present_sense(get_pin(spec, pin_idx), 1);
> mutex_unlock(&spec->pcm_lock);
> }
> @@ -1647,7 +1648,10 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
> per_pin->dev_id, &eld->monitor_present,
> eld->eld_buffer, ELD_MAX_SIZE);
> eld->eld_valid = (eld->eld_size > 0);
> - update_eld(codec, per_pin, eld, 0);
> + if (per_pin->repoll_count)
> + schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300));
> + else
> + update_eld(codec, per_pin, eld, 0);
> mutex_unlock(&per_pin->lock);
> }
>
> @@ -1669,6 +1673,11 @@ static void hdmi_repoll_eld(struct work_struct *work)
> struct hdmi_spec *spec = codec->spec;
> struct hda_jack_tbl *jack;
>
> + if (codec_has_acomp(codec)) {
> + per_pin->repoll_count = 0;
> + goto check;
> + }
> +
> jack = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
> per_pin->dev_id);
> if (jack)
> @@ -1677,6 +1686,7 @@ static void hdmi_repoll_eld(struct work_struct *work)
> if (per_pin->repoll_count++ > 6)
> per_pin->repoll_count = 0;
>
> + check:
> mutex_lock(&spec->pcm_lock);
> hdmi_present_sense(per_pin, per_pin->repoll_count);
> mutex_unlock(&spec->pcm_lock);
Re: [PATCH 0/1] Fiji GPU audio register timeout when in BACO state
On Thu, Apr 30, 2020 at 07:01:08PM +0200, Takashi Iwai wrote: > On Thu, 30 Apr 2020 18:52:20 +0200, > Nicholas Johnson wrote: > > > > On Thu, Apr 30, 2020 at 05:14:56PM +0200, Takashi Iwai wrote: > > > On Wed, 29 Apr 2020 18:19:57 +0200, > > > Alex Deucher wrote: > > > > > > > > On Wed, Apr 29, 2020 at 12:05 PM Takashi Iwai wrote: > > > > > Well, but the code path there is the runtime PM resume of the audio > > > > > device and it means that GPU must have been runtime-resumed again > > > > > beforehand via the device link. So, it should have worked from the > > > > > beginning but in reality not -- that is, apparently some inconsistency > > > > > is found in the initial attempt of the runtime resume... > > > > > > > > Yeah, it should be covered, but I wonder if there is something in the > > > > ELD update sequence that needs to call pm_runtime_get_sync()? The ELD > > > > sequence on AMD GPUs doesn't work the same as on other vendors. The > > > > GPU driver has a backdoor into the HDA device's verbs to set update > > > > the audio state rather than doing it via an ELD buffer update. We > > > > still update the ELD buffer for consistency. Maybe when the GPU > > > > driver sets the audio state at monitor detection time that triggers an > > > > interrupt or something on the HDA side which races with the CPU and > > > > the power down of the GPU. That still seems unlikely though since the > > > > runtime pm on the GPU side defaults to a 5 second suspend timer. > > > > > > I'm not sure whether it's the race between runtime suspend of GPU vs > > > runtime resume of audio. My wild guess is rather that it's the timing > > > GPU notifies to the audio; then the audio driver notifies to > > > user-space and user-space opens the stream, which in turn invokes the > > > runtime resume of GPU. But in GPU side, it's still under processing, > > > so it proceeds before the GPU finishes its initialization job. > > > > > > Nicholas, could you try the patch below and see whether the problem > > > still appears? The patch artificially delays the notification and ELD > > > update for 300msec. If this works, it means the timing problem. > > The bug still occurred after applying the patch. > > > > But you were absolutely correct - it just needed to be increased to > > 3000ms - then the bug stopped. > > Interesting. 3 seconds are too long, but I guess 1 second would work > as well? 1000ms indeed worked as well. > > In anyway, the success with a long delay means that the sound setup > after the full runtime resume of GPU seems working. > > > Now the question is, what do we do now that we know this? > > > > Also, are you still interested in the contents of the ELD# files? I can > > dump them all into a file at some specific moment in time which you > > request, if needed. > > Yes, please take the snapshot before plugging, right after plugging > and right after enabling. I'm not sure whether your monitor supports > the audio, and ELD contents should show that, at least. The monitor supports the audio. There is 3.5mm audio out jack. No inbuilt speakers, although Samsung did sell a sound bar to suit it. The sound bar, which I do not own, presumably attaches via 3.5mm jack. I am not sure if by plugging, you mean hot-adding Thunderbolt GPU or plugging the monitor to the GPU, so I have covered extra cases to be sure. I have taken the eld# files with the 1000ms patch applied, so the error is not triggered. Before hot-adding the Thunderbolt GPU: /proc/asound/card1 not present After hot-adding the GPU with no monitor attached: /proc/asound/card1 contains: eld#0.0 eld#0.1 eld#0.2 eld#0.3 eld#0.4 eld#0.5 All of the above have the same contents: monitor_present 0 eld_valid 0 Monitor attached to Fiji GPU but not enabled: Same as above Monitor enabled: All files with same contents except for eld#0.1 which looks like: monitor_present 1 eld_valid 1 monitor_nameU32E850 connection_type DisplayPort eld_version [0x2] CEA-861D or below edid_version[0x3] CEA-861-B, C or D manufacture_id 0x2d4c product_id 0xce3 port_id 0x0 support_hdcp0 support_ai 0 audio_sync_delay0 speakers[0x1] FL/FR sad_count 1 sad0_coding_type[0x1] LPCM sad0_channels 2 sad0_rates [0xe0] 32000 44100 48000 sad0_bits [0xe] 16 20 24 Cheers. Regards, Nicholas. > > > thanks, > > Takashi
[PATCH 0/1] Add support for setting MMIO PREF hotplug bridge size
This patch adds support for two new kernel parameters. This patch has been in the making for quite some time, and has changed several times based on feedback. I realised I was making the mistake of putting it as part of my Thunderbolt patch series. Although the other patches in the series are very important for my goal, I realised that they are just a heap of patches that are not Thunderbolt-specific. The only thing that is Thunderbolt-related is the intended use case. I hope that posting this alone can ease the difficulty of reviewing it. Nicholas Johnson (1): PCI: Add hp_mmio_size and hp_mmio_pref_size parameters .../admin-guide/kernel-parameters.txt | 9 ++- drivers/pci/pci.c | 17 ++--- drivers/pci/pci.h | 3 ++- drivers/pci/setup-bus.c | 25 +++ 4 files changed, 38 insertions(+), 16 deletions(-) -- 2.23.0
[PATCH 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
Add kernel parameter pci=hpmmiosize=nn[KMG] to set MMIO bridge window
size for hotplug bridges.
Add kernel parameter pci=hpmmioprefsize=nn[KMG] to set MMIO_PREF bridge
window size for hotplug bridges.
Leave pci=hpmemsize=nn[KMG] unchanged, to prevent disruptions to
existing users. This sets both MMIO and MMIO_PREF to the same size.
The two new parameters conform to the style of pci=hpiosize=nn[KMG].
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 9 ++-
drivers/pci/pci.c | 17 ++---
drivers/pci/pci.h | 3 ++-
drivers/pci/setup-bus.c | 25 +++
4 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index a84a83f88..cfe8c2b67 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3492,8 +3492,15 @@
hpiosize=nn[KMG]The fixed amount of bus space which is
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
+ hpmmiosize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO window.
+ Default size is 2 megabytes.
+ hpmmioprefsize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO_PREF window.
+ Default size is 2 megabytes.
hpmemsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's memory window.
+ reserved for hotplug bridge's MMIO and
+ MMIO_PREF window.
Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a97e2571a..f3adab84b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,10 +85,12 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6286,8 +6288,17 @@ static int __init pci_setup(char *str)
pcie_ecrc_get_policy(str + 5);
} else if (!strncmp(str, "hpiosize=", 9)) {
pci_hotplug_io_size = memparse(str + 9, &str);
+ } else if (!strncmp(str, "hpmmiosize=", 11)) {
+ pci_hotplug_mmio_size =
+ memparse(str + 11, &str);
+ } else if (!strncmp(str, "hpmmioprefsize=", 15)) {
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 15, &str);
} else if (!strncmp(str, "hpmemsize=", 10)) {
- pci_hotplug_mem_size = memparse(str + 10, &str);
+ pci_hotplug_mmio_size =
+ memparse(str + 10, &str);
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 10, &str);
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
simple_strtoul(str + 10, &str, 0);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3f6947ee3..9faa55a15 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -218,7 +218,8 @@ extern const struct device_type pci_dev_type;
extern const struct attribute_group *pci_bus_groups[];
extern unsigned long pci_hotplug_io_size;
-extern unsigned long pci_hotplug_mem_size;
+extern unsigned long pci_hotplug_mmio_size;
+extern unsigned long pci_hotplug_mmio_pref_size;
extern unsigned long pci_hotplug_bus_size;
/**
diff --git a/drivers/pci/setup-bus.c b/dr
Re: [PATCH v8 1/6] PCI: Consider alignment of hot-added bridges when distributing available resources
On Tue, Oct 08, 2019 at 02:38:12PM +0300, mika.westerb...@linux.intel.com wrote:
> Hi Nicholas,
Hi Mika,
I apologise for not responding quickly . I have switched off for a while
- taking my time to post the patches based on Linux 5.4. Hence, I was
not expecting any emails on this, and was not checking. Plus I was
starting to lose motivation.
I have been taking the time to change how I approach this. I am going to
post the patches to egpu.io forums to get a heap of people testing it
and hopefully saying nice things about it. Originally I thought it would
be quick to get the patches accepted so I was only going to announce
this after being accepted.
I also realised my patch series should not be a series. None of this is
specific to Thunderbolt and hence should not be a series. By separating
parts of this series, it may be easier to sign off and accept.
>
> On Fri, Jul 26, 2019 at 12:53:19PM +, Nicholas Johnson wrote:
> > Rewrite pci_bus_distribute_available_resources to better handle bridges
> > with different resource alignment requirements. Pass more details
> > arguments recursively to track the resource start and end addresses
> > relative to the initial hotplug bridge. This is especially useful for
> > Thunderbolt with native PCI enumeration, enabling external graphics
> > cards and other devices with bridge alignment higher than 1MB.
> >
> > Change extend_bridge_window to resize the actual resource, rather than
> > using add_list and dev_res->add_size. If an additional resource entry
> > exists for the given resource, zero out the add_size field to avoid it
> > interfering. Because add_size is considered optional when allocating,
> > using add_size could cause issues in some cases, because successful
> > resource distribution requires sizes to be guaranteed. Such cases
> > include hot-adding nested hotplug bridges in one enumeration, and
> > potentially others which are yet to be encountered.
> >
> > Solves bug report: https://bugzilla.kernel.org/show_bug.cgi?id=199581
>
> Here better to use:
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199581
>
> > Reported-by: Mika Westerberg
>
> This solves the issue I reported so,
>
> Tested-by: Mika Westerberg
So this is adding "Tested-by" on top of "Reported-by" and not replacing
one with the other?
>
> There are a couple of comments below.
>
> > Signed-off-by: Nicholas Johnson
> > ---
> > drivers/pci/setup-bus.c | 148 +++-
> > 1 file changed, 71 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 79b1fa651..6835fd64c 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1840,12 +1840,10 @@ static void extend_bridge_window(struct pci_dev
> > *bridge, struct resource *res,
> > }
> >
> > 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)
> > + struct list_head *add_list, struct resource io,
> > + struct resource mmio, struct resource mmio_pref)
>
> You pass a copy of each resource because you modify it inplace. I wonder
> if it makes more sense to explicitly take a copy here with comments?
I have no qualms with modifying parameters, and sometimes quite like
doing it. I could do as you suggest but that means more lines of diff,
and Bjorn seems to be sending me a strong message that the less lines of
diff, the better.
I just noticed this: https://lkml.org/lkml/2019/10/4/337
Bjorn says I am touching critical and complicated code that he does not
understand. This could explain his aversion to more lines of diff.
If Bjorn will trust you to sign this off and take your assurance that it
is fine, then I can start taking your advice over his. I have been
favouring his advice because I figured he would have the final say as
the PCI subsystem maintainer.
>
> > {
> > - resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
> > + resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
> > unsigned int normal_bridges = 0, hotplug_bridges = 0;
> > struct resource *io_res, *mmio_res, *mmio_pref_res;
> > struct pci_dev *dev, *bridge = bus->self;
> > @@ -1855,15 +1853,29 @@ static void
> > pci_bus_distribute_available_resources(struct pci_bus *bus,
> > mmio_pref_res = &
Re: [PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary
On Tue, Oct 08, 2019 at 03:09:07PM +0300, mika.westerb...@linux.intel.com wrote: > On Fri, Jul 26, 2019 at 12:54:22PM +0000, Nicholas Johnson wrote: > > Remove checks for resource size in extend_bridge_window(). This is > > necessary to allow the pci_bus_distribute_available_resources() to > > function when the kernel parameter pci=hpmemsize=nn[KMG] is used to > > allocate resources. Because the kernel parameter sets the size of all > > hotplug bridges to be the same, there are problems when nested hotplug > > bridges are encountered. Fitting a downstream hotplug bridge with size X > > and normal bridges with size Y into parent hotplug bridge with size X is > > impossible, and hence the downstream hotplug bridge needs to shrink to > > fit into its parent. > > Maybe you could show the topology here which needs shrinking. > > > Add check for if bridge is extended or shrunken and adjust pci_dbg to > > reflect this. > > > > Reset the resource if its new size is zero (if we have run out of a > > bridge window resource). If it is set to zero size and left, it can > > cause significant problems when it comes to enabling devices. > > Same comment here about explaining the "significant problems". I have in the past, but because the problems are very hard to describe succinctly, it just turns into a nightmare. I can try to do it again. > > > > Signed-off-by: Nicholas Johnson > > --- > > drivers/pci/setup-bus.c | 16 +++- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > index a072781ab..7e1dc892a 100644 > > --- a/drivers/pci/setup-bus.c > > +++ b/drivers/pci/setup-bus.c > > @@ -1823,13 +1823,19 @@ static void extend_bridge_window(struct pci_dev > > *bridge, struct resource *res, > > Since it is also shrinking now maybe name it adjust_bridge_window() instead? I am happy to do this. If we can drop the pci_dbg() calls, then I might be able to drop this function entirely. During the development of this patch, that is exactly what I did. How important are the pci_dbg calls to you? Another option is to simply print something with pci_dbg that simply says the bridge size has been set to maximum possible while still fitting in parent. That will remove the need for logic to detect if it shrunk or extended.
Re: [PATCH v8 1/6] PCI: Consider alignment of hot-added bridges when distributing available resources
On Wed, Oct 23, 2019 at 12:34:19PM +0300, mika.westerb...@linux.intel.com wrote: > On Wed, Oct 23, 2019 at 09:08:42AM +0000, Nicholas Johnson wrote: > > On Tue, Oct 08, 2019 at 02:38:12PM +0300, mika.westerb...@linux.intel.com > > wrote: > > > Hi Nicholas, > > > > Hi Mika, > > > > I apologise for not responding quickly . I have switched off for a while > > - taking my time to post the patches based on Linux 5.4. Hence, I was > > not expecting any emails on this, and was not checking. Plus I was > > starting to lose motivation. > > > > I have been taking the time to change how I approach this. I am going to > > post the patches to egpu.io forums to get a heap of people testing it > > and hopefully saying nice things about it. Originally I thought it would > > be quick to get the patches accepted so I was only going to announce > > this after being accepted. > > > > I also realised my patch series should not be a series. None of this is > > specific to Thunderbolt and hence should not be a series. By separating > > parts of this series, it may be easier to sign off and accept. > > > > > > > > On Fri, Jul 26, 2019 at 12:53:19PM +, Nicholas Johnson wrote: > > > > Rewrite pci_bus_distribute_available_resources to better handle bridges > > > > with different resource alignment requirements. Pass more details > > > > arguments recursively to track the resource start and end addresses > > > > relative to the initial hotplug bridge. This is especially useful for > > > > Thunderbolt with native PCI enumeration, enabling external graphics > > > > cards and other devices with bridge alignment higher than 1MB. > > > > > > > > Change extend_bridge_window to resize the actual resource, rather than > > > > using add_list and dev_res->add_size. If an additional resource entry > > > > exists for the given resource, zero out the add_size field to avoid it > > > > interfering. Because add_size is considered optional when allocating, > > > > using add_size could cause issues in some cases, because successful > > > > resource distribution requires sizes to be guaranteed. Such cases > > > > include hot-adding nested hotplug bridges in one enumeration, and > > > > potentially others which are yet to be encountered. > > > > > > > > Solves bug report: https://bugzilla.kernel.org/show_bug.cgi?id=199581 > > > > > > Here better to use: > > > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199581 > > > > > > > Reported-by: Mika Westerberg > > > > > > This solves the issue I reported so, > > > > > > Tested-by: Mika Westerberg > > So this is adding "Tested-by" on top of "Reported-by" and not replacing > > one with the other? > > Yes. > > > > > > > There are a couple of comments below. > > > > > > > Signed-off-by: Nicholas Johnson > > > > > > > > --- > > > > drivers/pci/setup-bus.c | 148 +++- > > > > 1 file changed, 71 insertions(+), 77 deletions(-) > > > > > > > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > > > > index 79b1fa651..6835fd64c 100644 > > > > --- a/drivers/pci/setup-bus.c > > > > +++ b/drivers/pci/setup-bus.c > > > > @@ -1840,12 +1840,10 @@ static void extend_bridge_window(struct pci_dev > > > > *bridge, struct resource *res, > > > > } > > > > > > > > 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) > > > > + struct list_head *add_list, struct resource io, > > > > + struct resource mmio, struct resource mmio_pref) > > > > > > You pass a copy of each resource because you modify it inplace. I wonder > > > if it makes more sense to explicitly take a copy here with comments? > > > > I have no qualms with modifying parameters, and sometimes quite like > > doing it. I could do as you suggest bu
Re: [PATCH 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
On Wed, Oct 23, 2019 at 12:47:43PM +0300, mika.westerb...@linux.intel.com wrote:
> On Wed, Oct 23, 2019 at 08:37:48AM +0000, Nicholas Johnson wrote:
> > } else if (!strncmp(str, "hpmemsize=", 10)) {
> > - pci_hotplug_mem_size = memparse(str + 10, &str);
> > + pci_hotplug_mmio_size =
> > + memparse(str + 10, &str);
> > + pci_hotplug_mmio_pref_size =
> > + memparse(str + 10, &str);
>
> Does this actually work correctly? The first memparse(str + 10, &str)
> modifies str so the next call will not start from the correct position
> anymore.
I have been using this for a long time now and have not had any issues.
Does it modify str? I thought that was done by the loop.
Can somebody else please weigh in here? I am worried now, and I want to
be sure. If it is a problem, then I will have to read it into
pci_hotplug_mmio_size and then set:
pci_hotplug_mmio_pref_size = pci_hotplug_mmio_size
>
> Otherwise the patch looks good to me.
Thanks, I will re-post with your last suggestions of going over 80
characters shortly.
Re: [PATCH 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
On Wed, Oct 23, 2019 at 01:03:59PM +0300, mika.westerb...@linux.intel.com wrote:
> On Wed, Oct 23, 2019 at 09:57:17AM +0000, Nicholas Johnson wrote:
> > On Wed, Oct 23, 2019 at 12:47:43PM +0300, mika.westerb...@linux.intel.com
> > wrote:
> > > On Wed, Oct 23, 2019 at 08:37:48AM +, Nicholas Johnson wrote:
> > > > } else if (!strncmp(str, "hpmemsize=", 10)) {
> > > > - pci_hotplug_mem_size = memparse(str +
> > > > 10, &str);
> > > > + pci_hotplug_mmio_size =
> > > > + memparse(str + 10, &str);
> > > > + pci_hotplug_mmio_pref_size =
> > > > + memparse(str + 10, &str);
> > >
> > > Does this actually work correctly? The first memparse(str + 10, &str)
> > > modifies str so the next call will not start from the correct position
> > > anymore.
> > I have been using this for a long time now and have not had any issues.
> > Does it modify str? I thought that was done by the loop.
>
> If you add "hpmemsize=xxx" in the command line and print both
> pci_hotplug_mmio_size and pci_hotplug_mmio_pref_size after the
> assignment, do they have the same value? If yes, then there is no
> problem.
Looking at lib/cmdline.c line 125, it looks like there is no point in me
testing it. It looks like you are right.
What is the better fix?
pci_hotplug_mmio_size = pci_hotplug_mmio_pref_size = memparse(str + 10, &str);
^ Could be too long, even if we are ignoring the 80-character limit.
>
> > Can somebody else please weigh in here? I am worried now, and I want to
> > be sure. If it is a problem, then I will have to read it into
> > pci_hotplug_mmio_size and then set:
> >
> > pci_hotplug_mmio_pref_size = pci_hotplug_mmio_size
>
> Yup.
Or do you prefer the above?
Thanks
Re: [PATCH 0/1] Add support for setting MMIO PREF hotplug bridge size
On Wed, Oct 23, 2019 at 12:45:22PM +0300, mika.westerb...@linux.intel.com wrote: > On Wed, Oct 23, 2019 at 08:36:59AM +0000, Nicholas Johnson wrote: > > This patch adds support for two new kernel parameters. This patch has > > been in the making for quite some time, and has changed several times > > based on feedback. > > > > I realised I was making the mistake of putting it as part of my > > Thunderbolt patch series. Although the other patches in the series are > > very important for my goal, I realised that they are just a heap of > > patches that are not Thunderbolt-specific. The only thing that is > > Thunderbolt-related is the intended use case. > > > > I hope that posting this alone can ease the difficulty of reviewing it. > > > > Nicholas Johnson (1): > > PCI: Add hp_mmio_size and hp_mmio_pref_size parameters > > > > .../admin-guide/kernel-parameters.txt | 9 ++- > > drivers/pci/pci.c | 17 ++--- > > drivers/pci/pci.h | 3 ++- > > drivers/pci/setup-bus.c | 25 +++ > > 4 files changed, 38 insertions(+), 16 deletions(-) > > If you want to add cover letter in the "normal way" so that threading is > preserved you can do something like 'git format-patch --cover-letter ...', > then edit the -...patch and send it along with the other patches I do everything above. > using git send-email. But I was told by Bjorn that I was sending emails with funny encoding which meant he had to manually apply the patches, which meant more work for him. I could not figure out how to make git send-email work with the correct encoding. I had to use mutt -H to fix the encoding, doing each email separately. I have no idea why my git send-email is strange.
[PATCH v2 1/1] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
Add kernel parameter pci=hpmmiosize=nn[KMG] to set MMIO bridge window
size for hotplug bridges.
Add kernel parameter pci=hpmmioprefsize=nn[KMG] to set MMIO_PREF bridge
window size for hotplug bridges.
Leave pci=hpmemsize=nn[KMG] unchanged, to prevent disruptions to
existing users. This sets both MMIO and MMIO_PREF to the same size.
The two new parameters conform to the style of pci=hpiosize=nn[KMG].
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 9 ++-
drivers/pci/pci.c | 13 +++---
drivers/pci/pci.h | 3 ++-
drivers/pci/setup-bus.c | 24 ++-
4 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index a84a83f88..cfe8c2b67 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3492,8 +3492,15 @@
hpiosize=nn[KMG]The fixed amount of bus space which is
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
+ hpmmiosize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO window.
+ Default size is 2 megabytes.
+ hpmmioprefsize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO_PREF window.
+ Default size is 2 megabytes.
hpmemsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's memory window.
+ reserved for hotplug bridge's MMIO and
+ MMIO_PREF window.
Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a97e2571a..e0406c663 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,10 +85,12 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6286,8 +6288,13 @@ static int __init pci_setup(char *str)
pcie_ecrc_get_policy(str + 5);
} else if (!strncmp(str, "hpiosize=", 9)) {
pci_hotplug_io_size = memparse(str + 9, &str);
+ } else if (!strncmp(str, "hpmmiosize=", 11)) {
+ pci_hotplug_mmio_size = memparse(str + 11,
&str);
+ } else if (!strncmp(str, "hpmmioprefsize=", 15)) {
+ pci_hotplug_mmio_pref_size = memparse(str + 15,
&str);
} else if (!strncmp(str, "hpmemsize=", 10)) {
- pci_hotplug_mem_size = memparse(str + 10, &str);
+ pci_hotplug_mmio_size = memparse(str + 10,
&str);
+ pci_hotplug_mmio_pref_size =
pci_hotplug_mmio_size;
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
simple_strtoul(str + 10, &str, 0);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 3f6947ee3..9faa55a15 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -218,7 +218,8 @@ extern const struct device_type pci_dev_type;
extern const struct attribute_group *pci_bus_groups[];
extern unsigned long pci_hotplug_io_size;
-extern unsigned long pci_hotplug_mem_size;
+extern unsigned long pci_hotplug_mmio_size;
+extern unsigned long pci_hotplug_mmio_pref_size;
extern unsigned long pci_hotplug_bus_size;
/**
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index e7dbe2170..93fd2070a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1178,7 +1178,8 @@ void __pci_bus_size_bridges(str
[PATCH v2 0/1] Add support for setting MMIO PREF hotplug bridge size
Since the first revision of this patch: Ignoring 80-character line limit based on the advice of Mika Westerberg. Mika noticed that memparse is modifying the str in pci_setup. Looking at the definition in lib/cmdline.c line 125, he is probably correct. I have no idea how this did not cause problems in testing. Fixed the alignment of some overflow lines. It turns out Outlook is causing my encoding issues with git send-email. If I get a new email for kernel development, what should it be? Gmail works, but looks tackier. Nicholas Johnson (1): PCI: Add hp_mmio_size and hp_mmio_pref_size parameters .../admin-guide/kernel-parameters.txt | 9 ++- drivers/pci/pci.c | 13 +++--- drivers/pci/pci.h | 3 ++- drivers/pci/setup-bus.c | 24 ++- 4 files changed, 33 insertions(+), 16 deletions(-) -- 2.23.0
Re: [PATCH 0/1] Fiji GPU audio register timeout when in BACO state
On Tue, Apr 28, 2020 at 09:57:24AM +0200, Takashi Iwai wrote:
> On Mon, 27 Apr 2020 20:43:54 +0200,
> Alex Deucher wrote:
> >
> > On Mon, Apr 27, 2020 at 2:39 PM Takashi Iwai wrote:
> > >
> > > On Mon, 27 Apr 2020 20:28:12 +0200,
> > > Alex Deucher wrote:
> > > >
> > > > On Mon, Apr 27, 2020 at 2:07 PM Nicholas Johnson
> > > > wrote:
> > > > >
> > > > > On Mon, Apr 27, 2020 at 05:15:55PM +0200, Takashi Iwai wrote:
> > > > > > On Mon, 27 Apr 2020 16:22:21 +0200,
> > > > > > Deucher, Alexander wrote:
> > > > > > >
> > > > > > > [AMD Public Use]
> > > > > > >
> > > > > > > > -Original Message-
> > > > > > > > From: Nicholas Johnson
> > > > > > > >
> > > > > > > > Sent: Sunday, April 26, 2020 12:02 PM
> > > > > > > > To: linux-kernel@vger.kernel.org
> > > > > > > > Cc: Deucher, Alexander ; Koenig,
> > > > > > > > Christian
> > > > > > > > ; Zhou, David(ChunMing)
> > > > > > > > ; Nicholas Johnson > > > > > > > opensou...@outlook.com.au>
> > > > > > > > Subject: [PATCH 0/1] Fiji GPU audio register timeout when in
> > > > > > > > BACO state
> > > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > Since Linux v5.7-rc1 / commit 4fdda2e66de0 ("drm/amdgpu/runpm:
> > > > > > > > enable
> > > > > > > > runpm on baco capable VI+ asics"), my AMD R9 Nano has been
> > > > > > > > using runpm /
> > > > > > > > BACO. You can tell visually when it sleeps, because the fan on
> > > > > > > > the graphics
> > > > > > > > card is switched off to save power. It did not spin down the
> > > > > > > > fan in v5.6.x.
> > > > > > > >
> > > > > > > > This is great (I love it), except that when it is sleeping, the
> > > > > > > > PCIe audio function
> > > > > > > > of the GPU has issues if anything tries to access it. You get
> > > > > > > > dmesg errors such
> > > > > > > > as these:
> > > > > > > >
> > > > > > > > snd_hda_intel :08:00.1: spurious response 0x0:0x0, last
> > > > > > > > cmd=0x170500
> > > > > > > > snd_hda_intel :08:00.1: azx_get_response timeout, switching
> > > > > > > > to polling
> > > > > > > > mode: last cmd=0x001f0500 snd_hda_intel :08:00.1: No
> > > > > > > > response from
> > > > > > > > codec, disabling MSI: last cmd=0x001f0500 snd_hda_intel
> > > > > > > > :08:00.1: No
> > > > > > > > response from codec, resetting bus: last cmd=0x001f0500
> > > > > > > > snd_hda_codec_hdmi hdaudioC1D0: Unable to sync register
> > > > > > > > 0x2f0d00. -11
> > > > > > > >
> > > > > > > > The above is with the Fiji XT GPU at :08:00.0 in a
> > > > > > > > Thunderbolt enclosure
> > > > > > > > (not that Thunderbolt should affect it, but I feel I should
> > > > > > > > mention it just in
> > > > > > > > case). I dropped a lot of duplicate dmesg lines, as some of
> > > > > > > > them repeated a
> > > > > > > > lot of times before the driver gave up.
> > > > > > > >
> > > > > > > > I offer this patch to disable runpm for Fiji while a fix is
> > > > > > > > found, if you decide
> > > > > > > > that is the best approach. Regardless, I will gladly test any
> > > > > > > > patches you come
> > > > > > > > up with instead and confirm that the above issue has been fixed.
> > > > > > > >
> > > > > > > > I cannot tell if any other GPUs are affected. The only other
> > > > > > > > cards to which I
> > > > > > > > have access are a couple of AMD R9 280X (Tahiti XT), w
[PATCH] PCI: Consider alignment of hot-added bridges when distributing available resources
lt dock can consume 1/4 of
the total IO space (16-bit, 0x) is evidence that the depreciated IO
bars need to be dropped from the kernel at some point. The docks have
four bridges each, with 4096-byte alignment. The IO BARs do not do
anything, crash the system if not claimed and spam dmesg when not
assigned.
The following bug report is solved by this patch, according to Mika
Westerberg:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=199581
Tested-by: Mika Westerberg
Signed-off-by: Nicholas-Johnson-opensource
---
drivers/pci/setup-bus.c | 172
1 file changed, 88 insertions(+), 84 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ed960436df5e..a8be1a90ff28 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1866,20 +1866,21 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
if (!dev_res)
return;
- /* Is there room to extend the window? */
- if (available - resource_size(res) <= dev_res->add_size)
- return;
-
+ /*
+* The add_size can be allowed to be shrunk, which allows for resources
+* to be distributed when allocated by "pci=hpmemsize=nnM", without
+* affecting the distribution of firmware-allocated resources.
+*/
dev_res->add_size = available - resource_size(res);
pci_dbg(bridge, "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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1889,25 +1890,32 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window. A single bridge
+* might not be able to occupy the whole parent region if the alignment
+* differs - for example, an external GPU at the end of a Thunderbolt
+* daisy chain.
*/
- 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);
+ align = pci_resource_alignment(bridge, io_res);
+ if (!io_res->parent && align)
+ io.start = ALIGN(io.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_res);
+ if (!mmio_res->parent && align)
+ mmio.start = ALIGN(mmio.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_pref_res);
+ if (!mmio_pref_res->parent && align)
+ mmio_pref.start = ALIGN(mmio_pref.start, align);
/*
-* Calculate the total amount of extra resource space we can
-* pass to bridges below this one. This is basically the
-* extra space reduced by the minimal required space for the
-* non-hotplug bridges.
+* Update the resources to fill as much remaining resource space in the
+* parent bridge as possible, while considering alignment.
*/
- remaining_io = available_io;
- remaining_mmio = available_mmio;
- remaining_mmio_pref = available_mmio_pref;
+ extend_bridge_window(bridge, io_res, add_list, resource_size(&io));
+ extend_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
+ extend_bridge_window(bridge, mmio_pref_res, add_list,
+ resource_size(&mmio_pref));
/*
* Calculate how many hotplug bridges and normal bridges there
@@ -1921,80 +1929,79 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
normal_bridges++;
}
+ /*
+* There is only one bridge on the bus so it gets all possible
+* resources which it can then distribute to the possible
+*
[PATCH] PCI: Consider alignment of hot-added bridges when distributing available resources
my other host card which
happens to be in SL0 mode. This means that devices are discovered much
more quickly. I noticed that multiple devices can be enumerated
together, rather than each getting enumerated before the next appears.
It turns out that this can break the allocation, but I have absolutely
no idea why. I have modified the patch to solve this problem. Before,
extend_bridge_window() used add_size to change the resource size. Now it
simply changes the size of the actual resource, and clears the add_size
to zero if a list entry exists. That solves the issue, and proves that
the calculated resource sizes are not at fault (the algorithm is sound).
Hence, I recommend this version with the modification be applied.
I have removed Mika Westerberg's "Tested-By" line to allow him to give
his approval for this new change.
Observation: the fact that a single Thunderbolt dock can consume 1/4 of
the total IO space (16-bit, 0x) is evidence that the depreciated IO
bars need to be dropped from the kernel at some point. The docks have
four bridges each, with 4096-byte alignment. The IO BARs do not do
anything, crash the system if not claimed and spam dmesg when not
assigned.
Signed-off-by: Nicholas-Johnson-opensource
---
drivers/pci/setup-bus.c | 188 +---
1 file changed, 99 insertions(+), 89 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ed960436df5e..09310b6fcdb3 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1859,27 +1859,34 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
if (res->parent)
return;
- if (resource_size(res) >= available)
- return;
+ /*
+* Hot-adding multiple Thunderbolt devices in SL0 might result in
+* multiple devices being enumerated together. This can break the
+* resource allocation if the resource sizes are specified with
+* add_size instead of simply changing the resource size.
+ */
+ pci_dbg(bridge, "bridge window %pR extended by 0x%016llx\n", res,
+ available - resource_size(res));
+ res->end = res->start + available - 1;
+ /*
+* If a list entry exists, we need to remove any additional size
+* requested because that could interfere with the alignment and
+* sizing done when distributing resources, causing resources to
+* fail to allocate later on.
+*/
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);
- pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
- &dev_res->add_size);
+ dev_res->add_size = 0;
}
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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1889,25 +1896,32 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window. A single bridge
+* might not be able to occupy the whole parent region if the alignment
+* differs - for example, an external GPU at the end of a Thunderbolt
+* daisy chain.
*/
- 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);
+ align = pci_resource_alignment(bridge, io_res);
+ if (!io_res->parent && align)
+ io.start = ALIGN(io.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_res);
+ if (!mmio_res->parent &
[PATCH v6 0/4] PCI: Patch series to support Thunderbolt without any BIOS support
Rebase patches to apply cleanly to 5.2-rc1 source. Remove patch for comment style cleanup as this has already been applied. Anybody interested in testing, you can do so with: a) Intel system with Thunderbolt 3 and native enumeration. The Gigabyte Z390 Designare is one of the most perfect for this that I have never had the opportunity to use - it does not even have the option for BIOS assisted enumeration present in the BIOS. b) Any system with PCIe and the Gigabyte GC-TITAN RIDGE add-in card, jump the header as described and use kernel parameters like: pci=assign-busses,hpbussize=0x33,realloc,hpmemsize=128M,hpmemprefsize=1G,nocrs pcie_ports=native [optional] pci.dyndbg ___ __/ \__ |o o o o o| When looking into the receptacle on back of PCIe card. |_| Jump pins 3 and 5. 1 2 3 4 5 The Intel system is nice in that it should just work. The add-in card setup is nice in that you can go nuts and assign copious amounts of MMIO_PREF - can anybody show a Xeon Phi coprocessor with 16G BAR working in an eGPU enclosure with these patches? However, if you specify the above kernel parameters on the Intel system, you should be able to override it to allocate more space. Nicholas Johnson (4): PCI: Consider alignment of hot-added bridges when distributing available resources PCI: Modify extend_bridge_window() to set resource size directly PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently .../admin-guide/kernel-parameters.txt | 7 +- drivers/pci/pci.c | 18 +- drivers/pci/setup-bus.c | 265 ++ include/linux/pci.h | 3 +- 4 files changed, 167 insertions(+), 126 deletions(-) -- 2.20.1
[PATCH v6 1/4] PCI: Consider alignment of hot-added bridges when distributing available resources
Rewrite pci_bus_distribute_available_resources to better handle bridges
with different resource alignment requirements. Pass more details
arguments recursively to track the resource start and end addresses
relative to the initial hotplug bridge. This is especially useful for
Thunderbolt with native PCI enumeration, enabling external graphics
cards and other devices with bridge alignment higher than 0x10
bytes.
Change extend_bridge_window to resize the actual resource, rather than
using add_list and dev_res->add_size. If an additional resource entry
exists for the given resource, zero out the add_size field to avoid it
interfering. Because add_size is considered optional when allocating,
using add_size could cause issues in some cases, because successful
resource distribution requires sizes to be guaranteed. Such cases
include hot-adding nested hotplug bridges in one enumeration, and
potentially others which are yet to be encountered.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 169
1 file changed, 84 insertions(+), 85 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 0cdd5ff38..1b5b851ca 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1835,12 +1835,10 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
}
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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1850,29 +1848,36 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window. A single bridge
+* might not be able to occupy the whole parent region if the alignment
+* differs - for example, an external GPU at the end of a Thunderbolt
+* daisy chain.
*/
- 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);
+ align = pci_resource_alignment(bridge, io_res);
+ if (!io_res->parent && align)
+ io.start = ALIGN(io.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_res);
+ if (!mmio_res->parent && align)
+ mmio.start = ALIGN(mmio.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_pref_res);
+ if (!mmio_pref_res->parent && align)
+ mmio_pref.start = ALIGN(mmio_pref.start, align);
/*
-* Calculate the total amount of extra resource space we can
-* pass to bridges below this one. This is basically the
-* extra space reduced by the minimal required space for the
-* non-hotplug bridges.
+* Update the resources to fill as much remaining resource space in the
+* parent bridge as possible, while considering alignment.
*/
- remaining_io = available_io;
- remaining_mmio = available_mmio;
- remaining_mmio_pref = available_mmio_pref;
+ extend_bridge_window(bridge, io_res, add_list, resource_size(&io));
+ extend_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
+ extend_bridge_window(bridge, mmio_pref_res, add_list,
+ resource_size(&mmio_pref));
/*
* Calculate how many hotplug bridges and normal bridges there
-* are on this bus. We will distribute the additional available
+* are on this bus. We will distribute the additional available
* resources between hotplug bridges.
*/
for_each_pci_bridge(dev, bus) {
@@ -1882,104 +1887,98 @@ static void
pci_bus_distribute_available_resources(s
[PATCH v6 2/4] PCI: Modify extend_bridge_window() to set resource size directly
Background
==
In the current state, the PCI allocation could fail with Thunderbolt
under certain unusual circumstances, because add_list resources are
"optional". Guaranteed allocation requires guaranteed resource sizes.
It is difficult to give examples of these failures - because without the
previous patch in the series, the symptoms of the problem are hidden by
larger problems. This patch has been split from the previous patch and
makes little sense on its own - as it is almost impossible to see the
effect of this patch without first fixing the problems addressed by the
previous patch. So the evidence I put forward for making this change is
that because add_list resources are "optional", there could be any
number of unforeseen bugs that are yet to be encountered if the kernel
decides not to assign all of the optional size. In kernel development,
we should not play around with chance.
Moving away from add_size also allows for use of pci=hpmemsize to assign
resources. Previously, when using add_size and not allowing the add_size
to shrink, it made it impossible to distribute resources. If a hotplug
bridge has size X, and below it is some devices with non-zero size Y and
a nested hotplug bridge of same size X, fitting X+Y into size X is
mathematically impossible.
This patch solves this by dropping add_size and giving each bridge the
maximum size possible without failing resource assignment. Using
pci=hpmemsize still works as pci_assign_unassigned_root_bus_resources()
does not call pci_bus_distribute_available_resources(). At boot,
pci_assign_unassigned_root_bus_resources() is used, instead of
pci_bridge_distribute_available_resources().
By allowing to use pci=hpmemsize, it removes the reliance on the
firmware to declare the window resources under the root port, and could
pay off in the future with USB4 (which is backward-compatible to
Thunderbolt devices, and not specific to Intel systems). Users of
Thunderbolt hardware on unsupported systems will be able to specify the
resources in the kernel parameters. Users of official systems will be
able to override the default firmware window sizes to allocate much
larger resource sizes, potentially enabling Thunderbolt support for
devices with massive BARs (with a few other problems solved by later
patches in this series).
Patch notes
==
Modify extend_bridge_window() to remove the resource from add_list and
change the resource size directly.
Modify extend_bridge_window() to reset resources that are being assigned
zero size. This is required to prevent the bridge not being enabled due
to resources with zero size. This is a direct requirement to prevent the
change away from using add_list from introducing a regression - because
before, it was not possible to end up with zero size.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 42 ++---
1 file changed, 27 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 1b5b851ca..5675254fa 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1810,28 +1810,40 @@ 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 list_head *add_list, resource_size_t new_size)
{
- struct pci_dev_resource *dev_res;
+ resource_size_t add_size;
if (res->parent)
return;
- if (resource_size(res) >= available)
- return;
-
- dev_res = res_to_dev_res(add_list, res);
- if (!dev_res)
- return;
+ if (new_size >= resource_size(res)) {
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
+ &add_size);
+ } else {
+ add_size = resource_size(res) - new_size;
+ pci_dbg(bridge, "bridge window %pR shrunken by %pa\n", res,
+ &add_size);
+ }
- /* Is there room to extend the window? */
- if (available - resource_size(res) <= dev_res->add_size)
- return;
+ /*
+* Resources requested using add_size in additional resource lists are
+* considered optional when allocated. Guaranteed size of allocation
+* is required to guarantee successful resource distribution. Hence,
+* the size of the actual resource must be adjusted, and the resource
+* removed from add_list to prevent any additional size interfering.
+*/
+ res->end = res->start + new_size - 1;
+ rem
[PATCH v6 3/4] PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window
Background
==
Solve bug report:
https://bugzilla.kernel.org/show_bug.cgi?id=203243
Currently, the kernel can sometimes assign the MMIO_PREF window
additional size into the MMIO window, resulting in double the MMIO
additional size, even if the MMIO_PREF window was successful.
This happens if in the first pass, the MMIO_PREF succeeds but the MMIO
fails. In the next pass, because MMIO_PREF is already assigned, the
attempt to assign MMIO_PREF returns an error code instead of success
(nothing more to do, already allocated).
Example of problem (more context can be found in the bug report URL):
Mainline kernel:
pci :06:01.0: BAR 14: assigned [mem 0x9010-0xa00f] = 256M
pci :06:04.0: BAR 14: assigned [mem 0xa020-0xb01f] = 256M
Patched kernel:
pci :06:01.0: BAR 14: assigned [mem 0x9010-0x980f] = 128M
pci :06:04.0: BAR 14: assigned [mem 0x9820-0xa01f] = 128M
This was using pci=realloc,hpmemsize=128M,nocrs - on the same machine
with the same configuration, with a Ubuntu mainline kernel and a kernel
patched with this patch series.
This patch is vital for the next patch in the series. The next patch
allows the user to specify MMIO and MMIO_PREF independently. If the
MMIO_PREF is set to be very large, this bug will end up more than
doubling the MMIO size. The bug results in the MMIO_PREF being added to
the MMIO window, which means doubling if MMIO_PREF size == MMIO size.
With a large MMIO_PREF, without this patch, the MMIO window will likely
fail to be assigned altogether due to lack of 32-bit address space.
Patch notes
==
Change find_free_bus_resource() to not skip assigned resources with
non-null parent.
Add checks in pbus_size_io() and pbus_size_mem() to return success if
resource returned from find_free_bus_resource() is already allocated.
This avoids pbus_size_io() and pbus_size_mem() returning error code to
__pci_bus_size_bridges() when a resource has been successfully assigned
in a previous pass. This fixes the existing behaviour where space for a
resource could be reserved multiple times in different parent bridge
windows. This also greatly reduces the number of failed BAR messages in
dmesg when Linux assigns resources.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 5675254fa..8e1bc7ee7 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -752,13 +752,18 @@ static void pci_bridge_check_ranges(struct pci_bus *bus)
}
/*
- * Helper function for sizing routines: find first available bus resource
- * of a given type. Note: we intentionally skip the bus resources which
- * have already been assigned (that is, have non-NULL parent resource).
+ * Helper function for sizing routines: find first bus resource of a given
+ * type. Note: we do not skip the bus resources which have already been
+ * assigned (r->parent != NULL). This is because a resource that is already
+ * assigned (nothing more to be done) will be indistinguishable from one that
+ * failed due to lack of space if we skip assigned resources. If the caller
+ * function cannot tell the difference then it might try to place the
+ * resources in a different window, doubling up on resources or causing
+ * unforeseeable issues.
*/
-static struct resource *find_free_bus_resource(struct pci_bus *bus,
- unsigned long type_mask,
- unsigned long type)
+struct resource *find_bus_resource_of_type(struct pci_bus *bus,
+ unsigned long type_mask,
+ unsigned long type)
{
int i;
struct resource *r;
@@ -766,7 +771,7 @@ static struct resource *find_free_bus_resource(struct
pci_bus *bus,
pci_bus_for_each_resource(bus, r, i) {
if (r == &ioport_resource || r == &iomem_resource)
continue;
- if (r && (r->flags & type_mask) == type && !r->parent)
+ if (r && (r->flags & type_mask) == type)
return r;
}
return NULL;
@@ -866,14 +871,16 @@ static void pbus_size_io(struct pci_bus *bus,
resource_size_t min_size,
struct list_head *realloc_head)
{
struct pci_dev *dev;
- struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
- IORESOURCE_IO);
+ struct resource *b_res = find_bus_resource_of_type(bus, IORESOURCE_IO,
+ IORESOURCE_IO);
resource_size_t size = 0, size0 = 0, size
[PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently
Add kernel parameter pci=hpmemprefsize=nn[KMG] to control MMIO_PREF size
for PCI hotplug bridges.
Change behaviour of pci=hpmemsize=nn[KMG] to not set MMIO_PREF size if
hpmempref has been specified, rather than controlling both MMIO and
MMIO_PREF sizes unconditionally.
Update kernel-parameters documentation to reflect the above changes.
The effect of the above changes is to allow for MMIO and MMIO_PREF to be
specified independently, whilst ensuring no changes in functionality are
noticed by the user if updating kernel version with hpmemsize specified.
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 7 +-
drivers/pci/pci.c | 18 ++---
drivers/pci/setup-bus.c | 25 +++
include/linux/pci.h | 3 ++-
4 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b..fdecff06c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3439,7 +3439,12 @@
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
hpmemsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's memory window.
+ reserved for hotplug bridge's MMIO window. If
+ hpmemprefsize is not specified, then the same
+ size is applied to hotplug bridge's MMIO_PREF
+ window. Default size is 2 megabytes.
+ hpmemprefsize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO_PREF window.
Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8abc843b1..0e6292009 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,10 +85,12 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6198,6 +6200,8 @@ EXPORT_SYMBOL(pci_fixup_cardbus);
static int __init pci_setup(char *str)
{
+ bool mmio_pref_specified = false;
+
while (str) {
char *k = strchr(str, ',');
if (k)
@@ -6232,7 +6236,15 @@ static int __init pci_setup(char *str)
} else if (!strncmp(str, "hpiosize=", 9)) {
pci_hotplug_io_size = memparse(str + 9, &str);
} else if (!strncmp(str, "hpmemsize=", 10)) {
- pci_hotplug_mem_size = memparse(str + 10, &str);
+ pci_hotplug_mmio_size =
+ memparse(str + 10, &str);
+ if (!mmio_pref_specified)
+ pci_hotplug_mmio_pref_size =
+ pci_hotplug_mmio_size;
+ } else if (!strncmp(str, "hpmemprefsize=", 14)) {
+ mmio_pref_specified = true;
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 14, &str);
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
simple_strtoul(str + 10, &str, 0);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 8e1bc7ee7..44c37dda9 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1187,7 +1187,8 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
{
struct pci_dev *dev;
unsigned long mask, prefmask, type2 = 0, type3 = 0;
- resource_size_t additional_mem_size = 0, additional_io_size = 0;
+ resource_size_t
[PATCH v5 1/5] PCI: Consider alignment of hot-added bridges when distributing resources
This patch solves the following bug report:
https://bugzilla.kernel.org/show_bug.cgi?id=199581
An excerpt from the bug report:
==
Here is what happens when an Intel Gigabit ET2 quad port server adapter
is hot-added:
pci :39:00.0: BAR 14: assigned [mem 0x5330-0x6a0f]
^^
pci :3a:01.0: BAR 14: assigned [mem 0x5340-0x547f]
^^
The above shows that the downstream bridge (3a:01.0) window is aligned
to 2 MB instead of 1 MB as is the upstream bridge (39:00.0) window. The
remaining MMIO space (0x15a0) is assigned to the hotplug bridge
(3a:04.0) but it fails:
pci :3a:04.0: BAR 14: no space for [mem size 0x15a0]
pci :3a:04.0: BAR 14: failed to assign [mem size 0x15a0]
==
Rewrite pci_bus_distribute_available_resources() to better handle
bridges with different resource alignment requirements. Pass more
details arguments recursively to track the resource start and end
addresses relative to the initial hotplug bridge. This is especially
useful for Thunderbolt with native PCI enumeration, enabling external
graphics cards and other devices with bridge alignment higher than
0x10 bytes.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 164
1 file changed, 83 insertions(+), 81 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ec44a0f3a..dae4bae12 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1839,10 +1839,10 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
}
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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1852,25 +1852,32 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window. A single bridge
+* might not be able to occupy the whole parent region if the alignment
+* differs - for example, an external GPU at the end of a Thunderbolt
+* daisy chain.
*/
- 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);
+ align = pci_resource_alignment(bridge, io_res);
+ if (!io_res->parent && align)
+ io.start = ALIGN(io.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_res);
+ if (!mmio_res->parent && align)
+ mmio.start = ALIGN(mmio.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_pref_res);
+ if (!mmio_pref_res->parent && align)
+ mmio_pref.start = ALIGN(mmio_pref.start, align);
/*
-* Calculate the total amount of extra resource space we can
-* pass to bridges below this one. This is basically the
-* extra space reduced by the minimal required space for the
-* non-hotplug bridges.
+* Update the resources to fill as much remaining resource space in the
+* parent bridge as possible, while considering alignment.
*/
- remaining_io = available_io;
- remaining_mmio = available_mmio;
- remaining_mmio_pref = available_mmio_pref;
+ extend_bridge_window(bridge, io_res, add_list, resource_size(&io));
+ extend_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
+ extend_bridge_window(bridge, mmio_pref_res, add_list,
+ resource_size(&mmio_pref));
/*
* Calculate how many hotplug bridges and normal bridges there
@@ -1884,80 +1891,79 @@ static void
pci
[PATCH v5 5/5] PCI: Cleanup block comments in setup-bus.c to match kernel style
Change block comments to accepted style with asterisks on each line.
Justify block comments to 80-character limit to reduce the number of
lines where possible.
Change beginnings of sentences to have capital letter.
Place periods at the ends of sentences that are only separated by
newlines.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 310 +++-
1 file changed, 151 insertions(+), 159 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 4386ca941..400cf616c 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -51,11 +51,9 @@ static void free_list(struct list_head *head)
/**
* add_to_list() - add a new resource tracker to the list
* @head: Head of the list
- * @dev: device corresponding to which the resource
- * belongs
+ * @dev: device corresponding to which the resource belongs
* @res: The resource to be tracked
- * @add_size: additional size to be optionally added
- * to the resource
+ * @add_size: additional size to be optionally added to the resource
*/
static int add_to_list(struct list_head *head,
struct pci_dev *dev, struct resource *res,
@@ -158,7 +156,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct
list_head *head)
tmp->res = r;
tmp->dev = dev;
- /* fallback is smallest one or list is empty*/
+ /* fallback is smallest one or list is empty */
n = head;
list_for_each_entry(dev_res, head, list) {
resource_size_t align;
@@ -171,7 +169,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct
list_head *head)
break;
}
}
- /* Insert it just before n*/
+ /* Insert it just before n */
list_add_tail(&tmp->list, n);
}
}
@@ -181,7 +179,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
{
u16 class = dev->class >> 8;
- /* Don't touch classless devices or host bridges or ioapics. */
+ /* Don't touch classless devices or host bridges or ioapics */
if (class == PCI_CLASS_NOT_DEFINED || class == PCI_CLASS_BRIDGE_HOST)
return;
@@ -208,12 +206,10 @@ static inline void reset_resource(struct resource *res)
*
* @realloc_head : head of the list tracking requests requiring additional
* resources
- * @head : head of the list tracking requests with allocated
- * resources
+ * @head : head of the list tracking requests with allocated resources
*
- * Walk through each element of the realloc_head and try to procure
- * additional resources for the element, provided the element
- * is in the head list.
+ * Walk through each element of the realloc_head and try to procure additional
+ * resources for the element, provided the element is in the head list.
*/
static void reassign_resources_sorted(struct list_head *realloc_head,
struct list_head *head)
@@ -239,7 +235,7 @@ static void reassign_resources_sorted(struct list_head
*realloc_head,
break;
}
}
- if (!found_match)/* just skip */
+ if (!found_match) /* just skip */
continue;
idx = res - &add_res->dev->resource[0];
@@ -310,15 +306,14 @@ static unsigned long pci_fail_res_type_mask(struct
list_head *fail_head)
struct pci_dev_resource *fail_res;
unsigned long mask = 0;
- /* check failed type */
+ /* Check failed type */
list_for_each_entry(fail_res, fail_head, list)
mask |= fail_res->flags;
/*
-* one pref failed resource will set IORESOURCE_MEM,
-* as we can allocate pref in non-pref range.
-* Will release all assigned non-pref sibling resources
-* according to that bit.
+* One pref failed resource will set IORESOURCE_MEM, as we can allocate
+* pref in non-pref range. Will release all assigned non-pref sibling
+* resources according to that bit.
*/
return mask & (IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH);
}
@@ -328,11 +323,11 @@ static bool pci_need_to_release(unsigned long mask,
struct resource *res)
if (res->flags & IORESOURCE_IO)
return !!(mask & IORESOURCE_IO);
- /* check pref at first */
+ /* Check pref at first */
if (res->flags & IORESOURCE_PREFETCH) {
if (mask & IORESOURCE_PREFETCH)
return true;
- /* count pref if its parent is non-pref */
+ /* Count pref if its parent is non-pref */
else if ((mask & IORESOURCE_
[PATCH v5 4/5] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently
Add kernel parameter pci=hpmemprefsize=nn[KMG] to control MMIO_PREF size
for PCI hotplug bridges.
Change behaviour of pci=hpmemsize=nn[KMG] to not set MMIO_PREF size if
hpmempref has been specified, rather than controlling both MMIO and
MMIO_PREF sizes unconditionally.
Update kernel-parameters documentation to reflect the above changes.
The effect of the above changes is to allow for MMIO and MMIO_PREF to be
specified independently, whilst ensuring no changes in functionality are
noticed by the user if updating kernel version with hpmemsize specified.
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 7 +-
drivers/pci/pci.c | 18 ++---
drivers/pci/setup-bus.c | 25 +++
include/linux/pci.h | 3 ++-
4 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 2b8ee90bb..400407c27 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3361,7 +3361,12 @@
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
hpmemsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's memory window.
+ reserved for hotplug bridge's MMIO window. If
+ hpmemprefsize is not specified, then the same
+ size is applied to hotplug bridge's MMIO_PREF
+ window. Default size is 2 megabytes.
+ hpmemprefsize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO_PREF window.
Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7c1b362f5..7c7b95aab 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,10 +85,12 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6211,6 +6213,8 @@ EXPORT_SYMBOL(pci_fixup_cardbus);
static int __init pci_setup(char *str)
{
+ bool mmio_pref_specified = false;
+
while (str) {
char *k = strchr(str, ',');
if (k)
@@ -6245,7 +6249,15 @@ static int __init pci_setup(char *str)
} else if (!strncmp(str, "hpiosize=", 9)) {
pci_hotplug_io_size = memparse(str + 9, &str);
} else if (!strncmp(str, "hpmemsize=", 10)) {
- pci_hotplug_mem_size = memparse(str + 10, &str);
+ pci_hotplug_mmio_size =
+ memparse(str + 10, &str);
+ if (!mmio_pref_specified)
+ pci_hotplug_mmio_pref_size =
+ pci_hotplug_mmio_size;
+ } else if (!strncmp(str, "hpmemprefsize=", 14)) {
+ mmio_pref_specified = true;
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 14, &str);
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
simple_strtoul(str + 10, &str, 0);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index e7126cc0e..4386ca941 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1187,7 +1187,8 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
{
struct pci_dev *dev;
unsigned long mask, prefmask, type2 = 0, type3 = 0;
- resource_size_t additional_mem_size = 0, additional_io_size = 0;
+ resource_size_t
[PATCH v5 0/5] PCI: Patch series to support Thunderbolt without any BIOS support
Since PATCH v4: I have added some of the evidence and bug reports into the applicable patches. Users of pci=hpmemsize should not notice any changes in functionality with this patch series when upgrading the kernel. I realised I could make the variable to achieve this reside in pci_setup, rather than globally. Please let me know if anything else needs changing. Nicholas Johnson (5): PCI: Consider alignment of hot-added bridges when distributing resources PCI: Modify extend_bridge_window() to set resource size directly PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently PCI: Cleanup block comments in setup-bus.c to match kernel style .../admin-guide/kernel-parameters.txt | 7 +- drivers/pci/pci.c | 18 +- drivers/pci/setup-bus.c | 568 +- include/linux/pci.h | 3 +- 4 files changed, 317 insertions(+), 279 deletions(-) -- 2.19.1
[PATCH v5 3/5] PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window
Background
==
Solve bug report:
https://bugzilla.kernel.org/show_bug.cgi?id=203243
Currently, the kernel can sometimes assign the MMIO_PREF window
additional size into the MMIO window, resulting in double the MMIO
additional size, even if the MMIO_PREF window was successful.
This happens if in the first pass, the MMIO_PREF succeeds but the MMIO
fails. In the next pass, because MMIO_PREF is already assigned, the
attempt to assign MMIO_PREF returns an error code instead of success
(nothing more to do, already allocated).
Example of problem (more context can be found in the bug report URL):
Mainline kernel:
pci :06:01.0: BAR 14: assigned [mem 0x9010-0xa00f] = 256M
pci :06:04.0: BAR 14: assigned [mem 0xa020-0xb01f] = 256M
Patched kernel:
pci :06:01.0: BAR 14: assigned [mem 0x9010-0x980f] = 128M
pci :06:04.0: BAR 14: assigned [mem 0x9820-0xa01f] = 128M
This was using pci=realloc,hpmemsize=128M,nocrs - on the same machine
with the same configuration, with a Ubuntu mainline kernel and a kernel
patched with this patch series.
This patch is vital for the next patch in the series. The next patch
allows the user to specify MMIO and MMIO_PREF independently. If the
MMIO_PREF is set to be very large, this bug will end up more than
doubling the MMIO size. The bug results in the MMIO_PREF being added to
the MMIO window, which means doubling if MMIO_PREF size == MMIO size.
With a large MMIO_PREF, without this patch, the MMIO window will likely
fail to be assigned altogether due to lack of 32-bit address space.
Patch notes
==
Change find_free_bus_resource() to not skip assigned resources with
non-null parent.
Add checks in pbus_size_io() and pbus_size_mem() to return success if
resource returned from find_free_bus_resource() is already allocated.
This avoids pbus_size_io() and pbus_size_mem() returning error code to
__pci_bus_size_bridges() when a resource has been successfully assigned
in a previous pass. This fixes the existing behaviour where space for a
resource could be reserved multiple times in different parent bridge
windows. This also greatly reduces the number of failed BAR messages in
dmesg when Linux assigns resources.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 28 +++-
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 5214815c7..e7126cc0e 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -752,11 +752,17 @@ static void pci_bridge_check_ranges(struct pci_bus *bus)
}
}
-/* Helper function for sizing routines: find first available
- bus resource of a given type. Note: we intentionally skip
- the bus resources which have already been assigned (that is,
- have non-NULL parent resource). */
-static struct resource *find_free_bus_resource(struct pci_bus *bus,
+/*
+ * Helper function for sizing routines: find first bus resource of a given
+ * type. Note: we do not skip the bus resources which have already been
+ * assigned (r->parent != NULL). This is because a resource that is already
+ * assigned (nothing more to be done) will be indistinguishable from one that
+ * failed due to lack of space if we skip assigned resources. If the caller
+ * function cannot tell the difference then it might try to place the
+ * resources in a different window, doubling up on resources or causing
+ * unforeseeable issues.
+ */
+static struct resource *find_bus_resource_of_type(struct pci_bus *bus,
unsigned long type_mask, unsigned long type)
{
int i;
@@ -765,7 +771,7 @@ static struct resource *find_free_bus_resource(struct
pci_bus *bus,
pci_bus_for_each_resource(bus, r, i) {
if (r == &ioport_resource || r == &iomem_resource)
continue;
- if (r && (r->flags & type_mask) == type && !r->parent)
+ if (r && (r->flags & type_mask) == type)
return r;
}
return NULL;
@@ -863,14 +869,16 @@ static void pbus_size_io(struct pci_bus *bus,
resource_size_t min_size,
resource_size_t add_size, struct list_head *realloc_head)
{
struct pci_dev *dev;
- struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
- IORESOURCE_IO);
+ struct resource *b_res = find_bus_resource_of_type(bus, IORESOURCE_IO,
+ IORESOURCE_IO);
resource_size_t size = 0, size0 = 0, size1 = 0;
resource_size_t children_add_size = 0;
resource_size_t min_align, align;
if (!b_res)
return;
+ if (b_res->parent)
+ return;
[PATCH v5 2/5] PCI: Modify extend_bridge_window() to set resource size directly
Background
==
In the current state, the PCI allocation could fail with Thunderbolt
under certain unusual circumstances, because add_list resources are
"optional". Guaranteed allocation requires guaranteed resource sizes.
It is difficult to give examples of these failures - because without the
previous patch in the series, the symptoms of the problem are hidden by
larger problems. This patch has been split from the previous patch and
makes little sense on its own - as it is almost impossible to see the
effect of this patch without first fixing the problems addressed by the
previous patch. So the evidence I put forward for making this change is
that because add_list resources are "optional", there could be any
number of unforeseen bugs that are yet to be encountered if the kernel
decides not to assign all of the optional size. In kernel development,
we should not play around with chance.
Moving away from add_size also allows for use of pci=hpmemsize to assign
resources. Previously, when using add_size and not allowing the add_size
to shrink, it made it impossible to distribute resources. If a hotplug
bridge has size X, and below it is some devices with non-zero size Y and
a nested hotplug bridge of same size X, fitting X+Y into size X is
mathematically impossible.
This patch solves this by dropping add_size and giving each bridge the
maximum size possible without failing resource assignment. Using
pci=hpmemsize still works as pci_assign_unassigned_root_bus_resources()
does not call pci_bus_distribute_available_resources(). At boot,
pci_assign_unassigned_root_bus_resources() is used, instead of
pci_bridge_distribute_available_resources().
By allowing to use pci=hpmemsize, it removes the reliance on the
firmware to declare the window resources under the root port, and could
pay off in the future with USB4 (which is backward-compatible to
Thunderbolt devices, and not specific to Intel systems). Users of
Thunderbolt hardware on unsupported systems will be able to specify the
resources in the kernel parameters. Users of official systems will be
able to override the default firmware window sizes to allocate much
larger resource sizes, potentially enabling Thunderbolt support for
devices with massive BARs (with a few other problems solved by later
patches in this series).
Patch notes
==
Modify extend_bridge_window() to remove the resource from add_list and
change the resource size directly.
Modify extend_bridge_window() to reset resources that are being assigned
zero size. This is required to prevent the bridge not being enabled due
to resources with zero size. This is a direct requirement to prevent the
change away from using add_list from introducing a regression - because
before, it was not possible to end up with zero size.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 41 +++--
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index dae4bae12..5214815c7 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1815,27 +1815,40 @@ 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 list_head *add_list, resource_size_t new_size)
{
- struct pci_dev_resource *dev_res;
+ resource_size_t add_size;
if (res->parent)
return;
- if (resource_size(res) >= available)
- return;
-
- dev_res = res_to_dev_res(add_list, res);
- if (!dev_res)
- return;
+ if (new_size >= resource_size(res)) {
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
+ &add_size);
+ } else {
+ add_size = resource_size(res) - new_size;
+ pci_dbg(bridge, "bridge window %pR shrunken by %pa\n", res,
+ &add_size);
+ }
- /* Is there room to extend the window? */
- if (available - resource_size(res) <= dev_res->add_size)
- return;
+ /*
+* Resources requested using add_size in additional resource lists are
+* considered optional when allocated. Guaranteed size of allocation
+* is required to guarantee successful resource distribution. Hence,
+* the size of the actual resource must be adjusted, and the resource
+* removed from add_list to prevent any additional size interfering.
+*/
+ res->end = res->start + new_size - 1;
+ remove_from_list(add_list, res);
- dev_r
[PATCH v8 0/6] Patch series to support Thunderbolt without any BIOS support
Patch series rebased to 5.3-rc1. If possible, please have a quick read over while I am away (2019-07-27 to mid 2019-08-04), so I can fix any mistakes or make simple changes to get problems out of the way for a more thorough examination later. Thanks! Kind regards, Nicholas Nicholas Johnson (6): PCI: Consider alignment of hot-added bridges when distributing available resources PCI: In extend_bridge_window() change available to new_size PCI: Change extend_bridge_window() to set resource size directly PCI: Allow extend_bridge_window() to shrink resource if necessary PCI: Add hp_mmio_size and hp_mmio_pref_size parameters PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window .../admin-guide/kernel-parameters.txt | 9 +- drivers/pci/pci.c | 17 +- drivers/pci/setup-bus.c | 233 +- include/linux/pci.h | 3 +- 4 files changed, 143 insertions(+), 119 deletions(-) -- 2.22.0
[PATCH v8 2/6] PCI: In extend_bridge_window() change available to new_size
In extend_bridge_window() change "available" parameter name to new_size.
This makes more sense as this parameter represents the new size for the
window.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 6835fd64c..3b3055e0e 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1816,14 +1816,14 @@ 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)
+resource_size_t new_size)
{
struct pci_dev_resource *dev_res;
if (res->parent)
return;
- if (resource_size(res) >= available)
+ if (resource_size(res) >= new_size)
return;
dev_res = res_to_dev_res(add_list, res);
@@ -1831,10 +1831,10 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
return;
/* Is there room to extend the window? */
- if (available - resource_size(res) <= dev_res->add_size)
+ if (new_size - resource_size(res) <= dev_res->add_size)
return;
- dev_res->add_size = available - resource_size(res);
+ dev_res->add_size = new_size - resource_size(res);
pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
&dev_res->add_size);
}
--
2.22.0
[PATCH v8 1/6] PCI: Consider alignment of hot-added bridges when distributing available resources
Rewrite pci_bus_distribute_available_resources to better handle bridges
with different resource alignment requirements. Pass more details
arguments recursively to track the resource start and end addresses
relative to the initial hotplug bridge. This is especially useful for
Thunderbolt with native PCI enumeration, enabling external graphics
cards and other devices with bridge alignment higher than 1MB.
Change extend_bridge_window to resize the actual resource, rather than
using add_list and dev_res->add_size. If an additional resource entry
exists for the given resource, zero out the add_size field to avoid it
interfering. Because add_size is considered optional when allocating,
using add_size could cause issues in some cases, because successful
resource distribution requires sizes to be guaranteed. Such cases
include hot-adding nested hotplug bridges in one enumeration, and
potentially others which are yet to be encountered.
Solves bug report: https://bugzilla.kernel.org/show_bug.cgi?id=199581
Reported-by: Mika Westerberg
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 148 +++-
1 file changed, 71 insertions(+), 77 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 79b1fa651..6835fd64c 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1840,12 +1840,10 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
}
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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1855,15 +1853,29 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window.
*/
- extend_bridge_window(bridge, io_res, add_list, available_io);
- extend_bridge_window(bridge, mmio_res, add_list, available_mmio);
+ align = pci_resource_alignment(bridge, io_res);
+ if (!io_res->parent && align)
+ io.start = ALIGN(io.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_res);
+ if (!mmio_res->parent && align)
+ mmio.start = ALIGN(mmio.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_pref_res);
+ if (!mmio_pref_res->parent && align)
+ mmio_pref.start = ALIGN(mmio_pref.start, align);
+
+ /*
+* Update the resources to fill as much remaining resource space in the
+* parent bridge as possible, while considering alignment.
+*/
+ extend_bridge_window(bridge, io_res, add_list, resource_size(&io));
+ extend_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
extend_bridge_window(bridge, mmio_pref_res, add_list,
-available_mmio_pref);
+ resource_size(&mmio_pref));
/*
* Calculate how many hotplug bridges and normal bridges there
@@ -1884,108 +1896,90 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
*/
if (hotplug_bridges + normal_bridges == 1) {
dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
- if (dev->subordinate) {
+ if (dev->subordinate)
pci_bus_distribute_available_resources(dev->subordinate,
- add_list, available_io, available_mmio,
- available_mmio_pref);
- }
+ add_list, io, mmio, mmio_pref);
return;
}
- if (hotplug_bridges == 0)
- return;
-
/*
-* Calculate the total amount of extra resource space we can
-* pass to bridges below this one. This is basicall
[PATCH v8 3/6] PCI: Change extend_bridge_window() to set resource size directly
Change extend_bridge_window() to set resource size directly instead of
using additional resource lists.
Because additional resource lists are optional resources, any algorithm
that requires guaranteed allocation that uses them cannot be guaranteed
to work.
Remove the resource from add_list. If it is set to zero size and left,
it can cause significant problems when it comes to assigning resources.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 3b3055e0e..a072781ab 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1818,7 +1818,7 @@ static void extend_bridge_window(struct pci_dev *bridge,
struct resource *res,
struct list_head *add_list,
resource_size_t new_size)
{
- struct pci_dev_resource *dev_res;
+ resource_size_t add_size;
if (res->parent)
return;
@@ -1826,17 +1826,10 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
if (resource_size(res) >= new_size)
return;
- dev_res = res_to_dev_res(add_list, res);
- if (!dev_res)
- return;
-
- /* Is there room to extend the window? */
- if (new_size - resource_size(res) <= dev_res->add_size)
- return;
-
- dev_res->add_size = new_size - resource_size(res);
- pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
- &dev_res->add_size);
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res, &add_size);
+ res->end = res->start + new_size - 1;
+ remove_from_list(add_list, res);
}
static void pci_bus_distribute_available_resources(struct pci_bus *bus,
--
2.22.0
[PATCH v8 4/6] PCI: Allow extend_bridge_window() to shrink resource if necessary
Remove checks for resource size in extend_bridge_window(). This is
necessary to allow the pci_bus_distribute_available_resources() to
function when the kernel parameter pci=hpmemsize=nn[KMG] is used to
allocate resources. Because the kernel parameter sets the size of all
hotplug bridges to be the same, there are problems when nested hotplug
bridges are encountered. Fitting a downstream hotplug bridge with size X
and normal bridges with size Y into parent hotplug bridge with size X is
impossible, and hence the downstream hotplug bridge needs to shrink to
fit into its parent.
Add check for if bridge is extended or shrunken and adjust pci_dbg to
reflect this.
Reset the resource if its new size is zero (if we have run out of a
bridge window resource). If it is set to zero size and left, it can
cause significant problems when it comes to enabling devices.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index a072781ab..7e1dc892a 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1823,13 +1823,19 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
if (res->parent)
return;
- if (resource_size(res) >= new_size)
- return;
-
- add_size = new_size - resource_size(res);
- pci_dbg(bridge, "bridge window %pR extended by %pa\n", res, &add_size);
+ if (new_size > resource_size(res)) {
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
+ &add_size);
+ } else if (new_size < resource_size(res)) {
+ add_size = resource_size(res) - new_size;
+ pci_dbg(bridge, "bridge window %pR shrunken by %pa\n", res,
+ &add_size);
+ }
res->end = res->start + new_size - 1;
remove_from_list(add_list, res);
+ if (!new_size)
+ reset_resource(res);
}
static void pci_bus_distribute_available_resources(struct pci_bus *bus,
--
2.22.0
[PATCH v8 5/6] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
Add kernel parameter pci=hpmmiosize=nn[KMG] to set MMIO bridge window
size for hotplug bridges.
Add kernel parameter pci=hpmmioprefsize=nn[KMG] to set MMIO_PREF bridge
window size for hotplug bridges.
Leave pci=hpmemsize=nn[KMG] unchanged, to prevent disruptions to
existing users. This sets both MMIO and MMIO_PREF to the same size.
The two new parameters conform to the style of pci=hpiosize=nn[KMG].
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 9 ++-
drivers/pci/pci.c | 17 ++---
drivers/pci/setup-bus.c | 25 +++
include/linux/pci.h | 3 ++-
4 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 46b826fcb..9bc54cb99 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3467,8 +3467,15 @@
hpiosize=nn[KMG]The fixed amount of bus space which is
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
+ hpmmiosize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO window.
+ Default size is 2 megabytes.
+ hpmmioprefsize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO_PREF window.
+ Default size is 2 megabytes.
hpmemsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's memory window.
+ reserved for hotplug bridge's MMIO and
+ MMIO_PREF window.
Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29ed5ec1a..6b3857cad 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,10 +85,12 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6281,8 +6283,17 @@ static int __init pci_setup(char *str)
pcie_ecrc_get_policy(str + 5);
} else if (!strncmp(str, "hpiosize=", 9)) {
pci_hotplug_io_size = memparse(str + 9, &str);
+ } else if (!strncmp(str, "hpmmiosize=", 11)) {
+ pci_hotplug_mmio_size =
+ memparse(str + 11, &str);
+ } else if (!strncmp(str, "hpmmioprefsize=", 15)) {
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 15, &str);
} else if (!strncmp(str, "hpmemsize=", 10)) {
- pci_hotplug_mem_size = memparse(str + 10, &str);
+ pci_hotplug_mmio_size =
+ memparse(str + 10, &str);
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 10, &str);
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
simple_strtoul(str + 10, &str, 0);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 7e1dc892a..345ecf16d 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1178,7 +1178,8 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
{
struct pci_dev *dev;
unsigned long mask, prefmask, type2 = 0, type3 = 0;
- resource_size_t additional_mem_size = 0, additional_io_size = 0;
+ resource_size_t additional_io_size = 0, additional_mmio_size = 0,
+ addit
[PATCH v8 6/6] PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window
Background
==
Currently, the kernel can sometimes assign the MMIO_PREF window
additional size into the MMIO window, resulting in double the MMIO
additional size, even if the MMIO_PREF window was successful.
This happens if in the first pass, the MMIO_PREF succeeds but the MMIO
fails. In the next pass, because MMIO_PREF is already assigned, the
attempt to assign MMIO_PREF returns an error code instead of success
(nothing more to do, already allocated).
Example of problem (more context can be found in the bug report URL):
Mainline kernel:
pci :06:01.0: BAR 14: assigned [mem 0x9010-0xa00f] = 256M
pci :06:04.0: BAR 14: assigned [mem 0xa020-0xb01f] = 256M
Patched kernel:
pci :06:01.0: BAR 14: assigned [mem 0x9010-0x980f] = 128M
pci :06:04.0: BAR 14: assigned [mem 0x9820-0xa01f] = 128M
This was using pci=realloc,hpmemsize=128M,nocrs - on the same machine
with the same configuration, with a Ubuntu mainline kernel and a kernel
patched with this patch series.
This patch is vital for the next patch in the series. The next patch
allows the user to specify MMIO and MMIO_PREF independently. If the
MMIO_PREF is set to be very large, this bug will end up more than
doubling the MMIO size. The bug results in the MMIO_PREF being added to
the MMIO window, which means doubling if MMIO_PREF size == MMIO size.
With a large MMIO_PREF, without this patch, the MMIO window will likely
fail to be assigned altogether due to lack of 32-bit address space.
Patch notes
==
Change find_free_bus_resource() to not skip assigned resources with
non-null parent.
Add checks in pbus_size_io() and pbus_size_mem() to return success if
resource returned from find_free_bus_resource() is already allocated.
This avoids pbus_size_io() and pbus_size_mem() returning error code to
__pci_bus_size_bridges() when a resource has been successfully assigned
in a previous pass. This fixes the existing behaviour where space for a
resource could be reserved multiple times in different parent bridge
windows. This also greatly reduces the number of failed BAR messages in
dmesg when Linux assigns resources.
See related from Logan Gunthorpe (same problem, different solution):
https://lore.kernel.org/lkml/20190531171216.20532-2-log...@deltatee.com/T/#u
Solves bug report: https://bugzilla.kernel.org/show_bug.cgi?id=203243
Reported-by: Kit Chow
Reported-by: Nicholas Johnson
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 345ecf16d..5b831af06 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -752,13 +752,18 @@ static void pci_bridge_check_ranges(struct pci_bus *bus)
}
/*
- * Helper function for sizing routines: find first available bus resource
- * of a given type. Note: we intentionally skip the bus resources which
- * have already been assigned (that is, have non-NULL parent resource).
+ * Helper function for sizing routines: find first bus resource of a given
+ * type. Note: we do not skip the bus resources which have already been
+ * assigned (r->parent != NULL). This is because a resource that is already
+ * assigned (nothing more to be done) will be indistinguishable from one that
+ * failed due to lack of space if we skip assigned resources. If the caller
+ * function cannot tell the difference then it might try to place the
+ * resources in a different window, doubling up on resources or causing
+ * unforeseeable issues.
*/
-static struct resource *find_free_bus_resource(struct pci_bus *bus,
- unsigned long type_mask,
- unsigned long type)
+static struct resource *find_bus_resource_of_type(struct pci_bus *bus,
+ unsigned long type_mask,
+ unsigned long type)
{
int i;
struct resource *r;
@@ -766,7 +771,7 @@ static struct resource *find_free_bus_resource(struct
pci_bus *bus,
pci_bus_for_each_resource(bus, r, i) {
if (r == &ioport_resource || r == &iomem_resource)
continue;
- if (r && (r->flags & type_mask) == type && !r->parent)
+ if (r && (r->flags & type_mask) == type)
return r;
}
return NULL;
@@ -866,14 +871,16 @@ static void pbus_size_io(struct pci_bus *bus,
resource_size_t min_size,
struct list_head *realloc_head)
{
struct pci_dev *dev;
- struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
-
Re: [PATCH v6 1/4] PCI: Consider alignment of hot-added bridges when distributing available resources
This time I am using reply-all "group=g" in Mutt instead of reply
"reply=r". It looks like I have been doing this incorrectly for a while
now. At least I have found out that git send-email uses terrible
encoding and adds the patch as an attachment, so things might start to
look up.
I copied the body of my previous reply into this reply so below the
line, it should be exactly the same. I thank Bjorn for his patience
here.
___
In addition to my responses to your concerns below, I would like to make
an initial clarification: does the fact that you have not replied to
patches 3 and 4 in the series indicate that you are happy with them, or
have not looked at them yet?
On Sat, Jun 15, 2019 at 02:56:36PM -0500, Bjorn Helgaas wrote:
> Mika, this patch changes code you added in 1a5767725cec ("PCI:
> Distribute available resources to hotplug-capable bridges"). Is there
> any chance you could help review this?
>
> On Wed, May 22, 2019 at 02:30:44PM +, Nicholas Johnson wrote:
> > Rewrite pci_bus_distribute_available_resources to better handle bridges
> > with different resource alignment requirements. Pass more details
> > arguments recursively to track the resource start and end addresses
> > relative to the initial hotplug bridge. This is especially useful for
> > Thunderbolt with native PCI enumeration, enabling external graphics
> > cards and other devices with bridge alignment higher than 0x10
> > bytes.
> >
> > Change extend_bridge_window to resize the actual resource, rather than
> > using add_list and dev_res->add_size. If an additional resource entry
> > exists for the given resource, zero out the add_size field to avoid it
> > interfering. Because add_size is considered optional when allocating,
> > using add_size could cause issues in some cases, because successful
> > resource distribution requires sizes to be guaranteed. Such cases
> > include hot-adding nested hotplug bridges in one enumeration, and
> > potentially others which are yet to be encountered.
> >
> > Signed-off-by: Nicholas Johnson
> > ---
> > drivers/pci/setup-bus.c | 169
> > 1 file changed, 84 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 0cdd5ff38..1b5b851ca 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1835,12 +1835,10 @@ static void extend_bridge_window(struct pci_dev
> > *bridge, struct resource *res,
> > }
> >
> > 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)
> > + struct list_head *add_list, struct resource io,
> > + struct resource mmio, struct resource mmio_pref)
>
> Follow the parameter indentation style of the rest of the file.
I will look at this. I will admit that handling line overflows has been
tricky for me to get a handle on.
>
> > {
> > - resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
> > + resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
> > unsigned int normal_bridges = 0, hotplug_bridges = 0;
> > struct resource *io_res, *mmio_res, *mmio_pref_res;
> > struct pci_dev *dev, *bridge = bus->self;
> > @@ -1850,29 +1848,36 @@ static void
> > pci_bus_distribute_available_resources(struct pci_bus *bus,
> > 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.
> > +* The alignment of this bridge is yet to be considered, hence it must
> > +* be done now before extending its bridge window. A single bridge
> > +* might not be able to occupy the whole parent region if the alignment
> > +* differs - for example, an external GPU at the end of a Thunderbolt
> > +* daisy chain.
>
> The example seems needlessly specific. There isn't anything GPU- or
> Thunderbolt-specific about this, is there?
>
> Bridge windows can be aligned to any multiple of 1MB. But a de
Re: [PATCH v6 1/4] PCI: Consider alignment of hot-added bridges when distributing available resources
Again, I am re-sending my previous response with reply-all instead of
plain reply. Mika, you can ignore this as you have already seen it. For
everybody else, everything below the line should be exactly the same as
I copied it over.
___
On Mon, Jun 17, 2019 at 12:35:13PM +0300, mika.westerb...@linux.intel.com wrote:
> On Wed, May 22, 2019 at 02:30:44PM +0000, Nicholas Johnson wrote:
> > Rewrite pci_bus_distribute_available_resources to better handle bridges
> > with different resource alignment requirements. Pass more details
> > arguments recursively to track the resource start and end addresses
> > relative to the initial hotplug bridge. This is especially useful for
> > Thunderbolt with native PCI enumeration, enabling external graphics
> > cards and other devices with bridge alignment higher than 0x10
>
> Instead of 0x10 you could say 1MB here.
My train of thought is 1MB is sometimes means base-10 but then again,
that is probably outside of the kernel world. I can change it to 1MB.
>
> > bytes.
> >
> > Change extend_bridge_window to resize the actual resource, rather than
> > using add_list and dev_res->add_size. If an additional resource entry
> > exists for the given resource, zero out the add_size field to avoid it
> > interfering. Because add_size is considered optional when allocating,
> > using add_size could cause issues in some cases, because successful
> > resource distribution requires sizes to be guaranteed. Such cases
> > include hot-adding nested hotplug bridges in one enumeration, and
> > potentially others which are yet to be encountered.
> >
> > Signed-off-by: Nicholas Johnson
>
> The logic makes sense to me but since you probably need to spin another
> revision anyway please find a couple of additional comments below.
>
> > ---
> > drivers/pci/setup-bus.c | 169
> > 1 file changed, 84 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 0cdd5ff38..1b5b851ca 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1835,12 +1835,10 @@ static void extend_bridge_window(struct pci_dev
> > *bridge, struct resource *res,
> > }
> >
> > 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)
> > + struct list_head *add_list, struct resource io,
> > + struct resource mmio, struct resource mmio_pref)
> > {
> > - resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
> > + resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
> > unsigned int normal_bridges = 0, hotplug_bridges = 0;
> > struct resource *io_res, *mmio_res, *mmio_pref_res;
> > struct pci_dev *dev, *bridge = bus->self;
> > @@ -1850,29 +1848,36 @@ static void
> > pci_bus_distribute_available_resources(struct pci_bus *bus,
> > 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.
> > +* The alignment of this bridge is yet to be considered, hence it must
> > +* be done now before extending its bridge window. A single bridge
> > +* might not be able to occupy the whole parent region if the alignment
> > +* differs - for example, an external GPU at the end of a Thunderbolt
> > +* daisy chain.
>
> As Bjorn also commented there is nothing Thunderbolt specific here so I
> would leave it out of the comment because it is kind of confusing.
Okay, I can remove "A single bridge daisy chain".
Please correct me if that is not what you meant.
>
> > */
> > - 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);
> > + align = pci_resource_alignment(bridge, io_res);
> > + if (!io_res->parent &&
[PATCH v7 0/7] PCI: Patch series to support Thunderbolt without any BIOS support
Included patches from Bjorn. I had already started my own equivalent patches, but it will be easier for Bjorn to sign off if he wrote them. Moved the bug fix to the end of the series, in case we accept Logan's equivalent patch instead - in which case, the last patch in my series can easily be dropped. I split the [previously 2/4] patch into two, in the hope it can make things easier. We are running out of time and do not have high hopes of hitting this merge window, other than perhaps Bjorn's 1-2 and my 3 which solves Mika Westerberg's bug. That would be a win for me, and would make things easier next cycle. Hopefully I addressed as many of the concerns raised as possible with this series. There was no response so I went with kernel parameters hpmmiosize and hpmmioprefsize which fits in with hpiosize. The existing hpmemsize remains unchanged. I still have no idea why my git send-email is messing with the encoding, when it works for everybody else - but I will use "mutt -H" in the mean time, which we tested to be working. I have tried to test this as extensively as I can but I fear I may have left silly mistakes, or forgotten to make all of the requested changes. My apologies in advance if any slip through. Bjorn Helgaas (2): PCI: Simplify pci_bus_distribute_available_resources() PCI: Skip resource distribution when no hotplug bridges Nicholas Johnson (5): PCI: Consider alignment of hot-added bridges when distributing available resources PCI: Allow extend_bridge_window() to shrink resource if necessary PCI: Change extend_bridge_window() to set resource size directly PCI: Add hp_mmio_size and hp_mmio_pref_size parameters PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window .../admin-guide/kernel-parameters.txt | 8 +- drivers/pci/pci.c | 17 +- drivers/pci/setup-bus.c | 247 +- include/linux/pci.h | 3 +- 4 files changed, 150 insertions(+), 125 deletions(-) -- 2.20.1
[PATCH v7 1/8] PCI: Simplify pci_bus_distribute_available_resources()
Reorder pci_bus_distribute_available_resources() to group related code
together. No functional change intended.
Link:
https://lore.kernel.org/r/ps2p216mb0642c7a485649d2d787a1c6f80...@ps2p216mb0642.korp216.prod.outlook.com
Based-on-patch-by: Nicholas Johnson
[bhelgaas: extracted from larger patch]
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 50 -
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 0cdd5ff38..af28af898 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1860,16 +1860,6 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
extend_bridge_window(bridge, mmio_pref_res, add_list,
available_mmio_pref);
- /*
-* Calculate the total amount of extra resource space we can
-* pass to bridges below this one. This is basically the
-* extra space reduced by the minimal required space for the
-* non-hotplug bridges.
-*/
- remaining_io = available_io;
- remaining_mmio = available_mmio;
- remaining_mmio_pref = available_mmio_pref;
-
/*
* Calculate how many hotplug bridges and normal bridges there
* are on this bus. We will distribute the additional available
@@ -1882,6 +1872,31 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
normal_bridges++;
}
+ /*
+* There is only one bridge on the bus so it gets all available
+* resources which it can then distribute to the possible hotplug
+* bridges below.
+*/
+ if (hotplug_bridges + normal_bridges == 1) {
+ dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
+ if (dev->subordinate) {
+ pci_bus_distribute_available_resources(dev->subordinate,
+ add_list, available_io, available_mmio,
+ available_mmio_pref);
+ }
+ return;
+ }
+
+ /*
+* Calculate the total amount of extra resource space we can
+* pass to bridges below this one. This is basically the
+* extra space reduced by the minimal required space for the
+* non-hotplug bridges.
+*/
+ remaining_io = available_io;
+ remaining_mmio = available_mmio;
+ remaining_mmio_pref = available_mmio_pref;
+
for_each_pci_bridge(dev, bus) {
const struct resource *res;
@@ -1905,21 +1920,6 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
remaining_mmio_pref -= resource_size(res);
}
- /*
-* There is only one bridge on the bus so it gets all available
-* resources which it can then distribute to the possible hotplug
-* bridges below.
-*/
- if (hotplug_bridges + normal_bridges == 1) {
- dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
- if (dev->subordinate) {
- pci_bus_distribute_available_resources(dev->subordinate,
- add_list, available_io, available_mmio,
- available_mmio_pref);
- }
- return;
- }
-
/*
* Go over devices on this bus and distribute the remaining
* resource space between hotplug bridges.
--
2.20.1
[PATCH v7 2/8] PCI: Skip resource distribution when no hotplug bridges
If "hotplug_bridges == 0", "!dev->is_hotplug_bridge" is always true, so the loop that divides the remaining resources among hotplug-capable bridges does nothing. Check for "hotplug_bridges == 0" earlier, so we don't even have to compute the amount of remaining resources. No functional change intended. Signed-off-by: Nicholas Johnson --- drivers/pci/setup-bus.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index af28af898..04adeebe8 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -1887,6 +1887,9 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus, return; } + if (hotplug_bridges == 0) + return; + /* * Calculate the total amount of extra resource space we can * pass to bridges below this one. This is basically the @@ -1936,8 +1939,6 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus, * 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); -- 2.20.1
[PATCH v7 3/8] PCI: Consider alignment of hot-added bridges when distributing available resources
Rewrite pci_bus_distribute_available_resources to better handle bridges
with different resource alignment requirements. Pass more details
arguments recursively to track the resource start and end addresses
relative to the initial hotplug bridge. This is especially useful for
Thunderbolt with native PCI enumeration, enabling external graphics
cards and other devices with bridge alignment higher than 1MB.
Change extend_bridge_window to resize the actual resource, rather than
using add_list and dev_res->add_size. If an additional resource entry
exists for the given resource, zero out the add_size field to avoid it
interfering. Because add_size is considered optional when allocating,
using add_size could cause issues in some cases, because successful
resource distribution requires sizes to be guaranteed. Such cases
include hot-adding nested hotplug bridges in one enumeration, and
potentially others which are yet to be encountered.
Solves bug report: https://bugzilla.kernel.org/show_bug.cgi?id=199581
Reported-by: Mika Westerberg
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 148 +++-
1 file changed, 71 insertions(+), 77 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 04adeebe8..898fea00c 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1835,12 +1835,10 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
}
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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1850,15 +1848,29 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window.
*/
- extend_bridge_window(bridge, io_res, add_list, available_io);
- extend_bridge_window(bridge, mmio_res, add_list, available_mmio);
+ align = pci_resource_alignment(bridge, io_res);
+ if (!io_res->parent && align)
+ io.start = ALIGN(io.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_res);
+ if (!mmio_res->parent && align)
+ mmio.start = ALIGN(mmio.start, align);
+
+ align = pci_resource_alignment(bridge, mmio_pref_res);
+ if (!mmio_pref_res->parent && align)
+ mmio_pref.start = ALIGN(mmio_pref.start, align);
+
+ /*
+* Update the resources to fill as much remaining resource space in the
+* parent bridge as possible, while considering alignment.
+*/
+ extend_bridge_window(bridge, io_res, add_list, resource_size(&io));
+ extend_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
extend_bridge_window(bridge, mmio_pref_res, add_list,
-available_mmio_pref);
+ resource_size(&mmio_pref));
/*
* Calculate how many hotplug bridges and normal bridges there
@@ -1879,108 +1891,90 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
*/
if (hotplug_bridges + normal_bridges == 1) {
dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
- if (dev->subordinate) {
+ if (dev->subordinate)
pci_bus_distribute_available_resources(dev->subordinate,
- add_list, available_io, available_mmio,
- available_mmio_pref);
- }
+ add_list, io, mmio, mmio_pref);
return;
}
- if (hotplug_bridges == 0)
- return;
-
/*
-* Calculate the total amount of extra resource space we can
-* pass to bridges below this one. This is basicall
[PATCH v7 4/8] PCI: In extend_bridge_window() change available to new_size
In extend_bridge_window() change "available" parameter name to new_size.
This makes more sense as this parameter represents the new size for the
window.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 898fea00c..add1dc373 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1811,14 +1811,14 @@ 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)
+resource_size_t new_size)
{
struct pci_dev_resource *dev_res;
if (res->parent)
return;
- if (resource_size(res) >= available)
+ if (resource_size(res) >= new_size)
return;
dev_res = res_to_dev_res(add_list, res);
@@ -1826,10 +1826,10 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
return;
/* Is there room to extend the window? */
- if (available - resource_size(res) <= dev_res->add_size)
+ if (new_size - resource_size(res) <= dev_res->add_size)
return;
- dev_res->add_size = available - resource_size(res);
+ dev_res->add_size = new_size - resource_size(res);
pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
&dev_res->add_size);
}
--
2.20.1
[PATCH v7 5/8] PCI: Change extend_bridge_window() to set resource size directly
Change extend_bridge_window() to set resource size directly instead of
using additional resource lists.
Because additional resource lists are optional resources, any algorithm
that requires guaranteed allocation that uses them cannot be guaranteed
to work.
Remove the resource from add_list. If it is set to zero size and left,
it can cause significant problems when it comes to assigning resources.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index add1dc373..d65982438 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1813,7 +1813,7 @@ static void extend_bridge_window(struct pci_dev *bridge,
struct resource *res,
struct list_head *add_list,
resource_size_t new_size)
{
- struct pci_dev_resource *dev_res;
+ resource_size_t add_size;
if (res->parent)
return;
@@ -1821,17 +1821,10 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
if (resource_size(res) >= new_size)
return;
- dev_res = res_to_dev_res(add_list, res);
- if (!dev_res)
- return;
-
- /* Is there room to extend the window? */
- if (new_size - resource_size(res) <= dev_res->add_size)
- return;
-
- dev_res->add_size = new_size - resource_size(res);
- pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
- &dev_res->add_size);
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res, &add_size);
+ res->end = res->start + new_size - 1;
+ remove_from_list(add_list, res);
}
static void pci_bus_distribute_available_resources(struct pci_bus *bus,
--
2.20.1
[PATCH v7 6/8] PCI: Allow extend_bridge_window() to shrink resource if necessary
Remove checks for resource size in extend_bridge_window(). This is
necessary to allow the pci_bus_distribute_available_resources() to
function when the kernel parameter pci=hpmemsize=nn[KMG] is used to
allocate resources. Because the kernel parameter sets the size of all
hotplug bridges to be the same, there are problems when nested hotplug
bridges are encountered. Fitting a downstream hotplug bridge with size X
and normal bridges with size Y into parent hotplug bridge with size X is
impossible, and hence the downstream hotplug bridge needs to shrink to
fit into its parent.
Add check for if bridge is extended or shrunken and adjust pci_dbg to
reflect this.
Reset the resource if its new size is zero (if we have run out of a
bridge window resource). If it is set to zero size and left, it can
cause significant problems when it comes to enabling devices.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d65982438..9064fd964 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1818,13 +1818,19 @@ static void extend_bridge_window(struct pci_dev
*bridge, struct resource *res,
if (res->parent)
return;
- if (resource_size(res) >= new_size)
- return;
-
- add_size = new_size - resource_size(res);
- pci_dbg(bridge, "bridge window %pR extended by %pa\n", res, &add_size);
+ if (new_size > resource_size(res)) {
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
+ &add_size);
+ } else if (new_size < resource_size(res)) {
+ add_size = resource_size(res) - new_size;
+ pci_dbg(bridge, "bridge window %pR shrunken by %pa\n", res,
+ &add_size);
+ }
res->end = res->start + new_size - 1;
remove_from_list(add_list, res);
+ if (!new_size)
+ reset_resource(res);
}
static void pci_bus_distribute_available_resources(struct pci_bus *bus,
--
2.20.1
[PATCH v7 7/8] PCI: Add hp_mmio_size and hp_mmio_pref_size parameters
Add kernel parameter pci=hpmmiosize=nn[KMG] to set MMIO bridge window
size for hotplug bridges.
Add kernel parameter pci=hpmmioprefsize=nn[KMG] to set MMIO_PREF bridge
window size for hotplug bridges.
Leave pci=hpmemsize=nn[KMG] unchanged, to prevent disruptions to
existing users. This sets both MMIO and MMIO_PREF to the same size.
The two new parameters conform to the style of pci=hpiosize=nn[KMG].
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 9 ++-
drivers/pci/pci.c | 17 ++---
drivers/pci/setup-bus.c | 25 +++
include/linux/pci.h | 3 ++-
4 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b..f3cda0b07 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3438,8 +3438,15 @@
hpiosize=nn[KMG]The fixed amount of bus space which is
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
+ hpmmiosize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO window.
+ Default size is 2 megabytes.
+ hpmmioprefsize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO_PREF window.
+ Default size is 2 megabytes.
hpmemsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's memory window.
+ reserved for hotplug bridge's MMIO and
+ MMIO_PREF window.
Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8abc843b1..4ee1aaf5b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,10 +85,12 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6231,8 +6233,17 @@ static int __init pci_setup(char *str)
pcie_ecrc_get_policy(str + 5);
} else if (!strncmp(str, "hpiosize=", 9)) {
pci_hotplug_io_size = memparse(str + 9, &str);
+ } else if (!strncmp(str, "hpmmiosize=", 11)) {
+ pci_hotplug_mmio_size =
+ memparse(str + 11, &str);
+ } else if (!strncmp(str, "hpmmioprefsize=", 15)) {
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 15, &str);
} else if (!strncmp(str, "hpmemsize=", 10)) {
- pci_hotplug_mem_size = memparse(str + 10, &str);
+ pci_hotplug_mmio_size =
+ memparse(str + 10, &str);
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 10, &str);
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
simple_strtoul(str + 10, &str, 0);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 9064fd964..873df5482 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1178,7 +1178,8 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
{
struct pci_dev *dev;
unsigned long mask, prefmask, type2 = 0, type3 = 0;
- resource_size_t additional_mem_size = 0, additional_io_size = 0;
+ resource_size_t additional_io_size = 0, additional_mmio_size = 0,
+ addit
[PATCH v7 8/8] PCI: Fix bug resulting in double hpmemsize being assigned to MMIO window
Background
==
Currently, the kernel can sometimes assign the MMIO_PREF window
additional size into the MMIO window, resulting in double the MMIO
additional size, even if the MMIO_PREF window was successful.
This happens if in the first pass, the MMIO_PREF succeeds but the MMIO
fails. In the next pass, because MMIO_PREF is already assigned, the
attempt to assign MMIO_PREF returns an error code instead of success
(nothing more to do, already allocated).
Example of problem (more context can be found in the bug report URL):
Mainline kernel:
pci :06:01.0: BAR 14: assigned [mem 0x9010-0xa00f] = 256M
pci :06:04.0: BAR 14: assigned [mem 0xa020-0xb01f] = 256M
Patched kernel:
pci :06:01.0: BAR 14: assigned [mem 0x9010-0x980f] = 128M
pci :06:04.0: BAR 14: assigned [mem 0x9820-0xa01f] = 128M
This was using pci=realloc,hpmemsize=128M,nocrs - on the same machine
with the same configuration, with a Ubuntu mainline kernel and a kernel
patched with this patch series.
This patch is vital for the next patch in the series. The next patch
allows the user to specify MMIO and MMIO_PREF independently. If the
MMIO_PREF is set to be very large, this bug will end up more than
doubling the MMIO size. The bug results in the MMIO_PREF being added to
the MMIO window, which means doubling if MMIO_PREF size == MMIO size.
With a large MMIO_PREF, without this patch, the MMIO window will likely
fail to be assigned altogether due to lack of 32-bit address space.
Patch notes
==
Change find_free_bus_resource() to not skip assigned resources with
non-null parent.
Add checks in pbus_size_io() and pbus_size_mem() to return success if
resource returned from find_free_bus_resource() is already allocated.
This avoids pbus_size_io() and pbus_size_mem() returning error code to
__pci_bus_size_bridges() when a resource has been successfully assigned
in a previous pass. This fixes the existing behaviour where space for a
resource could be reserved multiple times in different parent bridge
windows. This also greatly reduces the number of failed BAR messages in
dmesg when Linux assigns resources.
See related from Logan Gunthorpe (same problem, different solution):
https://lore.kernel.org/lkml/20190531171216.20532-2-log...@deltatee.com/T/#u
Solves bug report: https://bugzilla.kernel.org/show_bug.cgi?id=203243
Reported-by: Kit Chow
Reported-by: Nicholas Johnson
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 873df5482..df4bf43b5 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -752,13 +752,18 @@ static void pci_bridge_check_ranges(struct pci_bus *bus)
}
/*
- * Helper function for sizing routines: find first available bus resource
- * of a given type. Note: we intentionally skip the bus resources which
- * have already been assigned (that is, have non-NULL parent resource).
+ * Helper function for sizing routines: find first bus resource of a given
+ * type. Note: we do not skip the bus resources which have already been
+ * assigned (r->parent != NULL). This is because a resource that is already
+ * assigned (nothing more to be done) will be indistinguishable from one that
+ * failed due to lack of space if we skip assigned resources. If the caller
+ * function cannot tell the difference then it might try to place the
+ * resources in a different window, doubling up on resources or causing
+ * unforeseeable issues.
*/
-static struct resource *find_free_bus_resource(struct pci_bus *bus,
- unsigned long type_mask,
- unsigned long type)
+static struct resource *find_bus_resource_of_type(struct pci_bus *bus,
+ unsigned long type_mask,
+ unsigned long type)
{
int i;
struct resource *r;
@@ -766,7 +771,7 @@ static struct resource *find_free_bus_resource(struct
pci_bus *bus,
pci_bus_for_each_resource(bus, r, i) {
if (r == &ioport_resource || r == &iomem_resource)
continue;
- if (r && (r->flags & type_mask) == type && !r->parent)
+ if (r && (r->flags & type_mask) == type)
return r;
}
return NULL;
@@ -866,14 +871,16 @@ static void pbus_size_io(struct pci_bus *bus,
resource_size_t min_size,
struct list_head *realloc_head)
{
struct pci_dev *dev;
- struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
-
Re: [PATCH v6 0/4] PCI: Patch series to support Thunderbolt without any BIOS support
On Sun, Jun 16, 2019 at 03:56:19AM +0800, Bjorn Helgaas wrote: > [+cc Ben, Logan] > > Ben, Logan, since you're looking at the resource code, maybe you'd be > interested in this as well? > > On Wed, May 22, 2019 at 02:30:30PM +, Nicholas Johnson wrote: > > Rebase patches to apply cleanly to 5.2-rc1 source. Remove patch for > > comment style cleanup as this has already been applied. > > Thanks for rebasing these. > > They do apply cleanly, but they seem to be base64-encoded MIME > attachments, and I don't know how to make mutt extract them easily. I > had to save each patch attachment individually, apply it, insert the > commit log manually, etc. > > Is there any chance you could send the next series as plain-text > patches? That would be a lot easier for me. > > > Anybody interested in testing, you can do so with: > > > > a) Intel system with Thunderbolt 3 and native enumeration. The Gigabyte > > Z390 Designare is one of the most perfect for this that I have never had > > the opportunity to use - it does not even have the option for BIOS > > assisted enumeration present in the BIOS. > > > > b) Any system with PCIe and the Gigabyte GC-TITAN RIDGE add-in card, > > jump the header as described and use kernel parameters like: > > > > pci=assign-busses,hpbussize=0x33,realloc,hpmemsize=128M,hpmemprefsize=1G,nocrs > > > > pcie_ports=native > > > > [optional] pci.dyndbg > > > > ___ > > __/ \__ > > |o o o o o| When looking into the receptacle on back of PCIe card. > > |_| Jump pins 3 and 5. > > > > 1 2 3 4 5 > > > > The Intel system is nice in that it should just work. The add-in card > > setup is nice in that you can go nuts and assign copious amounts of > > MMIO_PREF - can anybody show a Xeon Phi coprocessor with 16G BAR working > > in an eGPU enclosure with these patches? > > > > However, if you specify the above kernel parameters on the Intel system, > > you should be able to override it to allocate more space. > > > > Nicholas Johnson (4): > > PCI: Consider alignment of hot-added bridges when distributing > > available resources > > PCI: Modify extend_bridge_window() to set resource size directly > > PCI: Fix bug resulting in double hpmemsize being assigned to MMIO > > window > > PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size > > independently > > > > .../admin-guide/kernel-parameters.txt | 7 +- > > drivers/pci/pci.c | 18 +- > > drivers/pci/setup-bus.c | 265 ++ > > include/linux/pci.h | 3 +- > > 4 files changed, 167 insertions(+), 126 deletions(-) > > > > -- > > 2.20.1 > > I posted PATCH v7, finally. I needed a place to announce that the patches 1-2/8 which were made by Bjorn would not send with him as the "from" which seems to attribute the author. Credits go to Bjorn for PATCH v7 1-2/8 (the first two patches) but to send them I had to put myself in that field. When applying them, I guess you will have to modify that field, Bjorn. My apologies if there was a way around it. Thanks for all the comments and feedback from everybody.
[PATCH v2 0/4] PCI: Patch series to support Thunderbolt without any BIOS support
sizes independently. The other patches are prep-work for this patch, allowing it to work flawlessly. This will also make at least one other person very happy, providing a solution where none previously existed (if this is accepted, I will be answering their question with this patch): https://superuser.com/questions/1054657/how-can-i-reserve-hotplug-bridges-memory-only-for-prefetchable-memory-using-the Nicholas Johnson (4): PCI: Consider alignment of hot-added bridges when distributing available resources PCI: Cleanup comments in setup-bus.c to meet kernel coding style guidelines PCI: Fix serious bug when sizing bridges with additional size PCI: modify kernel parameters to differentiate between MMIO and MMIO_PREF sizes .../admin-guide/kernel-parameters.txt | 21 +- drivers/pci/pci.c | 39 +- drivers/pci/setup-bus.c | 513 +- include/linux/pci.h | 3 +- 4 files changed, 323 insertions(+), 253 deletions(-) -- 2.20.1
[PATCH v2 1/4] PCI: Consider alignment of hot-added bridges when distributing available resources
Rewrite pci_bus_distribute_available_resources to better handle bridges
with different resource alignment requirements. Pass more details
arguments recursively to track the resource start and end addresses
relative to the initial hotplug bridge. This is especially useful for
Thunderbolt with native PCI enumeration, enabling external graphics
cards and other devices with bridge alignment higher than 0x10
bytes.
Change extend_bridge_window to resize the actual resource, rather than
using add_list and dev_res->add_size. If an additional resource entry
exists for the given resource, zero out the add_size field to avoid it
interfering. Because add_size is considered optional when allocating,
using add_size could cause issues in some cases, because successful
resource distribution requires sizes to be guaranteed. Such cases
include hot-adding nested hotplug bridges in one enumeration, and
potentially others which are yet to be encountered.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 203 ++--
1 file changed, 110 insertions(+), 93 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ed960436d..75827fb06 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1852,34 +1852,48 @@ 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 list_head *add_list, resource_size_t new_size)
{
struct pci_dev_resource *dev_res;
+ resource_size_t add_size;
if (res->parent)
return;
- if (resource_size(res) >= available)
- return;
-
- dev_res = res_to_dev_res(add_list, res);
- if (!dev_res)
- return;
+ /*
+* Resources requested using add_size in additional resource lists are
+* considered optional when allocated. Guaranteed size of allocation
+* is required to guarantee successful resource distribution. Hence,
+* the size of the actual resource must be adjusted.
+*/
+ if (new_size >= resource_size(res)) {
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
+ &add_size);
+ } else {
+ add_size = resource_size(res) - new_size;
+ pci_dbg(bridge, "bridge window %pR shrunken by %pa\n", res,
+ &add_size);
+ }
- /* Is there room to extend the window? */
- if (available - resource_size(res) <= dev_res->add_size)
- return;
+ res->end = res->start + new_size - 1;
- dev_res->add_size = available - resource_size(res);
- pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
- &dev_res->add_size);
+ /*
+* If a list entry exists, we need to remove any additional size
+* requested because that could interfere with the alignment and
+* sizing done when distributing resources, causing resources to
+* fail to allocate later on.
+*/
+ dev_res = res_to_dev_res(add_list, res);
+ if (dev_res)
+ dev_res->add_size = 0;
}
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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1889,25 +1903,32 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window. A single bridge
+* might not be able to occupy the whole parent region if the alignment
+* differs - for example, an external GPU at the end of a Thunderbolt
+* daisy chain.
*/
- extend_bridge_window(bridge, io_res, add_list, available_io);
- extend_br
[PATCH v2 2/4] PCI: Cleanup comments in setup-bus.c to meet kernel coding style guidelines
Change block comments to accepted style with asterisks on each line.
Justify block comments to 80-character limit to reduce the number of
lines where possible.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 265
1 file changed, 130 insertions(+), 135 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 75827fb06..0ce641282 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -51,11 +51,9 @@ static void free_list(struct list_head *head)
/**
* add_to_list() - add a new resource tracker to the list
* @head: Head of the list
- * @dev: device corresponding to which the resource
- * belongs
+ * @dev: device corresponding to which the resource belongs
* @res: The resource to be tracked
- * @add_size: additional size to be optionally added
- * to the resource
+ * @add_size: additional size to be optionally added to the resource
*/
static int add_to_list(struct list_head *head,
struct pci_dev *dev, struct resource *res,
@@ -158,7 +156,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct
list_head *head)
tmp->res = r;
tmp->dev = dev;
- /* fallback is smallest one or list is empty*/
+ /* fallback is smallest one or list is empty */
n = head;
list_for_each_entry(dev_res, head, list) {
resource_size_t align;
@@ -171,7 +169,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct
list_head *head)
break;
}
}
- /* Insert it just before n*/
+ /* Insert it just before n */
list_add_tail(&tmp->list, n);
}
}
@@ -181,7 +179,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
{
u16 class = dev->class >> 8;
- /* Don't touch classless devices or host bridges or ioapics. */
+ /* Don't touch classless devices or host bridges or ioapics */
if (class == PCI_CLASS_NOT_DEFINED || class == PCI_CLASS_BRIDGE_HOST)
return;
@@ -208,12 +206,10 @@ static inline void reset_resource(struct resource *res)
*
* @realloc_head : head of the list tracking requests requiring additional
* resources
- * @head : head of the list tracking requests with allocated
- * resources
+ * @head : head of the list tracking requests with allocated resources
*
- * Walk through each element of the realloc_head and try to procure
- * additional resources for the element, provided the element
- * is in the head list.
+ * Walk through each element of the realloc_head and try to procure additional
+ * resources for the element, provided the element is in the head list.
*/
static void reassign_resources_sorted(struct list_head *realloc_head,
struct list_head *head)
@@ -315,10 +311,9 @@ static unsigned long pci_fail_res_type_mask(struct
list_head *fail_head)
mask |= fail_res->flags;
/*
-* one pref failed resource will set IORESOURCE_MEM,
-* as we can allocate pref in non-pref range.
-* Will release all assigned non-pref sibling resources
-* according to that bit.
+* one pref failed resource will set IORESOURCE_MEM, as we can allocate
+* pref in non-pref range. Will release all assigned non-pref sibling
+* resources according to that bit.
*/
return mask & (IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH);
}
@@ -351,25 +346,24 @@ static void __assign_resources_sorted(struct list_head
*head,
struct list_head *fail_head)
{
/*
-* Should not assign requested resources at first.
-* they could be adjacent, so later reassign can not reallocate
-* them one by one in parent resource window.
-* Try to assign requested + add_size at beginning
-* if could do that, could get out early.
-* if could not do that, we still try to assign requested at first,
-*then try to reassign add_size for some resources.
+* Should not assign requested resources at first. They could be
+* adjacent, so later reassign can not reallocate them one by one in
+* parent resource window.
+*
+* Try to assign requested + add_size at beginning. If could do that,
+* could get out early. If could not do that, we still try to assign
+* requested at first, then try to reassign add_size for some resources.
*
* Separate three resource type checking if we need to release
* assigned resource after requested + add_size try.
-* 1. if there is io port assign fail, will release assigned
-* io port.
-*
[PATCH v2 4/4] PCI: modify kernel parameters to differentiate between MMIO and MMIO_PREF sizes
Add new kernel parameters:
- hp_io_size=nn
- hp_mmio_size=nnM
- hp_mmio_pref_size=nnM
Depreciate old kernel parameters and print warning when used:
- hpiosize=nn
- hpmemsize=nnM
Make hp_io_size function identically to hpiosize.
Make hp_mmio_size set additional size for MMIO bridge window.
Make hp_mmio_pref_size set additional size for MMIO_PREF bridge window.
Override old kernel parameters if any of the new ones are specified.
Make sure old kernel parameters still work identically when none of the
new kernel parameters are specified.
Make sure it is easy to remove old kernel parameters from the source
when it is deemed that sufficient time has passed to allow users to
migrate to the new kernel parameters.
Update documentation to reflect changes.
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 21 --
drivers/pci/pci.c | 39 +--
drivers/pci/setup-bus.c | 26 +++--
include/linux/pci.h | 3 +-
4 files changed, 69 insertions(+), 20 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 858b6c0b9..29406ae8f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3326,12 +3326,27 @@
the default.
off: Turn ECRC off
on: Turn ECRC on.
- hpiosize=nn[KMG]The fixed amount of bus space which is
+
+ hpiosize=nn[KMG]Depreciated. Overridden by hp_io_size
+ value if any of hp_io_size, hp_mmio_size
+ or hp_mmio_pref_size are used. This sets
+ hp_io_size to the given value if not
+ overridden.
+
+ hpmemsize=nn[KMG] Depreciated. Overridden if any of
+ hp_io_size, hp_mmio_size or
+ hp_mmio_pref_size are used. This sets
+ hp_mmio_size and hp_mmio_pref_size to
+ the given value if not overridden.
+ hp_io_size=nn[KMG] The fixed amount of bus space which is
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
- hpmemsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's memory window.
+ hp_mmio_size=nn[KMG]The fixed amount of bus space which is
+ reserved for hotplug bridge's 32-bit mmio
window.
Default size is 2 megabytes.
+ hp_mmio_pref_size=nn[KMG] The fixed amount of bus space
+ which is reserved for hotplug bridge's 64-bit
+ mmio_pref window. Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
Default is 1.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c25acace7..64978bf84 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,12 +85,15 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
-/* pci=hpmemsize=nnM,hpiosize=nn can override this */
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
+/* Override with pci=hp_io_size=nn,hp_mmio_size=nnM,hp_mmio_pref_size=nnM */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
+/* Override with pci=hpbussize=nn,assign-busses */
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
enum pcie_bus_config_types pcie_bus_config = PCIE_BUS_DEFAULT;
@@ -6144,6 +6147,15 @@ EXPORT_SYMBOL(pci_fixup_cardbus);
static int __init pci_setup(char *str)
{
+ /*
+* Depreciated pci=hpmemsize=nnM but keep the functionality for now.
+* If none of hp_io_size, hp_mmio_size or hp_mmio_pref_size are set
+* then override hp_mmio_size and hp_mmio_pref_size with hpmemsize.
+*/
+ unsigned long pci_hotplug_io_size_old = DEFAULT_HOTPLUG_IO_SIZE;
+ unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MMI
[PATCH v2 3/4] PCI: Fix serious bug when sizing bridges with additional size
Change find_free_bus_resource function to not skip assigned resources
with non-null parent.
Add checks in pbus_size_io and pbus_size_mem to return success if
resource returned from find_free_bus_resource is already allocated.
This avoids pbus_size_io and pbus_size_mem returning error code to
__pci_bus_size_bridges when a resource has been successfully assigned in
a previous pass. This fixes the existing behaviour where space for a
resource could be reserved multiple times in different parent bridge
windows. This also greatly reduces the number of failed BAR messages in
dmesg when Linux assigns resources.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 0ce641282..efe899b02 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -791,11 +791,16 @@ static void pci_bridge_check_ranges(struct pci_bus *bus)
}
/*
- * Helper function for sizing routines: find first available bus resource of a
- * given type. Note: we intentionally skip the bus resources which have already
- * been assigned (that is, have non-NULL parent resource).
+ * Helper function for sizing routines: find first bus resource of a
+ * given type. Note: we do not skip the bus resources which have already
+ * been assigned (r->parent != NULL). This is because a resource that is
+ * already assigned (nothing more to be done) will be indistinguishable
+ * from one that failed due to lack of space if we skip assigned
+ * resources. If the caller function cannot tell the difference then it
+ * might try to place the resources in a different window, doubling up on
+ * resources or causing unforeseeable issues.
*/
-static struct resource *find_free_bus_resource(struct pci_bus *bus,
+static struct resource *find_bus_resource_of_type(struct pci_bus *bus,
unsigned long type_mask, unsigned long type)
{
int i;
@@ -804,7 +809,7 @@ static struct resource *find_free_bus_resource(struct
pci_bus *bus,
pci_bus_for_each_resource(bus, r, i) {
if (r == &ioport_resource || r == &iomem_resource)
continue;
- if (r && (r->flags & type_mask) == type && !r->parent)
+ if (r && (r->flags & type_mask) == type)
return r;
}
return NULL;
@@ -903,14 +908,16 @@ static void pbus_size_io(struct pci_bus *bus,
resource_size_t min_size,
resource_size_t add_size, struct list_head *realloc_head)
{
struct pci_dev *dev;
- struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
- IORESOURCE_IO);
+ struct resource *b_res = find_bus_resource_of_type(bus, IORESOURCE_IO,
+ IORESOURCE_IO);
resource_size_t size = 0, size0 = 0, size1 = 0;
resource_size_t children_add_size = 0;
resource_size_t min_align, align;
if (!b_res)
return;
+ if (b_res->parent)
+ return;
min_align = window_alignment(bus, IORESOURCE_IO);
list_for_each_entry(dev, &bus->devices, bus_list) {
@@ -1015,7 +1022,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
long mask,
resource_size_t min_align, align, size, size0, size1;
resource_size_t aligns[18]; /* Alignments from 1Mb to 128Gb */
int order, max_order;
- struct resource *b_res = find_free_bus_resource(bus,
+ struct resource *b_res = find_bus_resource_of_type(bus,
mask | IORESOURCE_PREFETCH, type);
resource_size_t children_add_size = 0;
resource_size_t children_add_align = 0;
@@ -1023,6 +1030,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
long mask,
if (!b_res)
return -ENOSPC;
+ if (b_res->parent)
+ return 0;
memset(aligns, 0, sizeof(aligns));
max_order = 0;
--
2.20.1
[PATCH v3 0/5] PCI: Patch series to support Thunderbolt without any BIOS support
I have split the last patch into the two patches as suggested. One to add the new parameter and another to rename them nicely. There is no longer depreciation of the old kernel parameters - they are simply dropped, as suggested. I would find it cleaner to just do a single patch without the depreciation, as both patches change the same things, doubling up on the number of patch lines. I can still do this if requested. Bjorn has fixed a trivial problem with the second patch in the series not applying cleanly due to changes since I first sent it. He said not to bother fixing it. Please let me know if there are any more issues to be addressed. Sorry I took longer than intended. Cold / flu. The code is not as thoroughly tested as my last revision but the changes are pretty simple and it still works as intended and compiles cleanly. I just hope I did not let a oopsie through. Nicholas Johnson (5): PCI: Consider alignment of hot-added bridges when distributing available resources PCI: Cleanup comments in setup-bus.c to meet kernel coding style guidelines PCI: Fix serious bug when sizing bridges with additional size PCI: modify kernel parameters to differentiate between MMIO and MMIO_PREF sizes PCI: Rename pci=hpiosize,hpmemsize,hpmemprefsize to be more readable .../admin-guide/kernel-parameters.txt | 7 +- drivers/pci/pci.c | 18 +- drivers/pci/setup-bus.c | 512 +- include/linux/pci.h | 3 +- 4 files changed, 287 insertions(+), 253 deletions(-) -- 2.20.1
[PATCH v3 1/5] PCI: Consider alignment of hot-added bridges when distributing available resources
Rewrite pci_bus_distribute_available_resources to better handle bridges
with different resource alignment requirements. Pass more details
arguments recursively to track the resource start and end addresses
relative to the initial hotplug bridge. This is especially useful for
Thunderbolt with native PCI enumeration, enabling external graphics
cards and other devices with bridge alignment higher than 0x10
bytes.
Change extend_bridge_window to resize the actual resource, rather than
using add_list and dev_res->add_size. If an additional resource entry
exists for the given resource, zero out the add_size field to avoid it
interfering. Because add_size is considered optional when allocating,
using add_size could cause issues in some cases, because successful
resource distribution requires sizes to be guaranteed. Such cases
include hot-adding nested hotplug bridges in one enumeration, and
potentially others which are yet to be encountered.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 203 ++--
1 file changed, 110 insertions(+), 93 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ed960436d..75827fb06 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1852,34 +1852,48 @@ 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 list_head *add_list, resource_size_t new_size)
{
struct pci_dev_resource *dev_res;
+ resource_size_t add_size;
if (res->parent)
return;
- if (resource_size(res) >= available)
- return;
-
- dev_res = res_to_dev_res(add_list, res);
- if (!dev_res)
- return;
+ /*
+* Resources requested using add_size in additional resource lists are
+* considered optional when allocated. Guaranteed size of allocation
+* is required to guarantee successful resource distribution. Hence,
+* the size of the actual resource must be adjusted.
+*/
+ if (new_size >= resource_size(res)) {
+ add_size = new_size - resource_size(res);
+ pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
+ &add_size);
+ } else {
+ add_size = resource_size(res) - new_size;
+ pci_dbg(bridge, "bridge window %pR shrunken by %pa\n", res,
+ &add_size);
+ }
- /* Is there room to extend the window? */
- if (available - resource_size(res) <= dev_res->add_size)
- return;
+ res->end = res->start + new_size - 1;
- dev_res->add_size = available - resource_size(res);
- pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
- &dev_res->add_size);
+ /*
+* If a list entry exists, we need to remove any additional size
+* requested because that could interfere with the alignment and
+* sizing done when distributing resources, causing resources to
+* fail to allocate later on.
+*/
+ dev_res = res_to_dev_res(add_list, res);
+ if (dev_res)
+ dev_res->add_size = 0;
}
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)
+ struct list_head *add_list, struct resource io,
+ struct resource mmio, struct resource mmio_pref)
{
- resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+ resource_size_t io_per_hp, mmio_per_hp, mmio_pref_per_hp, align;
unsigned int normal_bridges = 0, hotplug_bridges = 0;
struct resource *io_res, *mmio_res, *mmio_pref_res;
struct pci_dev *dev, *bridge = bus->self;
@@ -1889,25 +1903,32 @@ static void
pci_bus_distribute_available_resources(struct pci_bus *bus,
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.
+* The alignment of this bridge is yet to be considered, hence it must
+* be done now before extending its bridge window. A single bridge
+* might not be able to occupy the whole parent region if the alignment
+* differs - for example, an external GPU at the end of a Thunderbolt
+* daisy chain.
*/
- extend_bridge_window(bridge, io_res, add_list, available_io);
- extend_br
[PATCH v3 3/5] PCI: Fix serious bug when sizing bridges with additional size
Change find_free_bus_resource function to not skip assigned resources
with non-null parent.
Add checks in pbus_size_io and pbus_size_mem to return success if
resource returned from find_free_bus_resource is already allocated.
This avoids pbus_size_io and pbus_size_mem returning error code to
__pci_bus_size_bridges when a resource has been successfully assigned in
a previous pass. This fixes the existing behaviour where space for a
resource could be reserved multiple times in different parent bridge
windows. This also greatly reduces the number of failed BAR messages in
dmesg when Linux assigns resources.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 0ce641282..efe899b02 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -791,11 +791,16 @@ static void pci_bridge_check_ranges(struct pci_bus *bus)
}
/*
- * Helper function for sizing routines: find first available bus resource of a
- * given type. Note: we intentionally skip the bus resources which have already
- * been assigned (that is, have non-NULL parent resource).
+ * Helper function for sizing routines: find first bus resource of a
+ * given type. Note: we do not skip the bus resources which have already
+ * been assigned (r->parent != NULL). This is because a resource that is
+ * already assigned (nothing more to be done) will be indistinguishable
+ * from one that failed due to lack of space if we skip assigned
+ * resources. If the caller function cannot tell the difference then it
+ * might try to place the resources in a different window, doubling up on
+ * resources or causing unforeseeable issues.
*/
-static struct resource *find_free_bus_resource(struct pci_bus *bus,
+static struct resource *find_bus_resource_of_type(struct pci_bus *bus,
unsigned long type_mask, unsigned long type)
{
int i;
@@ -804,7 +809,7 @@ static struct resource *find_free_bus_resource(struct
pci_bus *bus,
pci_bus_for_each_resource(bus, r, i) {
if (r == &ioport_resource || r == &iomem_resource)
continue;
- if (r && (r->flags & type_mask) == type && !r->parent)
+ if (r && (r->flags & type_mask) == type)
return r;
}
return NULL;
@@ -903,14 +908,16 @@ static void pbus_size_io(struct pci_bus *bus,
resource_size_t min_size,
resource_size_t add_size, struct list_head *realloc_head)
{
struct pci_dev *dev;
- struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO,
- IORESOURCE_IO);
+ struct resource *b_res = find_bus_resource_of_type(bus, IORESOURCE_IO,
+ IORESOURCE_IO);
resource_size_t size = 0, size0 = 0, size1 = 0;
resource_size_t children_add_size = 0;
resource_size_t min_align, align;
if (!b_res)
return;
+ if (b_res->parent)
+ return;
min_align = window_alignment(bus, IORESOURCE_IO);
list_for_each_entry(dev, &bus->devices, bus_list) {
@@ -1015,7 +1022,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
long mask,
resource_size_t min_align, align, size, size0, size1;
resource_size_t aligns[18]; /* Alignments from 1Mb to 128Gb */
int order, max_order;
- struct resource *b_res = find_free_bus_resource(bus,
+ struct resource *b_res = find_bus_resource_of_type(bus,
mask | IORESOURCE_PREFETCH, type);
resource_size_t children_add_size = 0;
resource_size_t children_add_align = 0;
@@ -1023,6 +1030,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
long mask,
if (!b_res)
return -ENOSPC;
+ if (b_res->parent)
+ return 0;
memset(aligns, 0, sizeof(aligns));
max_order = 0;
--
2.20.1
[PATCH v3 2/5] PCI: Cleanup comments in setup-bus.c to meet kernel coding style guidelines
Change block comments to accepted style with asterisks on each line.
Justify block comments to 80-character limit to reduce the number of
lines where possible.
Signed-off-by: Nicholas Johnson
---
drivers/pci/setup-bus.c | 265
1 file changed, 130 insertions(+), 135 deletions(-)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 75827fb06..0ce641282 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -51,11 +51,9 @@ static void free_list(struct list_head *head)
/**
* add_to_list() - add a new resource tracker to the list
* @head: Head of the list
- * @dev: device corresponding to which the resource
- * belongs
+ * @dev: device corresponding to which the resource belongs
* @res: The resource to be tracked
- * @add_size: additional size to be optionally added
- * to the resource
+ * @add_size: additional size to be optionally added to the resource
*/
static int add_to_list(struct list_head *head,
struct pci_dev *dev, struct resource *res,
@@ -158,7 +156,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct
list_head *head)
tmp->res = r;
tmp->dev = dev;
- /* fallback is smallest one or list is empty*/
+ /* fallback is smallest one or list is empty */
n = head;
list_for_each_entry(dev_res, head, list) {
resource_size_t align;
@@ -171,7 +169,7 @@ static void pdev_sort_resources(struct pci_dev *dev, struct
list_head *head)
break;
}
}
- /* Insert it just before n*/
+ /* Insert it just before n */
list_add_tail(&tmp->list, n);
}
}
@@ -181,7 +179,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
{
u16 class = dev->class >> 8;
- /* Don't touch classless devices or host bridges or ioapics. */
+ /* Don't touch classless devices or host bridges or ioapics */
if (class == PCI_CLASS_NOT_DEFINED || class == PCI_CLASS_BRIDGE_HOST)
return;
@@ -208,12 +206,10 @@ static inline void reset_resource(struct resource *res)
*
* @realloc_head : head of the list tracking requests requiring additional
* resources
- * @head : head of the list tracking requests with allocated
- * resources
+ * @head : head of the list tracking requests with allocated resources
*
- * Walk through each element of the realloc_head and try to procure
- * additional resources for the element, provided the element
- * is in the head list.
+ * Walk through each element of the realloc_head and try to procure additional
+ * resources for the element, provided the element is in the head list.
*/
static void reassign_resources_sorted(struct list_head *realloc_head,
struct list_head *head)
@@ -315,10 +311,9 @@ static unsigned long pci_fail_res_type_mask(struct
list_head *fail_head)
mask |= fail_res->flags;
/*
-* one pref failed resource will set IORESOURCE_MEM,
-* as we can allocate pref in non-pref range.
-* Will release all assigned non-pref sibling resources
-* according to that bit.
+* one pref failed resource will set IORESOURCE_MEM, as we can allocate
+* pref in non-pref range. Will release all assigned non-pref sibling
+* resources according to that bit.
*/
return mask & (IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH);
}
@@ -351,25 +346,24 @@ static void __assign_resources_sorted(struct list_head
*head,
struct list_head *fail_head)
{
/*
-* Should not assign requested resources at first.
-* they could be adjacent, so later reassign can not reallocate
-* them one by one in parent resource window.
-* Try to assign requested + add_size at beginning
-* if could do that, could get out early.
-* if could not do that, we still try to assign requested at first,
-*then try to reassign add_size for some resources.
+* Should not assign requested resources at first. They could be
+* adjacent, so later reassign can not reallocate them one by one in
+* parent resource window.
+*
+* Try to assign requested + add_size at beginning. If could do that,
+* could get out early. If could not do that, we still try to assign
+* requested at first, then try to reassign add_size for some resources.
*
* Separate three resource type checking if we need to release
* assigned resource after requested + add_size try.
-* 1. if there is io port assign fail, will release assigned
-* io port.
-*
[PATCH v3 5/5] PCI: Rename pci=hpiosize,hpmemsize,hpmemprefsize to be more readable
Rename pci=hpiosize=nn[KMG] to pci=hp_io_size=nn[KMG]
Rename pci=hpmemsize=nn[KMG] to pci=hp_mmio_size=nn[KMG]
Rename pci=hpmemprefsize=nn[KMG] to pci=hp_mmio_pref_size=nn[KMG]
Update Documentation/admin-guide/kernel-parameters.txt to reflect the
above changes.
Update variable names in code to reflect the naming changes and become
more readable.
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 10
drivers/pci/pci.c | 23 ++-
drivers/pci/setup-bus.c | 23 ++-
include/linux/pci.h | 4 ++--
4 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 908d01027..66ffc0e7d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3326,15 +3326,15 @@
the default.
off: Turn ECRC off
on: Turn ECRC on.
- hpiosize=nn[KMG]The fixed amount of bus space which is
+ hp_io_size=nn[KMG] The fixed amount of bus space which is
reserved for hotplug bridge's IO window.
Default size is 256 bytes.
- hpmemsize=nn[KMG] The fixed amount of bus space which is
+ hp_mmio_size=nn[KMG]The fixed amount of bus space which is
reserved for hotplug bridge's memory window.
Default size is 2 megabytes.
- hpmemprefsize=nn[KMG] The fixed amount of bus space which is
- reserved for hotplug bridge's MMIO_PREF window.
- Default size is 2 megabytes.
+ hp_mmio_pref_size=nn[KMG] The fixed amount of bus space
+ which is reserved for hotplug bridge's MMIO_PREF
+ window. Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
Default is 1.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c36c92118..5ecc432a5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -85,12 +85,12 @@ unsigned long pci_cardbus_io_size = DEFAULT_CARDBUS_IO_SIZE;
unsigned long pci_cardbus_mem_size = DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
-#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
-#define DEFAULT_HOTPLUG_MEM_PREF_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MMIO_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
-unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
-unsigned long pci_hotplug_mem_pref_size = DEFAULT_HOTPLUG_MEM_PREF_SIZE;
+unsigned long pci_hotplug_mmio_size = DEFAULT_HOTPLUG_MMIO_SIZE;
+unsigned long pci_hotplug_mmio_pref_size = DEFAULT_HOTPLUG_MMIO_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6177,13 +6177,14 @@ static int __init pci_setup(char *str)
strlen(str + 19));
} else if (!strncmp(str, "ecrc=", 5)) {
pcie_ecrc_get_policy(str + 5);
- } else if (!strncmp(str, "hpiosize=", 9)) {
- pci_hotplug_io_size = memparse(str + 9, &str);
- } else if (!strncmp(str, "hpmemsize=", 10)) {
- pci_hotplug_mem_size = memparse(str + 10, &str);
- } else if (!strncmp(str, "hpmemprefsize=", 14)) {
- pci_hotplug_mem_pref_size =
- memparse(str + 14, &str);
+ } else if (!strncmp(str, "hp_io_size=", 11)) {
+ pci_hotplug_io_size = memparse(str + 11, &str);
+ } else if (!strncmp(str, "hp_mmio_size=", 13)) {
+ pci_hotplug_mmio_size = memparse(str + 13,
+ &str);
+ } else if (!strncmp(str, "hp_mmio_pref_size=", 18)) {
+ pci_hotplug_mmio_pref_size =
+ memparse(str + 18, &str);
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
[PATCH v3 4/5] PCI: modify kernel parameters to differentiate between MMIO and MMIO_PREF sizes
Add kernel parameter pci=hpmemprefsize=nn[KMG] to control MMIO_PREF
size.
Change behaviour of pci=hpmemsize=nn[KMG] to only control MMIO size,
rather than controlling both MMIO and MMIO_PREF sizes.
Update kernel-parameters documentation to reflect the above changes.
Signed-off-by: Nicholas Johnson
---
.../admin-guide/kernel-parameters.txt | 3 +++
drivers/pci/pci.c | 5 +
drivers/pci/setup-bus.c | 22 ++-
include/linux/pci.h | 1 +
4 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 858b6c0b9..908d01027 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3332,6 +3332,9 @@
hpmemsize=nn[KMG] The fixed amount of bus space which is
reserved for hotplug bridge's memory window.
Default size is 2 megabytes.
+ hpmemprefsize=nn[KMG] The fixed amount of bus space which is
+ reserved for hotplug bridge's MMIO_PREF window.
+ Default size is 2 megabytes.
hpbussize=nnThe minimum amount of additional bus numbers
reserved for buses below a hotplug bridge.
Default is 1.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c25acace7..c36c92118 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -86,9 +86,11 @@ unsigned long pci_cardbus_mem_size =
DEFAULT_CARDBUS_MEM_SIZE;
#define DEFAULT_HOTPLUG_IO_SIZE(256)
#define DEFAULT_HOTPLUG_MEM_SIZE (2*1024*1024)
+#define DEFAULT_HOTPLUG_MEM_PREF_SIZE (2*1024*1024)
/* pci=hpmemsize=nnM,hpiosize=nn can override this */
unsigned long pci_hotplug_io_size = DEFAULT_HOTPLUG_IO_SIZE;
unsigned long pci_hotplug_mem_size = DEFAULT_HOTPLUG_MEM_SIZE;
+unsigned long pci_hotplug_mem_pref_size = DEFAULT_HOTPLUG_MEM_PREF_SIZE;
#define DEFAULT_HOTPLUG_BUS_SIZE 1
unsigned long pci_hotplug_bus_size = DEFAULT_HOTPLUG_BUS_SIZE;
@@ -6179,6 +6181,9 @@ static int __init pci_setup(char *str)
pci_hotplug_io_size = memparse(str + 9, &str);
} else if (!strncmp(str, "hpmemsize=", 10)) {
pci_hotplug_mem_size = memparse(str + 10, &str);
+ } else if (!strncmp(str, "hpmemprefsize=", 14)) {
+ pci_hotplug_mem_pref_size =
+ memparse(str + 14, &str);
} else if (!strncmp(str, "hpbussize=", 10)) {
pci_hotplug_bus_size =
simple_strtoul(str + 10, &str, 0);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index efe899b02..3806c9ff0 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1223,7 +1223,8 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
{
struct pci_dev *dev;
unsigned long mask, prefmask, type2 = 0, type3 = 0;
- resource_size_t additional_mem_size = 0, additional_io_size = 0;
+ resource_size_t additional_io_size = 0, additional_mem_size = 0,
+ additional_mem_pref_size = 0;
struct resource *b_res;
int ret;
@@ -1258,6 +1259,7 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
if (bus->self->is_hotplug_bridge) {
additional_io_size = pci_hotplug_io_size;
additional_mem_size = pci_hotplug_mem_size;
+ additional_mem_pref_size = pci_hotplug_mem_pref_size;
}
/* Fall through */
default:
@@ -1274,9 +1276,9 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
if (b_res[2].flags & IORESOURCE_MEM_64) {
prefmask |= IORESOURCE_MEM_64;
ret = pbus_size_mem(bus, prefmask, prefmask,
- prefmask, prefmask,
- realloc_head ? 0 : additional_mem_size,
- additional_mem_size, realloc_head);
+ prefmask, prefmask,
+ realloc_head ? 0 : additional_mem_pref_size,
+ additional_mem_pref_size, realloc_head);
/*
* If successful, all non-prefetchable resources and any
@@ -1298,9 +1300,9 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct
list_head *realloc_head)
if (!type2) {
[PATCH] PCI: Consider alignment of hot-added bridges when distributing available resources
New systems with Thunderbolt are starting to use native PCI enumeration. Mika Westerberg's patch "PCI: Distribute available resources to hotplug capable PCIe downstream ports" (https://patchwork.kernel.org/patch/9972155/) adds code to expand downstream PCI hotplug bridges to consume all remaining resource space in the parent bridge. It is a crucial patch for supporting Thunderbolt native enumeration on Linux. However, it does not consider bridge alignment in all cases, which rules out hot-adding an external graphics processor at the end of a Thunderbolt daisy chain. Hot-adding such a device will likely fail to work with the existing code. It also might disrupt the operation of existing devices or prevent the subsequent hot-plugging of lower aligned devices if the kernel frees and reallocates upstream resources to attempt assign the resources that failed to assign in the first pass. By Intel's ruling, Thunderbolt external graphics processors are generally meant to function only as the first and only device in the chain. However, there is no technical reason that prevents it from being possible if sufficient resources are available, and people are likely to attempt such configurations with Thunderbolt devices if they own such hardware. Hence, I argue that we should improve the user experience and reduce the number of constraints placed on the user by always considering resource alignment, which will make such configurations possible. The other problem with the patch is that it is incompatible with resources allocated by "pci=hpmemsize=nnM" due to a check which does not allow for dev_res->add_size to be reduced. This check also makes a big assumption that the hpmemsize is small but non-zero, and no action has been taken to ensure that. In the current state, any bridge smaller than hpmemsize will likely fail to size correctly, which will cause major issues if the default were to change, or if the user also wants to configure non-Thunderbolt hotplug bridges simultaneously. I argue that if assumptions and limitations can be removed with no risks and adverse effects, then it should be done. The former problem is solved by rewriting the pci_bus_distribute_available_resources() function with more information passed in the arguments, eliminating assumptions about the initial bridge alignment. My patch makes no assumptions about the alignment of hot-added bridges, allowing for any device to be hot-added, given sufficient resources are available. The latter problem is solved by removing the check preventing the shrinking of dev_res->add_size, which allows for the distribution of resources if hpmemsize is non-zero. It can be made to work with zero hpmemsize with two-line patches in pbus_size_mem() and pbus_size_io(), or by modifying extend_bridge_window() to add a new entry to the additional resource list if one does not exist. In theory, and by my testing, the removal of this check does not affect the functionality of the resource distribution with firmware-allocated resources. But it does enable the same functionality when using pci=hpmemsize=nnM, which was not possible prior. This may be one piece of the puzzle needed to support Thunderbolt add-in cards that support native PCI enumeration, without any platform dependencies. I have tested this proposed patch extensively. Using Linux-allocated resources, it works perfectly. I have two Gigabyte GC-TITAN RIDGE Thunderbolt 3 add-in cards in my desktop, and a Dell XPS 9370 with the Dell XPS 9380 Thunderbolt NVM40 firmware flashed. My peripherals are three HP ZBook Thunderbolt 3 docks, two external graphics enclosures with AMD Fiji XT in both, a Promise SANLink3 N1 (AQC107S), and a Dell Thunderbolt 3 NVMe SSD. All configurations of these devices worked well, and I can no longer make it fail if I try to. My testing with firmware-allocated resources is limited due to using computers with Alpine Ridge BIOS support. However, I did get manage to test the patch with firmware-allocated resources by enabling the Alpine Ridge BIOS support and forcing pcie_ports=native, and the results were perfect. Mika Westerberg has agreed to test this on an official platform with native enumeration firmware support to be sure that it works in this situation. It is also appropriate that he reviews these changes as he wrote the original code and any changes made to Thunderbolt-critical code cannot be allowed to break any of the base requirements for Thunderbolt. I would like to thank him for putting up with my emails and trying to help where he can, and for the great work he has done on Thunderbolt in Linux. I have more patches in the pipeline to further improve the Thunderbolt experience on Linux on platforms without BIOS support. This is the most technical but least user-facing one planned. The most exciting changes are yet to come. Signed-off-by: Nicholas-Johnson-opensource --- drivers/pci/setup-bus.c | 172 ---
Re: [PATCH 0/1] Fiji GPU audio register timeout when in BACO state
On Sat, May 02, 2020 at 12:09:13PM +0200, Takashi Iwai wrote:
> On Sat, 02 May 2020 09:27:31 +0200,
> Takashi Iwai wrote:
> >
> > On Sat, 02 May 2020 09:17:28 +0200,
> > Lukas Wunner wrote:
> > >
> > > On Sat, May 02, 2020 at 09:11:58AM +0200, Takashi Iwai wrote:
> > > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > @@ -673,6 +673,12 @@ static int amdgpu_dm_audio_component_bind(struct
> > > > device *kdev,
> > > > struct amdgpu_device *adev = dev->dev_private;
> > > > struct drm_audio_component *acomp = data;
> > > >
> > > > + if (!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS |
> > > > +DL_FLAG_PM_RUNTIME)) {
> > > > + DRM_ERROR("DM: cannot add device link to audio
> > > > device\n");
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > >
> > > Doesn't this duplicate drivers/pci/quirks.c:quirk_gpu_hda() ?
> >
> > Gah, you're right, that was the place I overlooked.
> > It was a typical "false Eureka right-after-wakeup" phenomenon :)
> > Need a vaccine aka coffee...
> >
> > So the runtime PM dependency must be already placed there, and the
> > problem is not the lack of the dependency tree but the really other
> > timing issue. Back to square.
>
> One interesting test is to open the stream while the mode isn't set
> yet and see whether the same problem appears.
> Namely, after the monitor is connected but no mode is set, run
> directly like
>aplay -Dhdmi:1,0 foo.wav
> You might need to wrap the command with pasuspender if PA is active.
I could not figure out how to get the interface for aplay set other than
not specifying it and having it find the default device (which can
change). I even used aplay -L and aplay -l to show devices. I could not
get it working.
Is there anything else I can try? I did not apply the last patch when it
was pointed out that it is already a quirk.
Regards,
Nicholas
>
>
> Takashi
