Re: [PATCH 1/2] PCI: rcar-host: Drop PMSR spinlock

[Marek Vasut <marek.vasut@mailbox.org>]

On 9/22/25 11:14 AM, Geert Uytterhoeven wrote:

Hello Geert,

> On Tue, 9 Sept 2025 at 18:27, Marek Vasut
> <marek.vasut+renesas@mailbox.org> wrote:
>> The pmsr_lock spinlock used to be necessary to synchronize access to the
>> PMSR register, because that access could have been triggered from either
>> config space access in rcar_pcie_config_access() or an exception handler
>> rcar_pcie_aarch32_abort_handler().
>>
>> The rcar_pcie_aarch32_abort_handler() case is no longer applicable since
>> commit 6e36203bc14c ("PCI: rcar: Use PCI_SET_ERROR_RESPONSE after read
>> which triggered an exception"), which performs more accurate, controlled
>> invocation of the exception, and a fixup.
>>
>> This leaves rcar_pcie_config_access() as the only call site from which
>> rcar_pcie_wakeup() is called. The rcar_pcie_config_access() can only be
>> called from the controller struct pci_ops .read and .write callbacks,
>> and those are serialized in drivers/pci/access.c using raw spinlock
>> 'pci_lock' . CONFIG_PCI_LOCKLESS_CONFIG is never set on this platform.
>>
>> Since the 'pci_lock' is a raw spinlock , and the 'pmsr_lock' is not a
>> raw spinlock, this constellation triggers 'BUG: Invalid wait context'
>> with CONFIG_PROVE_RAW_LOCK_NESTING=y .
>>
>> Remove the pmsr_lock to fix the locking.
>>
>> Fixes: a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook")
>> Reported-by: Duy Nguyen <duy.nguyen.rh@renesas.com>
>> Reported-by: Thuan Nguyen <thuan.nguyen-hong@banvien.com.vn>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> 
> Thanks for your patch!
> 
> Your reasoning above LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> My only worry is that PCI_LOCKLESS_CONFIG may be selected on non-x86
> one day, breaking your assumptions.  IMHO, the mechanism behind this
> config option, introduced in commit 714fe383d6c9bd95 ("PCI: Provide
> Kconfig option for lockless config space accessors") looks very fragile
> to me: it is intended to be selected by an architecture, if "all" low
> level PCI configuration space accessors use their own serialization or
> can operate completely lockless.  Usually we use the safer, inverted
> approach (PCI_NOLOCKLESS_CONFIG), to be selected by all drivers that
> do not adhere to the assumption.
> But perhaps I am missing something, and this does not depend on
> individual PCIe host drivers?
> 
> Regardless, improving that is clearly out-of-scope for this patch...
I could send a follow up patch which would add build-time assertion that 
PCI_LOCKLESS_CONFIG must not be selected for this driver to work. Would 
that be an option ?