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

["Gupta, Suraj" <suraj.gupta2@amd.com>]

On 10/10/2026 5:25 PM, netdev-bot+sashiko@kernel.org wrote:
> 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?

This is a pre-existing bug & can be fixed in separate series.>
> [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.
> 
>>   


Agree, will fix it>>   	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?


Setting pause configurations for XXV is not validated yet, I'll stop 
advertising pause for now and can be added later after proper validation.

> 
> [ ... ]
> 
>> +/**
>> + * 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?

Sure, will update the comment.>
> [ ... ]
> 
>> +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;
>> +}
> 

reset-done is refclk-driven, independent of link partner. block-lock 
(signal-dependent) is already non-fatal.> [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;
>> +}
> 


Agree — block-lock-not-asserted is a normal no-link state, not an error. 
Demote to netdev_dbg (not ratelimited; it's once per open, so level—not 
rate—is the issue) and reword to drop the ref-clock blame. The genuine 
netdev_err stays on the GT reset-done timeout, which does imply a 
bring-up/refclk fault."

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

I agree, will fix it.

> [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"?


I agree, will modify example to indicate inband in DT yaml& set 
cfg->default_an_inband = true in driver .>
> [ ... ]
> 
>> +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.
> 

Agree — set .jumbo=true for XXV (no reg programming needed).

> [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?
> 

Resolved by rebasing on Srinivas' residue series (97f9c11a6dd9, rx_len = 
max_frm_size - residue); note dependency.> [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.
> 


Bounded by residue series; note dependency.

Regards,
Suraj