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

[Thomas Falcon]

On 12/11/2015 06:53 PM, David Miller wrote:
> From: Thomas Falcon 
> 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(, 0, sizeof(crq));
>> +crq.change_mac_addr.first = IBMVNIC_CRQ_CMD;
>> +crq.change_mac_addr.cmd = CHANGE_MAC_ADDR;
>> +ether_addr_copy(_mac_addr.mac_addr[0], addr->sa_data);
>> +ibmvnic_send_crq(adapter, );
>> +
>> +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.

>
>> +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 >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

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[David Miller]

From: Thomas Falcon 
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(, 0, sizeof(crq));
> + crq.change_mac_addr.first = IBMVNIC_CRQ_CMD;
> + crq.change_mac_addr.cmd = CHANGE_MAC_ADDR;
> + ether_addr_copy(_mac_addr.mac_addr[0], addr->sa_data);
> + ibmvnic_send_crq(adapter, );
> +
> + 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 >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