Re: [PATCH net] eth: fbnic: Avoid rounding zero ring sizes

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

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

- [Medium] In fbnic_set_ringparam()
  (drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c), moving the minimum-…

--

eth: fbnic: Avoid rounding zero ring sizes

The patch moves the roundup_pow_of_two() normalization of the requested
ring sizes in fbnic_set_ringparam() to after the minimum-size checks, so
that a zero ring size coming from userspace is rejected instead of being
fed to roundup_pow_of_two().

[Severity: Medium]
With the roundup moved below the checks, the minimum-size test in
fbnic_set_ringparam() now compares the raw userspace request rather than
the rounded value.  Does this reject nonzero sizes that used to be
accepted?

The effective minimums all evaluate to 64:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.h:
    #define FBNIC_TX_DESC_MIN	roundup_pow_of_two(FBNIC_TX_DESC_WAKEUP)
    #define FBNIC_RX_DESC_MIN	roundup_pow_of_two(FBNIC_MAX_RX_PKT_DESC * 2)
    #define FBNIC_QUEUE_SIZE_MIN		64u

FBNIC_RX_DESC_MIN is 16 and FBNIC_TX_DESC_MIN is 64 for the common
MAX_SKB_FRAGS == 17, so max(FBNIC_QUEUE_SIZE_MIN, ...) is 64 in every
case.

Previously a request like "ethtool -G ethX tx 40" was rounded to 64 first
and then passed the check, ending up with a 64-entry ring.  Now the check
sees 40 and returns -EINVAL with "requested ring size too small".  The
same applies to any rx, rx-mini or rx-jumbo value in 33..63.

The ethtool core does not enforce a minimum, it only validates the maxima:

net/ethtool/rings.c:ethnl_set_rings() {
	...
	/* ensure new ring parameters are within limits */
	if (ringparam.rx_pending > ringparam.rx_max_pending)
	...
}

and ethtool_set_ringparam() in net/ethtool/ioctl.c does the same, so those
values do reach the driver unchanged.

 	/* These are absolute minimums allowing the device and driver to operate
 	 * but not necessarily guarantee reasonable performance. Settings below
 	 * Rx queue size of 128 and BDQs smaller than 64 are likely suboptimal
 	 * at best.
 	 */
 	if (ring->rx_pending < max(FBNIC_QUEUE_SIZE_MIN, FBNIC_RX_DESC_MIN) ||
 	    ring->rx_mini_pending < FBNIC_QUEUE_SIZE_MIN ||
 	    ring->rx_jumbo_pending < FBNIC_QUEUE_SIZE_MIN ||
 	    ring->tx_pending < max(FBNIC_QUEUE_SIZE_MIN, FBNIC_TX_DESC_MIN)) {
 		NL_SET_ERR_MSG_MOD(extack, "requested ring size too small");
 		return -EINVAL;
 	}

[Severity: Medium]
Was the reorder needed to reach the stated goal?  The commit message says
only:

    Validate the requested sizes before rounding them so zero is rejected
    deterministically.

It does not mention that previously accepted non-power-of-two sizes below
the 64-descriptor minimum now start failing with -EINVAL.  Since the patch
carries a Fixes: tag and is therefore a stable-backport candidate, could
the changelog spell out that user-visible change?

Would an explicit rejection of zero, or clamping in place with
max(min, roundup_pow_of_two(...)), address the undefined
roundup_pow_of_two(0) without altering which nonzero requests are
accepted?

 	if (!netif_running(netdev)) {
 		fbnic_set_rings(fbn, ring, kernel_ring);
 		return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915181229.4159158-1-bjorn%40kernel.org