Re: [PATCH v5 8/9] 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 4 potential issue(s) to consider:
- [High] PCI: deadlock during hibernation due to unhandled PM notifiers
- [High] PCI: probe succeeds with uninitialized controller on DT error
- [High] PCI: NULL pointer dereference if event IRQ fires before root bus creation
- [Medium] PCI: ghost devices left on bus if link bounces during suspend
--

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

This commit adds support for reacting to link-state changes by handling
the DL_UpDown signal on RZ/G3 and RZ/V2 PCIe controllers. It introduces
a link event interrupt, hotplug-like logic, and PM notifiers to serialize
with device suspend/resume.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 3b62b1be5b2a1..78e783928b9dc 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
> @@ -1095,6 +1103,98 @@ 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) {
> +		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,

[Severity: High]
The event IRQ is requested and enabled in rzg3s_pcie_host_setup() before
bridge->bus is allocated by pci_host_probe().

If the hardware generates a spurious interrupt during this narrow window,
bridge->bus could be NULL when this handler runs.

Will this dereference a NULL pointer?

> +						 bus_list)
> +			pci_stop_and_remove_bus_device(dev);
> +		pci_unlock_rescan_remove();
> +	} else {
> +		struct pci_bus *child;
> +		int ret;
> +
> +		dev_info(host->dev, "PCIe link up, rescanning bus\n");

[Severity: Medium]
On resume, the PM notifier wakes the IRQ thread.

If the link is currently up, it seems this branch is taken and rescans the bus
without removing the old devices that might have been hot-swapped while the
link was down.

Could this leave ghost devices on the bus if the link goes down and comes back
up while the system is suspended?

[ ... ]
> @@ -1723,8 +1838,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]
If a mandatory resource is missing, such as resource_list_first_type()
returning NULL in rzg3s_pcie_config_init(), it returns -ENODEV.

This would be caught here as a safe link-down state and return success,
potentially leaving the controller uninitialized.

Does this pattern mask genuine configuration errors?

[ ... ]
> @@ -1905,6 +2026,31 @@ static void rzv2h_pcie_release_lanes(void *data)
>  	rzv2h_num_total_lanes -= host->num_lanes;
>  }
>  
> +static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
> +				  unsigned long action, void *data)
> +{
> +	struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
> +						    pm_nb);
> +
> +	switch (action) {
> +	case PM_SUSPEND_PREPARE:
> +		/* Disable link up/down interrupts. */
> +		disable_irq(host->event_irq);
> +		break;

[Severity: High]
The switch statement only handles PM_SUSPEND_PREPARE and PM_POST_SUSPEND,
completely omitting PM_HIBERNATION_PREPARE and PM_RESTORE_PREPARE.

If a link event occurs during hibernation transitions, the threaded interrupt
could attempt to acquire the same lock the PM core is holding.

Can this missing hibernation support open a window for a deadlock?

> +
> +	case PM_POST_SUSPEND:
> +		/* Enable link up/down interrupts and force link re-scan. */
> +		writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
> +			       host->axi + RZG3S_PCI_PEIE0);
> +		WRITE_ONCE(host->link_rescan, true);
> +		enable_irq(host->event_irq);
> +		irq_wake_thread(host->event_irq, host);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}

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