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

[Jakub Kicinski <kuba@kernel.org>]

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);
-- 
pw-bot: cr