Re: [PATCH net-next v2 4/4] net: macb: Add TSN CBS TC offload support

["Karumanchi, Vineeth" <vineeth@amd.com>]

Hi Théo,

On 9/17/2026 3:02 PM, Théo Lebrun wrote:
> Hello Vineeth,
> 
> On Wed Sep 9, 2026 at 4:20 PM CEST, Vineeth Karumanchi wrote:
>> Add Credit-Based Shaper (CBS/IEEE 802.1Qav) TC offload support for
>> time-sensitive networking on GEM hardware. CBS is restricted to the
>> two highest-priority queues: Queue A (num_queues - 1) and Queue B
>> (num_queues - 2), matching hardware capability.
>>
>> Validate that idleslope is positive and does not exceed the link
>> speed, preventing negative values from bypassing the bounds check
>> due to signed-to-unsigned promotion.
>>
>> The idle slope register value is computed differently based on hardware
>> variant:
>>
>>   High-speed GEM: scale idleslope linearly to the full 32-bit register
>>   range relative to link speed.
>>
>>   Standard MACB: convert the kbps idleslope into the register's native
>>   unit, which depends on the interface width:
>>     - 1G (8-bit GMII):    bytes/sec,   scale kbps by 1000/8 (125)
>>     - 10/100M (4-bit MII): nibbles/sec, scale kbps by 1000/4 (250)
>>
>> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
>> ---
>> Changes in v2:
>> - macb_cbs_get_queue_params() now returns the idleslope register offset
>>   (u32 *idleslope_reg) instead of a bool flag, and the idleslope is
>>   programmed via bp->macb_reg_writel(), dropping the per-queue if/else
>>   that open-coded gem_writel(CBS_IDLESLOPE_Q_A/Q_B).
>> - Expanded the idleslope kbps-to-hardware-unit conversion comment.
>> - Zero-initialize kset in macb_cbs_add() so an unpopulated link speed
>>   reads as 0 and is rejected; reorder locals to keep the declarations
>>   in reverse-christmas-tree order.
>> - Rebased on net-next, which renamed the struct net_device pointer to
>>   "netdev" (was "dev"/"ndev").
>>
>>  drivers/net/ethernet/cadence/macb.h      |   9 ++
>>  drivers/net/ethernet/cadence/macb_main.c | 116 +++++++++++++++++++++++
>>  2 files changed, 125 insertions(+)
>>
> [...]
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 67150ff03066..00c1c619dea8 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> +static int macb_cbs_add(struct net_device *netdev,
>> +			struct tc_cbs_qopt_offload *qopt)
>> +{
> [...]
>> +	scoped_guard(spinlock_irqsave, &bp->lock) {
>> +		/* Disable CBS for the queue before updating idleslope */
>> +		ctrl = gem_readl(bp, CBS_CONTROL) & ~enable_bit;
>> +		gem_writel(bp, CBS_CONTROL, ctrl);
>> +		/* Update idleslope for the queue */
>> +		bp->macb_reg_writel(bp, idleslope_reg, idleslope);
>> +		/* Re-enable CBS for the queue with new idleslope */
>> +		gem_writel(bp, CBS_CONTROL, ctrl | enable_bit);
>> +	}
>> +
>> +	netdev_dbg(netdev, "CBS: Configured queue %d with idleslope 0x%x\n",
>> +		   qopt->queue, idleslope);
>> +
>> +	return 0;
>> +}
>> +
>> +static void macb_cbs_destroy(struct net_device *netdev, u8 queue_num)
>> +{
>> +	struct macb *bp = netdev_priv(netdev);
>> +	u32 enable_bit, idleslope_reg;
>> +
>> +	if (macb_cbs_get_queue_params(bp, queue_num, &enable_bit, &idleslope_reg))
>> +		return;
>> +
>> +	scoped_guard(spinlock_irqsave, &bp->lock) {
>> +		gem_writel(bp, CBS_CONTROL, gem_readl(bp, CBS_CONTROL) & ~enable_bit);
>> +		bp->macb_reg_writel(bp, idleslope_reg, 0);
>> +	}
>> +
>> +	netdev_dbg(netdev, "CBS: Disabled queue %d\n", queue_num);
>> +}
> 
> So we write cbs_control/0x04BC only on ndo_setup_tc(TC_SETUP_QDISC_CBS)
> callback. As Sashiko pointed how I think that causes an issue with HW
> resets & suspend:
>  - what if configured then link down then link up?
>  - what if configured before link up?
> 
> Also when not using CBS we hope tx_sched_ctrl/0x0580 is at its reset
> value (0b00, fixed priority) for all queues. I would expect an
> unconditional writel tx_sched_ctrl/0x0580 in macb_init_hw(). This
> solves the down/up issue, the speed change and also ensures we don't
> inherit CBS or DWRR from the boot stages (super unlikely though).
> 
> Also, it might be simpler to always use tx_sched_ctrl/0x0580 rather than
> sometimes tx_sched_ctrl/0x0580 and sometimes cbs_control/0x04BC, which
> are aliases.
> 
> What do you think?
> 

Sorry for the delayed response.

I tested the current implementation in the following four scenarios:

1. Configuring CBS while the link is down: CBS cannot be programmed
because the link speed is unknown.
2. Link down followed by link up: The hardware retains the CBS register
values, and CBS is effective again after the link comes up at the same
speed.

3. Suspend/resume: The CBS register contents are lost after resume.
However, the CBS child qdisc remains installed in the kernel and is
still shown by tc as offloaded, leaving the software configuration
inconsistent with the hardware state.

4. Link-speed change: The CBS register values are retained and shaping
remains enabled. However, those values were calculated for the previous
speed, so the resulting shaping bandwidth is incorrect.

I see two possible approaches:

- Retain the requested CBS parameters in the driver and restore the
hardware configuration, following the saved-state approach used by IGB.
For MACB, we would also recalculate the hardware idleslope on link-up
using the negotiated speed.
If the saved request cannot be supported at the new speed, retain the
request, leave CBS disabled for that queue, and warn the user that the
configuration is invalid for the new link speed.

- Explicitly reset the scheduling configuration in macb_init_hw() and
require users to remove and recreate the CBS qdisc. This establishes a
known hardware state, but leaves the installed qdisc inconsistent until
users reconfigure it. It would also need handling for speed changes that
do not invoke macb_init_hw().

Please let me know your thoughts on these approaches.

Thanks,

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

-- 
🙏 Vineeth