Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
Arjan, thank you very much for reviewing the driver.
Arjan van de Ven wrote:
On Sun, 2007-01-21 at 15:06 -0600, Jay Cliburn wrote:
[snip]
+void atl1_irq_disable(struct atl1_adapter *adapter)
+{
+ atomic_inc(>irq_sem);
+ iowrite32(0, adapter->hw.hw_addr + REG_IMR);
+ synchronize_irq(adapter->pdev->irq);
+}
doesn't this want a PCI posting flush?
I'm also a bit sceptical about irq_sem ...
PCI posting flush will be added. Would you mind elaborating on your
skepticism about irq_sem?
+/**
+ * When ACPI resume on some VIA MotherBoard, the Interrupt Disable bit/0x400
+ * on PCI Command register is disable.
+ * The function enable this bit.
+ * Brackett, 2006/03/15
+ */
+static void atl1_via_workaround(struct atl1_adapter *adapter)
+{
+ unsigned long value;
+
+ value = ioread16(adapter->hw.hw_addr + PCI_COMMAND);
+ if (value & PCI_COMMAND_INTX_DISABLE)
+ value &= ~PCI_COMMAND_INTX_DISABLE;
+ iowrite32(value, adapter->hw.hw_addr + PCI_COMMAND);
+}
hmm I wonder if this shouldn't be a more generic PCI level quirk, not so
much a driver level quirk...
The vendor put this code here, and we're loathe to remove it unless
absolutely necessary. Is it okay with you if we leave it?
Thanks,
Jay
-
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/
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
On Sun, 2007-01-21 at 15:06 -0600, Jay Cliburn wrote:
> +
> + /* PCI config space info */
> + pci_read_config_byte(pdev, PCI_REVISION_ID, >revision_id);
> + pci_read_config_word(pdev, PCI_COMMAND, >pci_cmd_word);
I'm highly suspicious of drivers that use the PCI_COMMAND word...
thankfully this seems to be a write only variable in this driver :)
> + if (adapter->pci_using_64) {
> + /* test whether HIDWORD dma buffer is not cross boundary */
> + if (unlikely(((ring_header->dma & 0xULL) >> 32)
> + != (((ring_header->dma + size) & 0xULL) >>
this is not needed; this is never ever supposed to happen..
what is more, you allocated consistent DMA memory, without setting the
consistent DMA mask to anything other than 32 bit... so you'll never
even go outside of the 32 bit region..
> + if (tpd_ring->buffer_info)
> + kfree(tpd_ring->buffer_info);
no need for the if(), kfree(NULL) is perfectly fine
> +static void atl1_clear_phy_int(struct atl1_adapter *adapter)
> +{
> + u16 phy_data;
> + spin_lock(>lock);
> + atl1_read_phy_reg(>hw, 19, _data);
> + spin_unlock(>lock);
are you sure this lock doesn't need to be irq safe?
> +/**
> + * atl1_irq_disable - Mask off interrupt generation on the NIC
> + * @adapter: board private structure
> + **/
> +void atl1_irq_disable(struct atl1_adapter *adapter)
> +{
> + atomic_inc(>irq_sem);
> + iowrite32(0, adapter->hw.hw_addr + REG_IMR);
> + synchronize_irq(adapter->pdev->irq);
> +}
doesn't this want a PCI posting flush?
I'm also a bit sceptical about irq_sem ...
> +/**
> + * When ACPI resume on some VIA MotherBoard, the Interrupt Disable bit/0x400
> + * on PCI Command register is disable.
> + * The function enable this bit.
> + * Brackett, 2006/03/15
> + */
> +static void atl1_via_workaround(struct atl1_adapter *adapter)
> +{
> + unsigned long value;
> +
> + value = ioread16(adapter->hw.hw_addr + PCI_COMMAND);
> + if (value & PCI_COMMAND_INTX_DISABLE)
> + value &= ~PCI_COMMAND_INTX_DISABLE;
> + iowrite32(value, adapter->hw.hw_addr + PCI_COMMAND);
> +}
hmm I wonder if this shouldn't be a more generic PCI level quirk, not so
much a driver level quirk...
-
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/
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
On Sun, 2007-01-21 at 15:06 -0600, Jay Cliburn wrote:
+
+ /* PCI config space info */
+ pci_read_config_byte(pdev, PCI_REVISION_ID, hw-revision_id);
+ pci_read_config_word(pdev, PCI_COMMAND, hw-pci_cmd_word);
I'm highly suspicious of drivers that use the PCI_COMMAND word...
thankfully this seems to be a write only variable in this driver :)
+ if (adapter-pci_using_64) {
+ /* test whether HIDWORD dma buffer is not cross boundary */
+ if (unlikely(((ring_header-dma 0xULL) 32)
+ != (((ring_header-dma + size) 0xULL)
this is not needed; this is never ever supposed to happen..
what is more, you allocated consistent DMA memory, without setting the
consistent DMA mask to anything other than 32 bit... so you'll never
even go outside of the 32 bit region..
+ if (tpd_ring-buffer_info)
+ kfree(tpd_ring-buffer_info);
no need for the if(), kfree(NULL) is perfectly fine
+static void atl1_clear_phy_int(struct atl1_adapter *adapter)
+{
+ u16 phy_data;
+ spin_lock(adapter-lock);
+ atl1_read_phy_reg(adapter-hw, 19, phy_data);
+ spin_unlock(adapter-lock);
are you sure this lock doesn't need to be irq safe?
+/**
+ * atl1_irq_disable - Mask off interrupt generation on the NIC
+ * @adapter: board private structure
+ **/
+void atl1_irq_disable(struct atl1_adapter *adapter)
+{
+ atomic_inc(adapter-irq_sem);
+ iowrite32(0, adapter-hw.hw_addr + REG_IMR);
+ synchronize_irq(adapter-pdev-irq);
+}
doesn't this want a PCI posting flush?
I'm also a bit sceptical about irq_sem ...
+/**
+ * When ACPI resume on some VIA MotherBoard, the Interrupt Disable bit/0x400
+ * on PCI Command register is disable.
+ * The function enable this bit.
+ * Brackett, 2006/03/15
+ */
+static void atl1_via_workaround(struct atl1_adapter *adapter)
+{
+ unsigned long value;
+
+ value = ioread16(adapter-hw.hw_addr + PCI_COMMAND);
+ if (value PCI_COMMAND_INTX_DISABLE)
+ value = ~PCI_COMMAND_INTX_DISABLE;
+ iowrite32(value, adapter-hw.hw_addr + PCI_COMMAND);
+}
hmm I wonder if this shouldn't be a more generic PCI level quirk, not so
much a driver level quirk...
-
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/
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
Arjan, thank you very much for reviewing the driver.
Arjan van de Ven wrote:
On Sun, 2007-01-21 at 15:06 -0600, Jay Cliburn wrote:
[snip]
+void atl1_irq_disable(struct atl1_adapter *adapter)
+{
+ atomic_inc(adapter-irq_sem);
+ iowrite32(0, adapter-hw.hw_addr + REG_IMR);
+ synchronize_irq(adapter-pdev-irq);
+}
doesn't this want a PCI posting flush?
I'm also a bit sceptical about irq_sem ...
PCI posting flush will be added. Would you mind elaborating on your
skepticism about irq_sem?
+/**
+ * When ACPI resume on some VIA MotherBoard, the Interrupt Disable bit/0x400
+ * on PCI Command register is disable.
+ * The function enable this bit.
+ * Brackett, 2006/03/15
+ */
+static void atl1_via_workaround(struct atl1_adapter *adapter)
+{
+ unsigned long value;
+
+ value = ioread16(adapter-hw.hw_addr + PCI_COMMAND);
+ if (value PCI_COMMAND_INTX_DISABLE)
+ value = ~PCI_COMMAND_INTX_DISABLE;
+ iowrite32(value, adapter-hw.hw_addr + PCI_COMMAND);
+}
hmm I wonder if this shouldn't be a more generic PCI level quirk, not so
much a driver level quirk...
The vendor put this code here, and we're loathe to remove it unless
absolutely necessary. Is it okay with you if we leave it?
Thanks,
Jay
-
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/
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
> +#include
Why do you need this one?
> +#include
Shouldn't be needed aswell.
> +#include
, please.
> + spin_lock_init(>stats_lock);
> + spin_lock_init(>tx_lock);
> + spin_lock_init(>mb_lock);
> + spin_lock_init(>vl_lock);
> + spin_lock_init(>mii_lock);
Do you really need that many locks?
> +static inline void atl1_irq_enable(struct atl1_adapter *adapter)
> +{
> + if (likely(0 == atomic_dec_and_test(>irq_sem)))
> + atl1_write32(>hw, REG_IMR, IMR_NORMAL_MASK);
> +}
We normally prefer atomic_dec_and_test(>irq_sem) == 0 or
just !atomic_dec_and_test(>irq_sem).
Also all these little helpers should probably not be marked inline
so gcc can decide on it's own merit which static function to inline.
> +static int atl1_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> +{
> + switch (cmd) {
> + case SIOCGMIIPHY:
> + case SIOCGMIIREG:
> + case SIOCSMIIREG:
> + return atl1_mii_ioctl(netdev, ifr, cmd);
> + case SIOCETHTOOL:
> + atl1_set_ethtool_ops(netdev);
ethtool doesn't use the ioctl path anymore at all.
> +static int atl1_open(struct net_device *netdev)
> +{
> + struct atl1_adapter *adapter = netdev_priv(netdev);
> + int err;
> +
> + /* allocate transmit descriptors */
> + if ((err = atl1_setup_ring_resources(adapter)))
> + return err;
> + if ((err = atl1_up(adapter)))
> + goto err_up;
Preffered style for this is:
err = atl1_setup_ring_resources(adapter);
if (err)
return err;
err = atl1_up(adapter);
if (err)
goto err_up;
(also applies in various other places)
-
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/
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
+#include asm/page.h
Why do you need this one?
+#include asm/system.h
Shouldn't be needed aswell.
+#include asm/checksum.h
net/checksum.h, please.
+ spin_lock_init(adapter-stats_lock);
+ spin_lock_init(adapter-tx_lock);
+ spin_lock_init(adapter-mb_lock);
+ spin_lock_init(adapter-vl_lock);
+ spin_lock_init(adapter-mii_lock);
Do you really need that many locks?
+static inline void atl1_irq_enable(struct atl1_adapter *adapter)
+{
+ if (likely(0 == atomic_dec_and_test(adapter-irq_sem)))
+ atl1_write32(adapter-hw, REG_IMR, IMR_NORMAL_MASK);
+}
We normally prefer atomic_dec_and_test(adapter-irq_sem) == 0 or
just !atomic_dec_and_test(adapter-irq_sem).
Also all these little helpers should probably not be marked inline
so gcc can decide on it's own merit which static function to inline.
+static int atl1_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
+{
+ switch (cmd) {
+ case SIOCGMIIPHY:
+ case SIOCGMIIREG:
+ case SIOCSMIIREG:
+ return atl1_mii_ioctl(netdev, ifr, cmd);
+ case SIOCETHTOOL:
+ atl1_set_ethtool_ops(netdev);
ethtool doesn't use the ioctl path anymore at all.
+static int atl1_open(struct net_device *netdev)
+{
+ struct atl1_adapter *adapter = netdev_priv(netdev);
+ int err;
+
+ /* allocate transmit descriptors */
+ if ((err = atl1_setup_ring_resources(adapter)))
+ return err;
+ if ((err = atl1_up(adapter)))
+ goto err_up;
Preffered style for this is:
err = atl1_setup_ring_resources(adapter);
if (err)
return err;
err = atl1_up(adapter);
if (err)
goto err_up;
(also applies in various other places)
-
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/
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
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, _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_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(>stats_lock, flags);
> + if (at_read_phy_reg
> + (>hw, data->reg_num & 0x1F, >val_out)) {
> + spin_unlock_irqrestore(>stats_lock, flags);
> + return -EIO;
> + }
> + spin_unlock_irqrestore(>stats_lock, flags);
> + break;
> + case SIOCSMIIREG:
> + if (!capable(CAP_NET_ADMIN))
> + return -EPERM;
> + if (data->reg_num & ~(0x1F))
> + return -EFAULT;
> + spin_lock_irqsave(>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(>hw, data->reg_num,
> data->val_in)) {
> + spin_unlock_irqrestore(>stats_lock, flags);
> + return -EIO;
> + }
> + spin_unlock_irqrestore(>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
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
>+char at_driver_name[] = "atl1";
>+static const char at_driver_string[] = "Attansic(R) L1 Ethernet Network
>Driver";
>+const char at_driver_version[] = DRV_VERSION;
>+static const char at_copyright[] =
>+"Copyright(c) 2005-2006 Attansic Corporation.";
>+
>+extern s32 at_read_mac_addr(struct at_hw *hw);
>+extern s32 at_init_hw(struct at_hw *hw);
>+extern s32 at_get_speed_and_duplex(struct at_hw *hw, u16 * speed, u16 *
>duplex);
>+extern s32 at_set_speed_and_duplex(struct at_hw *hw, u16 speed, u16 duplex);
>+extern u32 at_auto_get_fc(struct at_adapter *adapter, u16 duplex);
>+extern u32 at_hash_mc_addr(struct at_hw *hw, u8 * mc_addr);
>+extern void at_hash_set(struct at_hw *hw, u32 hash_value);
>+extern s32 at_read_phy_reg(struct at_hw *hw, u16 reg_addr, u16 * phy_data);
>+extern s32 at_write_phy_reg(struct at_hw *hw, u32 reg_addr, u16 phy_data);
>+extern s32 at_validate_mdi_setting(struct at_hw *hw);
>+extern void set_mac_addr(struct at_hw *hw);
>+extern int get_permanent_address(struct at_hw *hw);
>+extern s32 at_phy_enter_power_saving(struct at_hw *hw);
>+extern s32 at_reset_hw(struct at_hw *hw);
>+extern void at_check_options(struct at_adapter *adapter);
>+void at_set_ethtool_ops(struct net_device *netdev);
Put externs in a .h file.
>+static u16 at_alloc_rx_buffers(struct at_adapter *adapter)
>+{
...
>+ u16 rfd_next_to_use, next_next;
>+ struct rx_free_desc *rfd_desc;
>+
>+ next_next = rfd_next_to_use = (u16) atomic_read(_ring->next_to_use);
Cast not needed.
>+ buffer_info->length = (u16) adapter->rx_buffer_len;
>+ rrd_next_to_clean = (u16) atomic_read(_ring->next_to_clean);
Same (check the others too)
>+ if (++rfd_ring->next_to_clean == rfd_ring->count) {
>+ rfd_ring->next_to_clean = 0;
>+ }
>+ }
>+
>+ buffer_info = _ring->buffer_info[rrd->buf_indx];
>+ if (++rfd_ring->next_to_clean == rfd_ring->count) {
>+ rfd_ring->next_to_clean = 0;
>+ }
Here is a stylistic one: {} is not needed for single-statememnt ifs.
>+static irqreturn_t at_intr(int irq, void *data)
>+{
>+ struct at_adapter *adapter = ((struct net_device *)data)->priv;
(Ahem)
>+ if (0 == (status = adapter->cmb.cmb->int_stats))
Someone else is probably going to complain about this one...
I have not looked through all of it, so there are sure some more places.
-`J'
--
-
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/
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
+char at_driver_name[] = atl1;
+static const char at_driver_string[] = Attansic(R) L1 Ethernet Network
Driver;
+const char at_driver_version[] = DRV_VERSION;
+static const char at_copyright[] =
+Copyright(c) 2005-2006 Attansic Corporation.;
+
+extern s32 at_read_mac_addr(struct at_hw *hw);
+extern s32 at_init_hw(struct at_hw *hw);
+extern s32 at_get_speed_and_duplex(struct at_hw *hw, u16 * speed, u16 *
duplex);
+extern s32 at_set_speed_and_duplex(struct at_hw *hw, u16 speed, u16 duplex);
+extern u32 at_auto_get_fc(struct at_adapter *adapter, u16 duplex);
+extern u32 at_hash_mc_addr(struct at_hw *hw, u8 * mc_addr);
+extern void at_hash_set(struct at_hw *hw, u32 hash_value);
+extern s32 at_read_phy_reg(struct at_hw *hw, u16 reg_addr, u16 * phy_data);
+extern s32 at_write_phy_reg(struct at_hw *hw, u32 reg_addr, u16 phy_data);
+extern s32 at_validate_mdi_setting(struct at_hw *hw);
+extern void set_mac_addr(struct at_hw *hw);
+extern int get_permanent_address(struct at_hw *hw);
+extern s32 at_phy_enter_power_saving(struct at_hw *hw);
+extern s32 at_reset_hw(struct at_hw *hw);
+extern void at_check_options(struct at_adapter *adapter);
+void at_set_ethtool_ops(struct net_device *netdev);
Put externs in a .h file.
+static u16 at_alloc_rx_buffers(struct at_adapter *adapter)
+{
...
+ u16 rfd_next_to_use, next_next;
+ struct rx_free_desc *rfd_desc;
+
+ next_next = rfd_next_to_use = (u16) atomic_read(rfd_ring-next_to_use);
Cast not needed.
+ buffer_info-length = (u16) adapter-rx_buffer_len;
+ rrd_next_to_clean = (u16) atomic_read(rrd_ring-next_to_clean);
Same (check the others too)
+ if (++rfd_ring-next_to_clean == rfd_ring-count) {
+ rfd_ring-next_to_clean = 0;
+ }
+ }
+
+ buffer_info = rfd_ring-buffer_info[rrd-buf_indx];
+ if (++rfd_ring-next_to_clean == rfd_ring-count) {
+ rfd_ring-next_to_clean = 0;
+ }
Here is a stylistic one: {} is not needed for single-statememnt ifs.
+static irqreturn_t at_intr(int irq, void *data)
+{
+ struct at_adapter *adapter = ((struct net_device *)data)-priv;
(Ahem)
+ if (0 == (status = adapter-cmb.cmb-int_stats))
Someone else is probably going to complain about this one...
I have not looked through all of it, so there are sure some more places.
-`J'
--
-
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/
Re: [PATCH 3/4] atl1: Main C file for Attansic L1 driver
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/
