Re: [net-next,v2.2,3/5] eth fbnic: Add msix self test

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

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
eth fbnic: Add msix self test

This function is meant to test the global interrupt registers and the
PCIe IP MSI-X functionality. It essentially goes through and tests
various combinations of the set, clear, and mask bits in order to
verify the behavior is as we expect it to be from the driver.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
> index 779a083b9215..092fa9ad93d0 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
> @@ -194,6 +194,36 @@ void fbnic_synchronize_irq(struct fbnic_dev *fbd, int nr);
>  int fbnic_request_irq(struct fbnic_dev *dev, int nr, irq_handler_t handler,
>  		      unsigned long flags, const char *name, void *data);
>  void fbnic_free_irq(struct fbnic_dev *dev, int nr, void *data);
> +
> +/**
> + * enum fbnic_msix_self_test_codes - return codes from self test routines
> + *
> + * These are the codes returned from the self test routines and
> + * stored in the test result array indexed by the specific
> + * test name.
> + *
> + * @FBNIC_TEST_MSIX_SUCCESS: no errors
> + * @FBNIC_TEST_MSIX_NOMEM: allocation failure
> + * @FBNIC_TEST_MSIX_IRQ_REQ_FAIL: IRQ request failure
> + * @FBNIC_TEST_MSIX_MASK: masking failed to prevent IRQ
> + * @FBNIC_TEST_MSIX_UNMASK: unmasking failure w/ sw status set
> + * @FBNIC_TEST_MSIX_IRQ_CLEAR: interrupt when clearing mask
> + * @FBNIC_TEST_MSIX_NO_INTERRUPT: no interrupt when not masked
> + * @FBNIC_TEST_MSIX_NO_CLEAR_OR_MASK: status not cleared, or mask not set
> + */
> +enum fbnic_msix_self_test_codes {
> +	FBNIC_TEST_MSIX_SUCCESS = 0,
> +	FBNIC_TEST_MSIX_NOMEM = 5,
> +	FBNIC_TEST_MSIX_IRQ_REQ_FAIL = 10,
> +	FBNIC_TEST_MSIX_MASK = 20,
> +	FBNIC_TEST_MSIX_UNMASK = 30,
> +	FBNIC_TEST_MSIX_IRQ_CLEAR = 40,
> +	FBNIC_TEST_MSIX_NO_INTERRUPT = 50,
> +	FBNIC_TEST_MSIX_NO_CLEAR_OR_MASK = 60,
> +};

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_irq.c b/drivers/net/ethernet/meta/fbnic/fbnic_irq.c
> index 02e8b0b257fe..2099ee89fb51 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_irq.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_irq.c

[ ... ]

> @@ -238,6 +238,160 @@ void fbnic_free_irq(struct fbnic_dev *fbd, int nr, void *data)
>  	free_irq(irq, data);
>  }
>
> +struct fbnic_msix_test_data {
> +	struct fbnic_dev *fbd;
> +	unsigned long test_msix_status[BITS_TO_LONGS(FBNIC_MAX_MSIX_VECS)];
> +	int irq_vector[FBNIC_MAX_MSIX_VECS];
> +};
> +
> +static irqreturn_t fbnic_irq_test(int irq, void *data)
> +{
> +	struct fbnic_msix_test_data *test_data = data;
> +	struct fbnic_dev *fbd = test_data->fbd;
> +	int i;
> +
> +	for (i = fbd->num_irqs; i--;) {
> +		if (test_data->irq_vector[i] == irq) {
> +			set_bit(i, test_data->test_msix_status);
> +			break;
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * fbnic_msix_test - Verify behavior of NIC interrupts
> + * @fbd: device to test
> + *
> + * This function is meant to test the global interrupt registers and the
> + * PCIe IP MSI-X functionality. It essentially goes through and tests
> + * various combinations of the set, clear, and mask bits in order to
> + * verify the behavior is as we expect it to be from the driver.
> + *
> + * Return: See enum fbnic_msix_self_test_codes
> + **/
> +enum fbnic_msix_self_test_codes fbnic_msix_test(struct fbnic_dev *fbd)
> +{
> +	enum fbnic_msix_self_test_codes result = FBNIC_TEST_MSIX_SUCCESS;
> +	struct pci_dev *pdev = to_pci_dev(fbd->dev);
> +	struct fbnic_msix_test_data *test_data;
> +	u32 mask = 0;
> +	int i;
> +
> +	/* Allocate bitmap and IRQ vector table */
> +	test_data = kzalloc(sizeof(*test_data), GFP_KERNEL);
> +
> +	/* memory allocation failure */
> +	if (!test_data)
> +		return FBNIC_TEST_MSIX_NOMEM;
> +
> +	/* Initialize test data */
> +	test_data->fbd = fbd;
> +
> +	for (i = FBNIC_NON_NAPI_VECTORS; i < fbd->num_irqs; i++) {
> +		/* Add IRQ to vector table so it can be found */
> +		test_data->irq_vector[i] = pci_irq_vector(pdev, i);
> +
> +		/* Enable the interrupt */
> +		if (!fbnic_request_irq(fbd, i, fbnic_irq_test, 0,
> +				       fbd->netdev->name, test_data))
> +			continue;
> +
> +		while (i-- > FBNIC_NON_NAPI_VECTORS)
> +			fbnic_free_irq(fbd, i, test_data);
> +		kfree(test_data);
> +
> +		/* IRQ request failure */
> +		return FBNIC_TEST_MSIX_IRQ_REQ_FAIL;
> +	}
> +
> +	/* Test each bit individually */
> +	for (i = FBNIC_NON_NAPI_VECTORS; i < fbd->num_irqs; i++) {
> +		mask = 1U << (i % 32);
> +
> +		/* Start with mask set and interrupt cleared */
> +		fbnic_wr32(fbd, FBNIC_INTR_MASK_SET(i / 32), mask);
> +		fbnic_wrfl(fbd);
> +		fbnic_wr32(fbd, FBNIC_INTR_CLEAR(i / 32), mask);
> +		fbnic_wrfl(fbd);
> +
> +		/* masking failure to prevent interrupt */
> +		result = FBNIC_TEST_MSIX_MASK;
> +
> +		fbnic_wr32(fbd, FBNIC_INTR_SET(i / 32), mask);
> +		fbnic_wrfl(fbd);
> +		usleep_range(10000, 11000);
> +
> +		if (test_bit(i, test_data->test_msix_status))
> +			break;
> +
> +		/* unmasking failure w/ sw status set */
> +		result = FBNIC_TEST_MSIX_UNMASK;
> +
> +		fbnic_wr32(fbd, FBNIC_INTR_MASK_CLEAR(i / 32), mask);
> +		fbnic_wrfl(fbd);
> +		usleep_range(10000, 11000);
> +
> +		if (!test_bit(i, test_data->test_msix_status))
> +			break;
> +
> +		/* interrupt when clearing mask */
> +		result = FBNIC_TEST_MSIX_IRQ_CLEAR;
> +
> +		clear_bit(i, test_data->test_msix_status);
> +		fbnic_wr32(fbd, FBNIC_INTR_MASK_CLEAR(i / 32), mask);
> +		fbnic_wrfl(fbd);
> +		usleep_range(10000, 11000);
> +
> +		if (test_bit(i, test_data->test_msix_status))
> +			break;
> +
> +		/* interrupt not triggering when not masked */
> +		result = FBNIC_TEST_MSIX_NO_INTERRUPT;
> +
> +		fbnic_wr32(fbd, FBNIC_INTR_SET(i / 32), mask);
> +		fbnic_wrfl(fbd);
> +		usleep_range(10000, 11000);
> +
> +		if (!test_bit(i, test_data->test_msix_status))
> +			break;
> +
> +		/* status not cleared, or mask not set */
> +		result = FBNIC_TEST_MSIX_NO_CLEAR_OR_MASK;
> +		if (mask & fbnic_rd32(fbd, FBNIC_INTR_STATUS(i / 32)))
> +			break;
> +		if (!(mask & fbnic_rd32(fbd, FBNIC_INTR_MASK(i / 32))))
> +			break;
> +
> +		/* Result = 0 - Success */
> +		result = FBNIC_TEST_MSIX_SUCCESS;
> +
> +		clear_bit(i, test_data->test_msix_status);
> +	}
> +
> +	if (i < fbd->num_irqs) {
> +		fbnic_wr32(fbd, FBNIC_INTR_MASK_SET(i / 32), mask);
> +		fbnic_wrfl(fbd);
> +		fbnic_wr32(fbd, FBNIC_INTR_CLEAR(i / 32), mask);
> +		fbnic_wrfl(fbd);
> +		clear_bit(i, test_data->test_msix_status);
> +	}
> +
> +	for (i = FBNIC_NON_NAPI_VECTORS; i < fbd->num_irqs; i++) {
> +		/* Test for bits set after testing */
> +		if (test_bit(i, test_data->test_msix_status))
> +			result = 70;
                    ^^

Should the value 70 have a named constant in enum fbnic_msix_self_test_codes?

The function returns enum fbnic_msix_self_test_codes, which defines values
SUCCESS (0), NOMEM (5), IRQ_REQ_FAIL (10), MASK (20), UNMASK (30),
IRQ_CLEAR (40), NO_INTERRUPT (50), and NO_CLEAR_OR_MASK (60).

The value 70 appears to indicate a stray interrupt was detected after
testing completed, but this error condition has no corresponding named enum
constant. This makes the error code returned to userspace via ethtool
undocumented and prevents matching it to a failure description.

Should a constant such as FBNIC_TEST_MSIX_STRAY_INTERRUPT be added to the
enum to maintain consistency with all other error codes in this function?

> +
> +		/* Free IRQ */
> +		fbnic_free_irq(fbd, i, test_data);
> +	}
> +
> +	kfree(test_data);
> +
> +	return result;
> +}
-- 
pw-bot: cr