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 --- ============================= [ BUG: Invalid wait context ] 6.17.0-rc4-next-20250905-00048-ga08e553145e7-dirty #1116 Not tainted ----------------------------- swapper/0/1 is trying to lock: ffffffd92cf69c30 (pmsr_lock){....}-{3:3}, at: rcar_pcie_config_access+0x48/0x260 other info that might help us debug this: context-{5:5} 3 locks held by swapper/0/1: #0: ffffff84c0f890f8 (&dev->mutex){....}-{4:4}, at: device_lock+0x14/0x1c #1: ffffffd92cf675b0 (pci_rescan_remove_lock){+.+.}-{4:4}, at: pci_lock_rescan_remove+0x18/0x20 #2: ffffffd92cf674a0 (pci_lock){....}-{2:2}, at: pci_bus_read_config_dword+0x54/0xd8 stack backtrace: CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc4-next-20250905-00048-ga08e553145e7-dirty #1116 PREEMPT Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) Call trace: dump_backtrace+0x6c/0x7c (C) show_stack+0x14/0x1c dump_stack_lvl+0x68/0x8c dump_stack+0x14/0x1c __lock_acquire+0x3e8/0x1064 lock_acquire+0x17c/0x2ac _raw_spin_lock_irqsave+0x54/0x70 rcar_pcie_config_access+0x48/0x260 rcar_pcie_read_conf+0x44/0xd8 pci_bus_read_config_dword+0x78/0xd8 pci_bus_generic_read_dev_vendor_id+0x30/0x138 pci_bus_read_dev_vendor_id+0x60/0x68 pci_scan_single_device+0x11c/0x1ec pci_scan_slot+0x7c/0x170 pci_scan_child_bus_extend+0x5c/0x29c pci_scan_child_bus+0x10/0x18 pci_scan_root_bus_bridge+0x90/0xc8 pci_host_probe+0x24/0xc4 rcar_pcie_probe+0x5e8/0x650 platform_probe+0x58/0x88 really_probe+0x190/0x350 __driver_probe_device+0x120/0x138 driver_probe_device+0x38/0xec __driver_attach+0x158/0x168 bus_for_each_dev+0x7c/0xd0 driver_attach+0x20/0x28 bus_add_driver+0xe0/0x1d8 driver_register+0xac/0xe8 __platform_driver_register+0x1c/0x24 rcar_pcie_driver_init+0x18/0x20 do_one_initcall+0xd4/0x220 kernel_init_freeable+0x308/0x30c kernel_init+0x20/0x11c ret_from_fork+0x10/0x20 --- Cc: "Krzysztof Wilczyński" kwilczynski@kernel.org Cc: Bjorn Helgaas bhelgaas@google.com Cc: Geert Uytterhoeven geert+renesas@glider.be Cc: Lorenzo Pieralisi lpieralisi@kernel.org Cc: Magnus Damm magnus.damm@gmail.com Cc: Manivannan Sadhasivam mani@kernel.org Cc: Marc Zyngier maz@kernel.org Cc: Rob Herring robh@kernel.org Cc: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com Cc: linux-pci@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org --- drivers/pci/controller/pcie-rcar-host.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c index 4780e0109e583..625a00f3b2230 100644 --- a/drivers/pci/controller/pcie-rcar-host.c +++ b/drivers/pci/controller/pcie-rcar-host.c @@ -52,20 +52,13 @@ struct rcar_pcie_host { int (*phy_init_fn)(struct rcar_pcie_host *host); };
-static DEFINE_SPINLOCK(pmsr_lock); - static int rcar_pcie_wakeup(struct device *pcie_dev, void __iomem *pcie_base) { - unsigned long flags; u32 pmsr, val; int ret = 0;
- spin_lock_irqsave(&pmsr_lock, flags); - - if (!pcie_base || pm_runtime_suspended(pcie_dev)) { - ret = -EINVAL; - goto unlock_exit; - } + if (!pcie_base || pm_runtime_suspended(pcie_dev)) + return -EINVAL;
pmsr = readl(pcie_base + PMSR);
@@ -87,8 +80,6 @@ static int rcar_pcie_wakeup(struct device *pcie_dev, void __iomem *pcie_base) writel(L1FAEG | PMEL1RX, pcie_base + PMSR); }
-unlock_exit: - spin_unlock_irqrestore(&pmsr_lock, flags); return ret; }
The rcar_msi_irq_unmask() function may be called from a PCI driver request_threaded_irq() function. This triggers kernel/irq/manage.c __setup_irq() which locks raw spinlock &desc->lock descriptor lock and with that descriptor lock held, calls rcar_msi_irq_unmask().
Since the &desc->lock descriptor lock is a raw spinlock , and the rcar_msi .mask_lock is not a raw spinlock, this setup triggers 'BUG: Invalid wait context' with CONFIG_PROVE_RAW_LOCK_NESTING=y .
Use scoped_guard() to simplify the locking.
Fixes: 83ed8d4fa656 ("PCI: rcar: Convert to MSI domains") 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 --- ============================= [ BUG: Invalid wait context ] 6.17.0-rc4-next-20250905-00049-g13b74d3367a3-dirty #1117 Not tainted ----------------------------- swapper/0/1 is trying to lock: ffffff84c1974e58 (&msi->mask_lock){....}-{3:3}, at: rcar_msi_irq_unmask+0x28/0x70 other info that might help us debug this: context-{5:5} 6 locks held by swapper/0/1: #0: ffffff84c0e0d0f8 (&dev->mutex){....}-{4:4}, at: device_lock+0x14/0x1c #1: ffffffc0817675b0 (pci_rescan_remove_lock){+.+.}-{4:4}, at: pci_lock_rescan_remove+0x18/0x20 #2: ffffff84c2a691b0 (&dev->mutex){....}-{4:4}, at: device_lock+0x14/0x1c #3: ffffff84c1976108 (&dev->mutex){....}-{4:4}, at: device_lock+0x14/0x1c #4: ffffff84c2a71640 (&desc->request_mutex){+.+.}-{4:4}, at: __setup_irq+0xa4/0x5bc #5: ffffff84c2a714c8 (&irq_desc_lock_class){....}-{2:2}, at: __setup_irq+0x230/0x5bc stack backtrace: CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc4-next-20250905-00049-g13b74d3367a3-dirty #1117 PREEMPT Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) Call trace: dump_backtrace+0x6c/0x7c (C) show_stack+0x14/0x1c dump_stack_lvl+0x68/0x8c dump_stack+0x14/0x1c __lock_acquire+0x3e8/0x1064 lock_acquire+0x17c/0x2ac _raw_spin_lock_irqsave+0x54/0x70 rcar_msi_irq_unmask+0x28/0x70 irq_chip_unmask_parent+0x18/0x20 cond_startup_parent+0x40/0x44 pci_irq_startup_msi+0x20/0x58 __irq_startup+0x34/0x84 irq_startup+0x64/0x114 __setup_irq+0x3f8/0x5bc request_threaded_irq+0x11c/0x148 pcie_pme_probe+0xec/0x190 pcie_port_probe_service+0x34/0x5c really_probe+0x190/0x350 __driver_probe_device+0x120/0x138 driver_probe_device+0x38/0xec __device_attach_driver+0x100/0x114 bus_for_each_drv+0xa8/0xd0 __device_attach+0xdc/0x15c device_initial_probe+0x10/0x18 bus_probe_device+0x38/0xa0 device_add+0x554/0x68c device_register+0x1c/0x28 pcie_portdrv_probe+0x480/0x518 pci_device_probe+0xcc/0x13c really_probe+0x190/0x350 __driver_probe_device+0x120/0x138 driver_probe_device+0x38/0xec __device_attach_driver+0x100/0x114 bus_for_each_drv+0xa8/0xd0 __device_attach+0xdc/0x15c device_initial_probe+0x10/0x18 pci_bus_add_device+0xb8/0x130 pci_bus_add_devices+0x50/0x74 pci_host_probe+0x90/0xc4 rcar_pcie_probe+0x5e8/0x650 platform_probe+0x58/0x88 really_probe+0x190/0x350 __driver_probe_device+0x120/0x138 driver_probe_device+0x38/0xec __driver_attach+0x158/0x168 bus_for_each_dev+0x7c/0xd0 driver_attach+0x20/0x28 bus_add_driver+0xe0/0x1d8 driver_register+0xac/0xe8 __platform_driver_register+0x1c/0x24 rcar_pcie_driver_init+0x18/0x20 do_one_initcall+0xd4/0x220 kernel_init_freeable+0x308/0x30c kernel_init+0x20/0x11c ret_from_fork+0x10/0x20 --- Cc: "Krzysztof Wilczyński" kwilczynski@kernel.org Cc: Bjorn Helgaas bhelgaas@google.com Cc: Geert Uytterhoeven geert+renesas@glider.be Cc: Lorenzo Pieralisi lpieralisi@kernel.org Cc: Magnus Damm magnus.damm@gmail.com Cc: Manivannan Sadhasivam mani@kernel.org Cc: Marc Zyngier maz@kernel.org Cc: Rob Herring robh@kernel.org Cc: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com Cc: linux-pci@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org --- drivers/pci/controller/pcie-rcar-host.c | 27 ++++++++++++------------- 1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/controller/pcie-rcar-host.c b/drivers/pci/controller/pcie-rcar-host.c index 625a00f3b2230..213028052aa58 100644 --- a/drivers/pci/controller/pcie-rcar-host.c +++ b/drivers/pci/controller/pcie-rcar-host.c @@ -12,6 +12,7 @@ */
#include <linux/bitops.h> +#include <linux/cleanup.h> #include <linux/clk.h> #include <linux/clk-provider.h> #include <linux/delay.h> @@ -38,7 +39,7 @@ struct rcar_msi { DECLARE_BITMAP(used, INT_PCI_MSI_NR); struct irq_domain *domain; struct mutex map_lock; - spinlock_t mask_lock; + raw_spinlock_t mask_lock; int irq1; int irq2; }; @@ -602,28 +603,26 @@ static void rcar_msi_irq_mask(struct irq_data *d) { struct rcar_msi *msi = irq_data_get_irq_chip_data(d); struct rcar_pcie *pcie = &msi_to_host(msi)->pcie; - unsigned long flags; u32 value;
- spin_lock_irqsave(&msi->mask_lock, flags); - value = rcar_pci_read_reg(pcie, PCIEMSIIER); - value &= ~BIT(d->hwirq); - rcar_pci_write_reg(pcie, value, PCIEMSIIER); - spin_unlock_irqrestore(&msi->mask_lock, flags); + scoped_guard(raw_spinlock_irqsave, &msi->mask_lock) { + value = rcar_pci_read_reg(pcie, PCIEMSIIER); + value &= ~BIT(d->hwirq); + rcar_pci_write_reg(pcie, value, PCIEMSIIER); + } }
static void rcar_msi_irq_unmask(struct irq_data *d) { struct rcar_msi *msi = irq_data_get_irq_chip_data(d); struct rcar_pcie *pcie = &msi_to_host(msi)->pcie; - unsigned long flags; u32 value;
- spin_lock_irqsave(&msi->mask_lock, flags); - value = rcar_pci_read_reg(pcie, PCIEMSIIER); - value |= BIT(d->hwirq); - rcar_pci_write_reg(pcie, value, PCIEMSIIER); - spin_unlock_irqrestore(&msi->mask_lock, flags); + scoped_guard(raw_spinlock_irqsave, &msi->mask_lock) { + value = rcar_pci_read_reg(pcie, PCIEMSIIER); + value |= BIT(d->hwirq); + rcar_pci_write_reg(pcie, value, PCIEMSIIER); + } }
static void rcar_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) @@ -736,7 +735,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host) int err;
mutex_init(&msi->map_lock); - spin_lock_init(&msi->mask_lock); + raw_spin_lock_init(&msi->mask_lock);
err = of_address_to_resource(dev->of_node, 0, &res); if (err)
Hi Marek,
CC tegra
On Tue, 9 Sept 2025 at 18:27, Marek Vasut marek.vasut+renesas@mailbox.org wrote:
The rcar_msi_irq_unmask() function may be called from a PCI driver request_threaded_irq() function. This triggers kernel/irq/manage.c __setup_irq() which locks raw spinlock &desc->lock descriptor lock and with that descriptor lock held, calls rcar_msi_irq_unmask().
Since the &desc->lock descriptor lock is a raw spinlock , and the rcar_msi .mask_lock is not a raw spinlock, this setup triggers 'BUG: Invalid wait context' with CONFIG_PROVE_RAW_LOCK_NESTING=y .
Use scoped_guard() to simplify the locking.
Fixes: 83ed8d4fa656 ("PCI: rcar: Convert to MSI domains") 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
Reviewed-by: Geert Uytterhoeven geert+renesas@glider.be
drivers/pci/controller/pci-tegra.c seems to have the same issue.
============================= [ BUG: Invalid wait context ] 6.17.0-rc4-next-20250905-00049-g13b74d3367a3-dirty #1117 Not tainted
swapper/0/1 is trying to lock: ffffff84c1974e58 (&msi->mask_lock){....}-{3:3}, at: rcar_msi_irq_unmask+0x28/0x70 other info that might help us debug this: context-{5:5} 6 locks held by swapper/0/1: #0: ffffff84c0e0d0f8 (&dev->mutex){....}-{4:4}, at: device_lock+0x14/0x1c #1: ffffffc0817675b0 (pci_rescan_remove_lock){+.+.}-{4:4}, at: pci_lock_rescan_remove+0x18/0x20 #2: ffffff84c2a691b0 (&dev->mutex){....}-{4:4}, at: device_lock+0x14/0x1c #3: ffffff84c1976108 (&dev->mutex){....}-{4:4}, at: device_lock+0x14/0x1c #4: ffffff84c2a71640 (&desc->request_mutex){+.+.}-{4:4}, at: __setup_irq+0xa4/0x5bc #5: ffffff84c2a714c8 (&irq_desc_lock_class){....}-{2:2}, at: __setup_irq+0x230/0x5bc stack backtrace: CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc4-next-20250905-00049-g13b74d3367a3-dirty #1117 PREEMPT Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) Call trace: dump_backtrace+0x6c/0x7c (C) show_stack+0x14/0x1c dump_stack_lvl+0x68/0x8c dump_stack+0x14/0x1c __lock_acquire+0x3e8/0x1064 lock_acquire+0x17c/0x2ac _raw_spin_lock_irqsave+0x54/0x70 rcar_msi_irq_unmask+0x28/0x70 irq_chip_unmask_parent+0x18/0x20 cond_startup_parent+0x40/0x44 pci_irq_startup_msi+0x20/0x58 __irq_startup+0x34/0x84 irq_startup+0x64/0x114 __setup_irq+0x3f8/0x5bc request_threaded_irq+0x11c/0x148 pcie_pme_probe+0xec/0x190 pcie_port_probe_service+0x34/0x5c really_probe+0x190/0x350 __driver_probe_device+0x120/0x138 driver_probe_device+0x38/0xec __device_attach_driver+0x100/0x114 bus_for_each_drv+0xa8/0xd0 __device_attach+0xdc/0x15c device_initial_probe+0x10/0x18 bus_probe_device+0x38/0xa0 device_add+0x554/0x68c device_register+0x1c/0x28 pcie_portdrv_probe+0x480/0x518 pci_device_probe+0xcc/0x13c really_probe+0x190/0x350 __driver_probe_device+0x120/0x138 driver_probe_device+0x38/0xec __device_attach_driver+0x100/0x114 bus_for_each_drv+0xa8/0xd0 __device_attach+0xdc/0x15c device_initial_probe+0x10/0x18 pci_bus_add_device+0xb8/0x130 pci_bus_add_devices+0x50/0x74 pci_host_probe+0x90/0xc4 rcar_pcie_probe+0x5e8/0x650
--- a/drivers/pci/controller/pcie-rcar-host.c +++ b/drivers/pci/controller/pcie-rcar-host.c @@ -12,6 +12,7 @@ */
#include <linux/bitops.h> +#include <linux/cleanup.h> #include <linux/clk.h> #include <linux/clk-provider.h> #include <linux/delay.h> @@ -38,7 +39,7 @@ struct rcar_msi { DECLARE_BITMAP(used, INT_PCI_MSI_NR); struct irq_domain *domain; struct mutex map_lock;
spinlock_t mask_lock;
raw_spinlock_t mask_lock; int irq1; int irq2;}; @@ -602,28 +603,26 @@ static void rcar_msi_irq_mask(struct irq_data *d) { struct rcar_msi *msi = irq_data_get_irq_chip_data(d); struct rcar_pcie *pcie = &msi_to_host(msi)->pcie;
unsigned long flags; u32 value;spin_lock_irqsave(&msi->mask_lock, flags);value = rcar_pci_read_reg(pcie, PCIEMSIIER);value &= ~BIT(d->hwirq);rcar_pci_write_reg(pcie, value, PCIEMSIIER);spin_unlock_irqrestore(&msi->mask_lock, flags);
scoped_guard(raw_spinlock_irqsave, &msi->mask_lock) {value = rcar_pci_read_reg(pcie, PCIEMSIIER);value &= ~BIT(d->hwirq);rcar_pci_write_reg(pcie, value, PCIEMSIIER);}}
static void rcar_msi_irq_unmask(struct irq_data *d) { struct rcar_msi *msi = irq_data_get_irq_chip_data(d); struct rcar_pcie *pcie = &msi_to_host(msi)->pcie;
unsigned long flags; u32 value;spin_lock_irqsave(&msi->mask_lock, flags);value = rcar_pci_read_reg(pcie, PCIEMSIIER);value |= BIT(d->hwirq);rcar_pci_write_reg(pcie, value, PCIEMSIIER);spin_unlock_irqrestore(&msi->mask_lock, flags);
scoped_guard(raw_spinlock_irqsave, &msi->mask_lock) {value = rcar_pci_read_reg(pcie, PCIEMSIIER);value |= BIT(d->hwirq);rcar_pci_write_reg(pcie, value, PCIEMSIIER);}}
static void rcar_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) @@ -736,7 +735,7 @@ static int rcar_pcie_enable_msi(struct rcar_pcie_host *host) int err;
mutex_init(&msi->map_lock);
spin_lock_init(&msi->mask_lock);
raw_spin_lock_init(&msi->mask_lock); err = of_address_to_resource(dev->of_node, 0, &res); if (err)
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, 09 Sep 2025 17:26:25 +0100, Marek Vasut marek.vasut+renesas@mailbox.org wrote:
The rcar_msi_irq_unmask() function may be called from a PCI driver request_threaded_irq() function. This triggers kernel/irq/manage.c __setup_irq() which locks raw spinlock &desc->lock descriptor lock and with that descriptor lock held, calls rcar_msi_irq_unmask().
Since the &desc->lock descriptor lock is a raw spinlock , and the rcar_msi .mask_lock is not a raw spinlock, this setup triggers 'BUG: Invalid wait context' with CONFIG_PROVE_RAW_LOCK_NESTING=y .
Use scoped_guard() to simplify the locking.
Fixes: 83ed8d4fa656 ("PCI: rcar: Convert to MSI domains") 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
Acked-by: Marc Zyngier maz@kernel.org
M.
Hi Marek,
CC tglx
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...
Gr{oetje,eeting}s,
Geert
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 ?
Hi Marek,
On Mon, 22 Sept 2025 at 12:44, Marek Vasut marek.vasut@mailbox.org wrote:
On 9/22/25 11:14 AM, Geert Uytterhoeven wrote:
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 ?
Or simply just "depends on !CONFIG_PCI_LOCKLESS_CONFIG"? What do the PCIe maintainers think?
Thanks!
Gr{oetje,eeting}s,
Geert
On 9/22/25 12:53 PM, Geert Uytterhoeven wrote:
Hello Geert,
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 ?
Or simply just "depends on !CONFIG_PCI_LOCKLESS_CONFIG"? What do the PCIe maintainers think?
I send a patch in the meantime:
[PATCH] PCI: rcar-host: Add static assertion to check !PCI_LOCKLESS_CONFIG
On Tue, 09 Sep 2025 18:26:24 +0200, Marek Vasut 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.
[...]
Applied, thanks!
[1/2] PCI: rcar-host: Drop PMSR spinlock commit: 0a8f173d9dad13930d5888505dc4c4fd6a1d4262 [2/2] PCI: rcar-host: Convert struct rcar_msi mask_lock into raw spinlock commit: 945878aa8b574f66ead4ab1844185376c0d0add4
Best regards,
linux-stable-mirror@lists.linaro.org
-
Geert Uytterhoeven -
Manivannan Sadhasivam -
Marc Zyngier -
Marek Vasut -
Marek Vasut