Re: PCI service interrupt handlers & access to PCI config space

["Pali Rohár" <pali@kernel.org>]

Hello!

On Saturday 10 April 2021 16:25:24 Lukas Wunner wrote:
> On Sat, Apr 10, 2021 at 02:28:45PM +0200, Pali Rohár wrote:
> > I see that more PCI service drivers in their interrupt handlers are
> > accessing PCI config space.
> > 
> > E.g. in PCIe Hot Plug interrupt handler pciehp_isr handler is called
> > pcie_capability_read_word() function.
> > 
> > It is correct? Because these capability functions are protected by
> > pci_lock_config() / pci_unlock_config() calls.
> 
> Looks fine to me.  That's a raw_spin_lock, which is allowed to be taken
> in interrupt context.
> 
> > And what would happen when during execution of reading or writing to
> > PCIe config space (e.g. via pcie_capability_read_word()) is triggered
> > PCIe HP interrupt? Would not enter interrupt handler function in
> > deadlock (waiting for unlocking config space)?
> 
> The interrupt handler would spin until the lock is released.
> raw_spin_locks are not supposed to be held for a prolonged
> period of time.

What is "prolonged period of time"? Because for example PCIe controller
on A3720 has upper limit about 1.5s when accessing config space. This is
what I measured in real example. It really took PCIe HW more than 1s to
return error code if it happens.

> The interrupt is masked when it triggers and until it's been handled,
> so the interrupt handler never runs multiple times concurrently.
> See e.g. handle_level_irq() in kernel/irq/chip.c.

I'm thinking if it is really safe here. On dual-core system, there may
be two tasks (on each CPU) which calls pcie_capability_read_word() (or
other access function). Exactly one pass pci lock. And after that two
different interrupts (for each CPU) may occur which handlers would like
to call pcie_capability_read_word(). raw_spin_lock_irqsave (which is
called from pci_lock_config()) disable interrupts on local CPU, right?
But what with second CPU? Could not be there issue?

And what would happen if pcie_capability_read_word() is locked for 1.5s
as described above? Does not look safe for me.