| Message ID | 20250404-pcie-reset-slot-v1-0-98952918bf90@linaro.org |
|---|---|
| Headers | show |
| Series | PCI: Add support for resetting the slots in a platform specific way | expand |
On Fri, Apr 04, 2025 at 01:52:22PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > When the PCI error handling requires resetting the slot, reset it using the > host bridge specific 'reset_slot' callback if available before calling the > 'slot_reset' callback of the PCI drivers. > > The 'reset_slot' callback is responsible for resetting the given slot > referenced by the 'pci_dev' pointer in a platform specific way and bring it > back to the working state if possible. If any error occurs during the slot > reset operation, relevant errno should be returned. [...] > --- a/drivers/pci/pcie/err.c > +++ b/drivers/pci/pcie/err.c > @@ -234,11 +234,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, > } > > if (status == PCI_ERS_RESULT_NEED_RESET) { > - /* > - * TODO: Should call platform-specific > - * functions to reset slot before calling > - * drivers' slot_reset callbacks? > - */ > + if (host->reset_slot) { > + ret = host->reset_slot(host, bridge); > + if (ret) { > + pci_err(bridge, "failed to reset slot: %d\n", > + ret); > + status = PCI_ERS_RESULT_DISCONNECT; > + goto failed; > + } > + } > + This feels like something that should be plumbed into pcibios_reset_secondary_bus(), rather than pcie_do_recovery(). Note that in the DPC case, pcie_do_recovery() doesn't issue a reset itself. The reset has already happened, it was automatically done by the hardware and all the kernel needs to do is bring up the link again. Do you really need any special handling for that in the host controller driver? Only in the AER case do you want to issue a reset on the secondary bus and if there's any platform-specific support needed for that, it needs to go into pcibios_reset_secondary_bus(). Thanks, Lukas
On Fri, Apr 04, 2025 at 10:46:27AM +0200, Lukas Wunner wrote: > On Fri, Apr 04, 2025 at 01:52:22PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > When the PCI error handling requires resetting the slot, reset it using the > > host bridge specific 'reset_slot' callback if available before calling the > > 'slot_reset' callback of the PCI drivers. > > > > The 'reset_slot' callback is responsible for resetting the given slot > > referenced by the 'pci_dev' pointer in a platform specific way and bring it > > back to the working state if possible. If any error occurs during the slot > > reset operation, relevant errno should be returned. > [...] > > --- a/drivers/pci/pcie/err.c > > +++ b/drivers/pci/pcie/err.c > > @@ -234,11 +234,16 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, > > } > > > > if (status == PCI_ERS_RESULT_NEED_RESET) { > > - /* > > - * TODO: Should call platform-specific > > - * functions to reset slot before calling > > - * drivers' slot_reset callbacks? > > - */ > > + if (host->reset_slot) { > > + ret = host->reset_slot(host, bridge); > > + if (ret) { > > + pci_err(bridge, "failed to reset slot: %d\n", > > + ret); > > + status = PCI_ERS_RESULT_DISCONNECT; > > + goto failed; > > + } > > + } > > + > > This feels like something that should be plumbed into > pcibios_reset_secondary_bus(), rather than pcie_do_recovery(). > I did consider that, but didn't go for it since there was no platform reset code present in that function. But I will try to use it as I don't have a strong preference to do reset slot in pcie_do_recovery(). > Note that in the DPC case, pcie_do_recovery() doesn't issue a reset > itself. The reset has already happened, it was automatically done > by the hardware and all the kernel needs to do is bring up the link > again. Do you really need any special handling for that in the > host controller driver? > I haven't tested DPC, so I'm not sure if reset slot is needed or not. > Only in the AER case do you want to issue a reset on the secondary bus > and if there's any platform-specific support needed for that, it needs > to go into pcibios_reset_secondary_bus(). > Ok. I'm trying out this right now and will see if it satisfies my requirement (for both AER fatal and Link Down recovery). - Mani
On Tue, Apr 15, 2025 at 07:03:17PM +0530, Manivannan Sadhasivam wrote: > On Fri, Apr 04, 2025 at 10:46:27AM +0200, Lukas Wunner wrote: > > On Fri, Apr 04, 2025 at 01:52:22PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > > When the PCI error handling requires resetting the slot, reset it > > > using the host bridge specific 'reset_slot' callback if available > > > before calling the 'slot_reset' callback of the PCI drivers. > > > > > > The 'reset_slot' callback is responsible for resetting the given slot > > > referenced by the 'pci_dev' pointer in a platform specific way and > > > bring it back to the working state if possible. If any error occurs > > > during the slot reset operation, relevant errno should be returned. > > > > This feels like something that should be plumbed into > > pcibios_reset_secondary_bus(), rather than pcie_do_recovery(). > > I did consider that, but didn't go for it since there was no platform > reset code present in that function. But I will try to use it as I > don't have a strong preference to do reset slot in pcie_do_recovery(). The only platform overriding pcibios_reset_secondary_bus() is powerpc, and it only does that on PowerNV. I think you could continue to stick with the approach of adding a ->reset_slot() callback to struct pci_host_bridge, but it would be good if at the same time you could convert PowerNV to use the newly introduced callback as well. And then remove the way to override the reset procedure via pcibios_reset_secondary_bus(). All pci_host_bridge's which do not define a ->reset_slot() could be assigned a default callback which just calls pci_reset_secondary_bus(). Alternatively, pcibios_reset_secondary_bus() could be made to invoke the pci_host_bridge's ->reset_slot() callback if it's not NULL, else pci_reset_secondary_bus(). And in that case, the __weak attribute could be removed as well as the powerpc-specific version of pcibios_reset_secondary_bus(). I guess what I'm trying to say is, you don't *have* to plumb this into pcibios_reset_secondary_bus(). In fact, having a host bridge specific callback could be useful if the SoC contains several host bridges which require different callbacks to perform a reset. I just want to make sure that the code remains maintainable, i.e. we don't have two separate ways to override how a bus reset is performed. Thanks, Lukas
On Wed, Apr 16, 2025 at 04:38:01PM +0200, Lukas Wunner wrote: > On Tue, Apr 15, 2025 at 07:03:17PM +0530, Manivannan Sadhasivam wrote: > > On Fri, Apr 04, 2025 at 10:46:27AM +0200, Lukas Wunner wrote: > > > On Fri, Apr 04, 2025 at 01:52:22PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > > > When the PCI error handling requires resetting the slot, reset it > > > > using the host bridge specific 'reset_slot' callback if available > > > > before calling the 'slot_reset' callback of the PCI drivers. > > > > > > > > The 'reset_slot' callback is responsible for resetting the given slot > > > > referenced by the 'pci_dev' pointer in a platform specific way and > > > > bring it back to the working state if possible. If any error occurs > > > > during the slot reset operation, relevant errno should be returned. > > > > > > This feels like something that should be plumbed into > > > pcibios_reset_secondary_bus(), rather than pcie_do_recovery(). > > > > I did consider that, but didn't go for it since there was no platform > > reset code present in that function. But I will try to use it as I > > don't have a strong preference to do reset slot in pcie_do_recovery(). > > The only platform overriding pcibios_reset_secondary_bus() is powerpc, > and it only does that on PowerNV. > > I think you could continue to stick with the approach of adding a > ->reset_slot() callback to struct pci_host_bridge, but it would > be good if at the same time you could convert PowerNV to use the > newly introduced callback as well. And then remove the way to > override the reset procedure via pcibios_reset_secondary_bus(). > > All pci_host_bridge's which do not define a ->reset_slot() could be > assigned a default callback which just calls pci_reset_secondary_bus(). > > Alternatively, pcibios_reset_secondary_bus() could be made to invoke the > pci_host_bridge's ->reset_slot() callback if it's not NULL, else > pci_reset_secondary_bus(). Yes. This is what I now tried locally as well. > And in that case, the __weak attribute > could be removed as well as the powerpc-specific version of > pcibios_reset_secondary_bus(). > I don't think it is possible to get rid of the powerpc version. It has its own pci_dev::sysdata pointing to 'struct pci_controller' pointer which is internal to powerpc arch code. And the generic code would need 'struct pci_host_bridge' to access the callback. > I guess what I'm trying to say is, you don't *have* to plumb this > into pcibios_reset_secondary_bus(). In fact, having a host bridge > specific callback could be useful if the SoC contains several > host bridges which require different callbacks to perform a reset. > > I just want to make sure that the code remains maintainable, > i.e. we don't have two separate ways to override how a bus reset > is performed. > I think we need to have the override in powerpc anyway. But I will move the 'reset_slot' callback to it and get it called from pci_bus_error_reset() (for both AER and Link Down). - Mani
On Wed, Apr 16, 2025 at 08:34:21PM +0530, Manivannan Sadhasivam wrote: > I don't think it is possible to get rid of the powerpc version. It has > its own pci_dev::sysdata pointing to 'struct pci_controller' pointer > which is internal to powerpc arch code. And the generic code would need > 'struct pci_host_bridge' to access the callback. Below is my proposal to convert powerpc to the new ->slot_reset() callback. Compile-tested only. Feel free to include this in your series, alternatively I can submit it to powerpc maintainers once your series has landed. Thanks! -- >8 -- From: Lukas Wunner <lukas@wunner.de> Subject: [PATCH] powerpc/powernv/pci: Migrate to pci_host_bridge::reset_slot callback struct pci_host_bridge has just been amended with a ->reset_slot() callback to allow for a per-host-bridge Secondary Bus Reset procedure. PowerNV needs a platform-specific reset procedure and has historically implemented it by overriding pcibios_reset_secondary_bus(). Migrate PowerNV to the new ->reset_slot() callback for simplicity and cleanliness. Assign the callback as soon as the pci_host_bridge is allocated through the following call chain: pcibios_init() pcibios_scan_phb() pci_create_root_bus() pci_register_host_bridge() pcibios_root_bridge_prepare() The powerpc-specific implementation of pcibios_reset_secondary_bus() can thus be deleted and the remaining default implementation in the PCI core can be made private. The ->reset_secondary_bus() callback in struct pci_controller_ops likewise becomes obsolete and can be deleted. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- arch/powerpc/include/asm/pci-bridge.h | 1 - arch/powerpc/kernel/pci-common.c | 12 ------------ arch/powerpc/platforms/powernv/eeh-powernv.c | 14 +++++++++----- arch/powerpc/platforms/powernv/pci-ioda.c | 9 +++++++-- arch/powerpc/platforms/powernv/pci.h | 3 ++- drivers/pci/pci.c | 2 +- include/linux/pci.h | 1 - 7 files changed, 19 insertions(+), 23 deletions(-) diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h index 2aa3a091ef20..0de09fc90641 100644 --- a/arch/powerpc/include/asm/pci-bridge.h +++ b/arch/powerpc/include/asm/pci-bridge.h @@ -36,7 +36,6 @@ struct pci_controller_ops { unsigned long type); void (*setup_bridge)(struct pci_bus *bus, unsigned long type); - void (*reset_secondary_bus)(struct pci_dev *pdev); #ifdef CONFIG_PCI_MSI int (*setup_msi_irqs)(struct pci_dev *pdev, diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index eac84d687b53..dad15fbec4e0 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -233,18 +233,6 @@ void pcibios_setup_bridge(struct pci_bus *bus, unsigned long type) hose->controller_ops.setup_bridge(bus, type); } -void pcibios_reset_secondary_bus(struct pci_dev *dev) -{ - struct pci_controller *phb = pci_bus_to_host(dev->bus); - - if (phb->controller_ops.reset_secondary_bus) { - phb->controller_ops.reset_secondary_bus(dev); - return; - } - - pci_reset_secondary_bus(dev); -} - resource_size_t pcibios_default_alignment(void) { if (ppc_md.pcibios_default_alignment) diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c index db3370d1673c..9b9517cb6ab7 100644 --- a/arch/powerpc/platforms/powernv/eeh-powernv.c +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c @@ -890,18 +890,22 @@ static int pnv_eeh_bridge_reset(struct pci_dev *pdev, int option) return (rc == OPAL_SUCCESS) ? 0 : -EIO; } -void pnv_pci_reset_secondary_bus(struct pci_dev *dev) +int pnv_pci_reset_secondary_bus(struct pci_host_bridge *host, + struct pci_dev *dev) { struct pci_controller *hose; + int rc_hot, rc_dea; if (pci_is_root_bus(dev->bus)) { hose = pci_bus_to_host(dev->bus); - pnv_eeh_root_reset(hose, EEH_RESET_HOT); - pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE); + rc_hot = pnv_eeh_root_reset(hose, EEH_RESET_HOT); + rc_dea = pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE); } else { - pnv_eeh_bridge_reset(dev, EEH_RESET_HOT); - pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE); + rc_hot = pnv_eeh_bridge_reset(dev, EEH_RESET_HOT); + rc_dea = pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE); } + + return rc_hot ? : rc_dea ? : 0; } static void pnv_eeh_wait_for_pending(struct pci_dn *pdn, const char *type, diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index ae4b549b5ca0..e1b75a4bc681 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -2145,6 +2145,12 @@ static void pnv_pci_ioda_fixup(void) #endif } +static int pnv_pci_root_bridge_prepare(struct pci_host_bridge *bridge) +{ + bridge->reset_slot = pnv_pci_reset_secondary_bus; + return 0; +} + /* * Returns the alignment for I/O or memory windows for P2P * bridges. That actually depends on how PEs are segmented. @@ -2504,7 +2510,6 @@ static const struct pci_controller_ops pnv_pci_ioda_controller_ops = { .release_device = pnv_pci_release_device, .window_alignment = pnv_pci_window_alignment, .setup_bridge = pnv_pci_fixup_bridge_resources, - .reset_secondary_bus = pnv_pci_reset_secondary_bus, .shutdown = pnv_pci_ioda_shutdown, #ifdef CONFIG_IOMMU_API .device_group = pnv_pci_device_group, @@ -2515,7 +2520,6 @@ static const struct pci_controller_ops pnv_npu_ocapi_ioda_controller_ops = { .enable_device_hook = pnv_ocapi_enable_device_hook, .release_device = pnv_pci_release_device, .window_alignment = pnv_pci_window_alignment, - .reset_secondary_bus = pnv_pci_reset_secondary_bus, .shutdown = pnv_pci_ioda_shutdown, }; @@ -2724,6 +2728,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, } ppc_md.pcibios_default_alignment = pnv_pci_default_alignment; + ppc_md.pcibios_root_bridge_prepare = pnv_pci_root_bridge_prepare; #ifdef CONFIG_PCI_IOV ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov; diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h index 42075501663b..44e8969c7729 100644 --- a/arch/powerpc/platforms/powernv/pci.h +++ b/arch/powerpc/platforms/powernv/pci.h @@ -275,7 +275,8 @@ extern struct iommu_table *pnv_pci_table_alloc(int nid); extern void pnv_pci_init_ioda2_phb(struct device_node *np); extern void pnv_pci_init_npu2_opencapi_phb(struct device_node *np); -extern void pnv_pci_reset_secondary_bus(struct pci_dev *dev); +extern int pnv_pci_reset_secondary_bus(struct pci_host_bridge *host, + struct pci_dev *dev); extern int pnv_eeh_phb_reset(struct pci_controller *hose, int option); extern struct pnv_ioda_pe *pnv_pci_bdfn_to_pe(struct pnv_phb *phb, u16 bdfn); diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 13709bb898a9..fe66d69c6429 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4980,7 +4980,7 @@ void pci_reset_secondary_bus(struct pci_dev *dev) pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl); } -void __weak pcibios_reset_secondary_bus(struct pci_dev *dev) +static void pcibios_reset_secondary_bus(struct pci_dev *dev) { struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); int ret; diff --git a/include/linux/pci.h b/include/linux/pci.h index 76e977af2d52..43d952361e84 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1398,7 +1398,6 @@ int pci_probe_reset_slot(struct pci_slot *slot); int pci_probe_reset_bus(struct pci_bus *bus); int pci_reset_bus(struct pci_dev *dev); void pci_reset_secondary_bus(struct pci_dev *dev); -void pcibios_reset_secondary_bus(struct pci_dev *dev); void pci_update_resource(struct pci_dev *dev, int resno); int __must_check pci_assign_resource(struct pci_dev *dev, int i); void pci_release_resource(struct pci_dev *dev, int resno);
On Thu, Apr 17, 2025 at 02:23:26AM +0200, Lukas Wunner wrote: > On Wed, Apr 16, 2025 at 08:34:21PM +0530, Manivannan Sadhasivam wrote: > > I don't think it is possible to get rid of the powerpc version. It has > > its own pci_dev::sysdata pointing to 'struct pci_controller' pointer > > which is internal to powerpc arch code. And the generic code would need > > 'struct pci_host_bridge' to access the callback. > > Below is my proposal to convert powerpc to the new ->slot_reset() callback. > Compile-tested only. > > Feel free to include this in your series, alternatively I can submit it > to powerpc maintainers once your series has landed. Thanks! > > -- >8 -- > > From: Lukas Wunner <lukas@wunner.de> > Subject: [PATCH] powerpc/powernv/pci: Migrate to pci_host_bridge::reset_slot > callback > > struct pci_host_bridge has just been amended with a ->reset_slot() > callback to allow for a per-host-bridge Secondary Bus Reset procedure. > > PowerNV needs a platform-specific reset procedure and has historically > implemented it by overriding pcibios_reset_secondary_bus(). > > Migrate PowerNV to the new ->reset_slot() callback for simplicity and > cleanliness. Assign the callback as soon as the pci_host_bridge is > allocated through the following call chain: > > pcibios_init() > pcibios_scan_phb() > pci_create_root_bus() > pci_register_host_bridge() > pcibios_root_bridge_prepare() > > The powerpc-specific implementation of pcibios_reset_secondary_bus() can > thus be deleted and the remaining default implementation in the PCI core > can be made private. The ->reset_secondary_bus() callback in struct > pci_controller_ops likewise becomes obsolete and can be deleted. > Looks good to me, thanks! I think it would be better if it is submitted once my series has landed in mainline (just to avoid immutable branch hassle between powerpc and PCI trees). - Mani > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > arch/powerpc/include/asm/pci-bridge.h | 1 - > arch/powerpc/kernel/pci-common.c | 12 ------------ > arch/powerpc/platforms/powernv/eeh-powernv.c | 14 +++++++++----- > arch/powerpc/platforms/powernv/pci-ioda.c | 9 +++++++-- > arch/powerpc/platforms/powernv/pci.h | 3 ++- > drivers/pci/pci.c | 2 +- > include/linux/pci.h | 1 - > 7 files changed, 19 insertions(+), 23 deletions(-) > > diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h > index 2aa3a091ef20..0de09fc90641 100644 > --- a/arch/powerpc/include/asm/pci-bridge.h > +++ b/arch/powerpc/include/asm/pci-bridge.h > @@ -36,7 +36,6 @@ struct pci_controller_ops { > unsigned long type); > void (*setup_bridge)(struct pci_bus *bus, > unsigned long type); > - void (*reset_secondary_bus)(struct pci_dev *pdev); > > #ifdef CONFIG_PCI_MSI > int (*setup_msi_irqs)(struct pci_dev *pdev, > diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c > index eac84d687b53..dad15fbec4e0 100644 > --- a/arch/powerpc/kernel/pci-common.c > +++ b/arch/powerpc/kernel/pci-common.c > @@ -233,18 +233,6 @@ void pcibios_setup_bridge(struct pci_bus *bus, unsigned long type) > hose->controller_ops.setup_bridge(bus, type); > } > > -void pcibios_reset_secondary_bus(struct pci_dev *dev) > -{ > - struct pci_controller *phb = pci_bus_to_host(dev->bus); > - > - if (phb->controller_ops.reset_secondary_bus) { > - phb->controller_ops.reset_secondary_bus(dev); > - return; > - } > - > - pci_reset_secondary_bus(dev); > -} > - > resource_size_t pcibios_default_alignment(void) > { > if (ppc_md.pcibios_default_alignment) > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c > index db3370d1673c..9b9517cb6ab7 100644 > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c > @@ -890,18 +890,22 @@ static int pnv_eeh_bridge_reset(struct pci_dev *pdev, int option) > return (rc == OPAL_SUCCESS) ? 0 : -EIO; > } > > -void pnv_pci_reset_secondary_bus(struct pci_dev *dev) > +int pnv_pci_reset_secondary_bus(struct pci_host_bridge *host, > + struct pci_dev *dev) > { > struct pci_controller *hose; > + int rc_hot, rc_dea; > > if (pci_is_root_bus(dev->bus)) { > hose = pci_bus_to_host(dev->bus); > - pnv_eeh_root_reset(hose, EEH_RESET_HOT); > - pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE); > + rc_hot = pnv_eeh_root_reset(hose, EEH_RESET_HOT); > + rc_dea = pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE); > } else { > - pnv_eeh_bridge_reset(dev, EEH_RESET_HOT); > - pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE); > + rc_hot = pnv_eeh_bridge_reset(dev, EEH_RESET_HOT); > + rc_dea = pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE); > } > + > + return rc_hot ? : rc_dea ? : 0; > } > > static void pnv_eeh_wait_for_pending(struct pci_dn *pdn, const char *type, > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index ae4b549b5ca0..e1b75a4bc681 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -2145,6 +2145,12 @@ static void pnv_pci_ioda_fixup(void) > #endif > } > > +static int pnv_pci_root_bridge_prepare(struct pci_host_bridge *bridge) > +{ > + bridge->reset_slot = pnv_pci_reset_secondary_bus; > + return 0; > +} > + > /* > * Returns the alignment for I/O or memory windows for P2P > * bridges. That actually depends on how PEs are segmented. > @@ -2504,7 +2510,6 @@ static const struct pci_controller_ops pnv_pci_ioda_controller_ops = { > .release_device = pnv_pci_release_device, > .window_alignment = pnv_pci_window_alignment, > .setup_bridge = pnv_pci_fixup_bridge_resources, > - .reset_secondary_bus = pnv_pci_reset_secondary_bus, > .shutdown = pnv_pci_ioda_shutdown, > #ifdef CONFIG_IOMMU_API > .device_group = pnv_pci_device_group, > @@ -2515,7 +2520,6 @@ static const struct pci_controller_ops pnv_npu_ocapi_ioda_controller_ops = { > .enable_device_hook = pnv_ocapi_enable_device_hook, > .release_device = pnv_pci_release_device, > .window_alignment = pnv_pci_window_alignment, > - .reset_secondary_bus = pnv_pci_reset_secondary_bus, > .shutdown = pnv_pci_ioda_shutdown, > }; > > @@ -2724,6 +2728,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, > } > > ppc_md.pcibios_default_alignment = pnv_pci_default_alignment; > + ppc_md.pcibios_root_bridge_prepare = pnv_pci_root_bridge_prepare; > > #ifdef CONFIG_PCI_IOV > ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov; > diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h > index 42075501663b..44e8969c7729 100644 > --- a/arch/powerpc/platforms/powernv/pci.h > +++ b/arch/powerpc/platforms/powernv/pci.h > @@ -275,7 +275,8 @@ extern struct iommu_table *pnv_pci_table_alloc(int nid); > > extern void pnv_pci_init_ioda2_phb(struct device_node *np); > extern void pnv_pci_init_npu2_opencapi_phb(struct device_node *np); > -extern void pnv_pci_reset_secondary_bus(struct pci_dev *dev); > +extern int pnv_pci_reset_secondary_bus(struct pci_host_bridge *host, > + struct pci_dev *dev); > extern int pnv_eeh_phb_reset(struct pci_controller *hose, int option); > > extern struct pnv_ioda_pe *pnv_pci_bdfn_to_pe(struct pnv_phb *phb, u16 bdfn); > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 13709bb898a9..fe66d69c6429 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4980,7 +4980,7 @@ void pci_reset_secondary_bus(struct pci_dev *dev) > pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl); > } > > -void __weak pcibios_reset_secondary_bus(struct pci_dev *dev) > +static void pcibios_reset_secondary_bus(struct pci_dev *dev) > { > struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > int ret; > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 76e977af2d52..43d952361e84 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1398,7 +1398,6 @@ int pci_probe_reset_slot(struct pci_slot *slot); > int pci_probe_reset_bus(struct pci_bus *bus); > int pci_reset_bus(struct pci_dev *dev); > void pci_reset_secondary_bus(struct pci_dev *dev); > -void pcibios_reset_secondary_bus(struct pci_dev *dev); > void pci_update_resource(struct pci_dev *dev, int resno); > int __must_check pci_assign_resource(struct pci_dev *dev, int i); > void pci_release_resource(struct pci_dev *dev, int resno); > -- > 2.43.0 >
On Thu, Apr 17, 2025 at 11:09:21AM +0530, Manivannan Sadhasivam wrote: > On Thu, Apr 17, 2025 at 02:23:26AM +0200, Lukas Wunner wrote: > > On Wed, Apr 16, 2025 at 08:34:21PM +0530, Manivannan Sadhasivam wrote: > > > I don't think it is possible to get rid of the powerpc version. It has > > > its own pci_dev::sysdata pointing to 'struct pci_controller' pointer > > > which is internal to powerpc arch code. And the generic code would need > > > 'struct pci_host_bridge' to access the callback. > > > > Below is my proposal to convert powerpc to the new ->slot_reset() callback. > > Compile-tested only. > > > > Feel free to include this in your series, alternatively I can submit it > > to powerpc maintainers once your series has landed. Thanks! > > Looks good to me, thanks! I think it would be better if it is submitted > once my series has landed in mainline (just to avoid immutable branch > hassle between powerpc and PCI trees). Sure, this can wait until your series has landed. It would also be possible to merge through the pci tree if powerpc maintainers ack the patch. I've realized that pci_reset_secondary_bus() can be made private as well, so below is an updated patch. Just putting this out there FWIW. -- 8< -- From: Lukas Wunner <lukas@wunner.de> Subject: [PATCH] powerpc/powernv/pci: Migrate to pci_host_bridge::reset_slot callback struct pci_host_bridge has just been amended with a ->reset_slot() callback to allow for a host-bridge-specific bus reset procedure. PowerNV needs a platform-specific bus reset procedure and has historically implemented it by overriding pcibios_reset_secondary_bus(). Migrate PowerNV to the new ->reset_slot() callback to avoid having to maintain two different mechanisms for platform- and host-bridge-specific bus reset procedures. Assign the callback as soon as the pci_host_bridge is allocated through the following call chain: pcibios_init() pcibios_scan_phb() pci_create_root_bus() pci_register_host_bridge() pcibios_root_bridge_prepare() The powerpc-specific implementation of pcibios_reset_secondary_bus() can thus be deleted and the remaining default implementation in the PCI core can be made private. pci_reset_secondary_bus() can also be made private. The ->reset_secondary_bus() callback in struct pci_controller_ops becomes obsolete and can be deleted. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- arch/powerpc/include/asm/pci-bridge.h | 1 - arch/powerpc/kernel/pci-common.c | 12 ------------ arch/powerpc/platforms/powernv/eeh-powernv.c | 14 +++++++++----- arch/powerpc/platforms/powernv/pci-ioda.c | 9 +++++++-- arch/powerpc/platforms/powernv/pci.h | 3 ++- drivers/pci/pci.c | 4 ++-- include/linux/pci.h | 2 -- 7 files changed, 20 insertions(+), 25 deletions(-) diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h index 2aa3a091ef20..0de09fc90641 100644 --- a/arch/powerpc/include/asm/pci-bridge.h +++ b/arch/powerpc/include/asm/pci-bridge.h @@ -36,7 +36,6 @@ struct pci_controller_ops { unsigned long type); void (*setup_bridge)(struct pci_bus *bus, unsigned long type); - void (*reset_secondary_bus)(struct pci_dev *pdev); #ifdef CONFIG_PCI_MSI int (*setup_msi_irqs)(struct pci_dev *pdev, diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index eac84d687b53..dad15fbec4e0 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -233,18 +233,6 @@ void pcibios_setup_bridge(struct pci_bus *bus, unsigned long type) hose->controller_ops.setup_bridge(bus, type); } -void pcibios_reset_secondary_bus(struct pci_dev *dev) -{ - struct pci_controller *phb = pci_bus_to_host(dev->bus); - - if (phb->controller_ops.reset_secondary_bus) { - phb->controller_ops.reset_secondary_bus(dev); - return; - } - - pci_reset_secondary_bus(dev); -} - resource_size_t pcibios_default_alignment(void) { if (ppc_md.pcibios_default_alignment) diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c index db3370d1673c..9ea2fa892efc 100644 --- a/arch/powerpc/platforms/powernv/eeh-powernv.c +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c @@ -890,18 +890,22 @@ static int pnv_eeh_bridge_reset(struct pci_dev *pdev, int option) return (rc == OPAL_SUCCESS) ? 0 : -EIO; } -void pnv_pci_reset_secondary_bus(struct pci_dev *dev) +int pnv_pci_reset_secondary_bus(struct pci_host_bridge *host, + struct pci_dev *dev) { struct pci_controller *hose; + int rc_hot, rc_dea; if (pci_is_root_bus(dev->bus)) { hose = pci_bus_to_host(dev->bus); - pnv_eeh_root_reset(hose, EEH_RESET_HOT); - pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE); + rc_hot = pnv_eeh_root_reset(hose, EEH_RESET_HOT); + rc_dea = pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE); } else { - pnv_eeh_bridge_reset(dev, EEH_RESET_HOT); - pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE); + rc_hot = pnv_eeh_bridge_reset(dev, EEH_RESET_HOT); + rc_dea = pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE); } + + return rc_hot ? : rc_dea; } static void pnv_eeh_wait_for_pending(struct pci_dn *pdn, const char *type, diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index ae4b549b5ca0..e1b75a4bc681 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -2145,6 +2145,12 @@ static void pnv_pci_ioda_fixup(void) #endif } +static int pnv_pci_root_bridge_prepare(struct pci_host_bridge *bridge) +{ + bridge->reset_slot = pnv_pci_reset_secondary_bus; + return 0; +} + /* * Returns the alignment for I/O or memory windows for P2P * bridges. That actually depends on how PEs are segmented. @@ -2504,7 +2510,6 @@ static const struct pci_controller_ops pnv_pci_ioda_controller_ops = { .release_device = pnv_pci_release_device, .window_alignment = pnv_pci_window_alignment, .setup_bridge = pnv_pci_fixup_bridge_resources, - .reset_secondary_bus = pnv_pci_reset_secondary_bus, .shutdown = pnv_pci_ioda_shutdown, #ifdef CONFIG_IOMMU_API .device_group = pnv_pci_device_group, @@ -2515,7 +2520,6 @@ static const struct pci_controller_ops pnv_npu_ocapi_ioda_controller_ops = { .enable_device_hook = pnv_ocapi_enable_device_hook, .release_device = pnv_pci_release_device, .window_alignment = pnv_pci_window_alignment, - .reset_secondary_bus = pnv_pci_reset_secondary_bus, .shutdown = pnv_pci_ioda_shutdown, }; @@ -2724,6 +2728,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, } ppc_md.pcibios_default_alignment = pnv_pci_default_alignment; + ppc_md.pcibios_root_bridge_prepare = pnv_pci_root_bridge_prepare; #ifdef CONFIG_PCI_IOV ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov; diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h index 42075501663b..44e8969c7729 100644 --- a/arch/powerpc/platforms/powernv/pci.h +++ b/arch/powerpc/platforms/powernv/pci.h @@ -275,7 +275,8 @@ extern struct iommu_table *pnv_pci_table_alloc(int nid); extern void pnv_pci_init_ioda2_phb(struct device_node *np); extern void pnv_pci_init_npu2_opencapi_phb(struct device_node *np); -extern void pnv_pci_reset_secondary_bus(struct pci_dev *dev); +extern int pnv_pci_reset_secondary_bus(struct pci_host_bridge *host, + struct pci_dev *dev); extern int pnv_eeh_phb_reset(struct pci_controller *hose, int option); extern struct pnv_ioda_pe *pnv_pci_bdfn_to_pe(struct pnv_phb *phb, u16 bdfn); diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 13709bb898a9..28cdcd698914 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4962,7 +4962,7 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type) PCIE_RESET_READY_POLL_MS - delay); } -void pci_reset_secondary_bus(struct pci_dev *dev) +static void pci_reset_secondary_bus(struct pci_dev *dev) { u16 ctrl; @@ -4980,7 +4980,7 @@ void pci_reset_secondary_bus(struct pci_dev *dev) pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl); } -void __weak pcibios_reset_secondary_bus(struct pci_dev *dev) +static void pcibios_reset_secondary_bus(struct pci_dev *dev) { struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); int ret; diff --git a/include/linux/pci.h b/include/linux/pci.h index 76e977af2d52..829d7cbbf258 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1397,8 +1397,6 @@ int pci_try_reset_function(struct pci_dev *dev); int pci_probe_reset_slot(struct pci_slot *slot); int pci_probe_reset_bus(struct pci_bus *bus); int pci_reset_bus(struct pci_dev *dev); -void pci_reset_secondary_bus(struct pci_dev *dev); -void pcibios_reset_secondary_bus(struct pci_dev *dev); void pci_update_resource(struct pci_dev *dev, int resno); int __must_check pci_assign_resource(struct pci_dev *dev, int i); void pci_release_resource(struct pci_dev *dev, int resno);
Hi, Currently, in the event of AER/DPC, PCI core will try to reset the slot and its subordinate devices by invoking bridge control reset and FLR. But in some cases like AER Fatal error, it might be necessary to reset the slots using the PCI host bridge drivers in a platform specific way (as indicated by the TODO in the pcie_do_recovery() function in drivers/pci/pcie/err.c). Otherwise, the PCI link won't be recovered successfully. So this series adds a new callback 'pci_host_bridge::reset_slot' for the host bridge drivers to reset the slot when a fatal error happens. Also, this series allows the host bridge drivers to handle PCI link down event by resetting the slots and recovering the bus. This is accomplished by the help of a new API 'pci_host_handle_link_down()'. Host bridge drivers are expected to call this API (preferrably from a threaded IRQ handler) when a link down event is detected. The API will reuse the pcie_do_recovery() function to recover the link if AER support is enabled, otherwise it will directly call the reset_slot() callback of the host bridge driver (if exists). For reference, I've modified the pcie-qcom driver to call pci_host_handle_link_down() after receiving LINK_DOWN global_irq event and populated the 'pci_host_bridge::reset_slot()' callback to reset the controller (there by slots). Since the Qcom PCIe controllers support only a single root port (slot) per controller instance, reset_slot() callback is going to be invoked only once. For multi root port controllers, this callback is supposed to identify the slots using the supplied 'pci_dev' pointer and reset them. NOTE ==== This series is a reworked version of the earlier series [1] that I submitted for handling PCI link down event. In this series, I've made use of the AER helpers to recover the link as it allows notifying the device drivers and also allows saving/restoring the config space. Testing ======= This series is tested on Qcom RB5 and SA8775p Ride boards by triggering the link down event manually by writing to LTSSM register. For the error recovery to succeed (if AER is enabled), all the drivers in the bridge hierarchy should have the 'err_handlers' populated. Otherwise, the link recovery will fail. [1] https://lore.kernel.org/linux-pci/20250221172309.120009-1-manivannan.sadhasivam@linaro.org Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- Manivannan Sadhasivam (4): PCI/ERR: Remove misleading TODO regarding kernel panic PCI/ERR: Add support for resetting the slot in a platforms specific way PCI: Add link down handling for host bridges PCI: qcom: Add support for resetting the slot due to link down event drivers/pci/controller/dwc/pcie-qcom.c | 89 +++++++++++++++++++++++++++++++++- drivers/pci/pci.h | 22 +++++++++ drivers/pci/pcie/err.c | 29 ++++++++--- drivers/pci/probe.c | 7 +++ include/linux/pci.h | 2 + 5 files changed, 140 insertions(+), 9 deletions(-) --- base-commit: 08733088b566b58283f0f12fb73f5db6a9a9de30 change-id: 20250404-pcie-reset-slot-730bfa71a202 Best regards,