lkml.org 
[lkml]   [2012]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] net: add new QCA alx ethernet driver
cjren@qca.qualcomm.com <cjren@qca.qualcomm.com> :
[...]
> diff --git a/drivers/net/ethernet/atheros/alx/alx.h b/drivers/net/ethernet/atheros/alx/alx.h
> new file mode 100644
> index 0000000..2b4ecc1
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alx.h
[...]
> +#define ALX_VLAN_TO_TAG(_vlan, _tag) \
> + do { \
> + _tag = ((((_vlan) >> 8) & 0xFF) | (((_vlan) & 0xFF) << 8)); \
> + } while (0)
> +
> +#define ALX_TAG_TO_VLAN(_tag, _vlan) \
> + do { \
> + _vlan = ((((_tag) >> 8) & 0xFF) | (((_tag) & 0xFF) << 8)); \
> + } while (0)

You can replace these macros with non caps static inline functions

[...]
> +/*
> + * RRD : definition
> + */
> +
> +/* general parameter format of rrd */
> +struct alx_sw_rrdes_general {
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> + /* dword 0 */
> + u32 xsum:16;
> + u32 nor:4; /* number of RFD */
> + u32 si:12; /* start index of rfd-ring */
> + /* dword 1 */
> + u32 hash;
> + /* dword 2 */
> + u32 vlan_tag:16; /* vlan-tag */
> + u32 pid:8; /* Header Length of Header-Data Split. WORD unit */
> + u32 reserve0:1;

Please no bitfields nor endianness conditionals. The driver should use
cpu_to_leXY, leXY_to_cpu and plain bitmasks.

The extensive documentation is nice btw.

[...]
> +struct alx_adapter {
> + struct net_device *netdev;
> + struct pci_dev *pdev;
> + struct net_device_stats net_stats;
> + bool netdev_registered;
> + u16 bd_number; /* board number;*/
> +
> + struct alx_msix_param *msix[ALX_MAX_MSIX_INTRS];
> + struct msix_entry *msix_entries;
> + int num_msix_rxques;
> + int num_msix_txques;
> + int num_msix_noques; /* true count of msix_noques for device */
> + int num_msix_intrs;
> +
> + int min_msix_intrs;
> + int max_msix_intrs;
> +
> + /* All Descriptor memory */
> + struct alx_ring_header ring_header;
> +
> + /* TX */
> + struct alx_tx_queue *tx_queue[ALX_MAX_TX_QUEUES];
> + /* RX */
> + struct alx_rx_queue *rx_queue[ALX_MAX_RX_QUEUES];
> +
> + u16 num_txques;
> + u16 num_rxques; /* equal max(num_hw_rxques, num_sw_rxques) */
> + u16 num_hw_rxques;
> + u16 num_sw_rxques;
> + u16 max_rxques;
> + u16 max_txques;
[...]
> + spinlock_t tx_lock;
> + spinlock_t rx_lock;

You may consider packing tx and rx data into similar struct and
save cache lines.

[...]
> +#ifdef ETHTOOL_OPS_COMPAT
> +extern int ethtool_ioctl(struct ifreq *ifr);
> +#endif

It belongs to compat-something, not mainline.

> diff --git a/drivers/net/ethernet/atheros/alx/alx_abs.c b/drivers/net/ethernet/atheros/alx/alx_abs.c
> new file mode 100644
> index 0000000..7f7efb4
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alx_abs.c
[...]
> +/* PHY */
> +int alf_read_phy_reg(struct alx_hw *hw, u16 reg_addr, u16 *phy_data)
> +{
> + unsigned long flags;
^^
> + int retval = 0;
^^

Pattern: initializing retval (rc ?) is useless.

[...]
> +int alf_check_phy_link(struct alx_hw *hw, u8 *speed, bool *link_up)
> +{
> + u16 bmsr, giga;
> + int retval;
> +
> + alf_read_phy_reg(hw, MII_BMSR, &bmsr);
> + retval = alf_read_phy_reg(hw, MII_BMSR, &bmsr);
> + if (retval)
> + return retval;
> +
> + if (!(bmsr & BMSR_LSTATUS)) {
> + *link_up = false;
> + *speed = 0;
> + return retval;
> + }
> + *link_up = true;
> +
> + /* Read PHY Specific Status Register (17) */
> + retval = alf_read_phy_reg(hw, L1F_MII_GIGA_PSSR, &giga);
> + if (retval)
> + return retval;
> +
> +
> + if (!(giga & L1F_GIGA_PSSR_SPD_DPLX_RESOLVED)) {
> + alx_hw_err(hw, "error for speed duplex resolved\n");
> + return -EINVAL;
> + }
> +
> + switch (giga & L1F_GIGA_PSSR_SPEED) {
> + case L1F_GIGA_PSSR_1000MBS:
> + if (giga & L1F_GIGA_PSSR_DPLX)
> + *speed = LX_LC_1000F;
> + else
> + alx_hw_err(hw, "1000M half is invalid\n");

So it's possible to read an invalid half duplex status from a register ?

It could be worth to set retval adequately (and always have the caller
check alf_check_phy_link return status code).

[...]
> +int alf_init_mac(struct alx_hw *hw, u16 rxbuf_sz, u16 rx_qnum,
> + u16 rxring_sz, u16 tx_qnum, u16 txring_sz)
> +{
> + int retval = 0;
> +
> + l1f_init_mac_misc(hw, hw->mac_addr, hw->smb_timer, hw->imt_mod, true);
> +
> + retval = l1f_init_mac_rtx_ring_desc(hw, hw->dma.rfdmem_hi[0],
> + hw->dma.rfdmem_lo[0], hw->dma.rrdmem_lo[0],
> + rxring_sz, rxbuf_sz,
> + hw->dma.tpdmem_hi[0], hw->dma.tpdmem_lo,
> + tx_qnum, txring_sz);

The callee can infer &hw->dma from hw.

(...]
> +int alf_config_aspm(struct alx_hw *hw, bool l0s_en, bool l1_en)
> +{
> + int retval = 0;
> +
> + if (!CHK_HW_FLAG(L0S_CAP))
> + l0s_en = false;
> +
> + if (l0s_en)
> + SET_HW_FLAG(L0S_EN);
> + else
^ space after tab

[...]
> +int alf_config_msix(struct alx_hw *hw, u16 num_intrs, bool msix_en, bool msi_en)
> +{
> + u32 map[2];
> + u32 type;
> + int msix_idx;
> +
> + if (!msix_en)
> + goto configure_legacy;
> +
> + memset(map, 0, sizeof(map));
> + for (msix_idx = 0; msix_idx < num_intrs; msix_idx++) {
> + switch (msix_idx) {
> + case ALF_MSIX_INDEX_RXQ0:
> + FIELD_SETL(map[0], L1F_MSI_MAP_TBL1_RXQ0,
> + ALF_MSIX_INDEX_RXQ0);
> + break;
> + case ALF_MSIX_INDEX_RXQ1:
> + FIELD_SETL(map[0], L1F_MSI_MAP_TBL1_RXQ1,
> + ALF_MSIX_INDEX_RXQ1);
> + break;
[snip]
> + case ALF_MSIX_INDEX_PHY:
> + FIELD_SETL(map[1], L1F_MSI_MAP_TBL2_PHY,
> + ALF_MSIX_INDEX_PHY);

It could use an ALF_MSIX_XYZ indexed array.

[...]
> diff --git a/drivers/net/ethernet/atheros/alx/alx_ethtool.c b/drivers/net/ethernet/atheros/alx/alx_ethtool.c
> new file mode 100644
> index 0000000..92fb461
> --- /dev/null
> +++ b/drivers/net/ethernet/atheros/alx/alx_ethtool.c
[...]
> +static int alx_set_pauseparam(struct net_device *netdev,
> + struct ethtool_pauseparam *pause)
> +{
> + struct alx_adapter *adpt = netdev_priv(netdev);
> + struct alx_hw *hw = &adpt->hw;
> + enum alx_fc_mode req_fc_mode;
> + bool disable_fc_autoneg;
> + int retval;
> +
> + while (CHK_ADPT_FLAG(STATE_RESETTING))
> + msleep(20);
> + SET_ADPT_FLAG(STATE_RESETTING);
> +
> + req_fc_mode = hw->req_fc_mode;
> + disable_fc_autoneg = hw->disable_fc_autoneg;
> +
> +
> + if (pause->autoneg != AUTONEG_ENABLE)
> + disable_fc_autoneg = true;
> + else
> + disable_fc_autoneg = false;
> +
> + if ((pause->rx_pause && pause->tx_pause) || pause->autoneg)
> + req_fc_mode = alx_fc_full;
> + else if (pause->rx_pause && !pause->tx_pause)
> + req_fc_mode = alx_fc_rx_pause;
> + else if (!pause->rx_pause && pause->tx_pause)
> + req_fc_mode = alx_fc_tx_pause;
> + else if (!pause->rx_pause && !pause->tx_pause)
> + req_fc_mode = alx_fc_none;
> + else
> + return -EINVAL;

return in the middle of a {SET/CLI}_ADPT_FLAG(STATE_RESETTING) section ?

> +
> + if ((hw->req_fc_mode != req_fc_mode) ||
> + (hw->disable_fc_autoneg != disable_fc_autoneg)) {
> + hw->req_fc_mode = req_fc_mode;
> + hw->disable_fc_autoneg = disable_fc_autoneg;
> + if (!hw->disable_fc_autoneg) {
> + retval = alf_setup_phy_link(hw, hw->autoneg_advertised,
> + true, true);
> + }
> +
> + alf_config_fc(hw);
> + }
> +
> + CLI_ADPT_FLAG(STATE_RESETTING);
> + return 0;
> +}
> +
> +
> +static u32 alx_get_msglevel(struct net_device *netdev)
> +{
> + struct alx_adapter *adpt = netdev_priv(netdev);
> + return adpt->msg_enable;

Please insert an empty line after variables(s) declaration.

[...]
> +static void alx_get_drvinfo(struct net_device *netdev,
> + struct ethtool_drvinfo *drvinfo)
> +{
> + struct alx_adapter *adpt = netdev_priv(netdev);
> +
> + strlcpy(drvinfo->driver, alx_drv_name, sizeof(drvinfo->driver));
^^
> + strlcpy(drvinfo->fw_version, "alx", 32);

It should be "N/A".

--
Ueimor


\
 
 \ /
  Last update: 2012-09-01 01:01    [W:2.303 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site