Re: [PATCH v7 14/14] PCI: rzg3s-host: Add bridge::reset_root_port()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] PCI: rzg3s-host: unguarded MMIO access and lost INTx mask after start failure
- [High] PCI: rzg3s-host: lost MSI/INTx mask state across rzg3s_pcie_host_reset_root_port()
- [High] PCI: rzg3s-host: race and lost link event in rzg3s_pcie_host_reset_root_port()
- [Medium] PCI: rzg3s-host: race in rzg3s_pcie_host_reset_root_port() state ordering
- [Medium] PCI: rzg3s-host: race on RZG3S_PCI_PEIE0 between reset and PM notifier
- [Medium] PCI: rzg3s-host: ineffective disable_irq() on chained INTx IRQs in reset
--

commit 6658179b714958398bb0f41660bab61430893417
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

This commit implements pci_host_bridge::reset_root_port() for the Renesas
RZ/G3S PCIe host controller so the Root Port can be reset and recovered
across a link down/up cycle. It masks and disables MSI and INTx interrupts
around stopping and restarting the controller and adds a host->resetting
flag to block concurrent register accesses from the IRQ callbacks.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index a1c9e29f43e5d..f6823c9bc4149 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c

[ ... ]

> @@ -653,6 +655,9 @@ static void rzg3s_pcie_msi_irq_mask(struct irq_data *d)
>  
>  	guard(raw_spinlock_irqsave)(&host->hw_lock);
>  
> +	if (host->resetting)
> +		return;
> +
>  	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id), BIT(reg_bit),
>  			       BIT(reg_bit));
>  }
> @@ -666,6 +671,9 @@ static void rzg3s_pcie_msi_irq_unmask(struct irq_data *d)
>  
>  	guard(raw_spinlock_irqsave)(&host->hw_lock);
>  
> +	if (host->resetting)
> +		return;
> +
>  	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id), BIT(reg_bit),
>  			       0);
>  }
> @@ -972,6 +980,9 @@ static void rzg3s_pcie_intx_irq_mask(struct irq_data *d)
>  
>  	guard(raw_spinlock_irqsave)(&host->hw_lock);
>  
> +	if (host->resetting)
> +		return;
> +
>  	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIE,
>  			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq), 0);
>  }
> @@ -982,6 +993,9 @@ static void rzg3s_pcie_intx_irq_unmask(struct irq_data *d)
>  
>  	guard(raw_spinlock_irqsave)(&host->hw_lock);
>  
> +	if (host->resetting)
> +		return;
> +
>  	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIE,
>  			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq),
>  			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq));

[Severity: High]
Could returning early when host->resetting is true cause the hardware IRQ
masks in RZG3S_PCI_MSIRM(reg_id) and RZG3S_PCI_PINTRCVIE to go out of sync
with genirq?

In kernel/irq/chip.c, mask_irq() and unmask_irq() unconditionally update
IRQD_IRQ_MASKED after calling the chip callback. If an INTx or MSI vector is
masked or unmasked while host->resetting is true, these callbacks drop the
update without recording it, and rzg3s_pcie_host_reset_root_port() later
writes the pre-reset irqs snapshot back to RZG3S_PCI_PINTRCVIE.

In particular, rzg3s_pcie_host_reset_root_port() sets host->resetting = true
before reading RZG3S_PCI_PINTRCVIE into irqs (outside host->hw_lock) and
before calling disable_irq(host->intx_irqs[i]). If an INTx interrupt is in
flight on another CPU in handle_level_irq() or in a threaded IRQF_ONESHOT
handler on a child virq when irqs is snapshotted, its bit in
RZG3S_PCI_PINTRCVIE is 0. When that handler finishes and calls
cond_unmask_irq() or irq_finalize_oneshot() -> rzg3s_pcie_intx_irq_unmask(),
the unmask is ignored because host->resetting is true, and restoring the
stale irqs snapshot at unmask_irqs leaves the INTx line masked in hardware
while genirq considers it unmasked.

Also, because rzg3s_pcie_msi_parent_ops sets MSI_FLAG_PCI_MSI_MASK_PARENT,
per-vector MSI masking lives in RZG3S_PCI_MSIRM(reg_id) rather than
RZG3S_PCI_PINTRCVIE. Across rzg3s_pcie_host_stop() and
rzg3s_pcie_host_start(), RZG3S_PCI_MSIRM(reg_id) is reset and never saved or
restored.

> @@ -1185,12 +1199,46 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
>  {
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
>  	struct pci_bus *bus = bridge->bus, *child;
> +	unsigned long flags;
>  	u32 val;
>  	int ret;
>  
> -	pci_lock_rescan_remove();
> +	/*
> +	 * 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 = pci_get_slot(bus, PCI_DEVFN(0, 0));
> +
> +		if (port && pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> +			pci_host_handle_link_down(port);
>  
> +		pci_dev_put(port);
> +	}
> +
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +
> +	/*
> +	 * Don't continue if reset in progress or .reset_root_port() failed to
> +	 * start HW.
> +	 */
> +	if (host->resetting || !READ_ONCE(host->started)) {
> +		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +		return;
> +	}
> +
> +	/*
> +	 * Check 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);
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);

[Severity: High]
What prevents a concurrent rzg3s_pcie_host_reset_root_port() from stopping
the controller after host->hw_lock is released here while
rzg3s_pcie_link_event() is still accessing controller registers?

bridge->reset_root_port can be invoked outside rzg3s_pcie_link_event()
through pci_bridge_secondary_bus_reset() -> pcibios_reset_secondary_bus()
(for example via sysfs reset_subordinate, pci_reset_function(),
pci_reset_bus(), or aer_root_reset()). Once rzg3s_pcie_link_event() drops
host->hw_lock after reading RZG3S_PCI_PCSTAT1, it proceeds to touch
host->axi and host->pcie in rzg3s_pcie_device_replaced(),
rzg3s_pcie_set_max_link_speed(), pci_scan_child_bus(), and
rzg3s_pcie_cache_connected_dev_id(). Because rzg3s_pcie_host_reset_root_port()
only takes host->hw_lock briefly to set host->resetting = true and does not
synchronize with host->event_irq, it can enter rzg3s_pcie_host_stop() and
assert power_resets while rzg3s_pcie_link_event() is in the middle of those
MMIO accesses.

Conversely, if rzg3s_pcie_event_irq_thread() clears RZG3S_PCI_PEIS0_DL_UPDOWN
and host->link_rescan and then rzg3s_pcie_link_event() returns early here
because a concurrent reset set host->resetting = true, won't that link event
or rescan request be lost?

[ ... ]

> @@ -2038,6 +2095,91 @@ 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;
> +
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +	if (host->resetting) {
> +		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +		return -EBUSY;
> +	}
> +	host->resetting = true;
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +
> +	/* 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. */
> +	irqs = readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
> +	writel_relaxed(0, host->axi + RZG3S_PCI_PINTRCVIE);
> +
> +	/* 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))
> +		disable_irq(host->msi.irq);
> +	for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
> +		disable_irq(host->intx_irqs[i]);

[Severity: Medium]
Does disable_irq(host->intx_irqs[i]) actually mask or synchronize the INTx
interrupts here?

In rzg3s_pcie_init_irqdomain(), host->intx_irqs[i] are registered with
irq_set_chained_handler_and_data(irq, rzg3s_pcie_intx_irq_handler, host)
without IRQ_DISABLE_UNLAZY. On GICv3, irq_chip::irq_disable is NULL, so
__irq_disable() only sets IRQD_IRQ_DISABLED (lazy disable) without masking
the GIC SPI. And because chained handlers replace desc->handle_irq with
rzg3s_pcie_intx_irq_handler(), generic_handle_irq_desc() invokes
rzg3s_pcie_intx_irq_handler() directly without checking irqd_irq_disabled()
or setting IRQD_IRQ_INPROGRESS.

If an INTx SPI is already pending at the GIC before RZG3S_PCI_PINTRCVIE is
cleared, or glitches while resets are toggled, rzg3s_pcie_intx_irq_handler()
can still run during reset and call generic_handle_domain_irq() ->
handle_level_irq() -> mask_ack_irq() -> rzg3s_pcie_intx_irq_ack(), which
writes to host->axi + RZG3S_PCI_PINTRCVIS without checking host->resetting.

> +
> +	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
> +		 * re-enable the IRQs and return.
> +		 */
> +		goto enable_irqs;
> +	}

[Severity: High]
If rzg3s_pcie_host_start(host, false) fails here and jumps to enable_irqs,
won't subsequent callers access host->pcie and host->axi while power_resets
are asserted and RST_RSM_B is 0?

When rzg3s_pcie_host_reset_root_port() returns an error to
pcibios_reset_secondary_bus(), pci_bridge_secondary_bus_reset() still calls
pci_bridge_wait_for_secondary_bus(), which immediately polls config space
through rzg3s_pcie_root_ops (rzg3s_pcie_root_map_bus()) and
rzg3s_pcie_child_ops (rzg3s_pcie_child_read()), neither of which checks
host->started or host->resetting. Moreover, rzg3s_pcie_link_event() returns
early on !READ_ONCE(host->started) without removing or marking child devices
disconnected, so endpoint drivers and pci_pm_suspend() during the recovery
suspend/resume cycle will also access config space and MMIO while the IP is
held in reset.

In addition, jumping to enable_irqs discards the local irqs variable holding
the saved RZG3S_PCI_PINTRCVIE value. When rzg3s_pcie_resume_noirq() later
calls rzg3s_pcie_host_start(host, true), only RZG3S_PCI_PINTRCVIE_MSI is set
and the INTx enable bits in RZG3S_PCI_PINTRCVIE are never restored. And if
rzg3s_pcie_resume_noirq() itself fails in rzg3s_pcie_host_start(host, true),
host->started remains false while host->resetting is false, so
rzg3s_pcie_host_reset_root_port(), rzg3s_pcie_msi_irq_mask/unmask(),
rzg3s_pcie_intx_irq_mask/unmask(), and rzg3s_pcie_event_irq_thread() (which
only check host->resetting) will access host->axi while the IP is not
started.

> +
> +unmask_irqs:
> +	/* Unmask INTx and MSI interrupts. */
> +	writel_relaxed(irqs, host->axi + RZG3S_PCI_PINTRCVIE);
> +
> +	/*
> +	 * 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);

[Severity: High]
Is it safe to clear RZG3S_PCI_PEIS0_DL_UPDOWN here without setting
host->link_rescan or waking host->event_irq when .reset_root_port() is
called outside rzg3s_pcie_link_event()?

When rzg3s_pcie_host_reset_root_port() is invoked via
pci_bridge_secondary_bus_reset() -> pcibios_reset_secondary_bus() from a path
other than rzg3s_pcie_link_event() (such as pci_reset_function(),
pci_reset_bus(), or sysfs reset_subordinate), nothing evaluates the link
state after this function returns, so clearing RZG3S_PCI_PEIS0_DL_UPDOWN
drops any link transition that occurred during the reset.

> +	/* Unmask link up/down interrupts. */
> +	writel_relaxed(link_irqs, host->axi + RZG3S_PCI_PEIE0);
> +
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +	host->resetting = false;
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +
> +	WRITE_ONCE(host->started, true);

[Severity: Medium]
Can the ordering between unmasking RZG3S_PCI_PEIE0, clearing
host->resetting, and setting host->started = true cause false failure
detection or a dropped link interrupt?

First, RZG3S_PCI_PEIE0 is unmasked before host->resetting is cleared, while
host->event_irq remains enabled throughout rzg3s_pcie_host_reset_root_port().
If a link event fires right after RZG3S_PCI_PEIE0 is written,
rzg3s_pcie_event_irq_thread() can run while host->resetting is still true
and return IRQ_NONE.

Second, host->resetting = false is committed and host->hw_lock is released
before WRITE_ONCE(host->started, true) (since rzg3s_pcie_host_stop() set
host->started = false and rzg3s_pcie_host_start(host, false) was called with
set_started = false). In that window, both host->resetting and host->started
are false, so a reader holding host->hw_lock in rzg3s_pcie_link_event():

	if (host->resetting || !READ_ONCE(host->started)) {
		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
		return;
	}

or in rzg3s_pcie_pm_notifier() can observe host->resetting == false and
host->started == false and treat a successful reset as a failed hardware
start. Note that rzg3s_pcie_host_start() (when set_started is true) and
rzg3s_pcie_host_stop() also update host->started and host->resetting without
holding host->hw_lock.

> +
> +enable_irqs:
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		enable_irq(host->msi.irq);
> +	for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
> +		enable_irq(host->intx_irqs[i]);
> +
> +	return ret;
> +}
> +
>  static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
>  {
>  	struct device_node *np = host->dev->of_node;
> @@ -2113,18 +2255,22 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
>  {
>  	struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
>  						    pm_nb);
> +	unsigned long flags;
>  
>  	switch (action) {
>  	case PM_SUSPEND_PREPARE:
>  		/* Disable link up/down interrupts. */
> -		if (host->started)
> +		raw_spin_lock_irqsave(&host->hw_lock, flags);
> +		if (READ_ONCE(host->started) && !host->resetting)
>  			writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
>  		disable_irq(host->event_irq);
>  		break;
>  
>  	case PM_POST_SUSPEND:
>  		/* Enable link up/down interrupts and force link re-scan. */
> -		if (host->started) {
> +		raw_spin_lock_irqsave(&host->hw_lock, flags);
> +		if (READ_ONCE(host->started) && !host->resetting) {
>  			/*
>  			 * The link re-train in resume triggers the link up IRQ
>  			 * if there are connected devices. Clear the DL_UpDown
> @@ -2137,8 +2283,9 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
>  			writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
>  				       host->axi + RZG3S_PCI_PEIE0);
>  		}
> +		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
>  		enable_irq(host->event_irq);
> -		if (host->started)
> +		if (READ_ONCE(host->started))
>  			irq_wake_thread(host->event_irq, host);
>  		break;
>  	}

[Severity: Medium]
Can rzg3s_pcie_pm_notifier() and rzg3s_pcie_host_reset_root_port() race on
RZG3S_PCI_PEIE0?

PM_SUSPEND_PREPARE runs before tasks are frozen or devices are suspended. If
rzg3s_pcie_host_reset_root_port() is already running, it saves link_irqs =
RZG3S_PCI_PEIE0_DL_UPDOWN and sets host->resetting = true. When
rzg3s_pcie_pm_notifier(PM_SUSPEND_PREPARE) runs concurrently, it sees
host->resetting == true and skips writing 0 to RZG3S_PCI_PEIE0, and
rzg3s_pcie_host_reset_root_port() then writes link_irqs back to
RZG3S_PCI_PEIE0 at unmask_irqs, leaving link interrupts unmasked in hardware
during suspend.

Conversely, if PM_SUSPEND_PREPARE runs first and writes 0 to
RZG3S_PCI_PEIE0, and a reset is then triggered before devices suspend,
rzg3s_pcie_host_reset_root_port() saves link_irqs = 0. If suspend aborts and
PM_POST_SUSPEND runs while the reset is still in progress (host->resetting
== true, host->started == false), PM_POST_SUSPEND skips re-enabling
RZG3S_PCI_PEIE0_DL_UPDOWN and skips irq_wake_thread(), and
rzg3s_pcie_host_reset_root_port() restores link_irqs = 0 at unmask_irqs,
leaving link up/down interrupts disabled after resume.

[ ... ]

>
> -- 
> 2.43.0
>

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