Re: [PATCH net-next v10 5/6] net: stmmac: dwmac-rk: add SGMII support for RK3568

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: stmmac: memory leak and SGMII breakage due to xpcs overwrite
--

commit 9f6d3f852317e81db5e938c043afc40343ece6e9
Author: Coia Prant <coiaprant@gmail.com>

net: stmmac: dwmac-rk: add SGMII support for RK3568

This commit adds SGMII support for the RK3568 SoC to the dwmac-rk driver.
It configures the necessary glue logic, sets up the MAC for in-band
negotiation, and provides callbacks to initialize and manage an XPCS via
the Rockchip platform driver.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 72bdbcb5e863a..3b50ad3c600b9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

[ ... ]

> @@ -1211,6 +1239,43 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
> +static int rk_pcs_init(struct stmmac_priv *priv)
> +{
> +	struct device_node *np = priv->device->of_node;
> +	struct device_node *pcs_node;
> +	struct dw_xpcs *xpcs;
> +
> +	pcs_node = of_parse_phandle(np, "pcs-handle", 0);
> +	if (!pcs_node)
> +		return -ENODEV;
> +
> +	xpcs = xpcs_rk_create(priv->device, pcs_node);
> +	of_node_put(pcs_node);
> +	if (IS_ERR(xpcs))
> +		return PTR_ERR(xpcs);
> +
> +	priv->hw->xpcs = xpcs;

[Severity: High]
Does assigning priv->hw->xpcs here leak the allocated memory and break
SGMII initialization?

Looking at stmmac_pcs_setup() in
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c, the local xpcs variable
initializes to NULL and remains NULL when priv->plat->pcs_init() is called:

stmmac_mdio.c:stmmac_pcs_setup() {
    struct dw_xpcs *xpcs = NULL;
    ...
    if (priv->plat->pcs_init) {
        ret = priv->plat->pcs_init(priv);
    } else if (fwnode_property_present(devnode, "pcs-handle")) {
        ...
    }
    ...
    priv->hw->xpcs = xpcs;
}

Because rk_pcs_init() successfully allocates an XPCS and assigns it to
priv->hw->xpcs, the caller will immediately overwrite priv->hw->xpcs with
its local NULL variable. This drops the reference to the newly allocated
dw_xpcs structure and leaves the device without a valid XPCS, which breaks
rk_select_pcs() and the SGMII configuration.

Should rk_pcs_init() be updated to return the xpcs pointer, or does the
core stmmac_pcs_setup() logic need to be modified to avoid overwriting the
platform-allocated xpcs pointer?

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005223011.1124347-1-coiaprant@gmail.com?part=5