Re: [PATCH net-next 2/4] net: macb: Consolidate MACB_CAPS_ISR_CLEAR_ON_WRITE checks in IRQ handler

[Nicolai Buchwitz <nb@tipi-net.de>]

On 1.4.2026 11:30, Kevin Hao wrote:
>> On Sat, 28 Mar 2026 18:17:46 +0800 Kevin Hao wrote:
> On Tue, Mar 31, 2026 at 07:54:00PM -0700, Jakub Kicinski wrote:
>> > Currently, the MACB_CAPS_ISR_CLEAR_ON_WRITE flag is checked in every
>> > branch of the IRQ handler. This repeated evaluation is unnecessary.
>> > By consolidating the flag check, we eliminate redundant loads of
>> > bp->caps when TX and RX events occur simultaneously, a common scenario
>> > under high network throughput. Additionally, this optimization reduces
>> > the function size from 0x2e8 to 0x2c4.
>> 
>> feels a bit subjective TBH. An alternative improvement would be to
>> factor out the conditional to a helper:
>> 
>> static void macb_queue_isr_clear(bp, queue, mask)
>> {
>> 	if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>> 		queue_writel(queue, ISR, mask);
>> }
> 
> In addition to the similar pattern in the macb_interrupt() function, 
> there are
> seven other instances of this pattern in the macb driver.
>   $ git grep "if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)" 
> drivers/net/ethernet/cadence/ | wc -l
>   7
> 
> I agree that using a helper function, as you proposed, would reduce the 
> number
> of source code lines and improve the readability of the driver. 
> However, such
> changes would not affect the final generated code.
> 
> The goal of my changes is to reduce both the footprint of 
> macb_interrupt() and
> the number of assembly instructions executed within its event loop.
> 
> Before the patch, the condition `if (bp->caps & 
> MACB_CAPS_ISR_CLEAR_ON_WRITE)`
> produced the following assembly:
>   ffff800080d58b1c:       387b6a68        ldrb    w8, [x19, x27]
>   ffff800080d58b20:       360000c8        tbz     w8, #0, 
> ffff800080d58b38 <macb_interrupt+0xd0>
> 
> After the patch, the condition `if (isr_clear)` results in:
>   ffff800080d58a4c:       360000d8        tbz     w24, #0, 
> ffff800080d58a64 <macb_interrupt+0xac>
> 
> Thus, we eliminate two `ldrb` overheads per iteration of the event loop 
> in
> macb_interrupt() when both TX and RX events occur simultaneously. This 
> also
> reduces the function's footprint by 36 bytes, as the `ldrb` 
> instructions are
> omitted in each event branch.
> 
> Therefore, your proposed changes and mine serve different purposes.
> I acknowledge that my change represents only a very slight optimization 
> for
> the interrupt handler. I am also open to your preference for improving 
> source
> code readability. Please let me know your decision.

I'm always a fan of optimizations, but I guess in this case the
saved ldrb is negligible next to the MMIO in the same path. We're
talking a single L1 cache hit (~1ns) vs an uncacheable register
write (~100ns+). Patch 3 also re-reads bp->caps in
macb_interrupt_misc() anyway, undoing the local bool for the misc
path.

I'd ack Jakub's helper approach. A macb_queue_isr_clear() would
be consistent across all callsites, including the 7 other
instances you counted outside macb_interrupt().

> Thanks,
> Kevin

Nicolai