On Thu, Aug 11, 2016 at 03:01:44PM +1000, Andrew Donnellan wrote: >On 11/08/16 07:45, Mauricio Faria de Oliveira wrote: > >In cxl, we currently call: > > pci_remove_root_bus(phb->bus); > pcibios_free_controller(phb); > >which appears to break with this patch after I wire up >pci_set_host_bridge_release() in cxl, as phb can be freed before we call >pcibios_free_controller(). >
pcibios_free_controller() isn't called when bridge's release_fn() is wired up by pci_set_host_bridge_release(). So you can keep the code as what you have and no changes needed. >It seems slightly wrong for me to write: > > struct pci_bus *bus = phb->bus; > pcibios_free_controller(phb); > pci_remove_root_bus(bus); > Yeah, the sequence needs to be reversed. The released PHB instance could possibly be accessed inside pci_remove_root_bus(). Thanks, Gavin > >> >>Test-case: >> >> # ls -ld /sys/block/sd* | grep -m1 0021:01:00.0 >> <...> /sys/block/sdaa -> ../devices/pci0021:01/0021:01:00.0/<...> >> >> # ls -ld /sys/block/sd* | grep -m1 0021:01:00.1 >> <...> /sys/block/sdab -> ../devices/pci0021:01/0021:01:00.1/<...> >> >> # cat >/dev/sdaa & pid1=$! >> # cat >/dev/sdab & pid2=$! >> >> # drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r >> Validating PHB DLPAR capability...yes. >> [ 479.547020] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01 >> [ 479.547049] pci_hp_remove_devices: Removing 0021:01:00.0... >> ... >> [ 483.536303] pci_hp_remove_devices: Removing 0021:01:00.1... >> ... >> [ 497.072130] pci_bus 0021:01: busn_res: [bus 01-ff] is released >> [ 497.072209] rpadlpar_io: slot PHB 33 removed >> >> # kill -9 $pid1 >> # kill -9 $pid2 >> [ 506.604458] pcibios_host_bridge_release: domain 33, dynamic 1 >> >>Suggested-By: Gavin Shan <[email protected]> >>Signed-off-by: Mauricio Faria de Oliveira <[email protected]> >> > >Missing a '---' here :) > >>Changelog: >> - v3: different approach: struct pci_host_bridge.release_fn() >> - v2: different approach: struct pci_controller.refcount >>--- >> arch/powerpc/include/asm/pci-bridge.h | 2 ++ >> arch/powerpc/kernel/pci-common.c | 15 ++++++++++++++- >> arch/powerpc/platforms/pseries/pci.c | 3 +++ >> 3 files changed, 19 insertions(+), 1 deletion(-) >> >>diff --git a/arch/powerpc/include/asm/pci-bridge.h >>b/arch/powerpc/include/asm/pci-bridge.h >>index b5e88e4..9b11631 100644 >>--- a/arch/powerpc/include/asm/pci-bridge.h >>+++ b/arch/powerpc/include/asm/pci-bridge.h >>@@ -54,6 +54,7 @@ struct pci_controller_ops { >> */ >> struct pci_controller { >> struct pci_bus *bus; >>+ struct pci_host_bridge *bridge; /* associated 'PHB' in PCI subsystem */ >> char is_dynamic; >> #ifdef CONFIG_PPC64 >> int node; >>@@ -301,6 +302,7 @@ extern void pci_process_bridge_OF_ranges(struct >>pci_controller *hose, >> /* Allocate & free a PCI host bridge structure */ >> extern struct pci_controller *pcibios_alloc_controller(struct device_node >> *dev); >> extern void pcibios_free_controller(struct pci_controller *phb); >>+extern void pcibios_host_bridge_release(struct pci_host_bridge *bridge); >> >> #ifdef CONFIG_PCI >> extern int pcibios_vaddr_is_ioport(void __iomem *address); >>diff --git a/arch/powerpc/kernel/pci-common.c >>b/arch/powerpc/kernel/pci-common.c >>index a5c0153..c5b5f60 100644 >>--- a/arch/powerpc/kernel/pci-common.c >>+++ b/arch/powerpc/kernel/pci-common.c >>@@ -145,11 +145,23 @@ void pcibios_free_controller(struct pci_controller *phb) >> list_del(&phb->list_node); >> spin_unlock(&hose_spinlock); >> >>- if (phb->is_dynamic) >>+ /* if the associated pci_host_bridge has a release_fn(), rely on that. >>*/ >>+ if (!phb->bridge->release_fn && phb->is_dynamic) >> kfree(phb); >> } >> EXPORT_SYMBOL_GPL(pcibios_free_controller); >> >>+void pcibios_host_bridge_release(struct pci_host_bridge *bridge) >>+{ >>+ struct pci_controller *phb = (struct pci_controller *) >>bridge->release_data; >>+ >>+ pr_debug("domain %d, dynamic %d\n", phb->global_number, >>phb->is_dynamic); >>+ >>+ if (phb->is_dynamic) >>+ kfree(phb); >>+} >>+EXPORT_SYMBOL_GPL(pcibios_host_bridge_release); > >I figure this is being exported so we can use it in cxl. > >-- >Andrew Donnellan OzLabs, ADL Canberra >[email protected] IBM Australia Limited
