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

[Björn Töpel]

netdev-bot+sashiko@xxxxxxxxxx writes:

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