Re: [PATCH net-next v4 1/6] net: xilinx: axienet: Introduce axienet_config for MAC-specific ops

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 (1) · High: 0 · Medium: 1 (3) · Low: 5

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).

- [Medium] The commit message says "No functional change intended for the
  existing 1G path", but the patch changes 1G behaviour in…
- [Low] axienet_dma_err_handler() ignores the return value of the new
  int-returning mac_init callback, then re-enables TX/RX through…
- [Low] The new axienet_config::jumbo flag is documented as "MAC supports
  jumbo frames", but only axienet_device_reset() reads it, and only to…
- [Low] Mismatches between the kernel-doc and the code for the optional
  members of struct axienet_config.
- [Low] The sw_padding and tx_csum capability flags are honoured in
  axienet_start_xmit_dmaengine() but not in the legacy…
- [Low] On the non-dmaengine path, axienet_device_reset() returns early
  when mac_init() fails, after axienet_dma_bd_init() has already…

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but
  axienet_start_xmit_dmaengine() increments lp->tx_ring_head before…
- [Medium] xlnx,axi-ethernet.yaml lists the interrupt order as Ethernet
  core, Tx DMA, Rx DMA.
- [Medium] axienet_open() discards the return value of
  axienet_device_reset() by overwriting ret with…
- [Medium] On the legacy-DMA path, the BD rings and mapped RX skbs
  allocated by axienet_device_reset() are leaked when axienet_open()…

--

net: xilinx: axienet: Introduce axienet_config for MAC-specific ops

This commit adds struct axienet_config, which holds capability flags and
per-MAC callbacks and is filled in from the OF match table. The shared
axienet paths now use it instead of open-coded 1G behaviour, to prepare
for more MAC types. The commit says no functional change is intended for
the existing 1G path.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index ae8ed4495ea0e..5af48d5cbed3b 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c

[ ... ]

> @@ -711,19 +744,23 @@ static void axienet_dma_stop(struct axienet_local *lp)
>   */
>  static int axienet_device_reset(struct net_device *ndev)
>  {

[ ... ]

>  	if (ndev->mtu > XAE_MTU && ndev->mtu <= XAE_JUMBO_MTU) {
>  		lp->max_frm_size = ndev->mtu + VLAN_ETH_HLEN +
>  					XAE_TRL_SIZE;
>  
> -		if (lp->max_frm_size <= lp->rxmem)
> +		if (lp->max_frm_size <= lp->rxmem && lp->axienet_config->jumbo)
>  			lp->options |= XAE_OPTION_JUMBO;
>  	}

[Severity: Low]
Should the new jumbo flag also limit the MTU range? The kernel-doc says
@jumbo means "MAC supports jumbo frames", but this is the only place the
flag is read. The MTU limits do not check it:

axienet_probe() {
    ...
	ndev->max_mtu = XAE_JUMBO_MTU;
    ...
}

axienet_change_mtu() {
    ...
	if ((new_mtu + VLAN_ETH_HLEN +
		XAE_TRL_SIZE) > lp->rxmem)
		return -EINVAL;
    ...
}

So a MAC with .jumbo = false would still accept an MTU of up to 9000,
limited only by xlnx,rxmem. axienet_device_reset() would then raise
max_frm_size to mtu + VLAN_ETH_HLEN + XAE_TRL_SIZE without setting
XAE_OPTION_JUMBO.

Later in the series, axienet_10g25g_config leaves .jumbo unset but still
gets a max_mtu of 9000.

Is the @jumbo description wrong for XXV, or should max_mtu and
axienet_change_mtu() check .jumbo?

[ ... ]

> @@ -740,26 +777,18 @@ static int axienet_device_reset(struct net_device *ndev)
>  		}
>  	}
>  

[ ... ]

> +	ret = lp->axienet_config->mac_init(ndev);
> +	if (ret)
> +		return ret;

[Severity: Low]
On the non-dmaengine path, axienet_dma_bd_init() has already allocated
the BD rings, mapped the RX skbs and started the DMA by the time this
early return runs. If mac_init() fails, does this leak the BD rings and
the mapped RX skbs?

None of the configs in this series can hit this. Only axienet_1g_config
allows non-dmaengine mode, and axienet_1g_mac_init() always returns 0. A
future MAC with both non_dmaengine set and a mac_init() that can fail
would hit it.

[Severity: Medium]
This is a pre-existing issue, but at this point in the series callers of
axienet_open() never see the new mac_init() error:

axienet_open() {
    ...
	ret = axienet_device_reset(ndev);
	axienet_unlock_mii(lp);

	ret = phylink_fwnode_phy_connect(lp->phylink, dev_fwnode(lp->dev), 0);
    ...
}

The result of axienet_device_reset() is overwritten before anything
checks it. axienet_1g_mac_init() always returns 0, so this patch adds no
new failure.

The later patch "net: xilinx: axienet: Add 10G/25G (XXV) ethernet
support" in this series adds the missing check.

[Severity: Medium]
This isn't a bug introduced by this patch, but on the legacy DMA path,
does axienet_open() leak the BD rings and mapped RX skbs that
axienet_device_reset() allocated if a later step fails?

A phylink_fwnode_phy_connect() failure returns straight away:

	ret = phylink_fwnode_phy_connect(lp->phylink, dev_fwnode(lp->dev), 0);
	if (ret) {
		dev_err(lp->dev, "phylink_of_phy_connect() failed: %d\n", ret);
		return ret;
	}

The err_phy unwind, used for the request_irq() failures in
axienet_init_legacy_dma(), also calls neither axienet_dma_stop() nor
axienet_dma_bd_release():

err_phy:
	cancel_work_sync(&lp->rx_dim.work);
	cancel_delayed_work_sync(&lp->stats_work);
	phylink_stop(lp->phylink);
	phylink_disconnect_phy(lp->phylink);
	return ret;

ndo_stop is not called after ndo_open fails, and the next open
overwrites tx_bd_v and rx_bd_v.

[ ... ]

> @@ -941,19 +977,21 @@ axienet_start_xmit_dmaengine(struct sk_buff *skb, struct net_device *ndev)
>  		goto xmit_error_drop_skb;
>  
>  	/* Fill up app fields for checksum */

[ ... ]

[Severity: Critical]
This is a pre-existing issue and was not introduced by this patch. Can
the error paths in axienet_start_xmit_dmaengine() knock the TX ring out
of sync? The producer index moves forward before the steps that can
fail:

	lp->tx_ring_head++;
	sg_init_table(skbuf_dma->sgl, sg_len);
	ret = skb_to_sgvec(skb, skbuf_dma->sgl, 0, skb->len);
	if (ret < 0)
		goto xmit_error_drop_skb;

Neither xmit_error_unmap_sg nor xmit_error_drop_skb rolls it back. The
skipped slot has no DMA transaction behind it. Its skb and sg_len still
hold the previous lap's values, or NULL on the first lap.

axienet_dma_tx_cb() consumes slots strictly in order:

	skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_tail++);
	len = skbuf_dma->skb->len;
	...
	dev_consume_skb_any(skbuf_dma->skb);

So the next completion would process the skipped slot. On the first lap
that is a NULL pointer dereference. On later laps it calls dma_unmap_sg()
on an sgl that is already unmapped, reports a stale length to BQL, and
frees an skb that has already been freed. The off-by-one then stays for
the rest of the ring's lifetime.

Normal load seems enough to trigger this. xilinx_dma_prep_slave_sg()
returns NULL when xilinx_axidma_alloc_tx_segment() runs out of its
XILINX_DMA_NUM_DESCS (512) segments, while the axienet ring allows up to
TX_BD_NUM_MAX (4096) entries in flight. dma_map_sg() can also fail when
swiotlb or IOMMU space runs out.

The problem is still there at the end of the series.

[ ... ]

> @@ -2759,29 +2819,114 @@ static void axienet_dma_err_handler(struct work_struct *work)
>  
>  	axienet_dma_start(lp);
>  

[ ... ]

> +	lp->axienet_config->mac_init(ndev);

[Severity: Low]
Should the return value of mac_init() be checked here, as it is in
axienet_device_reset()? The handler re-enables TX and RX through
setoptions(ndev, lp->options) either way.

This handler only runs on the non-dmaengine path, which only the 1G
config can use, and axienet_1g_mac_init() always returns 0. The later XXV
patch in this series adds a comment at this call site saying so. It may
be clearer to add that comment here, where mac_init() first gets an int
return type.

[ ... ]

> @@ -2943,7 +3075,17 @@ static int axienet_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	if (lp->axienet_config->probe_init)
> +		lp->axienet_config->probe_init(lp);
> +
>  	if (!of_property_present(pdev->dev.of_node, "dmas")) {
> +		/* Non-dmaengine mode is only supported by some MACs */
> +		if (!lp->axienet_config->non_dmaengine) {
> +			dev_err(&pdev->dev,
> +				"Non-dmaengine mode not supported by this MAC, use dmaengine\n");
> +			return -EINVAL;
> +		}
> +

[Severity: Low]
This isn't a bug, but some of the new capability flags only take effect
on the dmaengine paths:

- sw_padding and tx_csum are checked in axienet_start_xmit_dmaengine()
  but not in axienet_start_xmit().
- mac_irq is checked in the dmaengine probe branch but not in this
  non-dmaengine branch.

Because of this gate, only axienet_1g_config reaches those paths today,
and its flags match what the legacy code does unconditionally. A future
config with non_dmaengine set and different flags would quietly get the
1G behaviour.

>  		/* Find the DMA node, map the DMA registers, and decode the DMA IRQs */
>  		np = of_parse_phandle(pdev->dev.of_node, "axistream-connected", 0);
>  

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch. The
interrupt order in Documentation/devicetree/bindings/net/xlnx,axi-ethernet.yaml
doesn't seem to match what this branch reads when the DMA resources are
on the Ethernet node.

The binding lists Ethernet core, Tx DMA, Rx DMA. The driver does:

			lp->rx_irq = platform_get_irq(pdev, 1);
			lp->tx_irq = platform_get_irq(pdev, 0);
			lp->eth_irq = platform_get_irq_optional(pdev, 2);

Would a device tree written to match the binding text end up with its
IRQs misrouted?

[ ... ]

> @@ -3109,23 +3256,21 @@ static int axienet_probe(struct platform_device *pdev)
>  			goto cleanup_mdio;
>  		}
>  		of_node_put(np);
> -		lp->pcs.ops = &axienet_pcs_ops;
> +		lp->pcs.ops = lp->axienet_config->pcs_ops;
> +		lp->pcs.poll = true;
> +	}
> +
> +	if (lp->axienet_config->internal_pcs) {
> +		lp->pcs.ops = lp->axienet_config->pcs_ops;
>  		lp->pcs.poll = true;
>  	}

[Severity: Low]
This isn't a bug, but a few kernel-doc entries for struct axienet_config
don't quite match how the members are used:

- @pcs_ops is documented as "or NULL if unused", but it is assigned to
  lp->pcs.ops here without a NULL check. Every config in the series sets
  it, so nothing breaks today.
- @stats_update is NULL-checked in axienet_refresh_stats() but isn't
  documented as optional.
- @internal_pcs says "MAC always drives an internal PCS", but
  axienet_mac_select_pcs() picks the PCS by interface mode only. By the
  end of the series it also handles 10GBASER and 25GBASER, which covers
  XXV.

[ ... ]

> @@ -3168,7 +3313,8 @@ static void axienet_remove(struct platform_device *pdev)
>  	if (lp->pcs_phy)
>  		put_device(&lp->pcs_phy->dev);
>  
> -	axienet_mdio_teardown(lp);
> +	if (lp->mii_bus)
> +		axienet_mdio_teardown(lp);
>  }

[Severity: Medium]
The commit message says:

  No functional change intended for the existing 1G path.

This hunk does change 1G remove behaviour, though. Before the patch, a
failed axienet_mdio_setup() left lp->mii_bus NULL:

drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c:axienet_mdio_setup() {
    ...
unregister:
	of_node_put(mdio_node);
	mdiobus_free(bus);
	lp->mii_bus = NULL;
	return ret;
}

axienet_probe() only warns and carries on. A 1G device using a
fixed-link, or a PHY on another bus, could therefore probe without an
MDIO bus. Unbinding it then went through:

axienet_remove()->axienet_mdio_teardown()->mdiobus_unregister(NULL)

and mdiobus_unregister() dereferences bus->state.

So the new check fixes a NULL pointer dereference. Could it be split out
as a separate fix for net with a Fixes: tag, so it can reach stable, and
the commit message here updated?

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