Re: [PATCH v2 5/5] PCI: dwc: rcar-gen4: Add support for R-Car X5H PCIe4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing `phy_exit()` call on error paths and driver teardown causes a leak of PHY resources.
- [High] Missing cleanup of core clocks and reset states when `phy_init()` fails in `rcar_gen5_pcie_init()`.
- [High] Incorrect initialization sequence in `rcar_gen5_pcie_ltssm_control()` and ignored error check for `phy_power_on()`.
- [High] A 32-bit read-modify-write of `PCI_EXP_LNKCTL` inadvertently clears Write-1-to-Clear (W1C) bits in the adjacent `PCI_EXP_LNKSTA` register.
- [High] Dereference of `ERR_PTR` in `rcar->phy_base` on Gen4 code paths if a malformed Device Tree is provided.
--

--- Patch [5]: [PATCH v2 5/5] PCI: dwc: rcar-gen4: Add support for R-Car X5H PCIe4 ---
commit cab5ad82173a65768d51a17315ea45161e805bdf
Author: Marek Vasut <marek.vasut+renesas@mailbox.org>

PCI: dwc: rcar-gen4: Add support for R-Car X5H PCIe4

Add support for R8A78000 (R-Car X5H) PCIe4. The controller initialization
sequence is factored out into controller-specific callbacks. Endpoint mode
is currently not implemented for R-Car Gen5 PCIe4.

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index f88168f2df195..794c42c53380d 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c

[ ... ]

> @@ -169,6 +177,35 @@ static int rcar_gen4_pcie_speed_control(struct rcar_gen4_pcie *rcar)
> +static int rcar_gen5_pcie_speed_control(struct rcar_gen4_pcie *rcar)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	u32 lnkcap = dw_pcie_readl_dbi(dw, EXPCAP(PCI_EXP_LNKCAP));
> +	u32 lnksta = dw_pcie_readw_dbi(dw, EXPCAP(PCI_EXP_LNKSTA));
> +	u32 val, retries;
> +
> +	if ((lnksta & PCI_EXP_LNKSTA_CLS) == (lnkcap & PCI_EXP_LNKCAP_SLS))
> +		return 0;
> +
> +	/* Retrain link */
> +	val = dw_pcie_readl_dbi(dw, EXPCAP(PCI_EXP_LNKCTL));
> +	val |= PCI_EXP_LNKCTL_RL;
> +	dw_pcie_writel_dbi(dw, EXPCAP(PCI_EXP_LNKCTL), val);

[Severity: High]
Will this 32-bit read-modify-write access unintentionally clear Write-1-to-Clear
(W1C) bits in the adjacent PCI_EXP_LNKSTA register?

Since PCI_EXP_LNKCTL (offset 0x10) and PCI_EXP_LNKSTA (offset 0x12) share the
same 32-bit Dword, any W1C bits set in the Link Status register will be read as
1 and written back as 1, clearing them. Could a 16-bit accessor be used instead?

> +
> +	/* Wait for link retrain */

[ ... ]

> @@ -284,6 +321,44 @@ static int rcar_gen4_v4h_v4m_pcie_init(struct rcar_gen4_pcie *rcar)
> +static int rcar_gen5_pcie_init(struct rcar_gen4_pcie *rcar)
> +{
> +	struct dw_pcie *dw = &rcar->dw;
> +	int ret;
> +	u32 val;
> +
> +	/* R-Car Gen4 and Gen5 common initialization. */
> +	ret = rcar_gen4_pcie_common_init(rcar);
> +	if (ret)
> +		return ret;
> +
> +	/* R-Car Gen5 specific additional initialization. */
> +	ret = phy_init(rcar->phy);
> +	if (ret)
> +		return ret;

[Severity: High]
If this fails, do we need to clean up the resources just initialized by
rcar_gen4_pcie_common_init()? It looks like returning the error directly leaves
core clocks enabled and resets unhandled.

[Severity: High]
Since phy_init() is called on the successful initialization path, shouldn't
there be a matching phy_exit() in the deinitialization paths (such as
rcar_gen4_pcie_common_deinit) to prevent leaking PHY resources when the device
is unbound?

> +
> +	dw_pcie_dbi_ro_wr_en(dw);

[ ... ]

> @@ -321,8 +396,11 @@ static int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar)
>  	struct device_node *root_port;
>  
>  	rcar->phy_base = devm_platform_ioremap_resource_byname(rcar->pdev, "phy");
> -	if (IS_ERR(rcar->phy_base))
> -		return PTR_ERR(rcar->phy_base);
> +	if (IS_ERR(rcar->phy_base)) {
> +		rcar->phy = devm_phy_get(dev, NULL);
> +		if (IS_ERR(rcar->phy))
> +			return PTR_ERR(rcar->phy);
> +	}

[Severity: High]
If the device tree lacks the phy memory region but provides a phys node,
the fallback devm_phy_get() succeeds, leaving rcar->phy_base containing an
error pointer.

Later on, if Gen4 code paths like rcar_gen4_pcie_ltssm_control() execute,
they unconditionally call rcar_gen4_pcie_phy_reg_update_bits():

rcar_gen4_pcie_phy_reg_update_bits()
	tmp = readl(rcar->phy_base + offset);

Can this dereference the error pointer and crash? Should rcar->phy_base be set
to NULL after falling back to devm_phy_get()?

>  
>  	root_port = of_get_next_available_child(dev->of_node, NULL);

[ ... ]

> @@ -743,6 +821,28 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +static int rcar_gen5_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +{
> +	u32 val;
> +
> +	val = readl(rcar->base + PCIERSTCTRL1);
> +	if (enable) {
> +		val |= APP_LTSSM_ENABLE;
> +		val &= ~APP_HOLD_PHY_RST;
> +	} else {
> +		val &= ~APP_LTSSM_ENABLE;
> +		val |= APP_HOLD_PHY_RST;
> +	}
> +	writel(val, rcar->base + PCIERSTCTRL1);
> +
> +	if (enable)
> +		phy_power_on(rcar->phy);
> +	else
> +		phy_power_off(rcar->phy);
> +
> +	return 0;
> +}

[Severity: High]
Is it expected to enable the LTSSM and drop the PHY reset before calling
phy_power_on()?  Normally, wouldn't the PHY need to be powered and stabilized
before activating the MAC's LTSSM?

Also, if phy_power_on() fails, shouldn't we handle the error instead of
unconditionally returning 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720020505.315335-1-marek.vasut+renesas@mailbox.org?part=5