Re: [PATCH net-next v2] Driver for IBM System i/p VNIC protocol

On 12/11/2015 06:53 PM, David Miller wrote:

From: Thomas Falcon <redacted>
Date: Tue,  8 Dec 2015 11:52:19 -0600

quoted

+static long h_reg_sub_crq(unsigned long unit_address, unsigned long token,
+			  unsigned long length, unsigned long *number,
+			  unsigned long *irq)
+{
+	long rc;
+	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];

Please declare local variables from longest to shortest line, otherwise
known as "reverse christmas tree" order.

Audit this in your entire driver.

quoted

+	pool->rx_buff = kcalloc(pool->size, sizeof(struct ibmvnic_rx_buff),
+				GFP_KERNEL);

Allocation failures not checked until much later in this function, where
several other resources have been allocated meanwhile.  That doesn't
make any sense at all.

quoted

+	adapter->closing = 1;

Please use type 'bool' and values 'true' and 'false' for boolean
values.

Audit this in your entire driver.

quoted

+	if (ip_hdr(skb)->version == 4)
+		tx_crq.v1.flags1 |= IBMVNIC_TX_PROT_IPV4;
+	else if (ip_hdr(skb)->version == 6)
+		tx_crq.v1.flags1 |= IBMVNIC_TX_PROT_IPV6;
+

You cannot dereference the protocol header of the SKB without
first checking the skb->protocol value, otherwise you're looking
at garbage.

quoted

+static int ibmvnic_set_mac(struct net_device *netdev, void *p)
+{
+	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
+	struct sockaddr *addr = p;
+	union ibmvnic_crq crq;
+
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memset(&crq, 0, sizeof(crq));
+	crq.change_mac_addr.first = IBMVNIC_CRQ_CMD;
+	crq.change_mac_addr.cmd = CHANGE_MAC_ADDR;
+	ether_addr_copy(&crq.change_mac_addr.mac_addr[0], addr->sa_data);
+	ibmvnic_send_crq(adapter, &crq);
+
+	return 0;
+}

You are responsible for copying the new MAC address into dev->dev_addr
on success.

We do this in another function (handle_change_mac_rsp), which handles the response from firmware indicating whether the CHANGE_MAC_ADDR command was successful.  Is this acceptable? 

Thank you for your review comments and your time.

quoted

+static int ibmvnic_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
+			     int cmd)
+{

 ...

quoted

+	return 0;
+}
+
+static int ibmvnic_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
+{
+	switch (cmd) {
+	case SIOCGMIIPHY:
+	case SIOCGMIIREG:
+	case SIOCSMIIREG:
+		return ibmvnic_mii_ioctl(netdev, ifr, cmd);
+	default:
+		return -EOPNOTSUPP;
+	}
+}

This really doesn't make any sense.  Please just delete this.  You
don't support MII reads or writes because they logically don't make
sense on this device.

quoted

+static struct net_device_stats *ibmvnic_get_stats(struct net_device *dev)
+{
+	struct ibmvnic_adapter *adapter = netdev_priv(dev);
+
+	/* only return the current stats */
+	return &adapter->net_stats;
+}

The default method does this for you as long as you properly use
net_device's embedded stats, therefore you don't need to provide this
at all.

That's all I have any energy for, and as you can see nobody else wants
to even try to review this driver.

It's going to take a lot of respins and time before this driver is
ready for upstream inclusion.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev