Re: [PATCH net-next 4/4] net: stmmac: dwmac-sun8i: Add Allwinner H616 EMAC clock index and internal PHY support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: stmmac: MAC soft reset is completely bypassed for internal PHY
- [Medium] net: stmmac: silent acceptance of missing clocks and resets for internal PHY
- [Medium] net: stmmac: internal PHY clock and reset leaked if MAC reset fails
--

commit a3ca47044e22ce48b791f8eb8f51a44d49d7ba43
Author: Alastair D'Silva <alastair@d-silva.org>

net: stmmac: dwmac-sun8i: Add Allwinner H616 EMAC clock index and internal PHY support

This commit adds support for EMAC1 and the internal Fast Ethernet PHY (AC300)
on Allwinner H616 and H618 SoCs. It allows reading a register index from the
syscon property and introduces emac_variant_h616_internal with internal PHY
support.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index 48c52eb962335..1728893589a68 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

[ ... ]

> @@ -589,6 +600,10 @@ static int sun8i_dwmac_init(struct device *dev, void *priv)
>  		ret = sun8i_dwmac_power_internal_phy(netdev_priv(ndev));
>  		if (ret)
>  			goto err_disable_regulator;
> +
> +		ret = sun8i_dwmac_reset(netdev_priv(ndev));
> +		if (ret)
> +			goto err_disable_regulator;

[Severity: Medium]
If sun8i_dwmac_reset() returns an error here, the code jumps directly to
err_disable_regulator. Will this skip the call to
sun8i_dwmac_unpower_internal_phy(), leaving the internal PHY powered and its
clock enabled?

[ ... ]

> @@ -784,16 +799,23 @@ static int get_ephy_nodes(struct stmmac_priv *priv)
>  	/* Seek for internal PHY */
>  	for_each_child_of_node_scoped(mdio_internal, iphynode) {
>  		gmac->ephy_clk = of_clk_get(iphynode, 0);
> -		if (IS_ERR(gmac->ephy_clk))
> -			continue;
> +		if (IS_ERR(gmac->ephy_clk)) {
> +			ret = PTR_ERR(gmac->ephy_clk);
> +			if (ret == -EPROBE_DEFER) {
> +				of_node_put(mdio_internal);
> +				return ret;
> +			}
> +			gmac->ephy_clk = NULL;
> +		}

[Severity: Medium]
If of_clk_get() returns an error other than -EPROBE_DEFER (such as -EINVAL),
the error handling now sets gmac->ephy_clk to NULL and continues to the reset
control fetch, which eventually returns 0 on success. Does this silently
accept a device tree node missing the required clock instead of skipping it
via continue?

>  		gmac->rst_ephy = of_reset_control_get_exclusive(iphynode, NULL);
>  		if (IS_ERR(gmac->rst_ephy)) {
>  			ret = PTR_ERR(gmac->rst_ephy);
>  			if (ret == -EPROBE_DEFER) {
> +				clk_put(gmac->ephy_clk);
>  				of_node_put(mdio_internal);
>  				return ret;
>  			}
> -			continue;
> +			gmac->rst_ephy = NULL;
>  		}

[Severity: Medium]
Similarly, if of_reset_control_get_exclusive() returns a non-EPROBE_DEFER
error, the pointer is set to NULL and the loop returns 0 (success). Will this
cause the driver to silently accept missing resets and later perform
operations with NULL pointers?

[ ... ]

> @@ -892,10 +916,13 @@ static int mdio_mux_syscon_switch_fn(int current_child, int desired_child,
>  		} else {
>  			sun8i_dwmac_unpower_internal_phy(gmac);
>  		}
> -		/* After changing syscon value, the MAC need reset or it will
> +		/* After changing syscon value, the MAC needs reset or it will
>  		 * use the last value (and so the last PHY set).
> +		 * For internal PHY, the MAC reset will timeout because the PHY
> +		 * is not yet enabled/clocked. Delay the reset to dwmac_init.
>  		 */
> -		ret = sun8i_dwmac_reset(priv);
> +		if (!gmac->use_internal_phy)
> +			ret = sun8i_dwmac_reset(priv);
>  	}
>  	return ret;

[Severity: High]
The comment states the MAC reset is delayed to dwmac_init. However, during
driver probe in sun8i_dwmac_probe(), plat_dat->init (which points to
sun8i_dwmac_init) executes before the MDIO mux is registered.

During that initial execution of sun8i_dwmac_init(), gmac->use_internal_phy
is false (zero-initialized), so the reset block is skipped. Later, this
MDIO switch function sets gmac->use_internal_phy = true but explicitly skips
the MAC reset here.

Because both execution paths skip the reset, does this mean the MAC soft
reset is completely bypassed during initialization when using the internal
PHY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916044119.475666-1-alastair@d-silva.org?part=4