netdev - Re: [RFC][PATCH 2/3] enic: add main file with module infrastructure, etc

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: <adaej4clmo0.fsf@cisco.com>
Date:	Mon, 25 Aug 2008 16:06:39 -0700
From:	Roland Dreier <rdreier@...co.com>
To:	Scott Feldman <scofeldm@...co.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC][PATCH 2/3] enic: add main file with module infrastructure, etc

General comment -- probably a good idea to run everything through
checkpatch.pl (catches a few trivial whitespace problems) and also
sparse with endianness checking (build with "make C=2
CF=-D__CHECK_ENDIAN__ SUBDIRS=drivers/net/enic") and look at all the
warnings you get.

 > +	{ PCI_DEVICE(PCI_VENDOR_ID_CISCO, PCI_DEVICE_ID_CISCO_ENIC) },

PCI_VDEVICE() makes this look slightly nicer.

 > +	netdev->features |= NETIF_F_LLTX;

LLTX is kind of deprecated for new drivers.  Would be nice to avoid this.

 > +	case VNIC_DEV_INTR_MODE_INTX:
 > +	case VNIC_DEV_INTR_MODE_MSI:
 > +		free_irq(enic->pdev->irq, netdev);
 > +		break;
 > +	case VNIC_DEV_INTR_MODE_MSIX:

Do you really need both MSI and MSI-X support?  Systems where MSI works
but MSI-X doesn't a pretty rare, and having both leads to a more complex
driver with one code path (MSI) that rarely gets tested.  I removed MSI
support from drivers/infiniband/hw/mthca at the beginning of the year
afer deprecating it for a while, and I've never heard anyone even
mention it, let alone complain.

 > +	mod_timer(&enic->notify_timer, jiffies + ENIC_NOTIFY_TIMER_PERIOD);

Would probably be nice to use round_jiffies() or a deferrable timer here
to avoid unnecessary wakeups.

 > +	pba = vnic_intr_legacy_pba(enic->legacy_pba);

enic->legacy_pba is just u32 * but vnic_intr_legacy_pba expects an
__iomem pointer.

 > +	if (skb->protocol == ntohs(ETH_P_IP)) {
 > +	} else if (skb->protocol == ntohs(ETH_P_IPV6)) {

trivial but you're converting from host to network order, so you should
use htons() not ntohs().

 > +	unsigned int vlan_tag = 0;
 > +	int vlan_tag_insert = 0;
 > +
 > +	if (enic->vlan_group && vlan_tx_tag_present(skb)) {
 > +		/* VLAN tag from trunking driver */
 > +		vlan_tag_insert = 1;
 > +		vlan_tag = cpu_to_le16(vlan_tx_tag_get(skb));
 > +	}

vlan_tag is declared as a plain int but you assign the result of
cpu_to_le16() to it.  This leads to a sparse warning if you build with
endianness checking on.

 > +	/* check if ip header and tcp header are complete */
 > +	if (iph->tot_len < ip_len + tcp_hdrlen(skb))
 > +		return -1;

Heh, looks like you copied and pasted a bad example from another driver
before I fixed it (in 3ff2cd23, "ehea: Access iph->tot_len with correct
endianness," where I was prescient enough in the changelog to say "this
... avoids having a bad example in the tree.").  iph->tot_len is stored
in network order so you should use ntohs(iph->tot_len).

 > +	u16 q_number, completed_index, bytes_written, vlan, checksum, len;
 > +	len = le16_to_cpu(bytes_written);
 > +			skb->csum = htons(checksum);
 > +					le16_to_cpu(vlan), cq_desc);

bytes_written is declared as u16 but then byte-swapped.  Creates a
sparse warning.  Similar for checksum, vlan.

 > +void enic_del_station_addr(struct enic *enic)
 > +{
 > +	vnic_dev_del_addr(enic->vdev, enic->mac_addr);
 > +}
 > 
 > +int enic_set_rss_key(struct enic *enic, dma_addr_t key_pa, u64 len)
 > +{
 > +	u64 a0 = (u64)key_pa, a1 = len;
 > +	int wait = 1000;
 > +
 > +	return vnic_dev_cmd(enic->vdev, CMD_RSS_KEY, &a0, &a1, wait);
 > +}
 > +
 > +int enic_set_rss_cpu(struct enic *enic, dma_addr_t cpu_pa, u64 len)
 > +{
 > +	u64 a0 = (u64)cpu_pa, a1 = len;
 > +	int wait = 1000;
 > +
 > +	return vnic_dev_cmd(enic->vdev, CMD_RSS_CPU, &a0, &a1, wait);
 > +}

I can't find anywhere any of these functions are actually used.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html