On Sat, 2018-08-18 at 21:24 -0500, Bjorn Helgaas wrote: > On Sat, Aug 18, 2018 at 01:24:51PM +1000, Benjamin Herrenschmidt wrote: > > On Fri, 2018-08-17 at 10:44 -0500, Bjorn Helgaas wrote: > > > On Fri, Aug 17, 2018 at 02:48:57PM +1000, Benjamin Herrenschmidt wrote: > > > > This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76. > > > > > > Just to be clear, if I understand correctly, this is a pure revert of > > > 44bda4b7d26e and as such it reintroduces the problem solved by that > > > commit. > > > > > > If your solution turns out to be better, that's great, but it would be > > > nice to avoid the bisection hole of reintroducing the problem, then > > > fixing it again later. > > > > There is no way to do that other than merging the revert and the fix > > into one. That said, the race is sufficiently hard to hit that I > > wouldn't worry too much about it. > > OK, then at least mention that in the changelog. Sure will do. This is just RFC at this stage :-) As for the race with enable, what's your take on my approach ? The basic premise is that we need some kind of mutex to make the updates to enable_cnt and the actual config space changes atomic. While at it we can fold pci_set_master vs. is_busmaster as well as those are racy too. I chose to create a new mutex which we should be able to address other similar races if we find them. The other solutions that I dismissed were: - Using the device_lock. A explained previously, this is tricky, I prefer not using this for anything other than locking against concurrent add/remove. The main issue is that drivers will be sometimes called in context where that's already held, so we can't take it inside pci_enable_device() and I'd rather not add new constraints such as "pci_enable_device() must be only called from probe() unless you also take the device lock". It would be tricky to audit everybody... - Using a global mutex. We could move the bridge lock from AER to core code for example, and use that. But it doesn't buy us much, and slightly redecuces parallelism. It also makes it a little bit more messy to walk up the bridge chain, we'd have to do a pci_enable_device_unlocked or something, messy. So are you ok with the approach ? Do you prefer one of the above regardless ? Something else ? Cheers, Ben.
