Re: [PATCH net-next v2 4/6] net: ftgmac100: Require phy-mode for AST2700

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new AST2700 check in ftgmac100_mii_probe() can never run,
  and the commit message describes behaviour that does not exist.
- [Low] The commit says it makes 'phy-mode' a required property for
  AST2700, but the DT binding in the same series does not say so.

--

net: ftgmac100: Require phy-mode for AST2700

This patch makes ftgmac100_mii_probe() return -EINVAL on AST2700 when
the phy-mode property is missing, instead of falling back to RGMII. The
commit message says the RGMII default is no longer valid because
AST2700 also supports RMII.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index b835472da3604..2d029d1262e6f 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1521,8 +1521,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;
> +		}

[Severity: Medium]
Can this check ever run on AST2700?

priv->mii_bus is set in only one place, ftgmac100_setup_mdio(). That
function is called from ftgmac100_probe() only for the Faraday, AST2400
and AST2500 variants:

ftgmac100_probe() {
    ...
	if (priv->mac_id == FTGMAC100_FARADAY ||
	    priv->mac_id == FTGMAC100_AST2400 ||
	    priv->mac_id == FTGMAC100_AST2500) {
		err = ftgmac100_setup_mdio(netdev);
    ...
}

So on AST2700 priv->mii_bus stays NULL, and ftgmac100_mii_probe()
returns before it reaches of_get_phy_mode():

ftgmac100_mii_probe() {
    ...
	if (!priv->mii_bus) {
		dev_err(priv->dev, "No MDIO bus available\n");
		return -ENODEV;
	}
    ...
}

That leaves the new dev_err() and -EINVAL return unreachable on AST2700.

The other AST2700 PHY path is phy-handle or fixed-link in
ftgmac100_probe_dt(). It goes through of_phy_get_and_connect(), which
already returns NULL when phy-mode is missing:

of_phy_get_and_connect() {
    ...
	ret = of_get_phy_mode(np, &iface);
	if (ret)
		return NULL;
    ...
}

Probe then fails with "Failed to connect to phy" and never uses an RGMII
default.

Given that, is this part of the commit message accurate for AST2700?

  "The driver currently defaults to RGMII when the "phy-mode" property
  is missing from the device tree."

  "... fail probe with an error message if it is not specified, instead
  of silently defaulting to RGMII."

One AST2700 configuration can still probe without phy-mode: an mdio child
node with no phy-handle. The requirement the commit describes is not
enforced there either.

The end of the series ("net: ftgmac100: Allow building on ARM64") has the
same layout. ftgmac100_setup_mdio() is still skipped for AST2700, and the
mii_bus check still comes before the new AST2700 check.

Would it make sense to move this check somewhere reachable, such as
ftgmac100_probe_dt(), and update the commit message to match? Or should
the patch be dropped?

> +		phy_intf = PHY_INTERFACE_MODE_RGMII;
> +	}

[Severity: Low]
The commit message says:

  "Make "phy-mode" a required property for AST2700"

The binding doesn't seem to say that. The top-level required list in
Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml is:

required:
  - compatible
  - reg
  - interrupts

The allOf if/then block for aspeed,ast2600-mac and aspeed,ast2700-mac only
covers resets. That means dtbs_check still accepts an AST2700 node with
no phy-mode.

The series already adds the AST2700 compatible in "dt-bindings: net:
ftgmac100: Add AST2700 compatible". Could the requirement go in that
patch, as an if/then for aspeed,ast2700-mac with required: [phy-mode]?

It would need to exclude use-ncsi nodes. ftgmac100_probe_ncsi() hardcodes
PHY_INTERFACE_MODE_RMII and never reads phy-mode.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-support_ast2700-v2-0-36de51fb8066%40aspeedtech.com