[lkml]   [2021]   [Jun]   [24]   [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
On 2021-06-24 22:57, Bjorn Helgaas wrote:
> 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 read *never* completes? That might be a bit problematic because
> it implies that we may not be able to recover from PCIe errors. Most
> controllers will timeout eventually, log an error, and either
> fabricate some data (typically ~0) to complete the CPU's read or cause
> some kind of abort or machine check.
> Just asking in case there's some controller configuration that should
> be tweaked.

If I'm following correctly, that'll be a read transaction to the native
side of the controller itself; it can't complete that read, or do
anything else either, because it's clock-gated, and thus completely
oblivious (it might be that if another CPU was able to enable the clocks
then everything would carry on as normal, or it might end up totally
deadlocking the SoC interconnect). I think it's safe to assume that in
that state nothing of importance would be happening on the PCIe side,
and even if it was we'd never get to know about it.

The only relevant configuration would be "don't turn the clocks off if
you're using the thing", which in actual operation can be taken for
granted. It's a fairly typical bug to register an IRQ as shared but
assume in the handler that you'll only ever be called for your own
device's IRQ while it's powered up/clocked/etc. in its normal
operational state, hence CONFIG_DEBUG_SHIRQ helps flush those kinds of
unreliable assumptions out.


(this reminds me of the "fun" I once had where a machine was locking up
during boot, but simply connecting an external debugger to find out
exactly where it was stuck happened to automatically enable the
offending power domain and un-stick it)

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