Re: [PATCH v3] powerpc/pseries: use pci_host_bridge.release_fn() to kfree(phb)
From: Andrew Donnellan <hidden>
Date: 2016-08-11 05:01:55
On 11/08/16 07:45, Mauricio Faria de Oliveira wrote:
This patch leverages 'struct pci_host_bridge' from the PCI subsystem in order to free the pci_controller only after the last reference to its devices is dropped (avoiding an oops in pcibios_release_device() if the last reference is dropped after pcibios_free_controller()). The patch relies on pci_host_bridge.release_fn() (and .release_data), which is called automatically by the PCI subsystem when the root bus is released (i.e., the last reference is dropped). Those fields are set via pci_set_host_bridge_release() (e.g. in the platform-specific implementation of pcibios_root_bridge_prepare()). It introduces the 'pcibios_host_bridge_release()' function to be set as .release_fn(), which expects .release_data to hold the pointer to the pci_controller to kfree(). It enables that functionality for pseries (although it isn't platform -specific, and may be used by cxl). It keeps pcibios_free_controller() backwards-compatible (i.e., kfree(phb) in it) in case no .release_fn() is defined for the pci_controller. Details on not-so-elegant design choices: - Added 'pci_controller.bridge' field (pointer to associated 'struct pci_host_bridge') so *not* to use 'pci_find_host_bridge(phb->bus)' in pcibios_free_controller(). That's because remove_phb_dynamic() sets 'phb->bus = NULL' before pcibios_free_controller(). That seems to be very important, with commit title 'powerpc/pci: Fix various pseries PCI hotplug issues' (so I'll not remove it just to avoid this null pointer dereference). - Used 'pci_host_bridge.release_data' field (pointer to associated 'struct pci_controller') so *not* to 'pci_bus_to_host(bridge->bus)' in pcibios_host_bridge_release(). That's because pci_remove_root_bus() sets 'host_bridge->bus = NULL' (so, if the last reference is released after pci_remove_root_bus() runs, which eventually reaches pcibios_host_bridge_release(), that would hit a null pointer dereference). The cxl/vphb.c code calls pci_remove_root_bus(), and the cxl folks are interested in this fix.
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(). It seems slightly wrong for me to write: struct pci_bus *bus = phb->bus; pcibios_free_controller(phb); pci_remove_root_bus(bus); instead. Thoughts?
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 <redacted> Signed-off-by: Mauricio Faria de Oliveira <redacted>
Missing a '---' here :)
quoted hunk ↗ jump to hunk
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 andrew.donnellan@au1.ibm.com IBM Australia Limited