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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] pci: dwc: rcar-gen4: unhandled reset deassertion in rcar_gen5_pcie_init() error path
- [Medium] pci: dwc: rcar-gen4: ignored return value of phy_power_on() in rcar_gen5_pcie_ltssm_control()
- [Medium] pci: dwc: rcar-gen4: NULL pointer dereference with malformed Gen4 DT
- [Low] pci: dwc: rcar-gen4: error masking in rcar_gen4_pcie_get_resources()
--

commit d7ef6825f8ed7b884f9b2940d56a8d85a5c57f6b
Author: Marek Vasut <marek.vasut+renesas@mailbox.org>
PCI: dwc: rcar-gen4: Add support for R-Car X5H PCIe4

This commit adds support for the R8A78000 (R-Car X5H) PCIe4 controller. It
factors out the controller-specific initialization sequences into callbacks
and adds PHY driver bindings that are handled via the resource retrieval
routine.

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index cf1f5de451820..0b7ae4a693085 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
[ ... ]
> @@ -285,6 +314,49 @@ static int rcar_gen4_v4h_v4m_pcie_init(struct rcar_gen4_pcie *rcar)
>  	return 0;
>  }
>  
> +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)
> +		goto err_unprepare;
[ ... ]
> +	return 0;
> +
> +err_unprepare:
> +	clk_bulk_disable_unprepare(DW_PCIE_NUM_CORE_CLKS, dw->core_clks);
> +
> +	return ret;
> +}

[Severity: Medium]
Will this leave the hardware reset unbalanced? rcar_gen4_pcie_common_init()
successfully deasserts the DW_PCIE_PWR_RST core reset. If the subsequent
phy_init() call fails, the code jumps to err_unprepare and disables the
clocks, but does not appear to reverse the reset deassertion by calling
reset_control_assert() or rcar_gen4_pcie_common_deinit(). Could this leave
the PCIe core in an undefined state?

[ ... ]
> @@ -322,8 +400,12 @@ 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_base = NULL;
> +		rcar->phy = devm_phy_get(dev, NULL);
> +		if (IS_ERR(rcar->phy))
> +			return PTR_ERR(rcar->phy);
> +	}

[Severity: Low]
Does this logic unintentionally mask original errors like -ENOMEM? If
devm_platform_ioremap_resource_byname() fails due to a memory allocation
issue, it will return -ENOMEM. The code falls back to devm_phy_get(), and if
that returns -ENODEV, the original -ENOMEM is lost. Should the fallback only
be attempted if the resource was explicitly missing (returning -EINVAL)?

[Severity: Medium]
Additionally, by setting rcar->phy_base to NULL and succeeding if
devm_phy_get() finds a PHY, could this cause a NULL pointer dereference on
older Gen4 controllers? 

If a malformed device tree provides a phys property but no phy memory region
for a Gen4 controller, rcar->phy_base is set to NULL. The driver uses
rcar_gen4_pcie_ltssm_control() for Gen4, which unconditionally dereferences
rcar->phy_base via rcar_gen4_pcie_phy_reg_update_bits() and
readl_poll_timeout().

[ ... ]
> @@ -804,6 +886,28 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
>  	return 0;
>  }
>  
> +static int rcar_gen5_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +{
> +	u32 val;
[ ... ]
> +	if (enable)
> +		phy_power_on(rcar->phy);
> +	else
> +		phy_power_off(rcar->phy);
> +
> +	return 0;
> +}

[Severity: Medium]
Should the return value of phy_power_on() be checked here? If powering on the
PHY fails, the function unconditionally returns 0 and swallows the error code.
Could this falsely report success to the caller and lead to silent link
training failures or cryptic timeouts later?

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