On Tue, 4 Sep 2018 18:12:16 -0700 Sinan Kaya <okaya@xxxxxxxxxx> wrote: > On 9/4/2018 3:52 PM, Alex Williamson wrote: > > I won't deny that we're getting a little sloppy with some of the reset > > interfaces and I'd love to solve the device lock issue more generally, > > but trying to abstract slot vs bus and hide the lower level interfaces > > from drivers doesn't seem like it's really working here. Thanks, > > Counter argument is that drivers need to know about bus/slot distinction > and call two different API for hotplug capable systems. I think that's > too much to ask. > > The motivation for original patch was that hotplug information was leaking > and we could do a better job about it. But, you are right; I partially > covered VFIO needs as it seemed to be doing more work than a simple > reset due to ownership dependencies. Maybe let's clarify what is the actual leak of the hotplug API to the driver. I think vfio assumes full responsibility here, it needs to know the scope of the reset in order to determine whether the user has ownership of all the devices affected by the reset. In that case it's even preferable that we have control of specifying which type of reset is performed or else we open ourselves for the possibility that vfio-pci and PCI-core could diverge and we don't get the reset we expect. I think that's a point against this sort of blind, unified reset interface. The hfi1 driver previously didn't care about hotplug slots, but the case I see where they should have is if the slot enabled surprise hotplug. In that case their direct call to pci_reset_bridge_secondary_bus might have triggered such an event; surprise! They also go to the trouble to manually save and restore config space around the reset and make sure they're the only device affected by the reset, so this really starts to sound like a place where we should use a pci_reset_function type interface (though they have quite a bit of setup working towards the SBR, so it may not be trivial to convert). In fact, we've even got and exported __pci_reset_function_locked() which can already handle the device_lock being held, but it will prefer function level resets before it gets to the bus/slot resets that this driver wants. It seems a common theme here is that the driver wants some degree of control of the type of reset used, vfio specifically wants to specify a bus or slot reset while hfi1 just wants to specify that the link is reset and doesn't care if it's a bus or slot reset that accomplishes that. So maybe the right "unified" interface is one that takes a parameter allowing the caller to specify the scope or type of reset with aliases so drivers can ignore the hotplug interface if they wish. An ugly version of that might be based around something like: #define PCI_RESET_DEV_SPECIFIC (1 << 0) #define PCI_RESET_FLR (1 << 1) #define PCI_RESET_PM (1 << 2) #define PCI_RESET_SLOT (1 << 3) #define PCI_RESET_BUS (1 << 4) #define PCI_RESET_ANY (~0) #define PCI_RESET_FUNC (PCI_RESET_DEV_SPECIFIC | \ PCI_RESET_FLR | PCI_RESET_PM) #define PCI_RESET_LINK (PCI_RESET_SLOT | PCI_RESET_BUS) Thanks, Alex
