Re: [PATCH net-next v11 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: dw_xpcs memory leak and overwrite in stmmac_pcs_setup
--

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

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

The RK3568 SoC integrates a Synopsys DesignWare XPCS that can be connected
to GMAC0 or GMAC1 in SGMII mode. This commit adds the necessary glue logic
to support this configuration in the dwmac-rk driver.

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

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
[ ... ]
> @@ -1211,6 +1239,44 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
[ ... ]
> +static int rk_pcs_init(struct stmmac_priv *priv)
> +{
> +	struct rk_priv_data *bsp_priv = priv->plat->bsp_priv;
> +	struct device_node *node = priv->device->of_node;
> +	struct device_node *pcs_node;
> +	struct dw_xpcs *xpcs;
> +
> +	pcs_node = of_parse_phandle(node, "pcs-handle", 0);
> +	if (!pcs_node)
> +		return -ENODEV;
> +
> +	xpcs = xpcs_rk_create(priv->device, pcs_node, bsp_priv->id);
> +	of_node_put(pcs_node);
> +	if (IS_ERR(xpcs))
> +		return PTR_ERR(xpcs);
> +
> +	priv->hw->xpcs = xpcs;
> +	return 0;
> +}

[Severity: High]
Does this code leak the newly allocated XPCS instance when interacting with
stmmac_pcs_setup() in the core driver?

Looking at stmmac_pcs_setup() in drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c,
it declares a local xpcs variable initialized to NULL:

int stmmac_pcs_setup(struct net_device *ndev)
{
	struct dw_xpcs *xpcs = NULL;
	...
	if (priv->plat->pcs_init) {
		ret = priv->plat->pcs_init(priv);
	...
	priv->hw->xpcs = xpcs;

	return 0;
}

Because rk_pcs_init() successfully allocates the XPCS and assigns it directly
to priv->hw->xpcs, it returns 0.

The local variable xpcs in stmmac_pcs_setup() remains NULL. At the end of
stmmac_pcs_setup(), it unconditionally overwrites the newly allocated pointer
in priv->hw->xpcs with NULL.

Will this cause the MAC to lose its PCS reference and break SGMII link
negotiation?

Additionally, does this leak the dw_xpcs object memory, since teardown
functions like rk_pcs_exit() will see a NULL pointer and exit early?

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