Re: [PATCH v4 7/8] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: rzg3s-host: misses PCIe link state changes during system suspend
- [Medium] pci: rzg3s-host: TOCTOU race in link event handler causes device confusion
--

commit d39725457acf3fbf2e1dca55279894b4bb68e696
Author: John Madieu <john.madieu.xa@bp.renesas.com>
Subject: PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

This commit enables the DL_UpDown interrupt to provide hotplug-like behavior
by re-enumerating the bus on link-state changes. It processes link up and
down events to scan or remove devices accordingly, and updates the probe path
to tolerate an absent device.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index f4b27a77a89df..1c0353598b599 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1095,6 +1098,89 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
>  	return ret;
>  }
>  
> +static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host)
> +{
> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
> +	struct pci_bus *bus = bridge->bus;
> +	u32 val;
> +
> +	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> +	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {

[Severity: Medium]
Is there a TOCTOU race here if the link flaps rapidly?

The handler reads the live link state to decide whether to remove or rescan
devices. If the link goes down and comes back up before this threaded handler
runs, it will see DL_DOWN_STS == 0 and take the else path.

> +		struct pci_dev *dev, *tmp;
> +
> +		dev_info(host->dev, "PCIe link down, removing devices\n");
> +
> +		pci_lock_rescan_remove();
> +		list_for_each_entry_safe_reverse(dev, tmp, &bus->devices,
> +						 bus_list)
> +			pci_stop_and_remove_bus_device(dev);
> +		pci_unlock_rescan_remove();
> +	} else {
> +		int ret;
> +
> +		dev_info(host->dev, "PCIe link up, rescanning bus\n");
> +
> +		/*
> +		 * Attempt link speed negotiation now that the link is up.
> +		 * Failure is non-fatal: the device works at the negotiated
> +		 * speed.
> +		 */
> +		ret = rzg3s_pcie_set_max_link_speed(host);
> +		if (ret)
> +			dev_info(host->dev, "Failed to set max link speed\n");
> +
> +		pci_host_common_link_train_delay(host->max_link_speed);
> +
> +		pci_lock_rescan_remove();
> +		pci_rescan_bus(bus);

Because the DOWN event was missed, old devices are never removed from
bus->devices. Calling pci_rescan_bus() here will skip enumerating them,
leaving the kernel with a stale pci_dev bound to a potentially reset or
swapped device.

Could we tear down existing devices before rescanning, or otherwise account
for missed DOWN events?

> +		pci_unlock_rescan_remove();
> +	}
> +}
[ ... ]
> @@ -1723,8 +1824,14 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
>  
>  	ret = rzg3s_pcie_host_init(host);
>  	if (ret) {
> -		dev_err_probe(dev, ret, "Failed to initialize the HW!\n");
> -		goto teardown_irqdomain;
> +		if (ret != -ENODEV) {
> +			dev_err_probe(dev, ret,
> +				      "Failed to initialize the HW!\n");
> +			goto teardown_irqdomain;
> +		}
> +
> +		/* Link is down: hotplug via DL_UpDown will recover. */
> +		return 0;

[Severity: High]
Does this miss PCIe link state changes that happen during system suspend?

During system suspend, power is lost and the link state is reset. On
resume, rzg3s_pcie_resume_noirq() restarts the host, which eventually
calls rzg3s_pcie_host_setup(). 

If the link fails to come up, -ENODEV is swallowed here, assuming the
DL_UPDOWN interrupt will recover it. However, the DL_UPDOWN bit won't
trigger an edge interrupt on resume because the controller was just
initialized and isn't transitioning. 

If a device was unplugged or plugged in during sleep, the PCI core is
never notified, potentially leading to device timeouts or state
corruption. Should we manually check the link state and synchronize the
PCI core after unmasking the interrupt in rzg3s_pcie_resume_noirq()?

>  	}
>  
>  	ret = rzg3s_pcie_set_max_link_speed(host);

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the read-modify-write on W1C
interrupt registers drop concurrent interrupts?

In rzg3s_pcie_msi_irq() and rzg3s_pcie_intx_irq_ack(), the driver uses
rzg3s_pcie_update_bits() to acknowledge specific interrupts:

	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS,
			       RZG3S_PCI_PINTRCVIS_MSI,
			       RZG3S_PCI_PINTRCVIS_MSI);

Registers like RZG3S_PCI_PINTRCVIS are W1C (Write 1 to Clear). Reading
yields a 1 for every currently pending interrupt, and writing that value
back writes a 1 to all those bits. Any concurrent interrupts that were
pending at the time of the read will be silently cleared and lost,
resulting in device timeouts.

[Severity: Medium]
This is a pre-existing issue, but is the MSI hardware configured
unconditionally even if CONFIG_PCI_MSI is disabled?

During device resume, rzg3s_pcie_host_start() passes
rzg3s_pcie_msi_hw_setup() as the initialization callback:

	ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
				    rzg3s_pcie_msi_hw_teardown);

If CONFIG_PCI_MSI is disabled, MSI software structures (like
msi->window_base) are never initialized and remain 0. This pushes
uninitialized zero values to hardware registers and routes MSI interrupts
to physical address 0. Any DMA write to address 0 will trigger a
spurious hardware event.

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