Re: [PATCH 07/11] PCI: rcar-gen4: Recover the Root Port on link down

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: use-after-free traversing bus devices without pci_bus_sem
- [Low] cleanup: mixing goto-based error handling with guard()
--

commit 945529b2e76a7eb8d868ae35745dd98c440ca944
Author: Koichiro Den <den@valinux.co.jp>

PCI: rcar-gen4: Recover the Root Port on link down

This commit introduces a mechanism to recover the Root Port on R-Car Gen4
when the link goes down. It uses a pre-MSI callback to handle the reset
status and schedules recovery work.

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index 5b983ef6dc47d..238755852045e 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
[ ... ]
> @@ -435,6 +486,134 @@ static int rcar_gen4_pcie_host_msi_init(struct dw_pcie_rp *pp)
>  	return ret;
>  }
>  
> +/* Returns true if a latched reset-request status was consumed. */
> +static bool rcar_gen4_pcie_handle_link_down(struct rcar_gen4_pcie *rcar)
> +{
[ ... ]
> +	if (schedule)
> +		schedule_work(&rcar->link_down_work);
> +
> +	return true;
> +}
> +
> +static void rcar_gen4_pcie_link_down_work(struct work_struct *work)
> +{
> +	struct rcar_gen4_pcie *rcar =
> +		container_of(work, struct rcar_gen4_pcie, link_down_work);
> +	struct dw_pcie_rp *pp = &rcar->dw.pp;
> +	struct pci_dev *port;
> +
> +	for_each_pci_bridge(port, pp->bridge->bus) {
> +		if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> +			pci_host_handle_link_down(port);
> +	}

[Severity: High]
Can this loop traverse the PCI bus devices list asynchronously without
holding the mandatory pci_bus_sem or acquiring a reference on port?

Iterating the devices list of the PCI bus without holding the required
read lock (pci_bus_sem) can lead to list corruption or Use-After-Free.
Concurrently, a PCI hotplug event or a sysfs device unbind could remove
the device from the list. Furthermore, the downstream call
pci_host_handle_link_down(port) uses the port pointer across
sleep/lock-acquisition points without holding a reference
(via pci_dev_get()).

Could this be protected by holding pci_bus_sem or acquiring a reference
to the device?

> +
> +	if (test_bit(RCAR_PCIE_LINK_DOWN, &rcar->state))
> +		dev_err(rcar->dw.dev,
> +			"Root Port reset failed; keeping the controller's interrupts masked until a reset succeeds\n");
> +}
[ ... ]
> @@ -574,7 +762,7 @@ static int rcar_gen4_pcie_reset_root_port(struct pci_host_bridge *bridge,
>  	dev_dbg(dev, "Root Port reset completed\n");
>  
>  out:
> -	rcar_gen4_pcie_resume_irqs(rcar);
> +	rcar_gen4_pcie_resume_irqs(rcar, !ret);
>  
>  	return ret;
>  }

[Severity: Low]
Does rcar_gen4_pcie_reset_root_port() mix guard() and goto error handling?

Looking at rcar_gen4_pcie_reset_root_port(), it defines a guard at the top
and uses goto for error handling:

	guard(mutex)(&rcar->reset_lock);

	rcar_gen4_pcie_quiesce_irqs(rcar);

	dw_pcie_stop_link(dw);

	ret = rcar_gen4_pcie_host_hw_init(pp);
	if (ret) {
		dev_err(dev, "Host init failed: %d\n", ret);
		goto out;
	}

The cleanup subsystem guidelines explicitly state that goto and guard()
should never be mixed in the same function, as it introduces potentially
confusing ownership semantics and ordering issues.

Could this be converted to avoid the goto jumps?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918032038.2216471-1-den@valinux.co.jp?part=7