From: Javier Martinez Canillas <javierm@redhat.com> Date: 2021-06-07 21:34:03
IRQ handlers that are registered for shared interrupts can be called at
any time after have been registered using the request_irq() function.
It's up to drivers to ensure that's always safe for these to be called.
Both the "pcie-sys" and "pcie-client" interrupts are shared, but since
their handlers are registered very early in the probe function, an error
later can lead to these handlers being executed before all the required
have been properly setup.
For example, the rockchip_pcie_read() function used by these IRQ handlers
expects that some PCIe clocks will already be enabled, otherwise trying
to access the PCIe registers causes the read to hang and never return.
The CONFIG_DEBUG_SHIRQ option tests if drivers are able to cope with their
shared interrupt handlers being called, by generating a spurious interrupt
just before a shared interrupt handler is unregistered.
But this means that if the option is enabled, any error in the probe path
of this driver could lead to one of the IRQ handlers to be executed.
In a rockpro64 board, the following sequence of events happens:
1) "pcie-sys" IRQ is requested and its handler registered.
2) "pcie-client" IRQ is requested and its handler registered.
3) probe later fails due readl_poll_timeout() returning a timeout.
4) the "pcie-sys" IRQ is unregistered.
5) CONFIG_DEBUG_SHIRQ triggers a spurious interrupt.
6) "pcie-client" IRQ handler is called for this spurious interrupt.
7) IRQ handler tries to read PCIE_CLIENT_INT_STATUS with clocks gated.
8) the machine hangs because rockchip_pcie_read() call never returns.
To avoid cases like this, the handlers don't have to be registered until
very late in the probe function, once all the resources have been setup.
So let's just move all the IRQ init before the pci_host_probe() call, that
will prevent issues like this and seems to be the correct thing to do too.
Reported-by: Peter Robinson <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
drivers/pci/controller/pcie-rockchip-host.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Shawn Lin <shawn.lin@rock-chips.com> Date: 2021-06-08 06:49:19
On 2021/6/8 5:33, Javier Martinez Canillas wrote:
IRQ handlers that are registered for shared interrupts can be called at
any time after have been registered using the request_irq() function.
It's up to drivers to ensure that's always safe for these to be called.
Both the "pcie-sys" and "pcie-client" interrupts are shared, but since
their handlers are registered very early in the probe function, an error
later can lead to these handlers being executed before all the required
have been properly setup.
For example, the rockchip_pcie_read() function used by these IRQ handlers
expects that some PCIe clocks will already be enabled, otherwise trying
to access the PCIe registers causes the read to hang and never return.
The CONFIG_DEBUG_SHIRQ option tests if drivers are able to cope with their
shared interrupt handlers being called, by generating a spurious interrupt
just before a shared interrupt handler is unregistered.
But this means that if the option is enabled, any error in the probe path
of this driver could lead to one of the IRQ handlers to be executed.
In a rockpro64 board, the following sequence of events happens:
1) "pcie-sys" IRQ is requested and its handler registered.
2) "pcie-client" IRQ is requested and its handler registered.
3) probe later fails due readl_poll_timeout() returning a timeout.
4) the "pcie-sys" IRQ is unregistered.
5) CONFIG_DEBUG_SHIRQ triggers a spurious interrupt.
6) "pcie-client" IRQ handler is called for this spurious interrupt.
7) IRQ handler tries to read PCIE_CLIENT_INT_STATUS with clocks gated.
8) the machine hangs because rockchip_pcie_read() call never returns.
To avoid cases like this, the handlers don't have to be registered until
very late in the probe function, once all the resources have been setup.
So let's just move all the IRQ init before the pci_host_probe() call, that
will prevent issues like this and seems to be the correct thing to do too.
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
Thanks.
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2021-06-08 08:02:43
On 6/8/21 8:48 AM, Shawn Lin wrote:
On 2021/6/8 5:33, Javier Martinez Canillas wrote:
[snip]
quoted
So let's just move all the IRQ init before the pci_host_probe() call, that
will prevent issues like this and seems to be the correct thing to do too.
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
Thanks a lot for the ack!
Peter Robinson noticed a missing word in the changelog, I'll send a v2 now.
Best regards,
--
Javier Martinez Canillas
Software Engineer
New Platform Technologies Enablement team
RHEL Engineering
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel