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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200421104818.1e9cfa14@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 21 Apr 2020 10:48:18 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:     davem@...emloft.net, Vitaly Lifshits <vitaly.lifshits@...el.com>,
        netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        kbuild test robot <lkp@...el.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Andre Guedes <andre.guedes@...el.com>,
        Aaron Brown <aaron.f.brown@...el.com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [net-next 03/13] igc: add support to interrupt, eeprom,
 registers and link self-tests

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?

> +}
> +