Re: [PATCH v2 13/15] PCI: rcar-gen4: Route Root Port AER to a virtual Root Port IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] PCI: rcar: locking anti-pattern caused by wrong IRQ flow handler
--

commit 06909c0e9b8f6e26337c04f8e704d2dde673633d
Author: Koichiro Den <den@valinux.co.jp>

    PCI: rcar-gen4: Route Root Port AER to a virtual Root Port IRQ

This commit routes Root Port AER notifications to a virtual Root Port IRQ by
allocating an IRQ in a single-entry domain and setting no_rp_msi. It also adds
logic to serialize dispatches from parent IRQs to this virtual IRQ using a
raw spinlock.

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index aa80232afabad..c01f157a7198b 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c

[ ... ]

> @@ -689,6 +707,21 @@ static bool rcar_gen4_pcie_handle_link_down(struct rcar_gen4_pcie *rcar)
>  	return rcar->reinit_pending;
>  }
>  
> +static void rcar_gen4_pcie_rp_irq_dispatch(struct rcar_gen4_pcie *rcar)
> +{
> +	/*
> +	 * Serialize dispatches from the parent IRQs. handle_level_irq() would
> +	 * ack an IRQ already in progress without running its handlers again.
> +	 * Recheck after taking the lock since the parent's check may be stale.
> +	 */
> +	guard(raw_spinlock_irqsave)(&rcar->rp_irq_lock);
> +
> +	if (rcar_gen4_pcie_handle_link_down(rcar))
> +		return;
> +
> +	generic_handle_domain_irq(rcar->rp_irq_domain, 0);

[Severity: High]
Does holding a raw spinlock across generic_handle_domain_irq() extend the
atomic section over arbitrary PCIe port service handlers (like AER and PME)?

On PREEMPT_RT, if a child handler acquires a sleepable lock (e.g. spinlock_t),
wouldn't this trigger a 'sleeping while atomic' panic?

> +}
> +
>  static void rcar_gen4_pcie_link_down_work(struct work_struct *work)
>  {

[ ... ]

> +static int rcar_gen4_pcie_rp_irq_map(struct irq_domain *domain,
> +				     unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &rcar_gen4_pcie_rp_irq_chip,
> +				 handle_level_irq);

[Severity: High]
Could the external serialization lock workaround above be avoided entirely?

Since handle_level_irq drops back-to-back latched interrupts by aborting if
IRQ_INPROGRESS, using handle_edge_irq for latched hardware sources instead
natively loops on IRQS_PENDING to handle concurrent parent triggers. This
would completely eliminate the need for an external serialization lock.

> +	irq_set_chip_data(irq, domain->host_data);
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928165230.3397664-1-den@valinux.co.jp?part=13