On Fri, 2018-08-17 at 14:48 +1000, Benjamin Herrenschmidt wrote: > The second part aims at fixing the enable/disable/set_master races, > and does so by providing a framework for future device state locking > issues. > > It introduces a pci_dev->state_mutex which is used at a lower level > than the device_lock (the device lock isn't suitable, as explained > in the cset comments) and uses it to protect enablement and set_master. As discussed in the series, I'm not using the device_lock because I don't like it creeping out of drivers/base too much unless you explicitly try to lock against concurrent add/remove. That being said, if we decided we prefer using it to solve the enable/disable race, then we have to be careful of a few things: - Driver callbacks hold it, so we can't take it from within pci_enable_device(), pci_set_master() etc... themselves. We'll have to assume the caller has it - The above means auditing callers of these various APIs that might be calling them from outside of a driver callback and add the necessary lock - We need to take great care about the possibility of the parent device(s) lock being held. It can happen for USB for example. Now we don't have USB->PCI adapters that I know of that uses the PCI stack in Linux but generally assuming your parent lock isn't held is risky. So I would be a bit weary of walking up the bridge chain and taking it unconditionally. Alternatively, we could hijack an existing global lock, for example mvoe the bridge_mutex outside of ACPI and use it for the upwards bridge walk, and ignore the other possible races with enable/disable. Cheers, Ben.
