Re: [PATCH v5 09/17] PCI: rcar-gen4: Add Root Port reset support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: rcar-gen4: missing ltssm_control() unwind in rcar_gen4_pcie_start_link()
--

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

PCI: rcar-gen4: Add Root Port reset support

This commit implements the host bridge reset_root_port() callback to
reset and reinitialize the R-Car PCIe controller without releasing clocks
or the R-Car Gen5 SoC PHY. It also synchronizes APP interrupt enables and
tracks the R-Car Gen5 PHY power state across resets.

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index cbd22e6bb44b9..420392ace349f 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c

[ ... ]

> @@ -661,14 +703,87 @@ static int rcar_gen4_pcie_host_setup(struct dw_pcie_rp *pp)
>  	return 0;
>  }
>  
> +static void rcar_gen4_pcie_quiesce_irqs(struct rcar_gen4_pcie *rcar)
> +{
> +	scoped_guard(raw_spinlock_irqsave, &rcar->app_lock) {
> +		rcar->reinit_pending = true;
> +		rcar_gen4_pcie_app_irq_sync_locked(rcar);
> +	}
> +}
> +
> +static void rcar_gen4_pcie_resume_irqs(struct rcar_gen4_pcie *rcar,
> +				       bool recovered)
> +{
> +	scoped_guard(raw_spinlock_irqsave, &rcar->app_lock) {
> +		rcar->reinit_pending = !recovered;
> +		rcar_gen4_pcie_app_irq_sync_locked(rcar);
> +	}
> +}
> +
> +/*
> + * R-Car Gen4 SoC PCIe controllers and R-Car Gen5 SoC PCIe4 controller have
> + * a single Root Port per instance, so the 'pci_dev' is ignored and the whole
> + * controller is reset. The PCI core serializes the callers through its
> + * device locks.
> + */
> +static int rcar_gen4_pcie_reset_root_port(struct pci_host_bridge *bridge,
> +					  struct pci_dev *pdev)
> +{
> +	struct rcar_gen4_pcie *rcar = dev_get_drvdata(bridge->dev.parent);
> +	struct dw_pcie *dw = &rcar->dw;
> +	struct dw_pcie_rp *pp = &dw->pp;
> +	struct device *dev = dw->dev;
> +	int ret;
> +
> +	/* A failed resume may have left the controller powered down. */
> +	if (dw->suspended)
> +		return -EBUSY;
> +
> +	rcar_gen4_pcie_quiesce_irqs(rcar);
> +
> +	dw_pcie_stop_link(dw);
> +
> +	rcar_gen4_pcie_host_perst_assert(pp, true);
> +
> +	ret = rcar_gen4_pcie_common_reinit(rcar);
> +	if (ret)
> +		goto out;
> +
> +	ret = rcar_gen4_pcie_host_setup(pp);
> +	if (ret)
> +		goto out;
> +
> +	ret = dw_pcie_setup_rc(pp);
> +	if (ret)
> +		goto out;
> +
> +	ret = dw_pcie_start_link(dw);
> +	if (ret)
> +		goto out;
> +
> +	/* Ignore errors, the link may come up later */
> +	dw_pcie_wait_for_link(dw);
> +	dev_dbg(dev, "Root Port reset completed\n");
> +
> +out:
> +	rcar_gen4_pcie_resume_irqs(rcar, !ret);
> +	if (ret)
> +		dev_err(dev, "Root Port reset failed (%d); interrupts stay masked until a reset succeeds\n",
> +			ret);
> +
> +	return ret;
> +}

[ ... ]

> @@ -944,11 +1061,17 @@ static int rcar_gen5_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable
>  	}
>  	writel(val, rcar->base + PCIERSTCTRL1);
>  
> +	/* A reset retry may find the PHY already powered off. */
> +	if (enable == rcar->phy_powered)
> +		return 0;
> +
>  	if (enable)
>  		phy_power_on(rcar->phy);
>  	else
>  		phy_power_off(rcar->phy);
>  
> +	rcar->phy_powered = enable;
> +
>  	return 0;
>  }

[Severity: Medium]
On R-Car Gen5, dw_pcie_start_link() calls rcar_gen4_pcie_start_link(), which
enables LTSSM and powers on the PHY before running speed_control():

drivers/pci/controller/dwc/pcie-rcar-gen4.c:rcar_gen4_pcie_start_link() {
	...
	if (rcar->drvdata->ltssm_control) {
		ret = rcar->drvdata->ltssm_control(rcar, true);
		if (ret)
			return ret;
	}

	return rcar->drvdata->speed_control(rcar);
}

If rcar_gen5_pcie_speed_control() times out and returns -ETIMEDOUT,
rcar_gen5_pcie_ltssm_control(rcar, true) has already enabled LTSSM, called
phy_power_on(rcar->phy), and set rcar->phy_powered = true.

In rcar_gen4_pcie_reset_root_port(), this failure jumps to out: and leaves
rcar->reinit_pending set to true (keeping APP interrupts masked) while
PERST# is deasserted, LTSSM remains enabled, and the PHY remains powered
on.

Similarly, if dw_pcie_start_link() fails during dw_pcie_resume_noirq() or
dw_pcie_host_init(), those callers bypass their dw_pcie_stop_link() error
label and call rcar_gen5_pcie_deinit() -> phy_exit(rcar->phy) while the PHY
is still powered on and rcar->phy_powered is still true. On a subsequent
resume, rcar_gen5_pcie_ltssm_control(rcar, true) will hit the (enable ==
rcar->phy_powered) check and skip phy_power_on().

Should rcar_gen4_pcie_start_link() unwind with
rcar->drvdata->ltssm_control(rcar, false) when speed_control() fails?

[ ... ]

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