Re: [PATCH net-next v5 2/3] net: macb: implement ethtool_ops.get|set_channels()

["Théo Lebrun" <theo.lebrun@bootlin.com>]

On Mon Oct 19, 2026 at 2:52 PM CEST, Nicolai Buchwitz wrote:
> On 16.10.2026 18:58, Théo Lebrun wrote:
>> bp->num_queues is the total number of queues and is constant from 
>> probe.
>> Introduce bp->max_num_queues which takes the current role of
>> bp->num_queues and allow `0 < bp->num_queues <= bp->max_num_queues`.
>> MACB/GEM does not know about Rx/Tx specific queues; it only has
>> combined queues.
>> 
>> set_channels is reserved to devices with MACB_CAPS_QUEUE_DISABLE.
>> The tieoff workaround would not work as packets would still be routed
>> into queues with a tieoff descriptor.
>> 
>> Implement .set_channels() operation by refusing if netif_running(). We
>> need context-swapping support upstream for proper on-the-fly channel
>> count change.
>> 
>> Note we refuse the operation if any TAPRIO rules are in place on queues
>> that would get disabled. If we allow the operation to take place, then
>> a transmission window would be reserved but unusable.
>> 
>> Nit: fix an alignment issue inside gem_ethtool_ops which does not
>> deserve its own patch.
>> 
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>
>> [...]
>
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
>> [...]
>
>> +static int macb_set_channels(struct net_device *netdev,
>> +			     struct ethtool_channels *ch)
>> +{
>> +	struct macb *bp = netdev_priv(netdev);
>> +	unsigned int count = ch->combined_count;
>> +	int ret;
>> +
>> +	/* MACB_CAPS_QUEUE_DISABLE means that the field QUEUE_DISABLE/BIT0 in
>> +	 * the per-queue RBQP/TBQP registers disable Tx/Rx queues. If we 
>> don't
>> +	 * have that capability we can have multiple queues but we must 
>> always
>> +	 * run with all enabled.
>> +	 */
>> +	if (!(bp->caps & MACB_CAPS_QUEUE_DISABLE))
>> +		return -EOPNOTSUPP;
>> +
>> +	/* TODO: reuse context swapping once it is merged. */
>
> Once context swapping is merged, this needs to be addressed in v6 ;-)

Done! The patch changed a lot, for the better. num_queues is now part of
the context, as any context "parameter" should be. It had no place in
the global interface struct.

This highlights a few callbacks that are reading num_queues while
interface might be down. I've not attempted to fix those yet. :-)

https://lore.kernel.org/netdev/20261113-macb-set-channels-v6-0-5de59d9e7f02@bootlin.com/

>
>> +	if (netif_running(bp->netdev))
>> +		return -EBUSY;
>> +
>> +	/* If we allow disable a queue which has TAPRIO active, we'll keep a
>> +	 * transmission window which will be unusable.
>> +	 */
>> +	if (bp->taprio_gated_queues >> count) {
>> +		netdev_err(bp->netdev, "TAPRIO schedule gates queues >= %u; destroy 
>> the qdisc before reducing channels\n",
>> +			   count);
>> +		return -EBUSY;
>> +	}
>
> I think this can get stuck:
>
> - delete the  taprio qdisc after the link has been down for a while
> - device is is runtime suspend
> - taprio setup bails out before _destroy() is called
> - ethtool -L keeps failing with -EBUSY, even though there is no qdisc 
> anymore
>
> Something like:
>
>    ip link set eth0 up
>    tc qdisc replace dev eth0 root taprio ... flags 0x2
>    ip link set eth0 down
>    sleep 1
>    tc qdisc del dev eth0 root
>    ethtool -L eth0 combined 1

Good catch. I was able to reproduce this. Fixed by running

   // macb_taprio_destroy()
   netdev_reset_tc(netdev);
   bp->taprio_gated_queues = 0;

Even if the interface is down.
We skip over the hardware reconfig however.

Thanks for the review,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com