Re: [PATCH net-next v4 08/15] net: ethernet: cortina: Rebuild free queue metadata for RX ring changes

[netdev-bot+sashiko@kernel.org]

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

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

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 patch fixes a regression from an earlier patch in the same
  series, but it has no Fixes: tag and does not say where that…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  gmac_set_ringparam() rounds rx_pending/tx_pending up to a power of two…

--

net: ethernet: cortina: Rebuild free queue metadata for RX ring changes

This commit adds geth_freeq_page_slots(), which computes how much free
queue metadata is needed. The geth_resize_freeq() fast path now uses it,
so an RX ring change that keeps the same free queue order no longer keeps
an undersized metadata pool. gmac_open() now fails when geth_resize_freeq()
returns any error, and gmac_set_ringparam() restores the old rxq_order if
the resize fails.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 284823bdfd50c..cd5ded7384a51 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1234,8 +1244,16 @@ static int geth_resize_freeq(struct gemini_ethernet_port *port)
>  	if (new_order <= PAGE_SHIFT - geth->freeq_frag_order)
>  		return -EINVAL;
>  
> -	if (geth->freeq_ring && geth->freeq_order == new_order)
> -		return 0;
> +	if (geth->freeq_ring) {
> +		page_slots = geth_freeq_page_slots(geth, geth->freeq_order);
> +		if (geth->num_freeq_pages >= page_slots &&
> +		    (geth->freeq_order == new_order ||
> +		     (other_running && geth->freeq_order > new_order)))
> +			return 0;
> +	}
> +
> +	if (other_running)
> +		return -EBUSY;

[Severity: Medium]
The code change looks right to me. Should this patch carry Fixes: tags,
though, or should the fast path part go into the earlier patch?

The undersized pool comes from the previous patch in this series, "net:
ethernet: cortina: Preserve in-flight free queue pages". That patch sizes
freeq_pages and the bitmaps as the free queue pages plus
(1 << port0->rxq_order) plus (1 << port1->rxq_order). But it keeps the
fast path that only checks the order:

	if (geth->freeq_ring && geth->freeq_order == new_order)
		return 0;

With only that patch applied, raising an RX ring without changing the
computed freeq order keeps the old, smaller pool. For example, take a port
whose rxq_order goes from 8 to 9 while the other port stays at 10. new_size
goes from 2560 to 3072, but new_order is 12 both times. The same thing
happens once new_order hits the cap of 15.

geth_freeq_alloc_slot() then returns -ENOSPC and geth_fill_freeq() stops
refilling early.

"Preserve in-flight free queue pages" carries a Fixes: 4d5ae32f5e1e tag,
which makes it a stable candidate. Could a stable kernel take it without
this untagged follow-up and end up with the undersized pool after
ethtool -G rx N? It also leaves a bisect window between the two patches.

The other two changes here fix behavior that goes back to the original
driver (4d5ae32f5e1e): gmac_set_ringparam() leaving the new rxq_order
installed after a failed resize, and gmac_open() accepting -EBUSY.

Could the fast path change be folded into "Preserve in-flight free queue
pages"? If not, could this patch get Fixes: tags so backporters keep the
two together?

[ ... ]

> @@ -2376,14 +2391,20 @@ static int gmac_set_ringparam(struct net_device *netdev,
>  			      struct netlink_ext_ack *extack)
>  {
>  	struct gemini_ethernet_port *port = netdev_priv(netdev);
> +	unsigned int old_rxq_order;
>  	int err = 0;
>  
>  	if (netif_running(netdev))
>  		return -EBUSY;
>  
>  	if (rp->rx_pending) {
> +		old_rxq_order = port->rxq_order;
>  		port->rxq_order = min(15, ilog2(rp->rx_pending - 1) + 1);

[Severity: Low]
This isn't a bug introduced by this patch, but gmac_set_ringparam() rounds
rx_pending up to a power of two and never writes the applied size back
into rp. include/linux/ethtool.h says:

 * @set_ringparam: Set ring sizes. The &struct ethtool_ringparam argument is
 *	also an output; drivers which normalize requested sizes must update it
 *	with the applied sizes.

After the callback returns, ethnl_set_rings() in net/ethtool/rings.c picks
up the driver's adjustments:

	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);

So a request for rx 300 stores 300 in cfg_pending, while the driver
actually uses 512 (rxq_order 9).

Should rp->rx_pending be set to 1 << port->rxq_order once the resize
succeeds? tx_pending is rounded the same way further down, so it could be
updated too.

>  		err = geth_resize_freeq(port);
> +		if (err) {
> +			port->rxq_order = old_rxq_order;
> +			return err;
> +		}
>  	}
>  	if (rp->tx_pending) {
>  		port->txq_order = min(15, ilog2(rp->tx_pending - 1) + 1);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org