Re: [net-next,05/14] net: stmmac: add stmmac core serdes support

[Jakub Kicinski <kuba@kernel.org>]

I haven't investigated too closely if this is legit or too defensive
TBH but since I'm sending the second one I'll include this as well..

This is an AI-generated review of your patch.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: add stmmac core serdes support

This commit adds SerDes PHY support to the stmmac core driver, moving it
from platform glue into the integrated PCS driver. The serdes field in
platform data is optional and NULL when not provided by platforms.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24a2555ca329..6c515f9efbe7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -3549,12 +3550,16 @@ static void stmmac_safety_feat_configuration(struct stmmac_priv *priv)
>
>  static void stmmac_clk_rx_i_require(struct stmmac_priv *priv)
>  {
> +	dwmac_serdes_power_on(priv);
> +	/* Only sets the SerDes mode if it wasn't already configured. */
> +	dwmac_serdes_init_mode(priv, priv->plat->phy_interface);

When priv->plat->serdes is NULL, dwmac_serdes_init_mode() will call
phy_get_mode(NULL). Unlike other phy_* functions (phy_power_on,
phy_power_off, phy_set_mode_ext, phy_init, phy_exit) which handle NULL
by returning early, phy_get_mode() is an inline that directly
dereferences the pointer:

    static inline enum phy_mode phy_get_mode(struct phy *phy)
    {
        return phy->attrs.mode;
    }

Can this NULL pointer dereference occur when a platform does not
populate plat->serdes? The call path stmmac_open() -> dwmac_serdes_init()
returns 0 (success) when serdes is NULL, allowing __stmmac_open() ->
stmmac_hw_setup() -> stmmac_clk_rx_i_require() to proceed.

> +	phylink_rx_clk_stop_block(priv->phylink);
>  }

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_serdes.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_serdes.c
> new file mode 100644
> index 000000000000..3003e1ae38d2
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_serdes.c

[ ... ]

> +int dwmac_serdes_init(struct stmmac_priv *priv)
> +{
> +	size_t i;
> +	int ret;
> +
> +	if (!priv->plat->serdes)
> +		return 0;

This returns success when serdes is NULL...

[ ... ]

> +int dwmac_serdes_init_mode(struct stmmac_priv *priv, phy_interface_t interface)
> +{
> +	struct phy *serdes = priv->plat->serdes;
> +
> +	if (phy_get_mode(serdes) == PHY_MODE_ETHERNET)
                         ^^^^^^

...but dwmac_serdes_init_mode() has no NULL check before calling
phy_get_mode(), which will crash when serdes is NULL. Should this
function also check for NULL serdes before calling phy_get_mode()?

> +		return 0;
> +
> +	return dwmac_serdes_set_mode(priv, interface);
> +}