Re: [PATCH net-next v4 08/15] net: ethernet: cortina: Rebuild free queue metadata for RX ring changes
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-12 18:45:35
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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? [ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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