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 -- மணிவண்ணன் சதாசிவம்
