Re: [PATCH 09/32] PCI: pciehp: Convert to threaded IRQ

[Lukas Wunner <lukas@wunner.de>]

[+cc Xiongfeng Wang]

On Tue, Jun 19, 2018 at 05:16:46PM -0600, Keith Busch wrote:
> I am a little concered about what may happen if we need to remove the
> bridge while its irq thread is running. The task removing the bridge
> is holding the pci_rescan_remove_lock so when it tries to free the
> bridge IRQ, the IRQ subsystem may not be able to progress because the
> action->thread may be waiting to take the same lock.
> 
> It actually looks like the same deadlock already exists in the current
> implementation when it takes down its workqueue, but it's a lot harder to
> follow all the different work tasks before this clean-up. Maybe removing
> bridges isn't very common, but it's just something I noticed.

In patch [03/32], "PCI: pciehp: Fix deadlock on unplug", I've fixed
this deadlock in case the lock is contended by two pciehp instances.

But when browsing patchwork yesterday, I came across Xiongfeng Wang's
patch "pciehp: fix a race between pciehp and removing operations by sysfs".
It deals with the same deadlock, but the contenders for the lock are a
pciehp instance and a sysfs "remove" request:
https://patchwork.ozlabs.org/patch/877835/

We need a generic solution which works regardless of the contenders'
type, so I'm withdrawing patch [03/32] and I'll try to come up with
something better.

It seems this is about the hierarchy, we need to prevent that the lock
is acquired to remove a device which is a child of another device which
is already being removed, wherefore the lock is currently held.  An idea
would be to change the API such that a struct pci_dev pointer is passed
in to pci_lock_rescan_remove().  That's the device being removed and
for which the lock is handed out.  If the lock is requested for a child,
the request is denied.  The invocation would thus look like this:

 void pci_stop_and_remove_bus_device_locked(struct pci_dev *dev)
 {
-	pci_lock_rescan_remove();
+	if (!pci_trylock_rescan_remove(dev));
+		return;
 	pci_stop_and_remove_bus_device(dev);
 	pci_unlock_rescan_remove();
 }

Thoughts?

Lukas