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

[Kevin Hao <haokexin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2416 bytes --]

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

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]