Re: [PATCH net-next v3 2/6] net: ftgmac100: Add AST2700 compatible support

[netdev-bot+sashiko@kernel.org]

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] The comment on struct ftgmac100::rclk still says '/*
  AST2500/AST2600 RMII ref clock gate */' (line 117 at this commit, and…
- [Low] (Baseline: discarded as resolved later in the series.) At this
  commit, AST2700 binds to the driver with no DMA support above 4 GiB.

Pre-existing issues:
- [High] On Aspeed variants, including the newly added AST2700, a NULL
  pointer dereference oopses ftgmac100_reset_and_config_mac() when the…

--

net: ftgmac100: Add AST2700 compatible support

This adds an FTGMAC100_AST2700 mac_id, its match data, and an
"aspeed,ast2700-mac" entry in the OF match table. In ftgmac100_probe(),
AST2700 is handled the same way as the other Aspeed variants: it gets
the BIT(30) EDORR/EDOTR masks and is_aspeed is set.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 6d2fe5c2f390..67b1fa464a42 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 is a pre-existing issue, not one introduced by this patch. Setting
is_aspeed for AST2700 does make it reachable with the new compatible,
though. Can ftgmac100_reset_and_config_mac() dereference a NULL phydev
here?

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_reset_and_config_mac() {
    ...
	/* Aspeed RMII needs SCU reset to clear status */
	if (priv->is_aspeed && priv->netdev->phydev->interface == PHY_INTERFACE_MODE_RMII) {
    ...
}

Suppose the DT node has an "mdio" child node but no "phy-handle", no
"fixed-link" and no "use-ncsi". In that case ftgmac100_probe_dt() skips
ftgmac100_mii_probe() and returns 0 without attaching a PHY:

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

	return 0;

ftgmac100_probe() also skips ftgmac100_setup_mdio() for AST2700. So
register_netdev() succeeds with netdev->phydev == NULL. The
faraday,ftgmac100.yaml binding allows this DT, because mdio and
phy-handle are both optional.

Bringing the interface up then runs:

ftgmac100_open()
  ftgmac100_reset_and_config_mac()
    priv->netdev->phydev->interface

The reset task reaches the same code through ftgmac100_reset().

Elsewhere, ftgmac100_open() treats phydev as optional:

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

The unchecked dereference came in with commit af350ee72e9dd ("net:
ftgmac100: Add optional reset control for RMII mode on Aspeed SoCs").
It already affects AST2400/2500/2600.

A later patch in this series, "net: ftgmac100: Enable AST2700 RMII
support", adds another unconditional phydev->interface dereference in
ftgmac100_start_hw(). Its commit message and code comment say that
probe_dt always attaches a phy_device before register_netdev(). Doesn't
the path above show that this assumption doesn't always hold?

[Severity: Low]
This isn't a bug, but the comment on the rclk member of struct ftgmac100
still reads:

	/* AST2500/AST2600 RMII ref clock gate */
	struct clk *rclk;

Since is_aspeed is now set for AST2700, ftgmac100_setup_clk() acquires
RCLK for it too. The binding also describes the clock as "RMII RCLK
gate for AST2500/2600/2700". Could the comment be updated to include
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]
Can AST2700 do DMA with only this match data? At this commit no quirks
are set, so dma_set_mask_and_coherent() is never called. In addition:

ftgmac100_init_hw() only programs the 32-bit RXR_BADR/NPTXR_BADR
registers.

ftgmac100_alloc_rx_buf() and the tx path only store the low 32 bits of
the buffer address in rxdes3/txdes3.

The AST2700 EVB puts all of its DRAM above 4 GiB:

	reg = <0x4 0x00000000 0x0 0x40000000>;

This looks to be handled later in the series. "net: ftgmac100: Add
AST2700 upper DMA address support" adds FTGMAC100_QUIRK_DMA_35BIT to
this match data. The same patch adds the RXR/TXR high base register
writes and the upper address bits in rxdes2/txdes2.

The driver also cannot be built for arm64 until "net: ftgmac100: Allow
building on ARM64". The Kconfig has "depends on ARM || COMPILE_TEST"
and "depends on !64BIT || BROKEN", so this entry has no effect at this
point in the series.

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