Re: [PATCH 3/5] 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: 1 (1) · Medium: 0 · 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).

- [High] net: ftgmac100: NULL pointer dereference in ftgmac100_start_hw()

Pre-existing issues:
- [High] An Aspeed MAC (AST2400/2500/2600, and AST2700 once the series
  adds it) can probe successfully with no PHY attached, and opening it…

--

net: ftgmac100: Enable AST2700 RMII support

This commit adds an FTGMAC100_QUIRK_RMII_ENABLE match-data quirk and sets
it for AST2700. When the quirk is set and the attached PHY uses RMII,
ftgmac100_start_hw() sets the new FTGMAC100_MACCR_RMII_ENABLE bit in
MACCR.

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

[ ... ]

> @@ -355,6 +359,7 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
>  static void ftgmac100_start_hw(struct ftgmac100 *priv)
>  {
>  	u32 maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);
> +	struct phy_device *phydev = priv->netdev->phydev;
>  
>  	/* Keep the original GMAC and FAST bits */
>  	maccr &= (FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_GIGA_MODE);
> @@ -383,6 +388,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: High]
Can phydev be NULL here on AST2700?

ftgmac100_probe_dt() can return 0 without connecting a PHY. That
happens when the MAC node has an "mdio" child node but no "use-ncsi",
"fixed-link" or "phy-handle" property. The mdio child check only
skips the legacy ftgmac100_mii_probe() scan, so nothing sets
netdev->phydev and register_netdev() still succeeds.

When the interface is later brought up, ftgmac100_open() reaches
ftgmac100_start_hw() through ftgmac100_init_all(). Because
FTGMAC100_QUIRK_RMII_ENABLE is set for AST2700, the new check reads
phydev->interface without testing phydev first. The same applies to
the other callers of ftgmac100_start_hw(): ftgmac100_set_rx_mode() and
the need_mac_restart path in ftgmac100_poll().

The rest of the driver treats a missing PHY as a valid state.
ftgmac100_open() calls phy_start() only when netdev->phydev is set, and
ftgmac100_reset() and ftgmac100_phy_disconnect() also check for NULL.
A devicetree mistake like this should either fail probe or be handled.
It should not oops the kernel with rtnl_lock held from dev_open().

The is_aspeed RMII check in ftgmac100_reset_and_config_mac() makes the
same assumption, so guarding only this spot may not be enough.

Would it make sense to reject the PHY-less configuration in
ftgmac100_probe_dt() when neither NCSI nor a PHY is present? Another
option is to cache the interface mode when the PHY is connected and
use the cached value in both places.

[Severity: High]
This isn't a bug introduced by this patch, but can phydev be NULL here?

ftgmac100_probe_dt() can return 0 without attaching a PHY. This happens
when the MAC node has an "mdio" child but no use-ncsi, fixed-link or
phy-handle property:

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

	return 0;
}

In that case register_netdev() succeeds and netdev->phydev is NULL.

The existing check in ftgmac100_reset_and_config_mac() already
dereferences the pointer without a NULL check:

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

On every path that reaches ftgmac100_start_hw() (ftgmac100_init_all() on
open and reset, ftgmac100_set_rx_mode(), and the need_mac_restart path in
ftgmac100_poll()), ftgmac100_reset_and_config_mac() either runs first or
has already succeeded. This means the NULL dereference is already
reachable on AST2400/2500/2600 through
ftgmac100_open()->ftgmac100_reset_and_config_mac(). The new check in
ftgmac100_start_hw() makes the same assumption for AST2700.

Elsewhere the driver treats a NULL phydev as valid. ftgmac100_open()
calls phy_start() only when netdev->phydev is set, and ftgmac100_reset()
and ftgmac100_phy_disconnect() also check for NULL.

dev_open() holds rtnl_lock, so an oops here would leave RTNL held.

Could probe reject PHY-less configurations instead? Another option is to
guard both dereferences, or to cache the interface mode when the PHY is
connected.

> +
>  	/* Hit the HW */
>  	iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927-support_ast2700-v1-0-9f03f29d2c82%40aspeedtech.com