PCI service interrupt handlers & access to PCI config space

PCI service interrupt handlers & access to PCI config space

[Pali Rohár]

Hello!

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.

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)?

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

[Lukas Wunner]

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.

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.

Thanks,

Lukas

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

[Pali Rohár]

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.

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

[Lukas Wunner]

On Sat, Apr 10, 2021 at 05:17:09PM +0200, Pali Rohár wrote:
> On Saturday 10 April 2021 16:25:24 Lukas Wunner wrote:
> > 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.

Linux Device Drivers (2005) says:

   "The last important rule for spinlock usage is that spinlocks must
    always be held for the minimum time possible. The longer you hold
    a lock, the longer another processor may have to spin waiting for
    you to release it, and the chance of it having to spin at all is
    greater. Long lock hold times also keep the current processor from
    scheduling, meaning that a higher priority process -- which really
    should be able to get the CPU -- may have to wait. The kernel
    developers put a great deal of effort into reducing kernel latency
    (the time a process may have to wait to be scheduled) in the 2.5
    development series. A poorly written driver can wipe out all that
    progress just by holding a lock for too long. To avoid creating
    this sort of problem, make a point of keeping your lock-hold times
    short."

1.5 sec is definitely too long.  This sounds like a quirk of this
specific hardware.  Try to find out if the hardware can be configured
differently to respond quicker.


> > 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?

No.


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

It's safe, but performs poorly.

Thanks,

Lukas

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

[Keith Busch]

On Sat, Apr 10, 2021 at 06:26:22PM +0200, Lukas Wunner wrote:
> 
> 1.5 sec is definitely too long.  This sounds like a quirk of this
> specific hardware.  Try to find out if the hardware can be configured
> differently to respond quicker.

While 1.5 sec is long, pcie spec's device control 2 register has an option to
be even longer: up to 64 seconds for a config access timeout! I'm not sure of
the reasoning to allow something that high, but I think the operating system
would be not be too happy with that.

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

[Pali Rohár]

On Sunday 11 April 2021 20:25:55 Keith Busch wrote:
> On Sat, Apr 10, 2021 at 06:26:22PM +0200, Lukas Wunner wrote:
> > 
> > 1.5 sec is definitely too long.  This sounds like a quirk of this
> > specific hardware.  Try to find out if the hardware can be configured
> > differently to respond quicker.
> 
> While 1.5 sec is long, pcie spec's device control 2 register has an option to
> be even longer: up to 64 seconds for a config access timeout! I'm not sure of
> the reasoning to allow something that high, but I think the operating system
> would be not be too happy with that.

So what can we do in this case? On single core CPU it means that raw
spin lock would completely block any operation on CPU for 64 seconds.

Do you know what is the timeout for other registers?

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

[Keith Busch]

On Mon, Apr 12, 2021 at 04:04:51PM +0200, Pali Rohár wrote:
> On Sunday 11 April 2021 20:25:55 Keith Busch wrote:
> > On Sat, Apr 10, 2021 at 06:26:22PM +0200, Lukas Wunner wrote:
> > > 
> > > 1.5 sec is definitely too long.  This sounds like a quirk of this
> > > specific hardware.  Try to find out if the hardware can be configured
> > > differently to respond quicker.
> > 
> > While 1.5 sec is long, pcie spec's device control 2 register has an option to
> > be even longer: up to 64 seconds for a config access timeout! I'm not sure of
> > the reasoning to allow something that high, but I think the operating system
> > would be not be too happy with that.
> 
> So what can we do in this case? On single core CPU it means that raw
> spin lock would completely block any operation on CPU for 64 seconds.

I don't think it would work here. I'm just saying that while 1.5s config
access is quite long, the spec provides an option where times exceeding
that are expected.

I have never seen a device configured that way, though. The completion
timeouts are usually set in milliseconds.

> Do you know what is the timeout for other registers?

The Device Control Register 2 timeout value is the setting for all
non-posted requests.

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

[Pali Rohár]

On Tuesday 13 April 2021 00:09:20 Keith Busch wrote:
> On Mon, Apr 12, 2021 at 04:04:51PM +0200, Pali Rohár wrote:
> > On Sunday 11 April 2021 20:25:55 Keith Busch wrote:
> > > On Sat, Apr 10, 2021 at 06:26:22PM +0200, Lukas Wunner wrote:
> > > > 
> > > > 1.5 sec is definitely too long.  This sounds like a quirk of this
> > > > specific hardware.  Try to find out if the hardware can be configured
> > > > differently to respond quicker.
> > > 
> > > While 1.5 sec is long, pcie spec's device control 2 register has an option to
> > > be even longer: up to 64 seconds for a config access timeout! I'm not sure of
> > > the reasoning to allow something that high, but I think the operating system
> > > would be not be too happy with that.
> > 
> > So what can we do in this case? On single core CPU it means that raw
> > spin lock would completely block any operation on CPU for 64 seconds.
> 
> I don't think it would work here. I'm just saying that while 1.5s config
> access is quite long, the spec provides an option where times exceeding
> that are expected.
> 
> I have never seen a device configured that way, though. The completion
> timeouts are usually set in milliseconds.
> 
> > Do you know what is the timeout for other registers?
> 
> The Device Control Register 2 timeout value is the setting for all
> non-posted requests.

Root Bridge of PCIe controller is seen by lspci as:

  DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+ 10BitTagReq- OBFF Disabled, ARIFwd-
           AtomicOpsCtl: ReqEn- EgressBlck-

So it does not look like that Device Control 2 is configured with higher
delays.

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

[Pali Rohár]

On Saturday 10 April 2021 18:26:22 Lukas Wunner wrote:
> On Sat, Apr 10, 2021 at 05:17:09PM +0200, Pali Rohár wrote:
> > On Saturday 10 April 2021 16:25:24 Lukas Wunner wrote:
> > > 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.
> 
> Linux Device Drivers (2005) says:
> 
>    "The last important rule for spinlock usage is that spinlocks must
>     always be held for the minimum time possible. The longer you hold
>     a lock, the longer another processor may have to spin waiting for
>     you to release it, and the chance of it having to spin at all is
>     greater. Long lock hold times also keep the current processor from
>     scheduling, meaning that a higher priority process -- which really
>     should be able to get the CPU -- may have to wait. The kernel
>     developers put a great deal of effort into reducing kernel latency
>     (the time a process may have to wait to be scheduled) in the 2.5
>     development series. A poorly written driver can wipe out all that
>     progress just by holding a lock for too long. To avoid creating
>     this sort of problem, make a point of keeping your lock-hold times
>     short."
> 
> 1.5 sec is definitely too long.  This sounds like a quirk of this
> specific hardware.  Try to find out if the hardware can be configured
> differently to respond quicker.

I have not found any option how to configure it to respond quicker.
Upper limit for controller hw in error condition (e.g. when card does
not respond) seems to be always 1.5 sec.