| Message ID | 20190615002359.29577-1-benh@kernel.crashing.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/4] arm64: pci: acpi: Use pci_assign_unassigned_root_bus_resources() | expand |
On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt wrote: > Instead of the simpler > > pci_bus_size_bridges(bus); > pci_bus_assign_resources(bus); > > Use pci_assign_unassigned_root_bus_resources(). This should have no effect > as long as we are reassigning everything. Once we start honoring FW > resource allocations, this will bring up the "reallocation" feature > which can help making room for SR-IOV when necessary. I would like to add more details on why we want to make this change, I will update the log when we merge it, it is a bit too late for v5.3, even if in theory no functional change is intended. > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > arch/arm64/kernel/pci.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index bb85e2f4603f..1419b1b4e9b9 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > if (!bus) > return NULL; > > - pci_bus_size_bridges(bus); > - pci_bus_assign_resources(bus); > + pci_assign_unassigned_root_bus_resources(bus); These hunks should be identical, minus the additional resource size handling and realloc policy (which are *missing* features in current code). We must document this change in the log. Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > list_for_each_entry(child, &bus->children, node) > pcie_bus_configure_settings(child); > -- > 2.17.1 >
On Thu, 2019-06-20 at 18:13 +0100, Lorenzo Pieralisi wrote: > On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt wrote: > > Instead of the simpler > > > > pci_bus_size_bridges(bus); > > pci_bus_assign_resources(bus); > > > > Use pci_assign_unassigned_root_bus_resources(). This should have no effect > > as long as we are reassigning everything. Once we start honoring FW > > resource allocations, this will bring up the "reallocation" feature > > which can help making room for SR-IOV when necessary. > > I would like to add more details on why we want to make this change, > I will update the log when we merge it, it is a bit too late for v5.3, > even if in theory no functional change is intended. Ok. The why is that a subsequent patch will turn the code into if (claim) claim() pci_assign_unassigned* And I wanted the patch replacing the existing 2 calls with the above separate and bisectable in case we missed some odd effect of the change. Cheers, Ben. > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > --- > > arch/arm64/kernel/pci.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > > index bb85e2f4603f..1419b1b4e9b9 100644 > > --- a/arch/arm64/kernel/pci.c > > +++ b/arch/arm64/kernel/pci.c > > @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > > if (!bus) > > return NULL; > > > > - pci_bus_size_bridges(bus); > > - pci_bus_assign_resources(bus); > > + pci_assign_unassigned_root_bus_resources(bus); > > These hunks should be identical, minus the additional resource size > handling and realloc policy (which are *missing* features in current > code). We must document this change in the log. > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > list_for_each_entry(child, &bus->children, node) > > pcie_bus_configure_settings(child); > > -- > > 2.17.1 > >
Match the subject line convention, e.g., arm64: PCI: Use pci_assign_unassigned_root_bus_resources() But the function name doesn't really tell us anything unless we already know how everything works. I think the point is that pci_assign_unassigned_root_bus_resources() gives us the possibility of reallocating things if necessary. A subject that hints at that would be good. On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt wrote: > Instead of the simpler > > pci_bus_size_bridges(bus); > pci_bus_assign_resources(bus); > > Use pci_assign_unassigned_root_bus_resources(). This should have no > effect as long as we are reassigning everything. pci_bus_size_bridges(bus) == __pci_bus_size_bridges(bus, NULL) pci_bus_assign_resources(bus) == __pci_bus_assign_resources(bus, NULL, NULL) and we have: pci_assign_unassigned_root_bus_resources() { ... __pci_bus_size_bridges(bus, add_list); __pci_bus_assign_resources(bus, add_list, &fail_head); so I guess this should have no effect as long as we were able to assign everything. If we were unable to assign something, previously we did nothing and left it unassigned, but after this patch, we will attempt to do some reallocation. Right? > Once we start honoring FW resource allocations, this will bring up > the "reallocation" feature which can help making room for SR-IOV > when necessary. I think this should be reordered so it's immediately before the patch that checks hb->preserve_config, i.e., the patch that honors FW assignments. > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > arch/arm64/kernel/pci.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index bb85e2f4603f..1419b1b4e9b9 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > if (!bus) > return NULL; > > - pci_bus_size_bridges(bus); > - pci_bus_assign_resources(bus); > + pci_assign_unassigned_root_bus_resources(bus); > > list_for_each_entry(child, &bus->children, node) > pcie_bus_configure_settings(child); > -- > 2.17.1 >
On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt wrote: > Instead of the simpler > > pci_bus_size_bridges(bus); > pci_bus_assign_resources(bus); > > Use pci_assign_unassigned_root_bus_resources(). This should have no effect > as long as we are reassigning everything. Once we start honoring FW > resource allocations, this will bring up the "reallocation" feature > which can help making room for SR-IOV when necessary. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> I applied these to pci/resource, with my comments and acks from Lorenzo and Ard. Let me know if I was too aggressive or got something wrong; I consider these branches malleable until the merge window. Thanks for the first step on this long journey :) > --- > arch/arm64/kernel/pci.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index bb85e2f4603f..1419b1b4e9b9 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > if (!bus) > return NULL; > > - pci_bus_size_bridges(bus); > - pci_bus_assign_resources(bus); > + pci_assign_unassigned_root_bus_resources(bus); > > list_for_each_entry(child, &bus->children, node) > pcie_bus_configure_settings(child); > -- > 2.17.1 >
On Fri, 2019-06-21 at 15:48 -0500, Bjorn Helgaas wrote: > On Sat, Jun 15, 2019 at 10:23:56AM +1000, Benjamin Herrenschmidt > wrote: > > Instead of the simpler > > > > pci_bus_size_bridges(bus); > > pci_bus_assign_resources(bus); > > > > Use pci_assign_unassigned_root_bus_resources(). This should have no > > effect > > as long as we are reassigning everything. Once we start honoring FW > > resource allocations, this will bring up the "reallocation" feature > > which can help making room for SR-IOV when necessary. > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > I applied these to pci/resource, with my comments and acks from > Lorenzo and Ard. Let me know if I was too aggressive or got > something > wrong; I consider these branches malleable until the merge window. > > Thanks for the first step on this long journey :) Heh thanks :-) Cheers, Ben. > > --- > > arch/arm64/kernel/pci.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > > index bb85e2f4603f..1419b1b4e9b9 100644 > > --- a/arch/arm64/kernel/pci.c > > +++ b/arch/arm64/kernel/pci.c > > @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct > > acpi_pci_root *root) > > if (!bus) > > return NULL; > > > > - pci_bus_size_bridges(bus); > > - pci_bus_assign_resources(bus); > > + pci_assign_unassigned_root_bus_resources(bus); > > > > list_for_each_entry(child, &bus->children, node) > > pcie_bus_configure_settings(child); > > -- > > 2.17.1 > >
BTW... You probably want to swap those 2: 2 hours PCI/ACPI: Evaluate PCI Boot Configuration _DSM Benjamin Herrenschmidt 3 -3/+18 2 hours PCI: Don't auto-realloc if we're preserving firmware config As "Don't auto-realloc..." tests a flag that is only created by "Evaluate PCI Boot..." Cheers, Ben.
On Sat, Jun 22, 2019 at 09:00:50AM +1000, Benjamin Herrenschmidt wrote: > BTW... > > You probably want to swap those 2: > > 2 hours PCI/ACPI: Evaluate PCI Boot Configuration _DSM Benjamin Herrenschmidt 3 -3/+18 > 2 hours PCI: Don't auto-realloc if we're preserving firmware config > > As "Don't auto-realloc..." tests a flag that is only created by "Evaluate PCI Boot..." Ouch, thanks. I don't know how I managed to swap those.
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index bb85e2f4603f..1419b1b4e9b9 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -193,8 +193,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) if (!bus) return NULL; - pci_bus_size_bridges(bus); - pci_bus_assign_resources(bus); + pci_assign_unassigned_root_bus_resources(bus); list_for_each_entry(child, &bus->children, node) pcie_bus_configure_settings(child);
Instead of the simpler pci_bus_size_bridges(bus); pci_bus_assign_resources(bus); Use pci_assign_unassigned_root_bus_resources(). This should have no effect as long as we are reassigning everything. Once we start honoring FW resource allocations, this will bring up the "reallocation" feature which can help making room for SR-IOV when necessary. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- arch/arm64/kernel/pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)