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

pciehp's IRQ handler queues up a work item for each event signaled by
the hardware.  A more modern alternative is to let a long running
kthread service the events.  The IRQ handler's sole job is then to check
whether the IRQ originated from the device in question, acknowledge its
receipt to the hardware to quiesce the interrupt and wake up the kthread.

One benefit is reduced latency to handle the IRQ, which is a necessity
for realtime environments.  Another benefit is that we can make pciehp
simpler and more robust by handling events synchronously in process
context, rather than asynchronously by queueing up work items.  pciehp's
usage of work items is a historic artifact, it predates the introduction
of threaded IRQ handlers by two years.  (The former was introduced in
2007 with commit 5d386e1ac402 ("pciehp: Event handling rework"), the
latter in 2009 with commit 3aa551c9b4c4 ("genirq: add threaded interrupt
handler support").)

Convert pciehp to threaded IRQ handling by retrieving the pending events
in pciehp_isr(), saving them for later consumption by the thread handler
pciehp_ist() and clearing them in the Slot Status register.

By clearing the Slot Status (and thereby acknowledging the events) in
pciehp_isr(), we can avoid requesting the IRQ with IRQF_ONESHOT, which
would have the unpleasant side effect of starving devices sharing the
IRQ until pciehp_ist() has finished.

pciehp_isr() does not count how many times each event occurred, but
merely records the fact *that* an event occurred.  If the same event
occurs a second time before pciehp_ist() is woken, that second event
will not be recorded separately, which is problematic according to
commit fad214b0aa72 ("PCI: pciehp: Process all hotplug events before
looking for new ones") because we may miss removal of a card in-between
two back-to-back insertions.  We're about to make pciehp_ist() resilient
to missed events.  The present commit regresses the driver's behavior
temporarily in order to separate the changes into reviewable chunks.
This doesn't affect regular slow-motion hotplug, only plug-unplug-plug
operations that happen in a timespan shorter than wakeup of the IRQ
thread.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Mayurkumar Patel <mayurkumar.patel@intel.com>
Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/pciehp.h     |  3 ++
 drivers/pci/hotplug/pciehp_hpc.c | 70 +++++++++++++++++---------------
 2 files changed, 41 insertions(+), 32 deletions(-)

On Sat, Jun 16, 2018 at 12:25:00PM -0700, Lukas Wunner wrote:
> pciehp's IRQ handler queues up a work item for each event signaled by
> the hardware.  A more modern alternative is to let a long running
> kthread service the events.  The IRQ handler's sole job is then to check
> whether the IRQ originated from the device in question, acknowledge its
> receipt to the hardware to quiesce the interrupt and wake up the kthread.
> 
> One benefit is reduced latency to handle the IRQ, which is a necessity
> for realtime environments.  Another benefit is that we can make pciehp
> simpler and more robust by handling events synchronously in process
> context, rather than asynchronously by queueing up work items.  pciehp's
> usage of work items is a historic artifact, it predates the introduction
> of threaded IRQ handlers by two years.  (The former was introduced in
> 2007 with commit 5d386e1ac402 ("pciehp: Event handling rework"), the
> latter in 2009 with commit 3aa551c9b4c4 ("genirq: add threaded interrupt
> handler support").)
> 
> Convert pciehp to threaded IRQ handling by retrieving the pending events
> in pciehp_isr(), saving them for later consumption by the thread handler
> pciehp_ist() and clearing them in the Slot Status register.
> 
> By clearing the Slot Status (and thereby acknowledging the events) in
> pciehp_isr(), we can avoid requesting the IRQ with IRQF_ONESHOT, which
> would have the unpleasant side effect of starving devices sharing the
> IRQ until pciehp_ist() has finished.
> 
> pciehp_isr() does not count how many times each event occurred, but
> merely records the fact *that* an event occurred.  If the same event
> occurs a second time before pciehp_ist() is woken, that second event
> will not be recorded separately, which is problematic according to
> commit fad214b0aa72 ("PCI: pciehp: Process all hotplug events before
> looking for new ones") because we may miss removal of a card in-between
> two back-to-back insertions.  We're about to make pciehp_ist() resilient
> to missed events.  The present commit regresses the driver's behavior
> temporarily in order to separate the changes into reviewable chunks.
> This doesn't affect regular slow-motion hotplug, only plug-unplug-plug
> operations that happen in a timespan shorter than wakeup of the IRQ
> thread.

I like the series over-all. Definitely an improvement.

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.

[+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