Re: [net-next 03/13] igc: add support to interrupt, eeprom, registers and link self-tests

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 20 Apr 2020 16:43:03 -0700 Jeff Kirsher wrote:
> +bool igc_intr_test(struct igc_adapter *adapter, u64 *data)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +	struct net_device *netdev = adapter->netdev;
> +	u32 mask, ics_mask = IGC_ICS_MASK_OTHER, i = 0, shared_int = true;
> +	u32 irq = adapter->pdev->irq;
> +
> +	*data = 0;
> +
> +	/* Hook up test interrupt handler just for this test */
> +	if (adapter->msix_entries) {
> +		if (request_irq(adapter->msix_entries[0].vector,
> +				&igc_test_intr_msix, 0,
> +				netdev->name, adapter)) {
> +			*data = 1;
> +			return false;
> +		}
> +		ics_mask = IGC_ICS_MASK_MSIX;
> +	} else if (adapter->flags & IGC_FLAG_HAS_MSI) {
> +		shared_int = false;
> +		if (request_irq(irq,
> +				igc_test_intr, 0, netdev->name, adapter)) {
> +			*data = 1;
> +			return false;
> +		}
> +	} else if (!request_irq(irq, igc_test_intr, IRQF_PROBE_SHARED,
> +				netdev->name, adapter)) {
> +		shared_int = false;
> +	} else if (request_irq(irq, &igc_test_intr, IRQF_SHARED,
> +		 netdev->name, adapter)) {
> +		*data = 1;
> +		return false;
> +	}

What's the meaning of shared_int here? Looks like MSI-Ss are shared but
not MSIs? Could you perhaps add a comment or rename so it's clear it's
not IRQF_SHARED we're talking about?

> +static void igc_diag_test(struct net_device *netdev,
> +			  struct ethtool_test *eth_test, u64 *data)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	bool if_running = netif_running(netdev);
> +
> +	if (eth_test->flags == ETH_TEST_FL_OFFLINE) {
> +		netdev_info(adapter->netdev, "offline testing starting");
> +		set_bit(__IGC_TESTING, &adapter->state);

> +	} else {
> +		netdev_info(adapter->netdev, "online testing starting");

I'm no expert on self-tests but this looks like a strange condition for
a bitfield. If only on bit is set we do offline, if no bit is set, or
offline and something else we do online?

Perhaps:

if (flags & OFFLINE) {
 ...
} else {
 ...
}

Or

if (flags == OFFLINE) {
 ...
} else if (flags == 0) {
 ...
}

Rather than the mix of the two?

> +		/* register, eeprom, intr and loopback tests not run online */
> +		data[TEST_REG] = 0;
> +		data[TEST_EEP] = 0;
> +		data[TEST_IRQ] = 0;
> +		data[TEST_LOOP] = 0;
> +
> +		if (!igc_link_test(adapter, &data[TEST_LINK]))
> +			eth_test->flags |= ETH_TEST_FL_FAILED;
> +	}
> +
> +	msleep_interruptible(4 * 1000);

Why?

> +}
> +