| Message ID | 1472662156-6238-1-git-send-email-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 31, 2016 at 10:49:16AM -0600, Tim Gardner wrote: > From: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> > > BugLink: http://bugs.launchpad.net/bugs/1618151 > > 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_free_controller_deferred()' .release_fn() > and it expects .release_data to hold a pointer to the pci_controller. > > The function implictly calls 'pcibios_free_controller()', so an user > must *NOT* explicitly call it if using the new _deferred() callback. > > The functionality is enabled for pseries (although it isn't platform > specific, and may be used by cxl). > > Details on not-so-elegant design choices: > > - Use 'pci_host_bridge.release_data' field as pointer to associated > 'struct pci_controller' so *not* to 'pci_bus_to_host(bridge->bus)' > in pcibios_free_controller_deferred(). > > 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_free_controller_deferred(), > 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. > > Test-case #1 (hold references) > > # 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. > [ 594.306719] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01 > [ 594.306738] pci_hp_remove_devices: Removing 0021:01:00.0... > ... > [ 598.236381] pci_hp_remove_devices: Removing 0021:01:00.1... > ... > [ 611.972077] pci_bus 0021:01: busn_res: [bus 01-ff] is released > [ 611.972140] rpadlpar_io: slot PHB 33 removed > > # kill -9 $pid1 > # kill -9 $pid2 > [ 632.918088] pcibios_free_controller_deferred: domain 33, dynamic 1 > > Test-case #2 (don't hold references) > > # drmgr -w 5 -d 1 -c phb -s 'PHB 33' -r > Validating PHB DLPAR capability...yes. > [ 916.357363] pci_hp_remove_devices: PCI: Removing devices on bus 0021:01 > [ 916.357386] pci_hp_remove_devices: Removing 0021:01:00.0... > ... > [ 920.566527] pci_hp_remove_devices: Removing 0021:01:00.1... > ... > [ 933.955873] pci_bus 0021:01: busn_res: [bus 01-ff] is released > [ 933.955977] pcibios_free_controller_deferred: domain 33, dynamic 1 > [ 933.955999] rpadlpar_io: slot PHB 33 removed > > Suggested-By: Gavin Shan <gwshan@linux.vnet.ibm.com> > Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com> > Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > Tested-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> # cxl > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > (cherry picked from commit 2dd9c11b9d4dfbd6c070eab7b81197f65e82f1a0) > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > arch/powerpc/include/asm/pci-bridge.h | 1 + > arch/powerpc/kernel/pci-common.c | 36 ++++++++++++++++++++++++++++++ > arch/powerpc/platforms/pseries/pci.c | 4 ++++ > arch/powerpc/platforms/pseries/pci_dlpar.c | 7 ++++-- > 4 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h > index 54843ca..7293a17 100644 > --- a/arch/powerpc/include/asm/pci-bridge.h > +++ b/arch/powerpc/include/asm/pci-bridge.h > @@ -296,6 +296,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_free_controller_deferred(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 880880c..e48b276 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -151,6 +151,42 @@ void pcibios_free_controller(struct pci_controller *phb) > EXPORT_SYMBOL_GPL(pcibios_free_controller); > > /* > + * This function is used to call pcibios_free_controller() > + * in a deferred manner: a callback from the PCI subsystem. > + * > + * _*DO NOT*_ call pcibios_free_controller() explicitly if > + * this is used (or it may access an invalid *phb pointer). > + * > + * The callback occurs when all references to the root bus > + * are dropped (e.g., child buses/devices and their users). > + * > + * It's called as .release_fn() of 'struct pci_host_bridge' > + * which is associated with the 'struct pci_controller.bus' > + * (root bus) - it expects .release_data to hold a pointer > + * to 'struct pci_controller'. > + * > + * In order to use it, register .release_fn()/release_data > + * like this: > + * > + * pci_set_host_bridge_release(bridge, > + * pcibios_free_controller_deferred > + * (void *) phb); > + * > + * e.g. in the pcibios_root_bridge_prepare() callback from > + * pci_create_root_bus(). > + */ > +void pcibios_free_controller_deferred(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); > + > + pcibios_free_controller(phb); > +} > +EXPORT_SYMBOL_GPL(pcibios_free_controller_deferred); > + > +/* > * The function is used to return the minimal alignment > * for memory or I/O windows of the associated P2P bridge. > * By default, 4KiB alignment for I/O windows and 1MiB for > diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c > index fe16a50..09eba5a 100644 > --- a/arch/powerpc/platforms/pseries/pci.c > +++ b/arch/powerpc/platforms/pseries/pci.c > @@ -119,6 +119,10 @@ int pseries_root_bridge_prepare(struct pci_host_bridge *bridge) > > bus = bridge->bus; > > + /* Rely on the pcibios_free_controller_deferred() callback. */ > + pci_set_host_bridge_release(bridge, pcibios_free_controller_deferred, > + (void *) pci_bus_to_host(bus)); > + > dn = pcibios_get_phb_of_node(bus); > if (!dn) > return 0; > diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c > index 5d4a3df..d3b8b82 100644 > --- a/arch/powerpc/platforms/pseries/pci_dlpar.c > +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c > @@ -138,8 +138,11 @@ int remove_phb_dynamic(struct pci_controller *phb) > release_resource(res); > } > > - /* Free pci_controller data structure */ > - pcibios_free_controller(phb); > + /* > + * The pci_controller data structure is freed by > + * the pcibios_free_controller_deferred() callback; > + * see pseries_root_bridge_prepare(). > + */ > > return 0; > } > -- > 2.7.4 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h index 54843ca..7293a17 100644 --- a/arch/powerpc/include/asm/pci-bridge.h +++ b/arch/powerpc/include/asm/pci-bridge.h @@ -296,6 +296,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_free_controller_deferred(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 880880c..e48b276 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -151,6 +151,42 @@ void pcibios_free_controller(struct pci_controller *phb) EXPORT_SYMBOL_GPL(pcibios_free_controller); /* + * This function is used to call pcibios_free_controller() + * in a deferred manner: a callback from the PCI subsystem. + * + * _*DO NOT*_ call pcibios_free_controller() explicitly if + * this is used (or it may access an invalid *phb pointer). + * + * The callback occurs when all references to the root bus + * are dropped (e.g., child buses/devices and their users). + * + * It's called as .release_fn() of 'struct pci_host_bridge' + * which is associated with the 'struct pci_controller.bus' + * (root bus) - it expects .release_data to hold a pointer + * to 'struct pci_controller'. + * + * In order to use it, register .release_fn()/release_data + * like this: + * + * pci_set_host_bridge_release(bridge, + * pcibios_free_controller_deferred + * (void *) phb); + * + * e.g. in the pcibios_root_bridge_prepare() callback from + * pci_create_root_bus(). + */ +void pcibios_free_controller_deferred(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); + + pcibios_free_controller(phb); +} +EXPORT_SYMBOL_GPL(pcibios_free_controller_deferred); + +/* * The function is used to return the minimal alignment * for memory or I/O windows of the associated P2P bridge. * By default, 4KiB alignment for I/O windows and 1MiB for diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c index fe16a50..09eba5a 100644 --- a/arch/powerpc/platforms/pseries/pci.c +++ b/arch/powerpc/platforms/pseries/pci.c @@ -119,6 +119,10 @@ int pseries_root_bridge_prepare(struct pci_host_bridge *bridge) bus = bridge->bus; + /* Rely on the pcibios_free_controller_deferred() callback. */ + pci_set_host_bridge_release(bridge, pcibios_free_controller_deferred, + (void *) pci_bus_to_host(bus)); + dn = pcibios_get_phb_of_node(bus); if (!dn) return 0; diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c index 5d4a3df..d3b8b82 100644 --- a/arch/powerpc/platforms/pseries/pci_dlpar.c +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c @@ -138,8 +138,11 @@ int remove_phb_dynamic(struct pci_controller *phb) release_resource(res); } - /* Free pci_controller data structure */ - pcibios_free_controller(phb); + /* + * The pci_controller data structure is freed by + * the pcibios_free_controller_deferred() callback; + * see pseries_root_bridge_prepare(). + */ return 0; }