netdev - RE: [PATCH net-next V3 09/18] liquidio: MSIX support for CN23XX

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] [day] [month] [year] [list]

Date:   Mon, 22 Aug 2016 17:36:07 +0000
From:   Yuval Mintz <Yuval.Mintz@...gic.com>
To:     Raghu Vatsavayi <rvatsavayi@...iumnetworks.com>,
        David Miller <davem@...emloft.net>
CC:     netdev <netdev@...r.kernel.org>,
        Derek Chickles <derek.chickles@...iumnetworks.com>,
        Satanand Burla <satananda.burla@...iumnetworks.com>,
        Felix Manlunas <felix.manlunas@...iumnetworks.com>,
        Raghu Vatsavayi <raghu.vatsavayi@...iumnetworks.com>
Subject: RE: [PATCH net-next V3 09/18] liquidio: MSIX support for CN23XX

> +	u64 time_threshold;
> +	u64 cnt_threshold;
...
> +		octeon_write_csr64(
> +		    oct, CN23XX_SLI_OQ_PKT_INT_LEVELS(oq_no),
> +		    ((u64)(time_threshold << 32 | cnt_threshold)));
No need for the cast.

> +	if (!droq) {
> +		dev_err(&oct->pci_dev->dev, "23XX bringup FIXME: oct
> pfnum:%d ioq_vector->ioq_num :%d droq is NULL\n",
> +			oct->pf_num, ioq_vector->ioq_num);
> +		return 0;
> +	}

> +	if (oct->msix_on != 1) {
> +		if (intr64 & CN23XX_INTR_PKT_DATA)
> +			oct->int_status |= OCT_DEV_INTR_PKT_DATA;
> +	}
What's the significance of 1? Is it actually a Boolean?
 
> +static int msix_enable = 1;
What's the purpose of this static variable? Is there any way to change it?

> +	if (OCTEON_CN23XX_PF(oct) && msix_enable) {
> +		if (!oct->msix_entries) {
> +			/* dev_err(&oct->pci_dev->dev,
> +			* "Memory Alloc failed...\n");
> +			 * This is commenting due WARNING: Possible
> unnecessary
> +			 * out of memory' message
> +			 */
> +			return 1;
> +		}
Don't submit commented prints.

> +		irqret = request_irq(msix_entries[num_ioq_vectors].vector,
> +				     liquidio_legacy_intr_handler, 0, "octeon",
> +				     oct);
> +		if (irqret) {
> +			dev_err(&oct->pci_dev->dev,
> +				"OCTEON: Request_irq failed for MSIX interrupt
> Error: %d\n",
> +				irqret);
> +			kfree(oct->msix_entries);
> +			return 1;
Shouldn't you also call pci_disable_msix() on error flow?

> +		}
> +		for (i = 0; i < num_ioq_vectors; i++) {
> +			irqret = request_irq(msix_entries[i].vector,
> +					     liquidio_msix_intr_handler, 0,
> +					     "octeon", &oct->ioq_vector[i]);
> +			if (irqret) {
> +				dev_err(&oct->pci_dev->dev,
> +					"OCTEON: Request_irq failed for MSIX
...
> +				kfree(oct->msix_entries);
> +				return 1;
Likewise