Re: [PATCH v7 13/14] 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 7 potential issue(s) to consider:
- [High] pci: rzg3s-host: rzg3s_pcie_remove_devices() marks Root Port disconnected
- [High] pci: rzg3s-host: rzg3s_pcie_pm_notifier() misses hibernation PM events
- [High] PCI: rzg3s-host: race between rzg3s_pcie_probe() and PM notifier callbacks
- [High] PCI: rzg3s-host: deadlock on pm_chain_head.rwsem in rzg3s_pcie_pm_notifier()
- [Medium] pci: rzg3s-host: late PEIS0_DL_UPDOWN clear loses link bounce events
- [Medium] pci: rzg3s-host: rzg3s_pcie_link_event() reads config before link settles
- [Medium] PCI: rzg3s-host: controller registers accessed after resume_noirq() failure
--

commit 1730fab066afd00592ed2f6ea77d04307ed48cf0
Author: John Madieu <john.madieu.xa@bp.renesas.com>

PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

This commit enables the DL_UpDown interrupt on RZ/G3S and related PCIe
controllers to detect link-state transitions and hotplug-like events,
removing or rescanning devices on the bus accordingly. It also adds a PM
notifier to disable link event interrupts across system suspend and force a
bus rescan on resume, and makes probe tolerant of an initially down link.

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

[ ... ]

> @@ -1094,12 +1108,187 @@ static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)

[ ... ]

> +static void rzg3s_pcie_remove_devices(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev, *tmp;
> +
> +	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
> +		pci_dev_set_disconnected(dev, NULL);
> +		if (pci_has_subordinate(dev)) {
> +			pci_walk_bus(dev->subordinate, pci_dev_set_disconnected,
> +				     NULL);
> +		}
> +		pci_stop_and_remove_bus_device(dev);
> +	}
> +}

[Severity: High]
Both callers in rzg3s_pcie_link_event() pass bridge->bus (the root bus) to
rzg3s_pcie_remove_devices(), where dev in &bus->devices is the controller's
own integrated Root Port (0000:00:00.0) rather than an endpoint across the
link.

Should pci_dev_set_disconnected() only be called on dev->subordinate (as
done in rzg3s_pcie_resume_noirq()) rather than on dev itself?

Because pci_read_config_*() and pci_write_config_*() immediately abort with
PCIBIOS_DEVICE_NOT_FOUND when pci_dev_is_disconnected(dev) is true, marking
the Root Port as disconnected before pci_stop_and_remove_bus_device(dev)
causes Root Port teardown (such as pcie_aspm_exit_link_state(),
pcie_pme_remove(), aer_remove(), pci_msi_shutdown(), and
do_pci_disable_device()) to skip clearing hardware state in the Root Port's
local MMIO registers (host->pcie), leaving stale ASPM, PME, AER, MSI, and
PCI_COMMAND settings across subsequent re-enumerations.

> +
> +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;
> +	u32 val;
> +	int ret;
> +
> +	pci_lock_rescan_remove();
> +
> +	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> +	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
> +		if (!list_empty(&bus->devices)) {
> +			dev_info(host->dev,
> +				 "PCIe link down, removing devices\n");
> +			rzg3s_pcie_remove_devices(bus);
> +		}
> +		goto unlock;
> +	}
> +
> +	if (!list_empty(&bus->devices)) {
> +		bool replaced = rzg3s_pcie_device_replaced(host);
> +
> +		/*
> +		 * If bounced = false it means link rescanning (after probe
> +		 * or system resume), no link transition was recorded. The
> +		 * enumerated devices are intact, so remove them only if the
> +		 * connected device was replaced while link events were not
> +		 * delivered.
> +		 *
> +		 * If bounced = true it means at least one real link down/up
> +		 * cycle occurred.
> +		 */
> +		if (!bounced && !replaced)
> +			goto unlock;
> +
> +		rzg3s_pcie_remove_devices(bus);
> +	}
> +
> +	dev_info(host->dev, "PCIe link up, rescanning bus\n");
> +
> +	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);

[Severity: Medium]
When bounced is true and !list_empty(&bus->devices), does calling
rzg3s_pcie_device_replaced(host) issue Type 0 configuration reads to the
downstream endpoint before the link has finished settling?

When a real DL_UpDown event occurs (bounced == true), the result of
rzg3s_pcie_device_replaced(host) is not needed because the devices are
removed unconditionally, and rzg3s_pcie_set_max_link_speed() (which polls
for LTSSM L0 and negotiates link speed) and
pci_host_common_link_train_delay() have not been called yet. Should
rzg3s_pcie_device_replaced(host) only be called when !bounced?

[ ... ]

> @@ -1901,6 +2108,44 @@ 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. */
> +		if (host->started)
> +			writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +		disable_irq(host->event_irq);
> +		break;

[Severity: High]
Can calling disable_irq(host->event_irq) inside rzg3s_pcie_pm_notifier()
deadlock on pm_chain_head.rwsem if rzg3s_pcie_event_irq_thread() is
concurrently probing or removing a PCI device whose driver registers or
unregisters a PM notifier?

During system suspend:

suspend_prepare()
  pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE, PM_POST_SUSPEND)
    blocking_notifier_call_chain_robust(&pm_chain_head, ...)
      down_read(&pm_chain_head.rwsem)
      rzg3s_pcie_pm_notifier()
        disable_irq(host->event_irq)
          synchronize_irq(host->event_irq)

If rzg3s_pcie_event_irq_thread() is simultaneously running
rzg3s_pcie_link_event(), it calls pci_bus_add_devices() or
rzg3s_pcie_remove_devices() -> pci_stop_and_remove_bus_device(), which
synchronously runs PCI endpoint driver .probe() or .remove() callbacks.
Several PCI drivers (such as ath11k_pci, wil6210, amdgpu, and xe) call
register_pm_notifier() or unregister_pm_notifier() in those callbacks,
which blocks on down_write(&pm_chain_head.rwsem) while the suspend thread
holds down_read(&pm_chain_head.rwsem) waiting for the IRQ thread to finish.

> +
> +	case PM_POST_SUSPEND:
> +		/* Enable link up/down interrupts and force link re-scan. */
> +		if (host->started) {
> +			/*
> +			 * The link re-train in resume triggers the link up IRQ
> +			 * if there are connected devices. Clear the DL_UpDown
> +			 * status and trigger link_rescan to avoid
> +			 * re-enumerating the already connected devices.
> +			 */
> +			writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
> +				       host->axi + RZG3S_PCI_PEIS0);
> +			WRITE_ONCE(host->link_rescan, true);
> +			writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
> +				       host->axi + RZG3S_PCI_PEIE0);
> +		}
> +		enable_irq(host->event_irq);
> +		if (host->started)
> +			irq_wake_thread(host->event_irq, host);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}

[Severity: High]
rzg3s_pcie_pm_ops uses NOIRQ_SYSTEM_SLEEP_PM_OPS(rzg3s_pcie_suspend_noirq,
rzg3s_pcie_resume_noirq), which wires up the freeze_noirq, thaw_noirq,
poweroff_noirq, and restore_noirq callbacks for hibernation. However,
rzg3s_pcie_pm_notifier() only handles PM_SUSPEND_PREPARE and
PM_POST_SUSPEND.

During hibernation, won't omitting PM_HIBERNATION_PREPARE /
PM_RESTORE_PREPARE leave host->event_irq enabled during device freeze and
poweroff, and won't omitting PM_POST_HIBERNATION / PM_POST_RESTORE leave
RZG3S_PCI_PEIE0 cleared to 0 (after rzg3s_pcie_resume_noirq() ->
rzg3s_pcie_host_start() -> rzg3s_pcie_irq_init() resets it) and skip waking
the IRQ thread to rescan the bus?

>  
>  static int rzg3s_pcie_probe(struct platform_device *pdev)
>  {

[ ... ]

> @@ -1999,15 +2244,38 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto power_resets_assert;
>  
> +	host->started = true;
> +	host->pm_nb.notifier_call = rzg3s_pcie_pm_notifier;
> +	ret = register_pm_notifier(&host->pm_nb);
> +	if (ret)
> +		goto host_probe_teardown;
> +
>  	bridge->sysdata = host;
>  	bridge->ops = &rzg3s_pcie_root_ops;
>  	bridge->child_ops = &rzg3s_pcie_child_ops;
>  	ret = pci_host_probe(bridge);
>  	if (ret)
> -		goto host_probe_teardown;
> +		goto pm_notifier_unregister;
> +
> +	pci_lock_rescan_remove();
> +	rzg3s_pcie_cache_connected_dev_id(host);
> +	pci_unlock_rescan_remove();
> +
> +	/*
> +	 * The link re-train triggers the link up IRQ if there are connected
> +	 * devices. Clear DL_UpDown status and trigger link_rescan to avoid
> +	 * re-enumerating the devices detected while pci_host_probe() and loose
> +	 * any potential event.
> +	 */
> +	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
> +	WRITE_ONCE(host->link_rescan, true);
> +	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
> +	irq_wake_thread(host->event_irq, host);
>  
>  	return 0;

[Severity: High]
Because rzg3s_pcie_driver sets PROBE_PREFER_ASYNCHRONOUS, and system
suspend runs pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE,
PM_POST_SUSPEND) before dpm_prepare() calls wait_for_device_probe(), can
rzg3s_pcie_probe() race with rzg3s_pcie_pm_notifier()?

1. host->started is set to true and register_pm_notifier(&host->pm_nb) is
called before pci_host_probe(bridge) initializes bridge->bus. If
suspend_prepare() aborts before pci_register_host_bridge() sets
bridge->bus, PM_POST_SUSPEND wakes rzg3s_pcie_event_irq_thread() ->
rzg3s_pcie_link_event(), which dereferences bridge->bus while it is still
NULL.

2. If PM_SUSPEND_PREPARE runs while rzg3s_pcie_probe() is inside
pci_host_probe(), rzg3s_pcie_pm_notifier() clears RZG3S_PCI_PEIE0 and calls
disable_irq(host->event_irq), but rzg3s_pcie_probe() then finishes before
wait_for_device_probe() returns and unconditionally enables
RZG3S_PCI_PEIE0_DL_UPDOWN and calls irq_wake_thread(host->event_irq, host),
waking the IRQ thread during suspend.

3. If PM_SUSPEND_PREPARE runs before register_pm_notifier(&host->pm_nb) is
called in rzg3s_pcie_probe(), PM_POST_SUSPEND will later call
enable_irq(host->event_irq) without a matching disable_irq().

[Severity: Medium]
In addition, clearing RZG3S_PCI_PEIS0_DL_UPDOWN here at the end of
rzg3s_pcie_probe() (after pci_host_probe(bridge)) and in
rzg3s_pcie_pm_notifier() under PM_POST_SUSPEND (after all device resume
callbacks or after an aborted suspend), rather than immediately after link
training in rzg3s_pcie_host_setup(), seems able to lose link bounce events:

If the same endpoint experiences a link down/up bounce during
pci_host_probe(), between rzg3s_pcie_resume_noirq() and PM_POST_SUSPEND, or
during an aborted suspend, clearing RZG3S_PCI_PEIS0_DL_UPDOWN discards the
hardware event so bounced is false. Because the same device is still
attached, rzg3s_pcie_device_replaced() returns false and
rzg3s_pcie_link_event() exits at:

	if (!bounced && !replaced)
		goto unlock;

leaving the endpoint with unconfigured BARs, PCI_COMMAND, and MSI state (or
at Gen1 speed if link-up raced with rzg3s_pcie_wait_for_link_up() timing
out in rzg3s_pcie_host_setup()). Should RZG3S_PCI_PEIS0_DL_UPDOWN be
cleared in rzg3s_pcie_host_setup() right after link training instead?

[ ... ]

> @@ -2041,8 +2309,25 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
>  static int rzg3s_pcie_resume_noirq(struct device *dev)
>  {
>  	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
> +	int ret;
>  
> -	return rzg3s_pcie_host_start(host);
> +	ret = rzg3s_pcie_host_start(host);
> +	if (ret)
> +		return ret;
> +
> +	if (rzg3s_pcie_device_replaced(host)) {
> +		struct pci_host_bridge *bridge =
> +			pci_host_bridge_from_priv(host);
> +		struct pci_dev *port __free(pci_dev_put) =
> +			pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
> +
> +		if (port && port->subordinate) {
> +			pci_walk_bus(port->subordinate,
> +				     pci_dev_set_disconnected, NULL);
> +		}
> +	}
> +
> +	return 0;
>  }

[Severity: Medium]
If rzg3s_pcie_host_start(host) fails in rzg3s_pcie_resume_noirq(), its
error path asserts host->power_resets and clears
RZG3S_SYSC_FUNC_ID_RST_RSM_B, leaving host->started as false.

Although rzg3s_pcie_pm_notifier() checks host->started to avoid touching
controller registers after a failed resume, the PM core still runs
pci_pm_resume_noirq() and pci_pm_resume() on the child PCI devices. Since
rzg3s_pcie_resume_noirq() returns early before marking the devices
disconnected, and neither rzg3s_pcie_root_ops nor rzg3s_pcie_child_ops
checks host->started, won't config-space accesses during child device
resume (and later runtime accesses to the remaining PCI devices) still read
and write host->pcie and host->axi while the controller is held in reset?

Should the devices also be marked disconnected (or config accesses guarded
by host->started) when rzg3s_pcie_host_start() fails?

[ ... ]

完美！现在检查所有规则：
1. Plain text only, no markdown headers, no backticks, no ALL CAPS shouting? Checked.
2. Commit header format: `commit 1730fab066afd00592ed2f6ea77d04307ed48cf0`, `Author: John Madieu <john.madieu.xa@bp.renesas.com>`, one line subject, brief summary (2 sentences), unified diff quoted with `> `. Checked.
3. No line numbers mentioned in comments? Checked!
4. Wrapped at 78 characters for added text? Checked.
5. `[Severity: <level>]` on its own line right above each inline comment block? Checked (VF1: High, VF4: Medium, PVF3: High, VF2: High, PVF1: High, VF3: Medium, PVF2: Medium - all 7 findings included).
6. Ends with a blank line? Yes, I will make sure the output ends with a blank line and contains only the report. Let's output the final text now.commit 1730fab066afd00592ed2f6ea77d04307ed48cf0
Author: John Madieu <john.madieu.xa@bp.renesas.com>

PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

This commit enables the DL_UpDown interrupt on RZ/G3S and related PCIe
controllers to detect link-state transitions and hotplug-like events,
removing or rescanning devices on the bus accordingly. It also adds a PM
notifier to disable link event interrupts across system suspend and force a
bus rescan on resume, and makes probe tolerant of an initially down link.

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

[ ... ]

> @@ -1094,12 +1108,187 @@ static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)

[ ... ]

> +static void rzg3s_pcie_remove_devices(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev, *tmp;
> +
> +	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
> +		pci_dev_set_disconnected(dev, NULL);
> +		if (pci_has_subordinate(dev)) {
> +			pci_walk_bus(dev->subordinate, pci_dev_set_disconnected,
> +				     NULL);
> +		}
> +		pci_stop_and_remove_bus_device(dev);
> +	}
> +}

[Severity: High]
Both callers in rzg3s_pcie_link_event() pass bridge->bus (the root bus) to
rzg3s_pcie_remove_devices(), where dev in &bus->devices is the controller's
own integrated Root Port (0000:00:00.0) rather than an endpoint across the
link.

Should pci_dev_set_disconnected() only be called on dev->subordinate (as
done in rzg3s_pcie_resume_noirq()) rather than on dev itself?

Because pci_read_config_*() and pci_write_config_*() immediately abort with
PCIBIOS_DEVICE_NOT_FOUND when pci_dev_is_disconnected(dev) is true, marking
the Root Port as disconnected before pci_stop_and_remove_bus_device(dev)
causes Root Port teardown (such as pcie_aspm_exit_link_state(),
pcie_pme_remove(), aer_remove(), pci_msi_shutdown(), and
do_pci_disable_device()) to skip clearing hardware state in the Root Port's
local MMIO registers (host->pcie), leaving stale ASPM, PME, AER, MSI, and
PCI_COMMAND settings across subsequent re-enumerations.

> +
> +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;
> +	u32 val;
> +	int ret;
> +
> +	pci_lock_rescan_remove();
> +
> +	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> +	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
> +		if (!list_empty(&bus->devices)) {
> +			dev_info(host->dev,
> +				 "PCIe link down, removing devices\n");
> +			rzg3s_pcie_remove_devices(bus);
> +		}
> +		goto unlock;
> +	}
> +
> +	if (!list_empty(&bus->devices)) {
> +		bool replaced = rzg3s_pcie_device_replaced(host);
> +
> +		/*
> +		 * If bounced = false it means link rescanning (after probe
> +		 * or system resume), no link transition was recorded. The
> +		 * enumerated devices are intact, so remove them only if the
> +		 * connected device was replaced while link events were not
> +		 * delivered.
> +		 *
> +		 * If bounced = true it means at least one real link down/up
> +		 * cycle occurred.
> +		 */
> +		if (!bounced && !replaced)
> +			goto unlock;
> +
> +		rzg3s_pcie_remove_devices(bus);
> +	}
> +
> +	dev_info(host->dev, "PCIe link up, rescanning bus\n");
> +
> +	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);

[Severity: Medium]
When bounced is true and !list_empty(&bus->devices), does calling
rzg3s_pcie_device_replaced(host) issue Type 0 configuration reads to the
downstream endpoint before the link has finished settling?

When a real DL_UpDown event occurs (bounced == true), the result of
rzg3s_pcie_device_replaced(host) is not needed because the devices are
removed unconditionally, and rzg3s_pcie_set_max_link_speed() (which polls
for LTSSM L0 and negotiates link speed) and
pci_host_common_link_train_delay() have not been called yet. Should
rzg3s_pcie_device_replaced(host) only be called when !bounced?

[ ... ]

> @@ -1901,6 +2108,44 @@ 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. */
> +		if (host->started)
> +			writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +		disable_irq(host->event_irq);
> +		break;

[Severity: High]
Can calling disable_irq(host->event_irq) inside rzg3s_pcie_pm_notifier()
deadlock on pm_chain_head.rwsem if rzg3s_pcie_event_irq_thread() is
concurrently probing or removing a PCI device whose driver registers or
unregisters a PM notifier?

During system suspend:

suspend_prepare()
  pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE, PM_POST_SUSPEND)
    blocking_notifier_call_chain_robust(&pm_chain_head, ...)
      down_read(&pm_chain_head.rwsem)
      rzg3s_pcie_pm_notifier()
        disable_irq(host->event_irq)
          synchronize_irq(host->event_irq)

If rzg3s_pcie_event_irq_thread() is simultaneously running
rzg3s_pcie_link_event(), it calls pci_bus_add_devices() or
rzg3s_pcie_remove_devices() -> pci_stop_and_remove_bus_device(), which
synchronously runs PCI endpoint driver .probe() or .remove() callbacks.
Several PCI drivers (such as ath11k_pci, wil6210, amdgpu, and xe) call
register_pm_notifier() or unregister_pm_notifier() in those callbacks,
which blocks on down_write(&pm_chain_head.rwsem) while the suspend thread
holds down_read(&pm_chain_head.rwsem) waiting for the IRQ thread to finish.

> +
> +	case PM_POST_SUSPEND:
> +		/* Enable link up/down interrupts and force link re-scan. */
> +		if (host->started) {
> +			/*
> +			 * The link re-train in resume triggers the link up IRQ
> +			 * if there are connected devices. Clear the DL_UpDown
> +			 * status and trigger link_rescan to avoid
> +			 * re-enumerating the already connected devices.
> +			 */
> +			writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
> +				       host->axi + RZG3S_PCI_PEIS0);
> +			WRITE_ONCE(host->link_rescan, true);
> +			writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
> +				       host->axi + RZG3S_PCI_PEIE0);
> +		}
> +		enable_irq(host->event_irq);
> +		if (host->started)
> +			irq_wake_thread(host->event_irq, host);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}

[Severity: High]
rzg3s_pcie_pm_ops uses NOIRQ_SYSTEM_SLEEP_PM_OPS(rzg3s_pcie_suspend_noirq,
rzg3s_pcie_resume_noirq), which wires up the freeze_noirq, thaw_noirq,
poweroff_noirq, and restore_noirq callbacks for hibernation. However,
rzg3s_pcie_pm_notifier() only handles PM_SUSPEND_PREPARE and
PM_POST_SUSPEND.

During hibernation, won't omitting PM_HIBERNATION_PREPARE /
PM_RESTORE_PREPARE leave host->event_irq enabled during device freeze and
poweroff, and won't omitting PM_POST_HIBERNATION / PM_POST_RESTORE leave
RZG3S_PCI_PEIE0 cleared to 0 (after rzg3s_pcie_resume_noirq() ->
rzg3s_pcie_host_start() -> rzg3s_pcie_irq_init() resets it) and skip waking
the IRQ thread to rescan the bus?

>  
>  static int rzg3s_pcie_probe(struct platform_device *pdev)
>  {

[ ... ]

> @@ -1999,15 +2244,38 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto power_resets_assert;
>  
> +	host->started = true;
> +	host->pm_nb.notifier_call = rzg3s_pcie_pm_notifier;
> +	ret = register_pm_notifier(&host->pm_nb);
> +	if (ret)
> +		goto host_probe_teardown;
> +
>  	bridge->sysdata = host;
>  	bridge->ops = &rzg3s_pcie_root_ops;
>  	bridge->child_ops = &rzg3s_pcie_child_ops;
>  	ret = pci_host_probe(bridge);
>  	if (ret)
> -		goto host_probe_teardown;
> +		goto pm_notifier_unregister;
> +
> +	pci_lock_rescan_remove();
> +	rzg3s_pcie_cache_connected_dev_id(host);
> +	pci_unlock_rescan_remove();
> +
> +	/*
> +	 * The link re-train triggers the link up IRQ if there are connected
> +	 * devices. Clear DL_UpDown status and trigger link_rescan to avoid
> +	 * re-enumerating the devices detected while pci_host_probe() and loose
> +	 * any potential event.
> +	 */
> +	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
> +	WRITE_ONCE(host->link_rescan, true);
> +	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
> +	irq_wake_thread(host->event_irq, host);
>  
>  	return 0;

[Severity: High]
Because rzg3s_pcie_driver sets PROBE_PREFER_ASYNCHRONOUS, and system
suspend runs pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE,
PM_POST_SUSPEND) before dpm_prepare() calls wait_for_device_probe(), can
rzg3s_pcie_probe() race with rzg3s_pcie_pm_notifier()?

1. host->started is set to true and register_pm_notifier(&host->pm_nb) is
called before pci_host_probe(bridge) initializes bridge->bus. If
suspend_prepare() aborts before pci_register_host_bridge() sets
bridge->bus, PM_POST_SUSPEND wakes rzg3s_pcie_event_irq_thread() ->
rzg3s_pcie_link_event(), which dereferences bridge->bus while it is still
NULL.

2. If PM_SUSPEND_PREPARE runs while rzg3s_pcie_probe() is inside
pci_host_probe(), rzg3s_pcie_pm_notifier() clears RZG3S_PCI_PEIE0 and calls
disable_irq(host->event_irq), but rzg3s_pcie_probe() then finishes before
wait_for_device_probe() returns and unconditionally enables
RZG3S_PCI_PEIE0_DL_UPDOWN and calls irq_wake_thread(host->event_irq, host),
waking the IRQ thread during suspend.

3. If PM_SUSPEND_PREPARE runs before register_pm_notifier(&host->pm_nb) is
called in rzg3s_pcie_probe(), PM_POST_SUSPEND will later call
enable_irq(host->event_irq) without a matching disable_irq().

[Severity: Medium]
In addition, clearing RZG3S_PCI_PEIS0_DL_UPDOWN here at the end of
rzg3s_pcie_probe() (after pci_host_probe(bridge)) and in
rzg3s_pcie_pm_notifier() under PM_POST_SUSPEND (after all device resume
callbacks or after an aborted suspend), rather than immediately after link
training in rzg3s_pcie_host_setup(), seems able to lose link bounce events:

If the same endpoint experiences a link down/up bounce during
pci_host_probe(), between rzg3s_pcie_resume_noirq() and PM_POST_SUSPEND, or
during an aborted suspend, clearing RZG3S_PCI_PEIS0_DL_UPDOWN discards the
hardware event so bounced is false. Because the same device is still
attached, rzg3s_pcie_device_replaced() returns false and
rzg3s_pcie_link_event() exits at:

	if (!bounced && !replaced)
		goto unlock;

leaving the endpoint with unconfigured BARs, PCI_COMMAND, and MSI state (or
at Gen1 speed if link-up raced with rzg3s_pcie_wait_for_link_up() timing
out in rzg3s_pcie_host_setup()). Should RZG3S_PCI_PEIS0_DL_UPDOWN be
cleared in rzg3s_pcie_host_setup() right after link training instead?

[ ... ]

> @@ -2041,8 +2309,25 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
>  static int rzg3s_pcie_resume_noirq(struct device *dev)
>  {
>  	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
> +	int ret;
>  
> -	return rzg3s_pcie_host_start(host);
> +	ret = rzg3s_pcie_host_start(host);
> +	if (ret)
> +		return ret;
> +
> +	if (rzg3s_pcie_device_replaced(host)) {
> +		struct pci_host_bridge *bridge =
> +			pci_host_bridge_from_priv(host);
> +		struct pci_dev *port __free(pci_dev_put) =
> +			pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
> +
> +		if (port && port->subordinate) {
> +			pci_walk_bus(port->subordinate,
> +				     pci_dev_set_disconnected, NULL);
> +		}
> +	}
> +
> +	return 0;
>  }

[Severity: Medium]
If rzg3s_pcie_host_start(host) fails in rzg3s_pcie_resume_noirq(), its
error path asserts host->power_resets and clears
RZG3S_SYSC_FUNC_ID_RST_RSM_B, leaving host->started as false.

Although rzg3s_pcie_pm_notifier() checks host->started to avoid touching
controller registers after a failed resume, the PM core still runs
pci_pm_resume_noirq() and pci_pm_resume() on the child PCI devices. Since
rzg3s_pcie_resume_noirq() returns early before marking the devices
disconnected, and neither rzg3s_pcie_root_ops nor rzg3s_pcie_child_ops
checks host->started, won't config-space accesses during child device
resume (and later runtime accesses to the remaining PCI devices) still read
and write host->pcie and host->axi while the controller is held in reset?

Should the devices also be marked disconnected (or config accesses guarded
by host->started) when rzg3s_pcie_host_start() fails?

[ ... ]

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