Re: [PATCH v5 9/9] PCI: rzg3s-host: Add bridge::reset_root_port()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] pci: rzg3s: INTx/MSI mask state loss due to stale register snapshot
- [High] pci: rzg3s: fatal unpowered MMIO access in PM notifier after reset failure
- [High] pci: rzg3s: lockless iteration over bus->devices causes list corruption
- [Medium] pci: rzg3s: missing I/O flush allows IRQs to fire after synchronize_irq
--

commit c5ffdc83b56df877b7b137c1229cfe9487c3fb34
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Subject: PCI: rzg3s-host: Add bridge::reset_root_port()

This commit adds a reset_root_port() callback for the rzg3s-host bridge.
It introduces the started boolean to track the controller state and prevents
double start/stop from breaking reference counters. It also updates the link
event and power management notifier paths to handle the interrupts correctly.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 78e783928b9dc..eea215818633b 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1115,6 +1117,11 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host)
>  
>  		dev_info(host->dev, "PCIe link down, removing devices\n");
>  
> +		for_each_pci_bridge(dev, bridge->bus) {
> +			if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> +				pci_host_handle_link_down(dev);
> +		}
> +
>  		pci_lock_rescan_remove();

[Severity: High]
Is it safe to iterate over bus->devices without holding the PCI
rescan/remove lock?

It looks like concurrent modifications to the list from sysfs rescan or
hotplug events could cause list corruption or a use-after-free. The
necessary lock is correctly taken immediately after this loop for the removal
step, but appears to be omitted for this iteration.

[ ... ]
> @@ -1956,6 +1974,64 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
>  	return ret;
>  }
>  
> +static int rzg3s_pcie_host_reset_root_port(struct pci_host_bridge *bridge,
> +					   struct pci_dev *pdev)
> +{
> +	struct rzg3s_pcie_host *host = pci_host_bridge_priv(bridge);
> +	unsigned long flags;
> +	u32 irqs;
> +	int ret;
> +
> +	/* Mask link up/down interrupts. */
> +	writel(0, host->axi + RZG3S_PCI_PEIE0);
> +
> +	/* Mask INTx and MSI interrupts. */
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +	irqs = readl(host->axi + RZG3S_PCI_PINTRCVIE);
> +	writel(0, host->axi + RZG3S_PCI_PINTRCVIE);
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +
> +	/*
> +	 * Make sure the next operations are not disturbed by any pending
> +	 * IRQs.
> +	 */
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		synchronize_irq(host->msi.irq);

[Severity: Medium]
Does the posted write to the interrupt mask register need to be flushed
with a dummy read before calling synchronize_irq()?

Without a read-back to ensure the mask has reached the physical hardware, the
controller might still emit an interrupt while synchronize_irq() runs,
potentially creating a race window where the handler crashes when the
controller clocks are subsequently cut in rzg3s_pcie_host_stop().

> +	for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
> +		synchronize_irq(host->intx_irqs[i]);
> +
> +	ret = rzg3s_pcie_host_stop(host);
> +	if (ret) {
> +		dev_err(host->dev, "Failed to stop the host!\n");
> +		goto unmask_irqs;
> +	}
> +
> +	ret = rzg3s_pcie_host_start(host, false);
> +	if (ret) {
> +		dev_err(host->dev, "Failed to start the host!\n");
> +
> +		/*
> +		 * Don't unmask IRQs. We are in a bad state here and we
> +		 * can recover only through a suspend/resume cycle. Just
> +		 * return and preserve the stop state.
> +		 */
> +		return ret;
> +	}
> +
> +unmask_irqs:
> +	/* Unmask INTx and MSI interrupts. */
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +	writel_relaxed(irqs, host->axi + RZG3S_PCI_PINTRCVIE);
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);

[Severity: High]
Could this unconditional restore overwrite mask changes made by concurrent
INTx mask/unmask operations?

Since hw_lock was dropped earlier to wait for IRQs and execute the
controller stop/start sequence, any intermediate updates to the register by
other drivers dynamically masking or unmasking their IRQs might be
permanently lost here when the old snapshot is restored.

> +
> +	/* Unmask link up/down interrupts. */
> +	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
> +
> +	host->started = true;
> +
> +	return ret;
> +}
[ ... ]
> @@ -2036,6 +2112,7 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
>  	case PM_SUSPEND_PREPARE:
>  		/* Disable link up/down interrupts. */
>  		disable_irq(host->event_irq);
> +		writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);

[Severity: High]
Could this write cause a synchronous external abort if the controller is
currently unpowered?

If rzg3s_pcie_host_start() fails inside rzg3s_pcie_host_reset_root_port(),
the function intentionally returns early, leaving host->started false and
the controller in reset with clocks disabled. A subsequent system suspend
would unconditionally hit this AXI write, which might hang the bus since
the target peripheral is held in reset.

>  		break;
>  
>  	case PM_POST_SUSPEND:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002110935.2004349-1-claudiu.beznea@kernel.org?part=9