On Fri, 2024-11-01 at 13:21 -0600, Keith Busch wrote: > On Tue, Oct 29, 2024 at 12:27:41PM +0100, Niklas Schnelle wrote: > > For what it's worth on s390 I think the previous proposal of having the > > attribute on the pci_bus would have been better in principle. On s390 > > the PCI bus probing is done by firmware and Linux doesn't see a pci_dev > > for bridges but we do create struct pci_bus for example for a PF and > > its child VFs. > > > > Then again we can't really do a reset on this level, other than > > manually going through the PCI functions and resetting them one by one. > > In fact we may see PCI functions on their own bus while another Linux > > instance (LPAR) sees other functions from that bus. So yeah, I guess > > it's fair not to have this attribute but still wanted to offer these > > thoughts. > > This attribute uses the pci_dev bridge control register. If you don't > have the bridge device, I don't think this would be useful, so I guess > you'd have to fallback to resetting individual functions. > > It seems people can navigate /sys/bus/pci/devices/ easier than finding a > pci_bus under /sys/devices/, though, so that's a plus for pci_dev. > Yes you're right, since the reset via __pci_reset_bus() and then pci_bridge_secondary_bus_reset() requires the bridge device (unlike pci_reset_bus()) it makes more sense to have it on the bridge thus also requiring the bridge device to exist. I still kind of wish pci_bridge_secondary_bus_reset() and pcibios_reset_secondary_bus() would take a struct pci_bus and then we could do the fallback in the latter platform specific code and having the attribute on the bus would make sense, but I'm not sure it's all that useful. One more question though, what would happen with this reset for a bus with an SR-IOV device with more than 256 VFs i.e. where pci_iov_virtfn_bus() returns anything other than 0. I'm guessing since VFs are physically still controlled by the bridge all VFs would be reset but at the same time virtfn_add_bus() sets the bridge device for the added bus as NULL so I think it might look odd in sysfs, sadly I don't have such a device to test with. Still, this might actually be an argument for having the attribute on the bridge. Thanks, Niklas