| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120831222429.GA12911@electric-eye.fr.zoreil.com>
Date: Sat, 1 Sep 2012 00:24:29 +0200
From: Francois Romieu <romieu@...zoreil.com>
To: cjren@....qualcomm.com
Cc: davem@...emloft.net, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, qca-linux-team@...lcomm.com,
nic-devel@...lcomm.com, Xiong Huang <xiong@....qualcomm.com>,
"Luis R. Rodriguez" <rodrigue@....qualcomm.com>
Subject: Re: [PATCH v3] net: add new QCA alx ethernet driver
cjren@....qualcomm.com <cjren@....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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists