Re: [PATCH net-next v4 5/7] net: ftgmac100: Require phy-mode for AST2700

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: ftgmac100: unreachable AST2700 phy-mode check in ftgmac100_mii_probe()
--

commit bef0234e231a9b711aa69253e6ec48a7d40857c2
Author: Jacky Chou <jacky_chou@aspeedtech.com>

net: ftgmac100: Require phy-mode for AST2700

This commit attempts to make the phy-mode property required for AST2700
in ftgmac100_mii_probe() by failing probe with -EINVAL instead of
defaulting to RGMII when the property is missing.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 9b7becb017262..5a578a62df673 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1526,8 +1526,13 @@ static int ftgmac100_mii_probe(struct net_device *netdev)
>  
>  	/* Default to RGMII. It's a gigabit part after all */
>  	err = of_get_phy_mode(np, &phy_intf);
> -	if (err)
> +	if (err) {
> +		if (priv->mac_id == FTGMAC100_AST2700) {
> +			dev_err(priv->dev, "phy-mode is required for this SoC\n");
> +			return -EINVAL;
> +		}
>  		phy_intf = PHY_INTERFACE_MODE_RGMII;
> +	}

[Severity: Medium]
Can this new FTGMAC100_AST2700 check in ftgmac100_mii_probe() ever be
reached?

In ftgmac100_probe(), ftgmac100_setup_mdio() is the only function that
allocates priv->mii_bus, and it is not called for FTGMAC100_AST2700:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_probe() {
	...
	if (priv->mac_id == FTGMAC100_FARADAY ||
	    priv->mac_id == FTGMAC100_AST2400 ||
	    priv->mac_id == FTGMAC100_AST2500) {
		err = ftgmac100_setup_mdio(netdev);
		if (err)
			return err;
	}
	...
}

When ftgmac100_probe_dt() falls back to ftgmac100_mii_probe(),
priv->mii_bus is always NULL for FTGMAC100_AST2700, so
ftgmac100_mii_probe() returns -ENODEV before reaching of_get_phy_mode():

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_mii_probe() {
	...
	if (!priv->mii_bus) {
		dev_err(priv->dev, "No MDIO bus available\n");
		return -ENODEV;
	}

	/* Default to RGMII. It's a gigabit part after all */
	err = of_get_phy_mode(np, &phy_intf);
	...
}

Furthermore, in ftgmac100_probe_dt(), AST2700 nodes with use-ncsi (which
calls ftgmac100_probe_ncsi() and hardcodes PHY_INTERFACE_MODE_RMII) or an
mdio child node return without calling ftgmac100_mii_probe() at all:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_probe_dt() {
	...
	if (of_get_property(np, "use-ncsi", NULL))
		return ftgmac100_probe_ncsi(netdev, priv, pdev);

	if (of_phy_is_fixed_link(np) ||
	    of_get_property(np, "phy-handle", NULL)) {
		phy = of_phy_get_and_connect(priv->netdev, np,
					     &ftgmac100_adjust_link);
		...
		return 0;
	}

	if (!ftgmac100_has_child_node(np, "mdio")) {
		err = ftgmac100_mii_probe(netdev);
	...
}

Should the phy-mode requirement for FTGMAC100_AST2700 be checked in
ftgmac100_probe_dt() instead?

>  
>  	/* Aspeed only supports these. I don't know about other IP
>  	 * block vendors so I'm going to just let them through for

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-support_ast2700-v4-0-4d36b40939b3@aspeedtech.com?part=5