Re: [PATCH v6 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:
- [Critical] pci: rzg3s-host: Missing sysfs/AER vs event_irq synchronization causing AXI abort
- [High] pci: PINTRCVIE INTx/MSI mask overwritten after unlocked hardware reset
- [Medium] pci: goto-based error handling mixed with __free() scope cleanups
- [Medium] pcie: rzg3s: data race on PEIE0 register during concurrent sysfs reset and suspend
--

commit 52776c6784c4d35d5a362a3b52bff852eb31f68f
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

PCI: rzg3s-host: Add bridge::reset_root_port()

This commit adds the bridge::reset_root_port() implementation to allow
reads and writes to an NVMe endpoint after a link down/up cycle. It
masks all enabled interrupts, resets the controller, and synchronizes
IRQs to prevent asynchronous events from interfering with the reset.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index c2fbacfe67044..a8b222625a2a6 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1193,8 +1193,27 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
>  	u32 val;
>  	int ret;
>  
> +	/*
> +	 * A latched DL_UpDown means at least one real link transition took
> +	 * place since the last enumeration. If devices are enumerated
> +	 * recover the Root Port first.
> +	 */
> +	if (bounced && !list_empty(&bus->devices)) {
> +		struct pci_dev *port __free(pci_dev_put) =
> +			pci_get_slot(bus, PCI_DEVFN(0, 0));

[Severity: Medium]
This adds a __free() variable to a function that relies on goto-based error
handling (e.g. goto unlock; later in this function).

The global cleanup guidelines state that goto and cleanup helpers should never
be mixed in the same function. Should all resources in this function be
converted to scope-based cleanup, or should we continue using traditional
cleanup here?

> +
> +		if (port && pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> +			pci_host_handle_link_down(port);
> +	}
> +
>  	pci_lock_rescan_remove();
>  
> +	/*
> +	 * Read the link state after the recovery: the .reset_root_port()
> +	 * retrains the link, so a device that is still present comes back
> +	 * up here and is re-enumerated right away instead of waiting for
> +	 * the next DL_UpDown event.
> +	 */
>  	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);

[Severity: Critical]
Can reading this AXI register cause a synchronous external abort (kernel panic)
if a concurrent sysfs/AER thread calls rzg3s_pcie_host_stop() and disables
the clocks?

Since rzg3s_pcie_host_reset_root_port() intentionally omits
synchronize_irq(host->event_irq), and no other locks are held here to
exclude the sysfs reset path, it seems this thread can read the register
while power resets are asserted.

>  	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
>  		if (!list_empty(&bus->devices)) {
[ ... ]
> @@ -2053,6 +2073,77 @@ 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, link_irqs;
> +	int ret;
> +
> +	/* Mask link up/down interrupts. */
> +	link_irqs = readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
> +	writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +
> +	/* Mask INTx and MSI interrupts. */
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +	irqs = readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
> +	writel_relaxed(0, host->axi + RZG3S_PCI_PINTRCVIE);
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);

[Severity: High]
This caches the RZG3S_PCI_PINTRCVIE register in the local variable irqs,
then drops the lock.

If an endpoint driver concurrently calls request_irq(), free_irq(), or
enable_irq() for an INTx/MSI interrupt during the unlocked window below,
won't the stale irqs value be written back later, permanently losing or
causing spurious IRQ masks?

> +
> +	/* Flush IRQ masks. */
> +	readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
> +	readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
> +
> +	/*
> +	 * Make sure the next operations are not disturbed by any pending
> +	 * IRQs.
> +	 */
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		synchronize_irq(host->msi.irq);
> +	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);

[Severity: High]
This restores the potentially stale irqs local variable, overwriting
any mask changes made concurrently by the IRQ subsystem while the lock
was dropped.

> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +
> +	/*
> +	 * rzg3s_pcie_host_start() retrains the link. If the link comes
> +	 * back up, DL_UpDown gets latched in PEIS0. Clear the latch.
> +	 * The link state is evaluated after this function returns, so
> +	 * the event is not lost. For the same reason there is no need
> +	 * to set host->link_rescan.
> +	 */
> +	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
> +	/* Unmask link up/down interrupts. */
> +	writel_relaxed(link_irqs, host->axi + RZG3S_PCI_PEIE0);

[Severity: Medium]
If a user concurrently triggers a sysfs reset while the system is
entering suspend, can this unconditionally restore an unmasked PEIE0 state
and overwrite the interrupt mask set by PM_SUSPEND_PREPARE?

Looking at rzg3s_pcie_pm_notifier(), it executes in PM_SUSPEND_PREPARE and
zeroes PEIE0 to mask interrupts for system sleep. A concurrent sysfs reset
will read the unmasked state, complete the reset, and blindly write the
unmasked state back, leaving interrupts unmasked during suspend and
potentially causing wake-up or power domain transition failures.

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

[Severity: Medium]
This zeroes PEIE0 for system sleep, but a racing sysfs reset in
rzg3s_pcie_host_reset_root_port() can unconditionally overwrite this
state with its saved unmasked value.

>  		break;
>  
>  	case PM_POST_SUSPEND:

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