Re: [net] net: airoha: enable RX_DONE interrupt for RX queue 31

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> - [High] Incomplete fix: the patch only adds BIT(31) to
>   RX_IRQ1_BANK_PIN_MASK, so the exact defect the commit message…
> 
> --
> 
> net: airoha: enable RX_DONE interrupt for RX queue 31
> 
> This adds BIT(31) to RX_IRQ1_BANK_PIN_MASK so that RX queue 31's RX_DONE
> interrupt is routed to IRQ bank 1.  The stated goal is that the queue gets
> drained and its buffers returned to the page pool.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index fa9a8edce22f..8277c1c87bb3 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> > @@ -538,7 +538,7 @@ struct airoha_wdma_info {
> >  
> >  /* RX queue to IRQ mapping: BIT(q) in IRQ(n) */
> >  #define RX_IRQ0_BANK_PIN_MASK			0x839f
> > -#define RX_IRQ1_BANK_PIN_MASK			0x7fe00000
> > +#define RX_IRQ1_BANK_PIN_MASK			0xffe00000
> >  #define RX_IRQ2_BANK_PIN_MASK			0x20
> >  #define RX_IRQ3_BANK_PIN_MASK			0x40
> 
> [Severity: High]
> Does this leave the same problem in place for the other RX rings?  After the
> change the union of the four bank masks is:
> 
>   0x839f | 0xffe00000 | 0x20 | 0x40 = 0xffe083ff
> 
> so rings 0-9, 15 and 21-31 are covered, but bits 10-14 and 16-20 are still
> clear in every bank mask.

Here I wanted just to align to the vendor sdk and enable rx queues 24-31 (31
was missing) in preparation for HW GRO support. The other rx queues are
out-of-scope.

Regards,
Lorenzo

> 
> Those rings are still allocated and filled, because airoha_qdma_init_rx() in
> drivers/net/ethernet/airoha/airoha_eth.c gates only on RX_DONE_INT_MASK:
> 
> 	if (!(RX_DONE_INT_MASK & BIT(i))) {
> 		/* rx-queue not binded to irq */
> 		continue;
> 	}
> 
> 	err = airoha_qdma_init_rx_queue(&qdma->q_rx[i], qdma,
> 					RX_DSCP_NUM(i));
> 
> and in drivers/net/ethernet/airoha/airoha_regs.h RX_DONE_INT_MASK expands to
> 0xffffffff:
> 
> 	#define RX_DONE_HIGH_OFFSET	fls(RX_DONE_HIGH_INT_MASK)
> 	#define RX_DONE_INT_MASK	\
> 		((RX_DONE_HIGH_INT_MASK << RX_DONE_HIGH_OFFSET) | RX_DONE_LOW_INT_MASK)
> 
> with RX_DONE_HIGH_INT_MASK and RX_DONE_LOW_INT_MASK both 0xffff.  So the
> "rx-queue not binded to irq" comment filters nothing and all 32 rings get
> programmed into hardware and filled from the page pool.
> 
> Meanwhile airoha_qdma_hw_init() only unmasks RX_DONE according to the pin
> masks:
> 
> 	airoha_qdma_irq_enable(&qdma->irq_banks[i], QDMA_INT_REG_IDX1,
> 			       INT_RX1_MASK(RX_IRQ_BANK_PIN_MASK(i)));
> 	airoha_qdma_irq_enable(&qdma->irq_banks[i], QDMA_INT_REG_IDX2,
> 			       INT_RX2_MASK(RX_IRQ_BANK_PIN_MASK(i)));
> 
> and airoha_qdma_rx_napi_poll() re-arms through the same mask:
> 
> 	for (i = 0; i < ARRAY_SIZE(qdma->irq_banks); i++) {
> 		if (!(BIT(qid) & RX_IRQ_BANK_PIN_MASK(i)))
> 			continue;
> 
> Since rx_intr_mask in airoha_irq_handler() comes from the per-bank irqmask,
> rings 10-14 and 16-20 are never passed to napi_schedule(), so
> airoha_qdma_rx_process() never runs for them and their buffers stay pinned
> from probe until airoha_qdma_cleanup_rx_queue().  Is that not exactly the
> behaviour the commit message describes as a bug for ring 31?
> 
> Ring 11 stands out here, since RX_DSCP_NUM() gives it 128 descriptors, the
> same as rings 2 and 15 which are mapped to bank 0:
> 
> 	#define RX_DSCP_NUM(_n)			\
> 		((_n) ==  2 ? 128 :		\
> 		 (_n) == 11 ? 128 :		\
> 		 (_n) == 15 ? 128 :		\
> 		 (_n) ==  0 ? 1024 : 16)
> 
> while BIT(11) is absent from all pin masks, and the ring this patch enables
> (31) uses the default 16 descriptors.
> 
> Would it make sense to give airoha_qdma_init_rx() and the pin masks a single
> source of truth, either by having the masks cover every ring init_rx arms, or
> by having init_rx key off RX_IRQ_BANK_PIN_MASK()?
> 
> Alternatively, if the sparse mapping is deliberate and those rings are simply
> unused by the hardware configuration, does the justification given for
> BIT(31) still hold?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260830-airoha-rxdone-rxq31-v1-1-830a91503f2f%40kernel.org

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