[lkml]   [2021]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2] PCI: rockchip: Avoid accessing PCIe registers with clocks gated
Hello Bjorn,

On 6/25/21 12:40 AM, Bjorn Helgaas wrote:
> [+cc Michal, Ley Foon, Jingoo, Thierry, Jonathan]
> On Tue, Jun 08, 2021 at 10:04:09AM +0200, 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
>> resources 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.
> I'm not an IRQ expert, but I think this is an issue regardless of
> CONFIG_DEBUG_SHIRQ, isn't it? Anything used by an IRQ handler should
> be initialized before the handler is registered. CONFIG_DEBUG_SHIRQ
> is just a way to help find latent problems.

Yes, it's an issue regardless. It's just that this debug option tests if the
drivers aren't making the wrong assumption, exactly to find issues like this.

>> 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.
> Previously we registered rockchip_pcie_subsys_irq_handler() and
> rockchip_pcie_client_irq_handler() before the PCIe clocks were
> enabled. That's a problem because they depend on those clocks being
> enabled, and your patch fixes that.
> rockchip_pcie_legacy_int_handler() depends on rockchip->irq_domain,
> which isn't initialized until rockchip_pcie_init_irq_domain().
> Previously we registered rockchip_pcie_legacy_int_handler() as the
> handler for the "legacy" IRQ before rockchip_pcie_init_irq_domain().
> I think you patch *also* fixes that problem, right?

Correct, that's why I moved the initialization and IRQ enable after that.

> I think this is also an issue with the following other drivers. They all
> set the handler to something that uses an IRQ domain before they
> actually initialize the domain:

Yes, I agreed with your assessment and also noticed that others drivers have
similar issues. I just don't have any of those platforms to try to reproduce
the bugs and test a fix.

Best regards,
Javier Martinez Canillas
Software Engineer
New Platform Technologies Enablement team
RHEL Engineering

 \ /
  Last update: 2021-06-25 09:10    [W:1.052 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site