Re: [PATCH net-next v4 3/6] net: xilinx: axienet: Add 10G/25G (XXV) ethernet support

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 4 · Medium: 2 (1) · Low: 4

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] Under the new xlnx,xxv-ethernet binding, the interface cannot be
  opened.
- [High] The new early return in axienet_open() leads to a double free /
  use-after-free after a failed resume.
- [High] The XXV RX path takes frame lengths from a source the XXV
  hardware does not write.
- [High] The XXV MAC's receive max-packet-length register is never
  programmed to match the RX buffer size.
- [Medium] Pause is advertised and reported for XXV but never applied to
  the hardware.
- [Medium] The GT reset-done timeout is only 1 ms, and this patch makes
  missing it fatal to ndo_open.
- [Low] A real fix for an existing 1G bug is bundled into this feature
  patch without a Fixes: tag.
- [Low] axienet_xxv_poll_link_ready()'s kernel-doc says missing PCS block
  lock is expected with an unplugged cable and must not fail bring-up,…
- [Low] The jumbo-frame flag and the MTU range disagree for XXV.
- [Low] The kernel-doc for axienet_device_reset() is now wrong for XXV.

Pre-existing issues:
- [Medium] axienet_dma_rx_cb() passes `rx_len = app_metadata[LEN_APP] &
  0xFFFF` straight to skb_put().

--

net: xilinx: axienet: Add 10G/25G (XXV) ethernet support

This patch adds support for the Xilinx XXV 10G/25G Ethernet IP to the
axienet driver. The XXV-specific code lives in new xilinx_axienet_xxv.c/.h
files and is described through an axienet_10g25g_config with callbacks. It
also propagates axienet_device_reset() failures from axienet_open().

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 5af48d5cbed3b..c09f88b552fce 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -1710,6 +1710,10 @@ static int axienet_open(struct net_device *ndev)
>  	axienet_lock_mii(lp);
>  	ret = axienet_device_reset(ndev);
>  	axienet_unlock_mii(lp);
> +	if (ret) {
> +		dev_err(lp->dev, "axienet_device_reset() failed: %d\n", ret);
> +		return ret;
> +	}

[Severity: Low]
This isn't a bug in the new code, but this check also fixes an existing
problem on the 1G path. Before this patch, phylink_fwnode_phy_connect()
overwrote the ret from axienet_device_reset() before anything checked it.

On the 1G legacy DMA path, axienet_device_reset() can already fail. That
happens when __axienet_device_reset() times out or when
axienet_dma_bd_init() cannot allocate descriptors:

axienet_device_reset() {
    ...
	ret = __axienet_device_reset(lp);
	if (ret)
		return ret;

	ret = axienet_dma_bd_init(ndev);
    ...
}

In those cases open continued into phylink_start() and
axienet_init_legacy_dma(). The core had not been reset, or there were no BD
rings.

The commit message only says this "matters for XXV". Could this go to net as
a separate fix with a Fixes: tag, so stable picks it up?

[Severity: High]
Can this early return lead to a double free after a failed resume?

axienet_suspend() calls axienet_stop(). In dmaengine mode, the only mode XXV
supports, that frees the skb rings and releases the DMA channels. It does not
clear the pointers:

axienet_stop() {
    ...
	for (i = 0; i < TX_BD_NUM_MAX; i++)
		kfree(lp->tx_skb_ring[i]);
	kfree(lp->tx_skb_ring);
    ...
	dma_release_channel(lp->rx_chan);
	dma_release_channel(lp->tx_chan);
    ...
}

axienet_resume() then ignores what axienet_open() returns:

axienet_resume() {
    ...
	rtnl_lock();
	axienet_open(ndev);
	rtnl_unlock();

	netif_device_attach(ndev);
    ...
}

Suppose axienet_xxv_mac_init() returns -ETIMEDOUT from the GT reset-done
poll. axienet_open() now returns before axienet_init_dmaengine() runs, but
the netdev is still netif_running().

The next ndo_stop can come from ifdown, from dev_close() in
axienet_shutdown(), or from unregister_netdev() in axienet_remove(). It would
run axienet_stop() again. That calls dmaengine_terminate_sync() on the
released channels and kfree()s the already freed tx_skb_ring/rx_skb_ring
arrays and their entries. It then calls dma_release_channel() a second time.

Before this patch the reset error was overwritten and open always
re-allocated, so stop stayed balanced. Resume ignoring the open error
predates this patch, but this early return is what makes the problem
reachable.

>  
>  	ret = phylink_fwnode_phy_connect(lp->phylink, dev_fwnode(lp->dev), 0);
>  	if (ret) {

[ ... ]

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c b/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c
> new file mode 100644
> index 0000000000000..9f7efab84f827
> --- /dev/null
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_xxv.c
> @@ -0,0 +1,323 @@

[ ... ]

> +/* Option table for setting up XXV Ethernet hardware options */
> +static struct axienet_option xxvenet_options[] = {
> +	{ /* Turn on FCS stripping on receive packets */
> +		.opt = XAE_OPTION_FCS_STRIP,
> +		.reg = XXV_RCW1_OFFSET,
> +		.m_or = XXV_RCW1_FCS_MASK,
> +	}, { /* Turn on FCS insertion on transmit packets */
> +		.opt = XAE_OPTION_FCS_INSERT,
> +		.reg = XXV_TC_OFFSET,
> +		.m_or = XXV_TC_FCS_MASK,
> +	}, { /* Enable transmitter */
> +		.opt = XAE_OPTION_TXEN,
> +		.reg = XXV_TC_OFFSET,
> +		.m_or = XXV_TC_TX_MASK,
> +	}, { /* Enable receiver */
> +		.opt = XAE_OPTION_RXEN,
> +		.reg = XXV_RCW1_OFFSET,
> +		.m_or = XXV_RCW1_RX_MASK,
> +	},
> +	{}
> +};

[Severity: Medium]
Is the pause configuration ever applied to the XXV MAC?

axienet_probe() sets MAC_SYM_PAUSE | MAC_ASYM_PAUSE in
lp->phylink_config.mac_capabilities for every MAC type.
axienet_xxv_phylink_set_capabilities() only adds speed bits, so XXV
advertises both pause modes.

axienet_10g25g_config has no .mac_link_up. axienet_mac_link_up() therefore
silently drops the resolved tx_pause/rx_pause:

	if (lp->axienet_config->mac_link_up)
		lp->axienet_config->mac_link_up(ndev, speed, tx_pause, rx_pause);

This table also has no XAE_OPTION_FLOW_CONTROL entry. The
XXV_CONFIG_TX_FLOW_CTRL1_OFFSET and XXV_CONFIG_RX_FLOW_CTRL1/2_OFFSET
registers are only read for ethtool -d and never written.

As a result, phylink and ethtool -a/-A report and accept pause settings while
the MAC stays at its reset default. Should XXV program the flow control
registers from a mac_link_up callback, or stop advertising pause?

[ ... ]

> +/**
> + * axienet_xxv_gt_reset - Pulse the XXV GT reset line
> + * @lp: Pointer to the axienet_local structure
> + */
> +static void axienet_xxv_gt_reset(struct axienet_local *lp)
> +{
> +	u32 val;
> +
> +	/* Reset GT */
> +	val = axienet_ior(lp, XXV_GT_RESET_OFFSET);
> +	val |= XXV_GT_RESET_MASK;
> +	axienet_iow(lp, XXV_GT_RESET_OFFSET, val);
> +	/* Allow 1 ms for the GT reset to settle (see timeout note above) */
> +	usleep_range(1000, 2000);
> +	val = axienet_ior(lp, XXV_GT_RESET_OFFSET);
> +	val &= ~XXV_GT_RESET_MASK;
> +	axienet_iow(lp, XXV_GT_RESET_OFFSET, val);
> +}

[Severity: Low]
The kernel-doc for axienet_device_reset() in xilinx_axienet_main.c still
says:

 * Ethernet core. No separate hardware reset is done for the Axi Ethernet
 * core.

For XXV, that function now calls config->gt_reset() first, and this pulses
XXV_GT_RESET_MASK. XXV is dmaengine-only, so the DMA reset and BD init
described there never happen. The function now also returns mac_init()
failures such as the GT reset-done timeout.

Could the comment be updated to match?

[ ... ]

> +static int axienet_xxv_poll_link_ready(struct net_device *ndev)
> +{
> +	struct axienet_local *lp = netdev_priv(ndev);
> +	u32 val;
> +	int ret;
> +
> +	/* Confirm XXV Ethernet is up: on IP v3.2+, wait for GT
> +	 * reset-done before further register access, then poll until
> +	 * RX PCS block lock is asserted.
> +	 */
> +	if (axienet_xxv_ip_has_gtwiz_status(lp->xxv_ip_version)) {
> +		ret = readl_poll_timeout(lp->regs + XXV_STAT_GTWIZ_OFFSET,
> +					 val,
> +					 (val & XXV_GTWIZ_RESET_DONE) == XXV_GTWIZ_RESET_DONE,
> +					 XXV_LINK_POLL_INTERVAL_US,
> +					 DELAY_OF_ONE_MILLISEC);

[Severity: Medium]
Is 1 ms long enough for GT reset-done?

axienet_xxv_gt_reset() releases ctl_gt_reset_all after 1-2 ms, and the GT
reset sequence only starts at that release. This poll then gives the GT
wizard DELAY_OF_ONE_MILLISEC to assert both TX and RX reset-done.

GT reset-done covers PLL re-lock and the RX reset sequence. With no signal
present, the CDR lock wait in that sequence can run until its internal
timeout. The comment at the top of the file says the bound is empirical and
that PG210 does not specify it.

With the new check in axienet_open(), a timeout here now fails ndo_open.
Could an unplugged cable push this past 1 ms and fail bring-up? The
kernel-doc above says an unplugged cable must not do that.

> +		if (ret) {
> +			netdev_err(ndev, "XXV MAC GT reset not complete! Cross-check the MAC ref clock configuration\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = readl_poll_timeout(lp->regs + XXV_STATRX_BLKLCK_OFFSET,
> +				 val, (val & XXV_RX_BLKLCK_MASK),
> +				 XXV_LINK_POLL_INTERVAL_US,
> +				 DELAY_OF_ONE_MILLISEC);
> +	if (ret)
> +		netdev_err(ndev, "XXV MAC block lock not complete! Cross-check the MAC ref clock configuration\n");
> +
> +	return 0;
> +}

[Severity: Low]
The kernel-doc says a missing block lock is expected with an unplugged
cable, and the function returns 0 in that case. Should this be logged below
netdev_err()?

As written, every ip link set up on a port with no link partner logs an
error that points at the ref clock configuration.

> +
> +/**
> + * axienet_xxv_mac_init - XXV MAC-specific bring-up after the DMA reset
> + * @ndev: Pointer to the net_device structure
> + *
> + * Return: 0 on success or a negative error number otherwise.
> + */
> +static int axienet_xxv_mac_init(struct net_device *ndev)
> +{
> +	struct axienet_local *lp = netdev_priv(ndev);
> +	int ret;
> +
> +	ret = axienet_xxv_poll_link_ready(ndev);
> +	if (ret)
> +		return ret;
> +
> +	lp->options |= XAE_OPTION_FCS_STRIP;
> +	lp->options |= XAE_OPTION_FCS_INSERT;
> +
> +	return 0;
> +}

[Severity: High]
Should the XXV RX max packet length be programmed here?

XXV_JUM_OFFSET (CONFIGURATION_RX_MTU, ctl_rx_max_packet_len) appears only in
the ethtool -d table and is never written. Its reset default is 9600 bytes.

At the default MTU, axienet_device_reset() sets lp->max_frm_size to
XAE_MAX_VLAN_FRAME_SIZE (1522). axienet_rx_submit_desc() maps each RX buffer
at exactly that size.

If a link partner sends frames of 1523 to 9600 bytes, the MAC accepts them
and splits each one across several S2MM descriptors. axienet_dma_rx_cb()
treats every completion as a whole frame. Would these fragmented frames be
passed up the stack?

Changing the MTU never updates the MAC either, so the software and hardware
frame limits can be out of sync in both directions. On 1G the MAC drops
frames above its configured limit, so this does not happen there.

> +
> +static void axienet_xxv_phylink_set_capabilities(struct axienet_local *lp,
> +						 struct phylink_config *cfg)
> +{
> +	u32 core_speed;
> +
> +	core_speed = axienet_ior(lp, XXV_STAT_CORE_SPEED_OFFSET);
> +	/* Bit[1:0]: 00=25G, 01=10G, 10=runtime-switchable 25G,
> +	 * 11=runtime-switchable 10G. Bit 0 is the active rate. Advertise
> +	 * only that rate.
> +	 */
> +	if (core_speed & XXV_STAT_CORE_SPEED_10G_MASK) {
> +		cfg->mac_capabilities |= MAC_10000FD;
> +		__set_bit(PHY_INTERFACE_MODE_10GBASER,
> +			  cfg->supported_interfaces);
> +	} else {
> +		cfg->mac_capabilities |= MAC_25000FD;
> +		__set_bit(PHY_INTERFACE_MODE_25GBASER,
> +			  cfg->supported_interfaces);
> +	}
> +}

[Severity: High]
Can the XXV interface be opened with a device tree that follows the new
binding?

The xlnx,xxv-ethernet binding comes from "dt-bindings: net:
xlnx,xxv-ethernet: Add Xilinx XXV 10G/25G Ethernet". It requires neither
phy-handle nor managed = "in-band-status", and neither of its examples has
them.

Nothing sets lp->phylink_config.default_an_inband for XXV. At the end of the
series there is no default_an_inband anywhere in drivers/net/ethernet/xilinx/.
phylink_parse_mode() therefore leaves cfg_link_an_mode as MLO_AN_PHY.

axienet_open() then calls phylink_fwnode_phy_connect(), which does:

	phy_fwnode = fwnode_get_phy_node(fwnode);
	if (IS_ERR(phy_fwnode)) {
		if (pl->cfg_link_an_mode == MLO_AN_PHY)
			return -ENODEV;
		return 0;
	}

axienet_open() passes that -ENODEV back to the caller, so the interface never
comes up. Should this function set cfg->default_an_inband = true, or should
the binding require managed = "in-band-status"?

[ ... ]

> +const struct axienet_config axienet_10g25g_config = {
> +	.sw_padding = true,
> +	.internal_pcs = true,
> +	.regs_n = XXV_REGS_N,
> +	.clk_init = axienet_10g25g_clk_init,
> +	.setoptions = axienet_xxv_setoptions,
> +	.probe_init = axienet_xxv_probe_init,
> +	.gt_reset = axienet_xxv_gt_reset,
> +	.mac_init = axienet_xxv_mac_init,
> +	.get_regs = axienet_xxv_get_regs,
> +	.phylink_set_caps = axienet_xxv_phylink_set_capabilities,
> +	.pcs_ops = &axienet_xxv_pcs_ops,
> +};

[Severity: Low]
This leaves .jumbo false, which the kernel-doc defines as "MAC supports jumbo
frames" being false. axienet_probe() still sets ndev->max_mtu = XAE_JUMBO_MTU
for every MAC. axienet_change_mtu() only checks against lp->rxmem:

	if ((new_mtu + VLAN_ETH_HLEN + XAE_TRL_SIZE) > lp->rxmem)
		return -EINVAL;

So ip link set mtu 9000 is accepted on XXV. For XXV, is the .jumbo flag
wrong, or the max_mtu? xxvenet_options also has no XAE_OPTION_JUMBO entry,
and XXV_JUM_OFFSET is never written.

[Severity: High]
Where does XXV get the RX frame length from?

.non_dmaengine is left false, so every XXV RX completion goes through
axienet_dma_rx_cb(). That callback reads the length from AXI DMA APP word 4:

	/* TODO: Derive app word index programmatically */
	rx_len = (app_metadata[LEN_APP] & 0xFFFF);

On the 1G AXI Ethernet, the MAC's RX status stream writes that word. PG210
describes the XXV IP with only AXI4-Stream data interfaces and no status
stream.

xilinx_dma only attaches metadata_ops when the DMA node has
xlnx,axistream-connected:

	if (chan->xdev->has_axistream_connected)
		desc->async_tx.metadata_ops = &xilinx_dma_metadata_ops;

Without that property, dmaengine_desc_get_metadata_ptr() fails and every
received frame is counted as rx_dropped. With it, app4 holds stale or zero BD
contents. Zero-length skbs or wrong lengths would then reach eth_type_trans()
and skb_put().

The real byte count is in the BD status length field, which xilinx_dma
reports as the residue. Does XXV need its own MAC-specific RX length source?

[Severity: Medium]
This is a pre-existing issue, but XXV now goes through the same callback.
axienet_dma_rx_cb() passes the metadata length straight to skb_put():

	rx_len = (app_metadata[LEN_APP] & 0xFFFF);
	skb_put(skb, rx_len);

The callback does not check the dmaengine result or residue. rx_len is not
bounded by lp->max_frm_size or skb_tailroom() either. Could a length larger
than the posted buffer hit skb_over_panic()? The 1G dmaengine path already
had this problem.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006110550.1990213-1-suraj.gupta2%40amd.com