Messages in this thread | | | From | "Ren, Cloud" <> | Subject | RE: [patch net-next]alx: Atheros AR8131/AR8151/AR8152/AR8161 Ethernet driver | Date | Thu, 20 Oct 2011 07:29:27 +0000 |
| |
Thanks for your information.
Drivers on different OSs share alf_hw.c, alf_hw.h, alc_hw.c and alc_hw.h
So it looks some of codes are duplicate, because other drivers need them.
-----Original Message----- From: Francois Romieu [mailto:romieu@fr.zoreil.com] Sent: Thursday, October 20, 2011 6:22 AM To: Ren, Cloud Cc: davem@davemloft.net; Rodriguez, Luis; netdev@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [patch net-next]alx: Atheros AR8131/AR8151/AR8152/AR8161 Ethernet driver
cloud.ren@atheros.com <cloud.ren@atheros.com> : > diff --git a/drivers/net/ethernet/atheros/alx/alc_cb.c > b/drivers/net/ethernet/atheros/alx/alc_cb.c [...] > +int alc_read_phy_reg(struct alx_hw *hw, u32 device_type, > + u16 reg_addr, u16 *phy_data) > +{ > + bool fast = false; > + bool ext = false; > + u16 error; > + int retval = 0; > + > + ALX_MDIO_LOCK(&hw->mdio_lock);
Frowned upon / useless wrapper.
> + > + if (device_type != ALX_MDIO_NORM_DEV) > + ext = true;
This method seems to be always called with device_type = ALX_MDIO_NORM_DEV.
> + > + error = l1c_read_phy(hw, ext, device_type, fast, > + reg_addr, phy_data);
Imho the driver wants something like l1c_read_phy_{fast/slow}.
> + if (error) { > + HW_PRINT(ERR, "Error when reading phy reg (%d).", error); > + retval = ALX_ERR_PHY_READ_REG;
I am a bit sceptical that private error codes are really useful in a mundane ethernet driver.
[...] > +int alc_write_phy_reg(struct alx_hw *hw, u32 device_type, > + u16 reg_addr, u16 phy_data) > +{ > + bool fast = false; > + bool ext = false; > + u16 error; > + int retval = 0; > + > + ALX_MDIO_LOCK(&hw->mdio_lock); > + > + if (device_type != ALX_MDIO_NORM_DEV) > + ext = true;
This method seems to be always called with device_type = ALX_MDIO_NORM_DEV.
> + > + error = l1c_write_phy(hw, ext, device_type, fast, > + reg_addr, phy_data);
It fits in a single 80 columns line.
[...] > +int alc_init_phy(struct alx_hw *hw) > +{ > + u16 phy_id[2]; > + int retval = 0;
Useless init.
> + > + HW_PRINT(DEBUG, "ENTER\n");
Old school.
> + > + /* 1. init mdio spin lock */ > + ALX_MDIO_LOCK_INIT(&hw->mdio_lock);
The comment adds no value.
> + > + /* 2. read phy id */ > + retval = alc_read_phy_reg(hw, ALX_MDIO_NORM_DEV, > + L1C_MII_PHYSID1, &phy_id[0]);
Sic.
> + if (retval) > + return retval; > + retval = alc_read_phy_reg(hw, ALX_MDIO_NORM_DEV, > + L1C_MII_PHYSID1, &phy_id[1]); > + if (retval) > + return retval; > + > + memcpy(&hw->phy_id, phy_id, sizeof(hw->phy_id)); > + > + hw->autoneg_advertised = (ALX_LINK_SPEED_1GB_FULL | > + ALX_LINK_SPEED_10_HALF | > + ALX_LINK_SPEED_10_FULL | > + ALX_LINK_SPEED_100_HALF | > + ALX_LINK_SPEED_100_FULL);
Parenthesis abuse.
[...] > +int alc_ack_phy_intr(struct alx_hw *hw) { > + int retval = 0;
Useless init.
> + u16 isr = 0; > + > + HW_PRINT(DEBUG, "ENTER\n"); > + > + retval = alc_read_phy_reg(hw, ALX_MDIO_NORM_DEV, > + L1C_MII_ISR, &isr); > + if (retval) > + return retval; > + > + return retval;
The style is terrible. What about:
return alc_read_phy_reg(hw, ALX_MDIO_NORM_DEV, L1C_MII_ISR, &isr);
[...] > +int alc_clear_mac_addr(struct alx_hw *hw) { > + int retval = 0; > + > + HW_PRINT(DEBUG, "ENTER\n"); > + > + return retval; > +}
{alc/alf}_clear_mac_addr always returns 0 and it does not seem to do anything else.
[...] > +int alc_config_rx(struct alx_hw *hw) > +{ > + int retval = 0; > + return retval; > +}
Useless. {alc/alf}_config_rx always returns 0 and it does not anything else.
[...] > +int alc_check_nvram(struct alx_hw *hw, bool *exist) { > + *exist = false; > + return 0; > +}
Strictly identical to alf_check_nvram.
[...] > +int alc_get_ethtool_regs(struct alx_hw *hw, void *buff) { > + u32 *regs = (u32 *)buff;
Useless cast from void.
> + int retval = 0; > + > + MEM_R32(hw, L1C_LNK_CAP, ®s[0]); > + MEM_R32(hw, L1C_PMCTRL, ®s[1]); > + MEM_R32(hw, L1C_HALFD, ®s[2]); > + MEM_R32(hw, L1C_SLD, ®s[3]); > + MEM_R32(hw, L1C_MASTER, ®s[4]); > + MEM_R32(hw, L1C_MANU_TIMER, ®s[5]); > + MEM_R32(hw, L1C_IRQ_MODU_TIMER, ®s[6]); > + MEM_R32(hw, L1C_PHY_CTRL, ®s[7]); > + MEM_R32(hw, L1C_LNK_CTRL, ®s[8]); > + MEM_R32(hw, L1C_MAC_STS, ®s[9]); > + > + MEM_R32(hw, L1C_MDIO, ®s[10]); > + MEM_R32(hw, L1C_SERDES, ®s[11]); > + MEM_R32(hw, L1C_MAC_CTRL, ®s[12]); > + MEM_R32(hw, L1C_GAP, ®s[13]); > + MEM_R32(hw, L1C_STAD0, ®s[14]); > + MEM_R32(hw, L1C_STAD1, ®s[15]); > + MEM_R32(hw, L1C_HASH_TBL0, ®s[16]); > + MEM_R32(hw, L1C_HASH_TBL1, ®s[17]); > + MEM_R32(hw, L1C_RXQ0, ®s[18]); > + MEM_R32(hw, L1C_RXQ1, ®s[19]); > + > + MEM_R32(hw, L1C_RXQ2, ®s[20]); > + MEM_R32(hw, L1C_RXQ3, ®s[21]); > + MEM_R32(hw, L1C_TXQ0, ®s[22]); > + MEM_R32(hw, L1C_TXQ1, ®s[23]); > + MEM_R32(hw, L1C_TXQ2, ®s[24]); > + MEM_R32(hw, L1C_MTU, ®s[25]); > + MEM_R32(hw, L1C_WOL0, ®s[26]); > + MEM_R32(hw, L1C_WOL1, ®s[27]); > + MEM_R32(hw, L1C_WOL2, ®s[28]);
Tabulate and save code ?
[...] > diff --git a/drivers/net/ethernet/atheros/alx/alc_hw.h > b/drivers/net/ethernet/atheros/alx/alc_hw.h > --- /dev/null > +++ b/drivers/net/ethernet/atheros/alx/alc_hw.h [...] > +#ifdef __cplusplus > +extern "C" { > +#endif/*__cplusplus*/
Please remove this stuff before it gets noticed.
[...] > +#define L1C_PCI_CMD 0x0004 /* 16bit */ > +#define L1C_PCI_CMD_DISINT BIT_10S > +#define L1C_PCI_CMD_MASTEREN BIT_2S > +#define L1C_PCI_CMD_MEMEN BIT_1S > +#define L1C_PCI_CMD_IOEN BIT_0S
Duplicates from <linux/pci_regs.h>
[...] > +/********************* PHY regs definition > +***************************/ > + > +/* PHY Control Register */ > +#define L1C_MII_BMCR 0x00 > +#define L1C_BMCR_SPEED_SELECT_MSB 0x0040 > +#define L1C_BMCR_COLL_TEST_ENABLE 0x0080 > +#define L1C_BMCR_FULL_DUPLEX 0x0100 > +#define L1C_BMCR_RESTART_AUTO_NEG 0x0200 > +#define L1C_BMCR_ISOLATE 0x0400 > +#define L1C_BMCR_POWER_DOWN 0x0800 > +#define L1C_BMCR_AUTO_NEG_EN 0x1000 > +#define L1C_BMCR_SPEED_SELECT_LSB 0x2000 > +#define L1C_BMCR_LOOPBACK 0x4000 > +#define L1C_BMCR_RESET 0x8000 > +#define L1C_BMCR_SPEED_MASK 0x2040 > +#define L1C_BMCR_SPEED_1000 0x0040 > +#define L1C_BMCR_SPEED_100 0x2000 > +#define L1C_BMCR_SPEED_10 0x0000
Please use existing #define from <linux/mii.h>.
> + > +/* PHY Status Register */ > +#define L1C_MII_BMSR 0x01 > +#define L1C_BMSR_EXTENDED_CAPS 0x0001 > +#define L1C_BMSR_JABBER_DETECT 0x0002 > +#define L1C_BMSR_LINK_STATUS 0x0004 > +#define L1C_BMSR_AUTONEG_CAPS 0x0008 > +#define L1C_BMSR_REMOTE_FAULT 0x0010 > +#define L1C_BMSR_AUTONEG_COMPLETE 0x0020 > +#define L1C_BMSR_PREAMBLE_SUPPRESS 0x0040 > +#define L1C_BMSR_EXTENDED_STATUS 0x0100 > +#define L1C_BMSR_100T2_HD_CAPS 0x0200 > +#define L1C_BMSR_100T2_FD_CAPS 0x0400 > +#define L1C_BMSR_10T_HD_CAPS 0x0800 > +#define L1C_BMSR_10T_FD_CAPS 0x1000 > +#define L1C_BMSR_100X_HD_CAPS 0x2000 > +#define L1C_BMMII_SR_100X_FD_CAPS 0x4000 > +#define L1C_BMMII_SR_100T4_CAPS 0x8000
Sic.
[...] > diff --git a/drivers/net/ethernet/atheros/alx/alf_cb.c > b/drivers/net/ethernet/atheros/alx/alf_cb.c > --- /dev/null > +++ b/drivers/net/ethernet/atheros/alx/alf_cb.c [...] > +int alf_get_ethtool_regs(struct alx_hw *hw, void *buff) { > + u32 *regs = (u32 *)buff; > + int i; > + int retval = 0; > + > + MEM_R32(hw, L1F_PM_CSR, ®s[0]); > + MEM_R32(hw, L1F_DEV_CAP, ®s[1]); > + MEM_R32(hw, L1F_DEV_CTRL, ®s[2]); > + MEM_R32(hw, L1F_LNK_CAP, ®s[3]); > + MEM_R32(hw, L1F_LNK_CTRL, ®s[4]); > + MEM_R32(hw, L1F_UE_SVRT, ®s[5]); > + MEM_R32(hw, L1F_EFLD, ®s[6]); > + MEM_R32(hw, L1F_SLD, ®s[7]); > + MEM_R32(hw, L1F_PPHY_MISC1, ®s[8]); > + MEM_R32(hw, L1F_PPHY_MISC2, ®s[9]); > + > + MEM_R32(hw, L1F_PDLL_TRNS1, ®s[10]); > + MEM_R32(hw, L1F_TLEXTN_STATS, ®s[11]); > + MEM_R32(hw, L1F_EFUSE_CTRL, ®s[12]); > + MEM_R32(hw, L1F_EFUSE_DATA, ®s[13]); > + MEM_R32(hw, L1F_SPI_OP1, ®s[14]); > + MEM_R32(hw, L1F_SPI_OP2, ®s[15]); > + MEM_R32(hw, L1F_SPI_OP3, ®s[16]); > + MEM_R32(hw, L1F_EF_CTRL, ®s[17]); > + MEM_R32(hw, L1F_EF_ADDR, ®s[18]); > + MEM_R32(hw, L1F_EF_DATA, ®s[19]); > + > + MEM_R32(hw, L1F_SPI_ID, ®s[20]); > + MEM_R32(hw, L1F_SPI_CFG_START, ®s[21]); > + MEM_R32(hw, L1F_PMCTRL, ®s[22]); > + MEM_R32(hw, L1F_LTSSM_CTRL, ®s[23]); > + MEM_R32(hw, L1F_MASTER, ®s[24]); > + MEM_R32(hw, L1F_MANU_TIMER, ®s[25]); > + MEM_R32(hw, L1F_IRQ_MODU_TIMER, ®s[26]); > + MEM_R32(hw, L1F_PHY_CTRL, ®s[27]); > + MEM_R32(hw, L1F_MAC_STS, ®s[28]); > + MEM_R32(hw, L1F_MDIO, ®s[29]); > + > + MEM_R32(hw, L1F_MDIO_EXTN, ®s[30]); > + MEM_R32(hw, L1F_PHY_STS, ®s[31]); > + MEM_R32(hw, L1F_BIST0, ®s[32]); > + MEM_R32(hw, L1F_BIST1, ®s[33]); > + MEM_R32(hw, L1F_SERDES, ®s[34]); > + MEM_R32(hw, L1F_LED_CTRL, ®s[35]); > + MEM_R32(hw, L1F_LED_PATN, ®s[36]); > + MEM_R32(hw, L1F_LED_PATN2, ®s[37]); > + MEM_R32(hw, L1F_SYSALV, ®s[38]); > + MEM_R32(hw, L1F_PCIERR_INST, ®s[39]); > + > + MEM_R32(hw, L1F_LPI_DECISN_TIMER, ®s[40]); > + MEM_R32(hw, L1F_LPI_CTRL, ®s[41]); > + MEM_R32(hw, L1F_LPI_WAIT, ®s[42]); > + MEM_R32(hw, L1F_HRTBT_VLAN, ®s[43]); > + MEM_R32(hw, L1F_HRTBT_CTRL, ®s[44]); > + MEM_R32(hw, L1F_RXPARSE, ®s[45]); > + MEM_R32(hw, L1F_MAC_CTRL, ®s[46]); > + MEM_R32(hw, L1F_GAP, ®s[47]); > + MEM_R32(hw, L1F_STAD1, ®s[48]); > + MEM_R32(hw, L1F_LED_CTRL, ®s[49]); > + > + MEM_R32(hw, L1F_HASH_TBL0, ®s[50]); > + MEM_R32(hw, L1F_HASH_TBL1, ®s[51]); > + MEM_R32(hw, L1F_HALFD, ®s[52]); > + MEM_R32(hw, L1F_DMA, ®s[53]); > + MEM_R32(hw, L1F_WOL0, ®s[54]); > + MEM_R32(hw, L1F_WOL1, ®s[55]); > + MEM_R32(hw, L1F_WOL2, ®s[56]); > + MEM_R32(hw, L1F_WRR, ®s[57]); > + MEM_R32(hw, L1F_HQTPD, ®s[58]); > + MEM_R32(hw, L1F_CPUMAP1, ®s[59]); > + MEM_R32(hw, L1F_CPUMAP2, ®s[60]); > + MEM_R32(hw, L1F_MISC, ®s[61]);
You ought to iterate with a well chosen data structure so as to avoid this huge code pollution.
[...] > +int alf_init_hw_callbacks(struct alx_hw *hw) { > + int retval = 0; > + > + /* NIC */ > + hw->cbs.identify_nic = &alf_identify_nic; > + /* MAC */ > + hw->cbs.reset_mac = &alf_reset_mac; > + hw->cbs.start_mac = &alf_start_mac; > + hw->cbs.stop_mac = &alf_stop_mac; > + hw->cbs.config_mac = &alf_config_mac; > + hw->cbs.get_mac_addr = &alf_get_mac_addr; > + hw->cbs.set_mac_addr = &alf_set_mac_addr; > + hw->cbs.clear_mac_addr = &alf_clear_mac_addr; > + hw->cbs.set_mc_addr = &alf_set_mc_addr; > + hw->cbs.clear_mc_addr = &alf_clear_mc_addr; > + > + /* PHY */ > + hw->cbs.init_phy = &alf_init_phy; > + hw->cbs.reset_phy = &alf_reset_phy; > + hw->cbs.read_phy_reg = &alf_read_phy_reg; > + hw->cbs.write_phy_reg = &alf_write_phy_reg; > + hw->cbs.check_phy_link = &alf_check_phy_link; > + hw->cbs.setup_phy_link = &alf_setup_phy_link; > + hw->cbs.setup_phy_link_speed = &alf_setup_phy_link_speed; > + > + /* Interrupt */ > + hw->cbs.ack_phy_intr = &alf_ack_phy_intr; > + hw->cbs.enable_legacy_intr = &alf_enable_legacy_intr; > + hw->cbs.disable_legacy_intr = &alf_disable_legacy_intr; > + hw->cbs.enable_msix_intr = &alf_enable_msix_intr; > + hw->cbs.disable_msix_intr = &alf_disable_msix_intr; > + > + /* Configure */ > + hw->cbs.config_rx = &alf_config_rx; > + hw->cbs.config_tx = &alf_config_tx; > + hw->cbs.config_fc = &alf_config_fc; > + hw->cbs.config_rss = &alf_config_rss; > + hw->cbs.config_msix = &alf_config_msix; > + hw->cbs.config_wol = &alf_config_wol; > + hw->cbs.config_aspm = &alf_config_aspm; > + hw->cbs.config_mac_ctrl = &alf_config_mac_ctrl; > + hw->cbs.config_pow_save = &alf_config_pow_save; > + hw->cbs.reset_pcie = &alf_reset_pcie; > + > + /* NVRam */ > + hw->cbs.check_nvram = &alf_check_nvram; > + > + /* Others */ > + hw->cbs.get_ethtool_regs = alf_get_ethtool_regs; > + > + retval = alf_set_hw_capabilities(hw); > + > + retval = alf_set_hw_infos(hw); > + > + /* print hw flags */ > + HW_PRINT(INFO, "HW Flags = 0x%x\n", hw->flags); > + return retval;
Almost every method in this file ought to be static.
[...] > diff --git a/drivers/net/ethernet/atheros/alx/alf_hw.c > b/drivers/net/ethernet/atheros/alx/alf_hw.c > --- /dev/null > +++ b/drivers/net/ethernet/atheros/alx/alf_hw.c [...] > +/* disable/enable MAC/RXQ/TXQ > + * en > + * true:enable > + * false:disable > + * return > + * 0:success > + * non-0-fail > + */ > +u16 l1f_enable_mac(PETHCONTEXT ctx, bool en, u16 en_ctrl)
The name of this method is rather poor.
> +{ > + u32 rxq, txq, mac, val; > + u16 i; > + > + MEM_R32(ctx, L1F_RXQ0, &rxq); > + MEM_R32(ctx, L1F_TXQ0, &txq); > + MEM_R32(ctx, L1F_MAC_CTRL, &mac); > + > + if (en) { /* enable */ > + MEM_W32(ctx, L1F_RXQ0, rxq | L1F_RXQ0_EN); > + MEM_W32(ctx, L1F_TXQ0, txq | L1F_TXQ0_EN); > + if (0 != (en_ctrl & LX_MACSPEED_1000)) { > + FIELD_SETL(mac, L1F_MAC_CTRL_SPEED, > + L1F_MAC_CTRL_SPEED_1000); > + } else { > + FIELD_SETL(mac, L1F_MAC_CTRL_SPEED, > + L1F_MAC_CTRL_SPEED_10_100); > + } > + if (0 != (en_ctrl & LX_MACDUPLEX_FULL)) { > + mac |= L1F_MAC_CTRL_FULLD; > + } else { > + mac &= ~L1F_MAC_CTRL_FULLD; > + } > + /* rx filter */ > + if (0 != (en_ctrl & LX_FLT_PROMISC)) { > + mac |= L1F_MAC_CTRL_PROMISC_EN; > + } else { > + mac &= ~L1F_MAC_CTRL_PROMISC_EN; > + } > + if (0 != (en_ctrl & LX_FLT_MULTI_ALL)) { > + mac |= L1F_MAC_CTRL_MULTIALL_EN; > + } else { > + mac &= ~L1F_MAC_CTRL_MULTIALL_EN; > + } > + if (0 != (en_ctrl & LX_FLT_BROADCAST)) { > + mac |= L1F_MAC_CTRL_BRD_EN; > + } else { > + mac &= ~L1F_MAC_CTRL_BRD_EN; > + } Code duplication. Who cares ? > + if (0 != (en_ctrl & LX_FLT_DIRECT)) { > + mac |= L1F_MAC_CTRL_RX_EN; > + } else { /* disable all recv if direct not enable */ > + mac &= ~L1F_MAC_CTRL_RX_EN; > + } > + if (0 != (en_ctrl & LX_FC_TXEN)) { > + mac |= L1F_MAC_CTRL_TXFC_EN; > + } else { > + mac &= ~L1F_MAC_CTRL_TXFC_EN; > + } <blink>do it</blink> > + if (0 != (en_ctrl & LX_FC_RXEN)) { > + mac |= L1F_MAC_CTRL_RXFC_EN; > + } else { > + mac &= ~L1F_MAC_CTRL_RXFC_EN; > + } > + if (0 != (en_ctrl & LX_VLAN_STRIP)) { > + mac |= L1F_MAC_CTRL_VLANSTRIP; > + } else { > + mac &= ~L1F_MAC_CTRL_VLANSTRIP; > + } Nevermind. > + if (0 != (en_ctrl & LX_LOOPBACK)) { > + mac |= (L1F_MAC_CTRL_LPBACK_EN); > + } else { > + mac &= ~L1F_MAC_CTRL_LPBACK_EN; > + } > + if (0 != (en_ctrl & LX_SINGLE_PAUSE)) { > + mac |= L1F_MAC_CTRL_SPAUSE_EN; > + } else { > + mac &= ~L1F_MAC_CTRL_SPAUSE_EN; > + } > + if (0 != (en_ctrl & LX_ADD_FCS)) { > + mac |= (L1F_MAC_CTRL_PCRCE | L1F_MAC_CTRL_CRCE); > + } else { > + mac &= ~(L1F_MAC_CTRL_PCRCE | L1F_MAC_CTRL_CRCE); > + } > + MEM_W32(ctx, L1F_MAC_CTRL, mac | L1F_MAC_CTRL_TX_EN);
It may make some sense to move these ~60 loc in a xyz_enable_something method...
> + } else { /* disable mac */
... and this would be the xyz_disable_something counterpart.
[...] > +u16 l1f_enable_aspm(PETHCONTEXT ctx, bool l0s_en, bool l1_en, u8 > +lnk_stat) { > + u32 pmctrl; > + u8 rev = (u8)(FIELD_GETX(ctx->pci_revid, L1F_PCI_REVID)); > + > + lnk_stat = lnk_stat; > + > + > +#if 0 /* let sys bios control the L0S/L1 enable/disable */
Don't #if 0. Just remove it.
[...] > +u16 l1f_write_phy(PETHCONTEXT ctx, bool ext, u8 dev, bool fast, > + u16 reg, u16 data) > +{ > + u32 val; > + u16 clk_sel, i, ret = 0; > + > +#if MAC_TYPE_FPGA == MAC_TYPE > + MEM_W32(ctx, L1F_MDIO, 0); > + US_DELAY(ctx, 30); > + for (i = 0; i < L1F_MDIO_MAX_AC_TO; i++) { > + MEM_R32(ctx, L1F_MDIO, &val); > + if (0 == (val & L1F_MDIO_BUSY)) { > + break; > + } > + US_DELAY(ctx, 10); > + }
I wonder how many times this 'for' loop is duplicated (4x ?).
[...] > diff --git a/drivers/net/ethernet/atheros/alx/alf_hw.h > b/drivers/net/ethernet/atheros/alx/alf_hw.h > --- /dev/null > +++ b/drivers/net/ethernet/atheros/alx/alf_hw.h [...] > +#ifdef __cplusplus > +extern "C" { > +#endif/*__cplusplus*/
Oops.
[...] > +#define L1F_PCI_CMD 0x0004 /* 16bit */ > +#define L1F_PCI_CMD_DISINT BIT_10S > +#define L1F_PCI_CMD_MASTEREN BIT_2S > +#define L1F_PCI_CMD_MEMEN BIT_1S > +#define L1F_PCI_CMD_IOEN BIT_0S
Duplicates <linux/pci_regs.h>
[...] > diff --git a/drivers/net/ethernet/atheros/alx/alx_ethtool.c > b/drivers/net/ethernet/atheros/alx/alx_ethtool.c > +++ b/drivers/net/ethernet/atheros/alx/alx_ethtool.c [...] > +static struct ethtool_ops alx_ethtool_ops = { > + .get_settings = alx_get_settings, > + .set_settings = alx_set_settings, > + .get_pauseparam = alx_get_pauseparam, > + .set_pauseparam = alx_set_pauseparam, > + .get_drvinfo = alx_get_drvinfo, > + .get_regs_len = alx_get_regs_len, > + .get_regs = alx_get_regs, > + .get_wol = alx_get_wol, > + .set_wol = alx_set_wol, > + .get_msglevel = alx_get_msglevel, > + .set_msglevel = alx_set_msglevel, > + .nway_reset = alx_nway_reset, > + .get_link = ethtool_op_get_link, > + .get_eeprom_len = alx_get_eeprom_len, > + .get_eeprom = alx_get_eeprom, > + .set_eeprom = alx_set_eeprom, > + .get_tx_csum = alx_get_tx_csum, > + .get_sg = ethtool_op_get_sg, > + .set_sg = ethtool_op_set_sg, > +#ifdef NETIF_F_TSO > + .get_tso = ethtool_op_get_tso, > +#endif
Please switch to ndo_{fix/set}_features.
> diff --git a/drivers/net/ethernet/atheros/alx/alx_hwcom.h > b/drivers/net/ethernet/atheros/alx/alx_hwcom.h > +++ b/drivers/net/ethernet/atheros/alx/alx_hwcom.h [...] > +#define LX_SWAP_DW(_x) (\ > + (((_x) << 24) & 0xFF000000UL) |\ > + (((_x) << 8) & 0x00FF0000UL) |\ > + (((_x) >> 8) & 0x0000FF00UL) |\ > + (((_x) >> 24) & 0x000000FFUL)) > + > +#define LX_SWAP_W(_x) (\ > + (((_x) >> 8) & 0x00FFU) |\ > + (((_x) << 8) & 0xFF00U))
Duplicates swabXY.
[...] > +/* interop between drivers */ > +#define LX_DRV_TYPE_MASK SHIFT27(0x1FUL) > +#define LX_DRV_TYPE_SHIFT 27 > +#define LX_DRV_TYPE_UNKNOWN 0 > +#define LX_DRV_TYPE_BIOS 1 > +#define LX_DRV_TYPE_BTROM 2 > +#define LX_DRV_TYPE_PKT 3 > +#define LX_DRV_TYPE_NDS2 4 > +#define LX_DRV_TYPE_UEFI 5 > +#define LX_DRV_TYPE_NDS5 6 > +#define LX_DRV_TYPE_NDS62 7 > +#define LX_DRV_TYPE_NDS63 8 > +#define LX_DRV_TYPE_LNX 9 > +#define LX_DRV_TYPE_ODI16 10 > +#define LX_DRV_TYPE_ODI32 11 > +#define LX_DRV_TYPE_FRBSD 12 > +#define LX_DRV_TYPE_NTBSD 13 > +#define LX_DRV_TYPE_WCE 14
No.
[...] > diff --git a/drivers/net/ethernet/atheros/alx/alx_main.c > b/drivers/net/ethernet/atheros/alx/alx_main.c > --- /dev/null > +++ b/drivers/net/ethernet/atheros/alx/alx_main.c [...] > +static int alx_validate_mac_addr(u8 *mac_addr) { > + int retval = 0; > + > + if (mac_addr[0] & 0x01) { > + printk(KERN_DEBUG "MAC address is multicast\n"); > + retval = ALX_ERR_MAC_ADDR; > + } else if (mac_addr[0] == 0xff && mac_addr[1] == 0xff) { > + printk(KERN_DEBUG "MAC address is broadcast\n"); > + retval = ALX_ERR_MAC_ADDR; > + } else if (mac_addr[0] == 0 && mac_addr[1] == 0 && > + mac_addr[2] == 0 && mac_addr[3] == 0 && > + mac_addr[4] == 0 && mac_addr[5] == 0) { > + printk(KERN_DEBUG "MAC address is all zeros\n"); > + retval = ALX_ERR_MAC_ADDR; > + } > + return retval; > +}
Please see is_valid_ether_addr, is_broadcast_ether_addr.
[...] > +static void alx_receive_skb(struct alx_msix_param *msix, > + struct sk_buff *skb, > + u32 vlan_tag, bool vlan_flag) > +{ > + struct alx_adapter *adpt = msix->adpt; > + > + if (adpt->vlgrp && vlan_flag) {
This kind of vlan support code is outdated. Please see ndo_{fix/set}_features.
[...] > +/* alx_alloc_tx_descriptor - allocate Tx Descriptors */ static int > +alx_alloc_tx_descriptor(struct alx_adapter *adpt, > + struct alx_tx_queue *txque) > +{ > + struct alx_ring_header *ring_header = &adpt->ring_header; > + int size; > + > + DRV_PRINT(IF, INFO, "tpq.count = %d\n", txque->tpq.count); > + > + size = sizeof(struct alx_buffer) * txque->tpq.count; > + txque->tpq.tpbuff = kzalloc(size, GFP_KERNEL); > + if (!txque->tpq.tpbuff) > + goto err_alloc_tpq_buffer; > + memset(txque->tpq.tpbuff, 0, size);
Useless memset (kZalloc).
> + > + /* round up to nearest 4K */ > + txque->tpq.size = txque->tpq.count * sizeof(struct alx_tpdesc); > + > + txque->tpq.tpdma = ring_header->dma + ring_header->used; > + txque->tpq.tpdesc = ring_header->desc + ring_header->used; > + adpt->hw.tpdma[txque->que_idx] = (u64)txque->tpq.tpdma; > + ring_header->used += ALIGN(txque->tpq.size, 8); > + > + txque->tpq.produce_idx = 0; > + txque->tpq.consume_idx = 0; > + txque->max_packets = txque->tpq.count; > + return 0; > + > +err_alloc_tpq_buffer: > + kfree(txque->tpq.tpbuff); > + txque->tpq.tpbuff = NULL;
txque->tpq.tpbuff is already NULL before the kfree.
[...] > +static int alx_alloc_rx_descriptor(struct alx_adapter *adpt, > + struct alx_rx_queue *rxque) > +{ [...] > + rxque->rfq.rfbuff = kzalloc(size, GFP_KERNEL); > + if (!rxque->rfq.rfbuff) > + goto err_alloc_rfq_buffer; > + memset(rxque->rfq.rfbuff, 0, size);
kzalloc + useless memset.
[...] > + if (CHK_RX_FLAG(SW_QUE)) { > + size = sizeof(struct alx_sw_buffer) * rxque->swq.count; > + rxque->swq.swbuff = kzalloc(size, GFP_KERNEL); > + if (!rxque->swq.swbuff) > + goto err_alloc_swq_buffer; > + memset(rxque->swq.swbuff, 0, size); > + > + rxque->swq.consume_idx = 0; > + rxque->swq.produce_idx = 0; > + } > + > + rxque->max_packets = rxque->rrq.count / 2; > + return 0; > + > +err_alloc_swq_buffer: > + kfree(rxque->swq.swbuff); > + rxque->swq.swbuff = NULL;
rxque->swq.swbuff is already NULL before the kfree.
[...] > +static int alx_alloc_all_rtx_descriptor(struct alx_adapter *adpt) { [...] > + ring_header->desc = pci_alloc_consistent(pdev, ring_header->size, > + &ring_header->dma); > + > + if (!ring_header->desc) { > + DRV_PRINT(IF, ERR, "pci_alloc_consistend failed\n"); > + retval = -ENOMEM; > + goto err_alloc_dma; > + } > + memset(ring_header->desc, 0, ring_header->size);
- pci_alloc_consistent returns a zeroed area. - obsolescent. Ought to be dma_alloc_coherent.
[...] > +#ifdef CONFIG_PM > +int alx_suspend(struct pci_dev *pdev, pm_message_t state) { > + int retval; > + > + retval = alx_shutdown_internal(pdev, state); > + if (retval) > + return retval; > + > + return 0;
int alx_suspend(struct pci_dev *pdev, pm_message_t state) { return alx_shutdown_internal(pdev, state); }
You may consider using device_driver.pm.
[...] > +netdev_tx_t alx_start_xmit_frames(struct sk_buff *skb, > + struct alx_adapter *adpt, > + struct alx_tx_queue *txque) > +{ [...] > + if (!spin_trylock_irqsave(&adpt->tx_lock, flags)) { > + DRV_PRINT(TX, EMERG, "tx locked!\n"); > + return NETDEV_TX_LOCKED; > + }
I am not sure that NETDEV_TX_LOCKED is welcome in new drivers.
> + > + if (!alx_check_num_tpdescs(txque, skb)) { > + /* no enough descriptor, just stop queue */ > + netif_stop_queue(adpt->netdev); > + spin_unlock_irqrestore(&adpt->tx_lock, flags); > + return NETDEV_TX_BUSY; > + }
The driver has gone a bridge too far : it should disable the tx queue when it detects that it may not be able to honor the next xmit request.
[...] > +static int alx_mac_ioctl(struct net_device *netdev, > + struct ifreq *ifr, int cmd) > +{ [...] > + switch (cmd) { > + case SIOCDEVGMACREG:
SIOCDEVPRIVATE abuse.
[...] > +int __devinit alx_init(struct pci_dev *pdev, > + const struct pci_device_id *ent) { [...] > + netdev->base_addr = (unsigned long)adpt->hw.hw_addr;
Use of base_addr ought to be avoided. It's old-fashioned.
[...] > + adpt->bd_number = cards_found;
bd_number is never used. Please remove it as well as cards_found.
[...] > +static void __devexit alx_remove(struct pci_dev *pdev) { [...] > + for (que_idx = 0; que_idx < adpt->num_txques; que_idx++) { > + kfree(adpt->tx_queue[que_idx]); > + adpt->tx_queue[que_idx] = NULL; > + } > + for (que_idx = 0; que_idx < adpt->num_rxques; que_idx++) { > + kfree(adpt->rx_queue[que_idx]); > + adpt->rx_queue[que_idx] = NULL; > + }
This is duplicating alx_free_all_rtx_queue.
[...] > diff --git a/drivers/net/ethernet/atheros/alx/alx_sw.h > b/drivers/net/ethernet/atheros/alx/alx_sw.h > --- /dev/null > +++ b/drivers/net/ethernet/atheros/alx/alx_sw.h [...] > +struct alx_hw { > + struct alx_adapter *adpt; > + struct alx_hw_callbacks cbs; > + u8 __iomem *hw_addr; /* inner register address */ > + u16 pci_venid; > + u16 pci_devid; > + u16 pci_sub_devid; > + u16 pci_sub_venid; > + u8 pci_revid;
Gross duplication of fields available through adpt->pdev.
[...] > + spinlock_t mdio_lock; > + unsigned long mdio_flags;
Please leave it on the stack, thanks.
[...] > +typedef struct alx_hw ETHCONTEXT; > +typedef ETHCONTEXT * PETHCONTEXT;
Please remove the obfuscating typedefs.
[...] > +/* Error Codes */ > +#define ALX_SUCCESS 0 > + > +#define ALX_ERR_MAC -1
Unused.
> +#define ALX_ERR_MAC_INIT -2 > +#define ALX_ERR_MAC_RESET -3 > +#define ALX_ERR_MAC_START -4 > +#define ALX_ERR_MAC_STOP -5 > +#define ALX_ERR_MAC_CONFIGURE -6 > +#define ALX_ERR_MAC_ADDR -7 > + > +#define ALX_ERR_PHY -20 > +#define ALX_ERR_PHY_INIT -21
Unused.
> +#define ALX_ERR_PHY_RESET -22 > +#define ALX_ERR_PHY_SETUP_LNK -23 > +#define ALX_ERR_PHY_CHECK_LNK -24 > +#define ALX_ERR_PHY_READ_REG -25 > +#define ALX_ERR_PHY_WRITE_REG -26 > +#define ALX_ERR_PHY_RESOLVED -27 > + > +#define ALX_ERR_PCIE -40
Unused.
> +#define ALX_ERR_PCIE_RESET -41 > +#define ALX_ERR_PWR_SAVING -42 > +#define ALX_ERR_ASPM -43 > +#define ALX_ERR_EEPROM -44
Unused.
> +#define ALX_ERR_DISABLE_DRV -45 > + > +#define ALX_ERR_FLOW_CONTROL -50 > +#define ALX_ERR_FC_NOT_NEGOTIATED -51
Unused.
> +#define ALX_ERR_FC_NOT_SUPPORTED -52
Unused.
> + > +#define ALX_ERR_INVALID_ARGUMENT 0x7FFFFFFD
Unused.
> +#define ALX_ERR_NOT_SUPPORTED 0x7FFFFFFE > +#define ALX_ERR_NOT_IMPLEMENTED 0x7FFFFFFF
Unused.
-- Ueimor
| |