Re: [PATCH net-next v4 3/7] net: ftgmac100: Add AST2700 compatible support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] (Baseline decision: false positive.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  exposes it on AST2700.

--

net: ftgmac100: Add AST2700 compatible support

This patch adds an FTGMAC100_AST2700 mac_id, match data for it, and an
"aspeed,ast2700-mac" entry in ftgmac100_of_match. In ftgmac100_probe(),
AST2700 is handled like the other Aspeed variants: is_aspeed is set and the
BIT(30) end-of-ring masks are used.

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

[ ... ]

> @@ -2017,7 +2018,8 @@ static int ftgmac100_probe(struct platform_device *pdev)
>  
>  	if (priv->mac_id == FTGMAC100_AST2400 ||
>  	    priv->mac_id == FTGMAC100_AST2500 ||
> -	    priv->mac_id == FTGMAC100_AST2600) {
> +	    priv->mac_id == FTGMAC100_AST2600 ||
> +	    priv->mac_id == FTGMAC100_AST2700) {
>  		priv->rxdes0_edorr_mask = BIT(30);
>  		priv->txdes0_edotr_mask = BIT(30);
>  		priv->is_aspeed = true;

[Severity: High]
This isn't a bug introduced by this patch, but setting is_aspeed for
AST2700 exposes an existing NULL phydev dereference on this variant.

Suppose an aspeed,ast2700-mac node has an mdio child but no phy-handle,
fixed-link or use-ncsi. In that case ftgmac100_probe_dt() skips both PHY
paths and returns success with netdev->phydev still NULL:

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() only calls ftgmac100_setup_mdio() for FARADAY, AST2400
and AST2500, so on AST2700 the mdio child is ignored entirely.

register_netdev() then succeeds. When the interface comes up,
ftgmac100_open() calls ftgmac100_reset_and_config_mac() before it checks
phydev:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_reset_and_config_mac() {
	...
	if (priv->is_aspeed && priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII) {
	...
}

Can this oops in ndo_open with RTNL held? The binding only requires
phy-mode for ast2700-mac and doesn't restrict mdio, so a DT like this
passes dt_binding_check. The same path already exists for AST2600.

The later patch "net: ftgmac100: Enable AST2700 RMII support" makes the
same assumption in ftgmac100_start_hw(). Its commit message says
"ftgmac100_probe_dt() always attaches a phy_device before register_netdev()
can succeed", but that does not hold for this configuration.

Could ftgmac100_probe_dt() reject the case with no PHY and no NC-SI before
register_netdev()? Alternatively, or in addition, the binding could forbid
mdio and require a PHY source for ast2600/ast2700.

[ ... ]

> @@ -2131,6 +2133,10 @@ static const struct ftgmac100_match_data ftgmac100_match_data_ast2600 = {
>  	.mac_id = FTGMAC100_AST2600
>  };
>  
> +static const struct ftgmac100_match_data ftgmac100_match_data_ast2700 = {
> +	.mac_id = FTGMAC100_AST2700
> +};
> +

[Severity: Low]
Once aspeed,ast2700-mac binds here, AST2700 uses a datapath that only
handles 32-bit DMA addresses.

ftgmac100_match_data_ast2700 has no FTGMAC100_QUIRK_DMA_35BIT, so
ftgmac100_probe() never widens the DMA mask. ftgmac100_init_hw() also
writes only the low 32 bits of the ring bases:

	iowrite32(priv->rxdes_dma, priv->base + FTGMAC100_OFFSET_RXR_BADR);
	...
	iowrite32(priv->txdes_dma, priv->base + FTGMAC100_OFFSET_NPTXR_BADR);

Buffer addresses are likewise written into the descriptors with
cpu_to_le32(map).

As far as I can tell, this can't be reached at this commit. The FTGMAC100
Kconfig entry still has "depends on ARM || COMPILE_TEST" and "depends on
!64BIT || BROKEN", so the driver can't be built for the arm64 AST2700.

Later in the series, "net: ftgmac100: Add AST2700 upper DMA address
support" adds the 35-bit quirk, programs the RXR/TXR BADDR_HIGH registers
and fills in the upper descriptor bits. "net: ftgmac100: Allow building on
ARM64" enables arm64 only after that, so the final state of the series
looks fine.

Is it intended that AST2700 binds to the driver before the upper DMA
address support is in place?

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