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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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