On Tue, Jun 13, 2017 at 09:08:10AM +0200, Christoph Hellwig wrote: > On Mon, Jun 12, 2017 at 06:14:23PM -0500, Bjorn Helgaas wrote: > > My main concern is being able to verify the locking. I think that is > > much easier if the locking is adjacent to the method invocation. But > > if you just add a comment at the method invocation about where the > > locking is, that should be sufficient. > > Ok. I can add comments for all the methods as a separate patch, > similar to Documentation/vfs/Locking > > > > Yes, I mentioned this earlier, and I also vaguely remember we got > > > bug reports from IBM on power for this a while ago. I just don't > > > feel confident enough to touch all these without a good test plan. > > > > Hmmm. I see your point, but I hate leaving a known bug unfixed. I > > wonder if some enterprising soul could tickle this bug by injecting > > errors while removing and rescanning devices below the bridge? > > I'm completely loaded up at the moment, but this sounds like a good > idea. > > In the meantime how do you want to proceed with this patch? Can you just add comments about the locking? I'd prefer that in the same patch that adds the locking because that's what I had a hard time reviewing. I'm not thinking of anything fancy like Documentation/filesystems/Locking; I'm just thinking of something along the lines of "caller must hold pci_dev_lock() to protect err_handler->reset_notify from racing ->remove()". And the changelog should contain the general principle about the locking strategy. Bjorn
