RE: [PATCH net] net: txgbe: fix interrupt mask for MISC cause in non-MSI-X mode

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Tue, Aug 18, 2026 4:58 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: txgbe: fix interrupt mask for MISC cause in non-MSI-X mode
> 
> The patch changes the trailing unmask in txgbe_misc_irq_thread_fn() so that
> it uses BIT(0) instead of TXGBE_INTR_MISC(wx) when MSI-X is not enabled,
> because the hardware merges all interrupt causes into BIT(0) in MSI/INTx
> mode. This is meant to avoid delaying subsequent MISC interrupts until NAPI
> polling completes.
> 
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
> > index 8746318ad3bcb..05b318f4e6ed6 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
> 
> [ ... ]
> 
> > @@ -183,7 +184,8 @@ static irqreturn_t txgbe_misc_irq_thread_fn(int irq, void *data)
> >  		nhandled++;
> >  	}
> >
> > -	wx_intr_enable(wx, TXGBE_INTR_MISC(wx));
> > +	misc_mask = wx->pdev->msix_enabled ? TXGBE_INTR_MISC(wx) : BIT(0);
> > +	wx_intr_enable(wx, misc_mask);
> >  	return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
> >  }
> 
> [Severity: Medium]
> Should this unmask be guarded by WX_STATE_DOWN now that it touches the live
> BIT(0) cause?
> 
> In non-MSI-X mode this write becomes a real WX_PX_IMC(0) = BIT(0), which
> re-arms the single merged queue+MISC cause. Before the change the value was
> BIT(wx->num_q_vectors) = BIT(1) with num_q_vectors == 1 in this mode, i.e. a
> bit the hardware does not use here, so the write left no hardware residue.
> 
> wx_irq_disable() in libwx/wx_hw.c masks everything before it waits for
> in-flight handlers:
> 
> void wx_irq_disable(struct wx *wx)
> {
> 	struct pci_dev *pdev = wx->pdev;
> 
> 	wr32(wx, WX_PX_MISC_IEN, 0);
> 	wx_intr_disable(wx, WX_INTR_ALL);
> 
> 	if (pdev->msix_enabled) {
> 		...
> 	} else {
> 		synchronize_irq(pdev->irq);
> 	}
> }
> 
> Can a misc thread that is already running execute its trailing
> wx_intr_enable(wx, BIT(0)) after those mask writes, so that synchronize_irq()
> returns with the only active cause unmasked while the driver considers
> interrupts off?
> 
> The window in txgbe_disable_device() spans wx_irq_disable() through
> wx_napi_disable_all(), timer_delete_sync(), cancel_work_sync() and the TX ring
> shutdown, and the mask is only restored later via txgbe_reset() ->
> txgbe_reset_hw() -> wx_stop_adapter(). Since the TX rings and TX DMA engine
> are stopped after wx_irq_disable(), a TX completion cause could assert on the
> re-unmasked BIT(0) during that window; on a shared INTx line the hard handler
> then takes the eicr == 0 path, returns IRQ_NONE and does not write
> WX_PX_INTA.
> 
> Every other unmask site in this driver family wraps the same operation in the
> down check, for example wx_poll() in libwx/wx_lib.c:
> 
> 	if (!test_bit(WX_STATE_DOWN, wx->state))
> 		wx_intr_enable(wx, WX_INTR_Q(q_vector->v_idx));
> 
> and ngbe_intr() in ngbe/ngbe_main.c:
> 
> 	if (!test_bit(WX_STATE_DOWN, wx->state))
> 		ngbe_irq_enable(wx, false);
> 
> txgbe_misc_irq_handle() itself uses the guard on its shared interrupt path.
> Would something like the following be more consistent?
> 
> 	misc_mask = wx->pdev->msix_enabled ? TXGBE_INTR_MISC(wx) : BIT(0);
> 	if (!test_bit(WX_STATE_DOWN, wx->state))
> 		wx_intr_enable(wx, misc_mask);

Right, I'll fix it in V2.