Re: [PATCH net 1/4] net: airoha: handle RX_NO_CPU_DSCP interrupt, not just RX_DONE

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> The QDMA hardware raises a dedicated interrupt (NO_CPU_DSCP, one bit
> per ring in QDMA_CSR_INT_ENABLE2/3) when an RX ring runs out of free
> CPU descriptors. airoha_qdma_hw_init() already unmasks this interrupt
> for every ring (INT_RX1_MASK()/INT_RX2_MASK() OR it together with the
> RX_DONE bits before writing the enable register), but
> airoha_irq_handler() only ever extracts the RX_DONE bits from the same
> status word - the NO_CPU_DSCP bits are read and acknowledged (cleared)
> along with everything else at the top of the handler, then silently
> dropped.
> 
> This matters because once a ring is genuinely drained to zero posted
> descriptors, no further RX_DONE interrupt can fire for it: hardware
> has nothing left to receive a frame into, so NAPI is never rescheduled
> and airoha_qdma_fill_rx_queue() (which reposts descriptors) is never
> called again. The ring is stuck until the interface is brought down
> and back up.
> 
> This is most visible on rings that carry low, bursty volumes of
> protocol control traffic, in particular RX ring 4, to which
> airoha_fe_vip_setup() force-routes ~15 unrelated VIP-classified
> protocols (BOOTP, PPPoE Discovery, ISAKMP, DHCPv6, SIP, LLDP, PPP
> LCP/IPCP/CHAP/PAP/IPv6CP, ...) via PATN_FCPU_EN_MASK, all sharing the
> same RX_DSCP_NUM() default of 16 descriptors. A short burst on that
> ring (e.g. a DHCP lease renewal exchange, or the LCP/IPCP/CHAP/PAP
> negotiation that follows a PPPoE PADO) can drain it faster than the
> CPU reposts descriptors, after which every one of those protocols
> silently stops being received on that device until it is reconfigured
> - with no error, warning, or netdev/ethtool counter indicating why.
> 
> Fix airoha_irq_handler() to treat NO_CPU_DSCP the same as RX_DONE for
> the purpose of scheduling NAPI: airoha_qdma_rx_process() already calls
> airoha_qdma_fill_rx_queue() unconditionally at the end of every poll,
> even when zero descriptors were reaped, so scheduling NAPI in response
> to NO_CPU_DSCP is sufficient to make an emptied ring recover on its
> own. airoha_qdma_rx_napi_poll() is updated to re-enable the
> NO_CPU_DSCP bit alongside RX_DONE when napi_complete() runs, mirroring
> the existing disable/enable dance so the interrupt isn't left masked
> after its first use.
> 
> One open question worth flagging explicitly: if the underlying
> no-free-descriptor condition re-latches this bit immediately after the
> ack write (rather than only on the next empty->non-empty transition),
> a ring that airoha_qdma_fill_rx_queue() genuinely cannot repost into
> (e.g. page_pool_dev_alloc_frag() returning NULL under memory pressure)
> would turn this into a self-reasserting interrupt storm on the hard
> IRQ path: mask -> napi_schedule() -> poll reaps 0, refills 0 ->
> napi_complete() -> unmask -> NO_CPU_DSCP fires again immediately. I
> don't have documentation confirming which behavior this bit actually
> has. Regardless of the answer, masking NO_CPU_DSCP until a refill
> actually succeeds - the natural-looking alternative - is worse: a
> fully memory-starved ring can never fire RX_DONE either (nothing was
> posted for hw to complete), so that would leave it permanently dead
> once the memory pressure clears rather than self-healing. A bounded
> storm tied to genuine memory pressure, if that's what this is, seems
> preferable to a ring with no way back either way.

Hi Vitaliy,

I do not have info about it. Adding airoha folks in CC.

> 
> Fixes: 23290c7bc190 ("net: airoha: Introduce Airoha NPU support")
> Link: https://github.com/openwrt/openwrt/issues/24715
> Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 28 +++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)

thx for working on it. Just a couple of nit inline. Fixing them:

Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>

> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 64619e9a704d..a3e5aaeb75b3 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -784,13 +784,15 @@ static int airoha_qdma_rx_napi_poll(struct napi_struct *napi, int budget)
>  		int i, qid = q - &qdma->q_rx[0];
>  		int intr_reg = qid < RX_DONE_HIGH_OFFSET ? QDMA_INT_REG_IDX1
>  							 : QDMA_INT_REG_IDX2;
> +		u32 bit = qid % RX_DONE_HIGH_OFFSET;

nit: what about doing something like:

		u32 irq_id = qid % RX_DONE_HIGH_OFFSET;
		u32 intr_mask = BIT(irq_id) |
				BIT(irq_id + RX_NO_CPU_DSCP_LOW_OFFSET);

>  
>  		for (i = 0; i < ARRAY_SIZE(qdma->irq_banks); i++) {
>  			if (!(BIT(qid) & RX_IRQ_BANK_PIN_MASK(i)))
>  				continue;
>  
>  			airoha_qdma_irq_enable(&qdma->irq_banks[i], intr_reg,
> -					       BIT(qid % RX_DONE_HIGH_OFFSET));
> +					       BIT(bit) |
> +					       BIT(bit + RX_NO_CPU_DSCP_LOW_OFFSET));
>  		}
>  	}
>  
> @@ -1468,16 +1470,32 @@ static irqreturn_t airoha_irq_handler(int irq, void *dev_instance)
>  	if (!test_bit(DEV_STATE_INITIALIZED, &qdma->eth->state))
>  		return IRQ_NONE;
>  
> -	rx_intr1 = intr[1] & RX_DONE_LOW_INT_MASK;
> +	/* A ring can also raise NO_CPU_DSCP when it runs out of free RX
> +	 * descriptors (e.g. a burst of VIP-classified control traffic
> +	 * forced onto a small ring). Once a ring is fully drained no more
> +	 * RX_DONE interrupts can fire for it, since there are no free
> +	 * descriptors left for hardware to receive into, so without this
> +	 * NAPI is never rescheduled and the ring never gets refilled again.
> +	 * Treat NO_CPU_DSCP the same as RX_DONE for scheduling NAPI:
> +	 * airoha_qdma_rx_process() unconditionally calls
> +	 * airoha_qdma_fill_rx_queue() at the end of every poll, even when
> +	 * zero descriptors were reaped, so this alone is enough to recover
> +	 * the ring.
> +	 */

I guess this comment (AI driven??) should go into the commit message, it seems too
verbose for the code and it is quite obvious what you want to do.

> +	rx_intr1 = intr[1] & (RX_DONE_LOW_INT_MASK | RX_NO_CPU_DSCP_LOW_INT_MASK);
>  	if (rx_intr1) {
>  		airoha_qdma_irq_disable(irq_bank, QDMA_INT_REG_IDX1, rx_intr1);
> -		rx_intr_mask |= rx_intr1;
> +		rx_intr_mask |= (rx_intr1 & RX_DONE_LOW_INT_MASK) |
> +				((rx_intr1 & RX_NO_CPU_DSCP_LOW_INT_MASK) >>
> +				 RX_NO_CPU_DSCP_LOW_OFFSET);

What about defining a macro in airoha_regs.h

#define RX_NO_CPU_DSCP_INT_RX1_MASK(_n) (((_n) & RX_NO_CPU_DSCP_LOW_INT_MASK) >>\
					 RX_NO_CPU_DSCP_LOW_OFFSET)

>  	}
>  
> -	rx_intr2 = intr[2] & RX_DONE_HIGH_INT_MASK;
> +	rx_intr2 = intr[2] & (RX_DONE_HIGH_INT_MASK | RX_NO_CPU_DSCP_HIGH_INT_MASK);
>  	if (rx_intr2) {
>  		airoha_qdma_irq_disable(irq_bank, QDMA_INT_REG_IDX2, rx_intr2);
> -		rx_intr_mask |= (rx_intr2 << 16);
> +		rx_intr_mask |= ((rx_intr2 & RX_DONE_HIGH_INT_MASK) |
> +				 ((rx_intr2 & RX_NO_CPU_DSCP_HIGH_INT_MASK) >>
> +				  RX_NO_CPU_DSCP_LOW_OFFSET)) << 16;

I guess here you do not need to shift RX_NO_CPU_DSCP_HIGH_INT_MASK mask since
it is already in the range [31:16], just mask like:

	(rx_intr2 & RX_NO_CPU_DSCP_HIGH_INT_MASK)

>  	}
>  
>  	for (i = 0; rx_intr_mask && i < ARRAY_SIZE(qdma->q_rx); i++) {
> -- 
> 2.55.0
> 

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