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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0 (2)

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).

- [High] The new AST2700 phy-mode check in ftgmac100_mii_probe() can
  never run, so the patch has no effect.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  reachable kernel oops.
- [Low] The AST2700 binding requires phy-mode in every configuration,
  including use-ncsi.
- [Low] In ftgmac100_mii_probe(), the comment "Aspeed only supports
  these" and the netdev_warn("Unsupported PHY mode %s !") fire for any…

--

net: ftgmac100: Require phy-mode for AST2700

This patch changes ftgmac100_mii_probe() so that on AST2700 it fails
with -EINVAL when the phy-mode property is missing, instead of falling
back to RGMII. The goal is to make phy-mode a required property for
AST2700, since that SoC also supports RMII.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 9b7becb01726..5a578a62df67 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;
> +		}

[Severity: High]
Can this new check ever be reached on AST2700?

ftgmac100_setup_mdio() is the only function that assigns priv->mii_bus.
ftgmac100_probe() calls it only for FARADAY, AST2400 and AST2500:

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

On AST2700, priv->mii_bus is therefore always NULL. ftgmac100_mii_probe()
returns at this check before it ever calls of_get_phy_mode():

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

That leaves the "phy-mode is required for this SoC" message unreachable.

The commit message says:

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

Is that true for AST2700? These are the probe paths in
ftgmac100_probe_dt():

- phy-handle or fixed-link: of_phy_get_and_connect() already returns
  NULL when of_get_phy_mode() fails, so probe fails with "Failed to
  connect to phy".
- use-ncsi: phy-mode is ignored and RMII is hardcoded.
- no mdio child: ftgmac100_mii_probe() fails with "No MDIO bus
  available".
- an mdio child with no phy-handle, fixed-link or use-ncsi:
  ftgmac100_probe_dt() returns 0 and phy-mode is never checked.

In that last case AST2700 still probes successfully without phy-mode.
That does not match "fail probe with an error message if it is not
specified".

The dt-binding patch in this series, "dt-bindings: net: ftgmac100: Add
AST2700 compatible", justifies required: phy-mode by saying it matches
the driver's probe-time requirement. This patch does not create that
requirement, and no later patch in the series changes
ftgmac100_mii_probe() or ftgmac100_probe_dt().

Would it work better to put the check in ftgmac100_probe_dt() (or
ftgmac100_probe())? It would need to run before the
fixed-link/phy-handle/mdio-child dispatch and outside the use-ncsi path.

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

[Severity: Low]
This isn't a bug introduced by this patch, but the comment and the
warning that follow it in ftgmac100_mii_probe() look out of date:

	if (priv->is_aspeed && !(phy_interface_mode_is_rgmii(phy_intf))) {
		netdev_warn(netdev,
			    "Unsupported PHY mode %s !\n",
			    phy_modes(phy_intf));
	}

The driver handles Aspeed RMII in two places: the SCU reset in
ftgmac100_reset_and_config_mac(), and FTGMAC100_QUIRK_RMII_ENABLE in
ftgmac100_start_hw(). The binding's phy-mode enum also allows rmii.

Do legacy AST2400/AST2500 RMII device trees with no mdio child get a
spurious "Unsupported PHY mode rmii !" warning here? phy_connect() still
gets the RMII mode, so the warning has no functional effect. This path
is not reachable on AST2700.

[Severity: High]
This isn't a bug introduced by this patch, but the mdio child path in
ftgmac100_probe_dt() can also lead to a NULL pointer dereference.

If an Aspeed DT node has an mdio child but no phy-handle, fixed-link or
use-ncsi, ftgmac100_probe_dt() returns 0 without attaching a PHY:

ftgmac100_probe_dt() {
    ...
	if (!ftgmac100_has_child_node(np, "mdio")) {
		...
	}

	return 0;
}

The netdev is then registered with netdev->phydev == NULL. On interface
up:

ftgmac100_open()
  ftgmac100_reset_and_config_mac()
    if (priv->is_aspeed && priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII)

Can this oops in ndo_open while rtnl_lock is held? ftgmac100_reset_task()
reaches the same dereference. ftgmac100_open() checks
if (netdev->phydev) later on, so a NULL phydev looks like an expected
state.

The series patch "net: ftgmac100: Enable AST2700 RMII support" adds a
second unguarded dereference in ftgmac100_start_hw():

	if ((priv->quirks & FTGMAC100_QUIRK_RMII_ENABLE) &&
	    phydev->interface == PHY_INTERFACE_MODE_RMII) {

That patch justifies the dereference with "ftgmac100_probe_dt() always
attaches a phy_device before register_netdev() can succeed". The mdio
child path above contradicts that.

In practice the crash would happen first in
ftgmac100_reset_and_config_mac(), because it runs before
ftgmac100_init_all()->ftgmac100_start_hw(). The phy-mode check in this
patch does not cover this path either.

[Severity: Low]
This is a pre-existing issue, but it bears on the binding requirement
this series adds. The AST2700 binding requires phy-mode in every
configuration, including use-ncsi, yet ftgmac100_probe_ncsi() never
reads phy-mode:

ftgmac100_probe_ncsi() {
    ...
	err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
				 PHY_INTERFACE_MODE_RMII);
    ...
}

A DT with use-ncsi and phy-mode = "rgmii" would pass the schema, and the
driver would silently use RMII. The binding documents use-ncsi as
"Currently assumes rmii (100bT)", so the driver's behaviour matches the
documentation.

Should the binding exempt use-ncsi configurations from the phy-mode
requirement?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-support_ast2700-v3-0-8d7999171ac4%40aspeedtech.com