[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 11 Dec 2015 19:53:58 -0500 (EST)
From: David Miller <davem@...emloft.net>
To: tlfalcon@...ux.vnet.ibm.com
Cc: netdev@...r.kernel.org, brking@...ux.vnet.ibm.com,
nfont@...ux.vnet.ibm.com, nguyendo@...ibm.com, formosa@...ibm.com,
linuxppc-dev@...ts.ozlabs.org, jallen@...ux.vnet.ibm.com
Subject: Re: [PATCH net-next v2] Driver for IBM System i/p VNIC protocol
From: Thomas Falcon <tlfalcon@...ux.vnet.ibm.com>
Date: Tue, 8 Dec 2015 11:52:19 -0600
> +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.
> + 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.
> + adapter->closing = 1;
Please use type 'bool' and values 'true' and 'false' for boolean
values.
Audit this in your entire driver.
> + 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.
> +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.
> +static int ibmvnic_mii_ioctl(struct net_device *netdev, struct ifreq *ifr,
> + int cmd)
> +{
...
> + 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.
> +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.
--
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
Powered by blists - more mailing lists