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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 (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] The new AST2700 "phy-mode is required" check in
  ftgmac100_mii_probe() (drivers/net/ethernet/faraday/ftgmac100.c) can…
- [High] The commit message says that on AST2700 the driver will "fail
  probe with an error message if [phy-mode] is not specified", and the…

Pre-existing issues:
- [High] NULL pointer dereference in ftgmac100_reset_and_config_mac() on
  ndo_open for an Aspeed MAC with no PHY attached.

--

net: ftgmac100: Require phy-mode for AST2700

This patch changes ftgmac100_mii_probe() so that on AST2700 a missing
phy-mode property returns -EINVAL. Previously the driver fell back to
RGMII. The goal is to make phy-mode a required property for AST2700,
because that SoC also supports RMII.

> 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;
> +		}

[Severity: High]
Can AST2700 ever reach this check? Earlier in ftgmac100_mii_probe()
there is:

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

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

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

On AST2700, ftgmac100_probe()->ftgmac100_probe_dt()->ftgmac100_mii_probe()
returns -ENODEV before of_get_phy_mode() is called. That makes the new
branch look like dead code.

The phy-handle and fixed-link paths go through of_phy_get_and_connect().
That function already returns NULL when phy-mode is missing:

drivers/net/mdio/of_mdio.c:of_phy_get_and_connect() {
	...
	ret = of_get_phy_mode(np, &iface);
	if (ret)
		return NULL;
	...
}

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

  The driver currently defaults to RGMII when the "phy-mode" property
  is missing from the device tree. This default is no longer valid for
  AST2700

As far as I can tell, AST2700 never reaches the RGMII fallback, and this
patch does not change how AST2700 probes. The companion patch
"dt-bindings: net: ftgmac100: Add AST2700 compatible" says the binding
matches "the driver's probe-time requirement", so it depends on this
check working.

Could the check move somewhere AST2700 actually reaches, such as the
start of ftgmac100_probe_dt() or ftgmac100_probe()? The later patches in
the series, "net: ftgmac100: Add AST2700 upper DMA address support" and
"net: ftgmac100: Allow building on ARM64", don't seem to touch this logic.

[Severity: High]
Two other AST2700 probe paths in ftgmac100_probe_dt() still succeed
without phy-mode.

The first is use-ncsi:

	if (of_get_property(np, "use-ncsi", NULL))
		return ftgmac100_probe_ncsi(netdev, priv, pdev);

ftgmac100_probe_ncsi() never reads phy-mode and always connects with
RMII:

	err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
				 PHY_INTERFACE_MODE_RMII);

An AST2700 NC-SI node without phy-mode therefore probes successfully. A
node with phy-mode = "rgmii" passes binding validation, but the driver
ignores that value and runs in RMII.

The second is a node with an mdio child but no phy-handle, fixed-link or
use-ncsi. ftgmac100_probe_dt() skips the ftgmac100_mii_probe() block and
returns 0 without reading phy-mode:

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

	return 0;

Doesn't this conflict with "fail probe with an error message if it is
not specified" in the commit message? The binding also requires phy-mode
for every aspeed,ast2700-mac node.

Should the binding allow a use-ncsi exception, or should the driver check
phy-mode on all of these paths?

>  		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: High]
This is a pre-existing issue and was not introduced by this patch. Can
ftgmac100_open() dereference a NULL phydev on the mdio child path
described above?

Take an Aspeed node with an mdio child but no phy-handle, fixed-link or
use-ncsi. ftgmac100_probe_dt() returns 0 without connecting a PHY, so the
netdev is registered with netdev->phydev == NULL.

On ip link set up, ftgmac100_open()->ftgmac100_reset_and_config_mac()
does:

	/* Aspeed RMII needs SCU reset to clear status */
	if (priv->is_aspeed && priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII) {

is_aspeed is true for every Aspeed MAC. ftgmac100_open() only checks
netdev->phydev later, just before phy_start().

This dereference came from commit af350ee72e9dda, before this series.
The series extends it to AST2700.

The earlier patch "net: ftgmac100: Enable AST2700 RMII support" adds
another unguarded read in ftgmac100_start_hw():

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

That patch's comment says ftgmac100_probe_dt() always attaches a
phy_device, but this path shows it doesn't. The phy-mode check added
here doesn't cover this path either.

Should these reads check for a NULL phydev, or should
ftgmac100_probe_dt() fail when no PHY is attached?

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