Re: [PATCH net-next v4 4/7] net: ftgmac100: Enable AST2700 RMII support

[netdev-bot+sashiko]

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

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

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 commit message and the new comment in ftgmac100_start_hw()
  (drivers/net/ethernet/faraday/ftgmac100.c) both say that an opened…

--

net: ftgmac100: Enable AST2700 RMII support

This patch adds a FTGMAC100_QUIRK_RMII_ENABLE match-data quirk for
AST2700. When the attached PHY uses RMII, ftgmac100_start_hw() now sets
FTGMAC100_MACCR_RMII_ENABLE. The commit message argues that
netdev->phydev is always populated by the time ftgmac100_start_hw() runs.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 67b1fa464a421..9b7becb017262 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c

[ ... ]

> @@ -355,6 +359,12 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
>  static void ftgmac100_start_hw(struct ftgmac100 *priv)
>  {
>  	u32 maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);
> +	/*
> +	 * Only reached once the device is open, so a phy_device is already
> +	 * attached: a discrete PHY via "phy-handle", a "fixed-link" node, or
> +	 * NC-SI's internal fixed 100M PHY.
> +	 */
> +	struct phy_device *phydev = priv->netdev->phydev;

[ ... ]

> @@ -383,6 +393,11 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv)
>  	if (priv->netdev->features & NETIF_F_HW_VLAN_CTAG_RX)
>  		maccr |= FTGMAC100_MACCR_RM_VLAN;
>  
> +	if ((priv->quirks & FTGMAC100_QUIRK_RMII_ENABLE) &&
> +	    phydev->interface == PHY_INTERFACE_MODE_RMII) {
> +		maccr |= FTGMAC100_MACCR_RMII_ENABLE;
> +	}

[Severity: Medium]
Can phydev be NULL here on AST2700?

The commit message says:

  ftgmac100_probe_dt() always attaches a phy_device before
  register_netdev() can succeed -- via a discrete PHY ("phy-handle"), a
  "fixed-link" node, or NC-SI's internal fixed 100Mbit PHY

ftgmac100_probe_dt() also has a fallback path. If the node has an "mdio"
child and no use-ncsi, fixed-link or phy-handle property, it returns
success without connecting a PHY:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_probe_dt() {
    ...
	if (!ftgmac100_has_child_node(np, "mdio")) {
		...
		err = ftgmac100_mii_probe(netdev);
		...
	}

	return 0;
}

ftgmac100_probe() then calls register_netdev() with netdev->phydev ==
NULL. The aspeed,ast2700-mac binding in faraday,ftgmac100.yaml appears to
allow this layout. phy-handle is optional, an mdio child is allowed, and
only phy-mode is required.

The rest of the driver also seems to treat phydev as optional. For
example, ftgmac100_open() does:

	if (netdev->phydev) {
		/* If we have a PHY, start polling */
		phy_start(netdev->phydev);
	}

ftgmac100_stop(), ftgmac100_reset_task(), ftgmac100_set_pauseparam() and
ftgmac100_phy_disconnect() check for NULL in the same way.

The quirk bit is always set for AST2700, so the && does not
short-circuit. With such a DT, this path would dereference a NULL phydev:

ftgmac100_open()
  ftgmac100_init_all()
    ftgmac100_start_hw()
      phydev->interface

Today the same open path oopses earlier. It hits the unguarded check in
ftgmac100_reset_and_config_mac() that the commit message cites as
precedent:

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

That existing check looks like a latent NULL dereference itself, so it
doesn't prove the invariant. This patch adds a second dereference, which
would become the crash point if the earlier check were fixed or
reordered.

The later patch in this series, "net: ftgmac100: Require phy-mode for
AST2700", only changes ftgmac100_mii_probe(). That function isn't called
when an mdio child exists, so the gap seems to remain at the end of the
series.

Would it make sense to check phydev && phydev->interface ==
PHY_INTERFACE_MODE_RMII here? Another option is to have probe fail when
no PHY is attached on Aspeed MACs, which would also cover
ftgmac100_reset_and_config_mac(). Either way, the new comment above the
phydev declaration and the commit message would need updating to match.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-support_ast2700-v4-0-4d36b40939b3%40aspeedtech.com