Re: [net-next,v6,15/15] ibmveth: Complete set_channels down-path and mq_fallback max_rx cap

[mingming cao <mmc@linux.ibm.com>]

Thanks for the review!

On 9/3/26 11:10 AM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 3
>
> - [Medium] Advertised RX channel range does not match the enforced range
>    once `mq_fallback` latches.
> - [Low] In `ibmveth_set_channels()` the comment above the RX rollback
>    describes code that no longer exists: it claims…
> - [Low] The new up-path TX-failure rollback in `ibmveth_set_channels()`
>    calls `ibmveth_resize_rx_queues_incremental(adapter, old_rx,…
> - [Low] `adapter->mq_fallback` is written with a plain store from
>    softirq/NAPI context — `ibmveth_replenish_task()` does…
>
> --
>
> ibmveth: Complete set_channels down-path and mq_fallback max_rx cap
>
> This completes the ethtool -L path in ibmveth: the down-path publishes the
> desired RX count without allocating, the up-path resizes RX then TX and
> rolls RX back if TX cannot reach the requested count, and get_channels()
> now caps max_rx at the live rx_count once mq_fallback is set.
> ibmveth_poll_controller() also returns early when the adapter is closed.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 5aef8a1f2c23d..4cd00ff3d43e4 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -3156,15 +3156,24 @@ static void ibmveth_get_channels(struct net_device *netdev,
>>   				 struct ethtool_channels *channels)
>>   {
>>   	struct ibmveth_adapter *adapter = netdev_priv(netdev);
>> +	unsigned int rx_count = ibmveth_get_num_rx_queues(adapter);
>>   
>>   	channels->max_tx = ibmveth_real_max_tx_queues();
>>   	channels->tx_count = netdev->real_num_tx_queues;
>>   
>> -	if (adapter->multi_queue)
>> +	/*
>> +	 * Always report the live RX count. ethtool -L is read-modify-
>> +	 * write, so a TX-only request echoes rx_count back at us; an
>> +	 * understated value would be applied as a silent RX shrink.
>> +	 * mq_fallback instead caps max_rx at the live count, which
>> +	 * blocks growth in the core without misreporting what is
>> +	 * currently configured.
>> +	 */
>> +	channels->rx_count = rx_count;
>> +	if (adapter->multi_queue && !adapter->mq_fallback)
>>   		channels->max_rx = IBMVETH_MAX_RX_QUEUES;
>>   	else
>> -		channels->max_rx = 1;
>> -	channels->rx_count = ibmveth_get_num_rx_queues(adapter);
>> +		channels->max_rx = rx_count;
>>   }
> [Severity: Medium]
> Once mq_fallback is set, does the advertised max_rx still match what the
> driver will accept?
>
> Here max_rx becomes the live rx_count, which can be 8 or 16.  The core only
> bounds the request by that value, in ethtool_set_channels():
>
>      if (channels.rx_count > curr.max_rx ||
>          channels.tx_count > curr.max_tx ||
>
> So every value in 1..rx_count reaches the driver.  But
> ibmveth_resize_rx_channels() refuses any changed count above 1 in that
> state, after the no-op shortcut:
>
>      if (goal_rx == old_rx)
>              return 0;
>      ...
>      if (goal_rx > 1 && (!adapter->multi_queue || adapter->mq_fallback)) {
>              netdev_err(netdev,
>                         "Cannot resize to %u RX queues: multi-queue mode not supported by firmware\n",
>                         goal_rx);
>              return -EOPNOTSUPP;
>      }
>
> With mq_fallback set and a live count of 8, ethtool -l reports max_rx 8 while
> ethtool -L ethX rx 2 through rx 7 all fail with -EOPNOTSUPP; only 1 and the
> current count are accepted.
>
> This state looks persistent rather than transient, since
> ibmveth_register_rx_queues() latches the flag and then fails the open:
>
>      if (rc) {
>              /* Firmware MQ gone: fall back to SQ on next open. */
>              if (rc == -EOPNOTSUPP)
>                      adapter->mq_fallback = true;
>              goto err_unregister;
>      }
>
> adapter->opened stays false and num_rx_queues keeps its old value above 1
> until the next successful open.
>
> Would it be clearer to advertise max_rx = 1 in the fallback state and keep
> the no-op shortcut for the read-modify-write case?  The kernel-doc for
> ibmveth_resize_rx_channels() ("Rejects rx > 1 without MQ firmware
> (-EOPNOTSUPP)") also does not mention that the no-op shortcut precedes the
> gate.
get_channels reports the live count and
caps max_rx at that count once mq_fallback.
Advertising max_rx = 1 while rx_count is
still live fails the core (rx_count >
max_rx) and blocks a TX-only ethtool -L.
Clamping rx_count would shrink RX. rx > 1
is -EOPNOTSUPP except the current-count
no-op.
> [Severity: Low]
> Is the read of adapter->mq_fallback here synchronized against its writer?
>
> The flag is stored from softirq/NAPI context in ibmveth_replenish_task(),
> after the replenish_lock has already been dropped:
>
>      spin_unlock_irqrestore(&rxq->replenish_lock, flags);
>      ...
>              adapter->mq_fallback = true;
>              schedule_work(&adapter->work);
>
> The new reader added here, and the capability gate in
> ibmveth_resize_rx_channels():
>
>      if (goal_rx > 1 && (!adapter->multi_queue || adapter->mq_fallback)) {
>
> run under RTNL / the netdev ops lock, which does not exclude the softirq
> writer.  There is no lock, no READ_ONCE()/WRITE_ONCE() and no acquire/release
> pairing on this field, while the sibling field num_rx_queues in the same
> struct is deliberately published with:
>
>      smp_store_release(&adapter->num_rx_queues, num);
>
> A stale false read here would advertise max_rx = IBMVETH_MAX_RX_QUEUES right
> after firmware refused MQ buffer adds, and a stale read in the gate would let
> the driver allocate and replenish RX queues on that firmware, re-latching
> mq_fallback and queueing another reset.  Would WRITE_ONCE()/READ_ONCE() on
> mq_fallback, or folding it into the existing publish/consume helpers, be
> appropriate?
No — RTNL does not exclude the NAPI store, so
this is an unannotated race. v7 keeps it as a
benign sticky latch: a stale false is one extra
MQ attempt, then the flag latches again.
WRITE_ONCE on the store without READ_ONCE on
apply / get_channels / set_channels is only half
a pass, so I left it plain. Happy to annotate
the store and all three readers together if you
would prefer that spelled out.
>> @@ -3233,28 +3242,83 @@ static int ibmveth_set_channels(struct net_device *netdev,
>>   				struct ethtool_channels *channels)
>>   {
>>   	struct ibmveth_adapter *adapter = netdev_priv(netdev);
>> -	unsigned int old = netdev->real_num_tx_queues,
>> -		     goal = channels->tx_count;
>> +	unsigned int old_rx = ibmveth_get_num_rx_queues(adapter);
>> +	unsigned int goal_rx = channels->rx_count;
>> +	unsigned int old_tx = netdev->real_num_tx_queues;
>> +	unsigned int goal_tx = channels->tx_count;
>> +	unsigned int want_tx = goal_tx;
>> +	bool rx_changed = false;
>>   	int rc, i;
>>   
>> -	/* Validate RX (and resize when opened) before the down-path
>> -	 * early return so MQ/range errors are reported here. Publishing
>> -	 * the desired RX count and CMO while down is the next patch.
>> -	 */
>> -	rc = ibmveth_resize_rx_channels(adapter, channels->rx_count);
>> +	if (goal_tx < 1 || goal_tx > ibmveth_real_max_tx_queues()) {
>> +		netdev_err(netdev,
>> +			   "Invalid TX queue count %u (must be 1-%u)\n",
>> +			   goal_tx, ibmveth_real_max_tx_queues());
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* RX range / MQ checks live in ibmveth_resize_rx_channels(). */
>> +	rc = ibmveth_resize_rx_channels(adapter, goal_rx);
>>   	if (rc)
>>   		return rc;
> [ ... down-path publish/rollback block snipped ... ]
>
>> -	return rc;
>> +	if (netdev->real_num_tx_queues != want_tx) {
>> +		if (rx_changed) {
>> +			/*
>> +			 * Only meaningful once RX is live. num_slots is
>> +			 * embedded in the adapter and outlives the DMA ring,
>> +			 * so reading it at function entry is safe but can
>> +			 * return a stale geometry from before the resize.
>> +			 */
>> +			int rxq_entries = adapter->rx_queue[0].num_slots;
> [Severity: Low]
> This isn't a bug, but does this comment still describe the code below it?
>
> It says num_slots is read "at function entry" and "can return a stale
> geometry from before the resize", yet the read it annotates happens right
> here, inside the if (rx_changed) block, after ibmveth_resize_rx_channels()
> has already changed the RX geometry.  Function entry only reads
> num_rx_queues, real_num_tx_queues, channels->rx_count and channels->tx_count.
>
> The comment looks like a leftover from the v5 layout mentioned in the
> changelog, and now warns about a hazard that would only exist if the read
> were moved back to function entry.
The comment was leftover from the earlier
layout. The read is after the RX resize.
v7 replaces it to match that restore.
>> +			int rb;
>> +
>> +			rb = ibmveth_resize_rx_queues_incremental(adapter,
>> +								  old_rx,
>> +								  rxq_entries);
>> +			if (rb)
>> +				netdev_err(netdev,
>> +					   "Failed to roll back RX queues to %u after TX failure: %d\n",
>> +					   old_rx, rb);
>> +		}
>> +		return rc ? rc : -ENOMEM;
>> +	}
> [Severity: Low]
> Can this rollback re-attempt a queue geometry that the forward path would
> have refused?
>
> The rollback calls ibmveth_resize_rx_queues_incremental() directly, so the
> multi_queue / mq_fallback gate in ibmveth_resize_rx_channels() is not
> consulted.  That gate only rejects goal_rx > 1:
>
>      if (goal_rx > 1 && (!adapter->multi_queue || adapter->mq_fallback)) {
>              ...
>              return -EOPNOTSUPP;
>      }
>
> Consider adapter->opened true, mq_fallback already latched from softirq in
> ibmveth_replenish_task():
>
>      adapter->mq_fallback = true;
>      schedule_work(&adapter->work);
>
> and a live RX count of 8.  A request of rx 1 with a larger tx passes the gate
> (goal_rx is 1), RX shrinks 8 -> 1 and rx_changed becomes true.  If the TX
> step then fails in ibmveth_allocate_tx_ltb() or
> netif_set_real_num_tx_queues(), the rollback runs the scale-up path back to
> old_rx = 8 on firmware that has already refused MQ buffer adds, so
> H_REG_LOGICAL_LAN_QUEUE / replenish hit the same H_FUNCTION, mq_fallback is
> re-latched and another schedule_work(&adapter->work) reset is queued from an
> ethtool error path.
>
> Would it be better to route the rollback through
> ibmveth_resize_rx_channels(), or to skip it when mq_fallback is set and leave
> RX at 1?
>
> [ ... ]
The rollback restores the count we just
left. If firmware already refused MQ, that
scale-up can fail and latch again. !opened
is the documented down path and does not
allocate.

Thanks,
Mingming