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

netdev-bot+sashiko@kernel.org writes:

quoted

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index 0e47088ec44ba..8954fcd08cb31 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c

@@ -334,11 +334,6 @@ fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
 	struct fbnic_net *clone;
 	int err;
 
-	ring->rx_pending	= roundup_pow_of_two(ring->rx_pending);
-	ring->rx_mini_pending	= roundup_pow_of_two(ring->rx_mini_pending);
-	ring->rx_jumbo_pending	= roundup_pow_of_two(ring->rx_jumbo_pending);
-	ring->tx_pending	= roundup_pow_of_two(ring->tx_pending);
-
 	/* 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

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

Indeed. V2!

Björn