On Sunday 19 November 2006 21:30, Jay Cliburn wrote:
> This patch contains the main C file for the Attansic L1 gigabit ethernet
> adapter driver.
Just a few style comments:
> + /* PCI config space info */
> + hw->vendor_id = pdev->vendor;
> + hw->device_id = pdev->device;
> + hw->subsystem_vendor_id = pdev->subsystem_vendor;
> + hw->subsystem_id = pdev->subsystem_device;
Do you actually need the copies of these fields? I guess you can
always access the data from pdev.
> + size = sizeof(struct at_buffer) * (tpd_ring->count + rfd_ring->count);
> + tpd_ring->buffer_info = kmalloc(size, GFP_KERNEL);
> + if (unlikely(!tpd_ring->buffer_info)) {
> + printk(KERN_WARNING "%s: kmalloc failed , size = D%d\n",
> + at_driver_name, size);
> + return -ENOMEM;
> + }
> + rfd_ring->buffer_info =
> + (struct at_buffer *)(tpd_ring->buffer_info + tpd_ring->count);
> +
> + memset(tpd_ring->buffer_info, 0, size);
Use kzalloc or kcalloc here.
> + ring_header->desc =
> + pci_alloc_consistent(pdev, ring_header->size, &ring_header->dma);
> + if (unlikely(!ring_header->desc)) {
> + kfree(tpd_ring->buffer_info);
> + printk(KERN_WARNING
> + "%s: pci_alloc_consistent failed, size = D%d\n",
> + at_driver_name, size);
> + return -ENOMEM;
> + }
Your cleanup path gets simpler if you use goto, and only one
instance of kfree at the end, instead of multiple return statements
in this function.
> + while (!buffer_info->alloced && !next_info->alloced) {
> + if (NULL != buffer_info->skb) {
> + buffer_info->alloced = 1;
> + goto next;
> + }
Instead of 'if (NULL != buffer_info->skb)', you should write
'if (buffer_info->skb)', like you do elsewhere.
> + next:
> + rfd_next_to_use = next_next;
> + if (unlikely(++next_next == rfd_ring->count))
> + next_next = 0;
Labels go to the start of a line.
> +#ifdef NETIF_F_HW_VLAN_TX
> + if (adapter->vlgrp && (rrd->pkt_flg & PACKET_FLAG_VLAN_INS)) {
> + u16 vlan_tag = (rrd->vlan_tag >> 4) |
> + ((rrd->vlan_tag & 7) << 13) |
> + ((rrd->vlan_tag & 8) << 9);
> + vlan_hwaccel_rx(skb, adapter->vlgrp, vlan_tag);
> + } else
> +#endif
No need for the #ifdef when submitting the driver for inclusion.
In this kernel version, NETIF_F_HW_VLAN_TX is always defined.
> +static int at_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int
> cmd)
> +{
> + struct at_adapter *adapter = netdev_priv(netdev);
> +/* struct mii_ioctl_data *data = (struct mii_ioctl_data *)&ifr->ifr_data;*/
> + struct mii_ioctl_data *data = if_mii(ifr);
> + unsigned long flags;
> +
> + switch (cmd) {
> + case SIOCGMIIPHY:
> + data->phy_id = 0;
> + break;
> + case SIOCGMIIREG:
> + if (!capable(CAP_NET_ADMIN))
> + return -EPERM;
> + spin_lock_irqsave(&adapter->stats_lock, flags);
> + if (at_read_phy_reg
> + (&adapter->hw, data->reg_num & 0x1F, &data->val_out)) {
> + spin_unlock_irqrestore(&adapter->stats_lock, flags);
> + return -EIO;
> + }
> + spin_unlock_irqrestore(&adapter->stats_lock, flags);
> + break;
> + case SIOCSMIIREG:
> + if (!capable(CAP_NET_ADMIN))
> + return -EPERM;
> + if (data->reg_num & ~(0x1F))
> + return -EFAULT;
> + spin_lock_irqsave(&adapter->stats_lock, flags);
> + printk(KERN_DEBUG "%s: at_mii_ioctl write %x %x\n",
> + at_driver_name, data->reg_num,
> + data->val_in);
> + if (at_write_phy_reg(&adapter->hw, data->reg_num,
> data->val_in)) {
> + spin_unlock_irqrestore(&adapter->stats_lock, flags);
> + return -EIO;
> + }
> + spin_unlock_irqrestore(&adapter->stats_lock, flags);
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + return AT_SUCCESS;
> +}
> +#endif /* SIOCGMIIPHY */
Any reason why you can't use generic_mii_ioctl?
> + err_init_hw:
> + err_reset:
> + err_register:
> + err_sw_init:
> + err_eeprom:
> + iounmap(adapter->hw.hw_addr);
> + err_ioremap:
> + free_netdev(netdev);
> + err_alloc_etherdev:
> + pci_release_regions(pdev);
> + return err;
It's more common to have a single label with multiple gotos instead
of multiple labels that all go to one statement.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
