[PATCH 0/6] alx: small fixes and cleanups

[PATCH 0/6] alx: small fixes and cleanups

[Johannes Berg]

The first two are technically fixes, but I'm not sure it's worthwhile
putting them into net at this point. For the first, I simply don't
think many people use ethtool to override pause parameters. As far as
the second one is concerned, I'm not even sure what the effect is.

Thanks Ben! I'll look into the flow control negotiation another day.

johannes

[PATCH 1/6] alx: treat flow control correctly in alx_set_pauseparam()

[Johannes Berg]

Even when alx_setup_speed_duplex() is called, we still
need to call alx_cfg_mac_flowcontrol() and set hw->flowctrl
if flow control changed.

This was a bug I accidentally introduced while simplifying
the original driver.

Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/ethernet/atheros/alx/ethtool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 6fa2aec..50a91d0 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -187,7 +187,8 @@ static int alx_set_pauseparam(struct net_device *netdev,
 
 	if (reconfig_phy) {
 		err = alx_setup_speed_duplex(hw, hw->adv_cfg, fc);
-		return err;
+		if (err)
+			return err;
 	}
 
 	/* flow control on mac */
-- 
1.8.0

[PATCH 2/6] alx: fix 100mbit/full duplex speed translation

[Johannes Berg]

100mbit full duplex is ADVERTISED_100baseT_Half, not
ADVERTISED_10baseT_Half.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/ethernet/atheros/alx/hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/atheros/alx/hw.c b/drivers/net/ethernet/atheros/alx/hw.c
index 220a16a..dc71cfb 100644
--- a/drivers/net/ethernet/atheros/alx/hw.c
+++ b/drivers/net/ethernet/atheros/alx/hw.c
@@ -1134,7 +1134,7 @@ static inline u32 alx_speed_to_ethadv(int speed)
 	case SPEED_100 + DUPLEX_FULL:
 		return ADVERTISED_100baseT_Full;
 	case SPEED_100 + DUPLEX_HALF:
-		return ADVERTISED_10baseT_Half;
+		return ADVERTISED_100baseT_Half;
 	case SPEED_10 + DUPLEX_FULL:
 		return ADVERTISED_10baseT_Full;
 	case SPEED_10 + DUPLEX_HALF:
-- 
1.8.0

[PATCH 3/6] alx: make sizes unsigned

[Johannes Berg]

The ring sizes should be unsigned, pointed out by Ben Hutchings.

Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/ethernet/atheros/alx/alx.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/atheros/alx/alx.h b/drivers/net/ethernet/atheros/alx/alx.h
index 50b3ae2..d71103d 100644
--- a/drivers/net/ethernet/atheros/alx/alx.h
+++ b/drivers/net/ethernet/atheros/alx/alx.h
@@ -85,16 +85,16 @@ struct alx_priv {
 	struct {
 		dma_addr_t dma;
 		void *virt;
-		int size;
+		unsigned int size;
 	} descmem;
 
 	/* protect int_mask updates */
 	spinlock_t irq_lock;
 	u32 int_mask;
 
-	int tx_ringsz;
-	int rx_ringsz;
-	int rxbuf_size;
+	unsigned int tx_ringsz;
+	unsigned int rx_ringsz;
+	unsigned int rxbuf_size;
 
 	struct napi_struct napi;
 	struct alx_tx_queue txq;
-- 
1.8.0

[PATCH 4/6] alx: separate link speed/duplex fields

[Johannes Berg]

As suggested by Ben Hutchings, use separate fields to track
current link speed and duplex setting.

Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/ethernet/atheros/alx/ethtool.c |  35 ++------
 drivers/net/ethernet/atheros/alx/hw.c      | 136 ++++++++++++-----------------
 drivers/net/ethernet/atheros/alx/hw.h      |  24 ++++-
 drivers/net/ethernet/atheros/alx/main.c    |  37 ++++----
 4 files changed, 106 insertions(+), 126 deletions(-)

diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 50a91d0..2170b61 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -58,7 +58,8 @@ static int alx_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 			  SUPPORTED_100baseT_Full |
 			  SUPPORTED_Autoneg |
 			  SUPPORTED_TP |
-			  SUPPORTED_Pause;
+			  SUPPORTED_Pause |
+			  SUPPORTED_Asym_Pause;
 	if (alx_hw_giga(hw))
 		ecmd->supported |= SUPPORTED_1000baseT_Full;
 
@@ -68,6 +69,7 @@ static int alx_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 
 	ecmd->port = PORT_TP;
 	ecmd->phy_address = 0;
+
 	if (hw->adv_cfg & ADVERTISED_Autoneg)
 		ecmd->autoneg = AUTONEG_ENABLE;
 	else
@@ -85,14 +87,8 @@ static int alx_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 		}
 	}
 
-	if (hw->link_speed != SPEED_UNKNOWN) {
-		ethtool_cmd_speed_set(ecmd,
-				      hw->link_speed - hw->link_speed % 10);
-		ecmd->duplex = hw->link_speed % 10;
-	} else {
-		ethtool_cmd_speed_set(ecmd, SPEED_UNKNOWN);
-		ecmd->duplex = DUPLEX_UNKNOWN;
-	}
+	ethtool_cmd_speed_set(ecmd, hw->link_speed);
+	ecmd->duplex = hw->duplex;
 
 	return 0;
 }
@@ -110,24 +106,11 @@ static int alx_set_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 			return -EINVAL;
 		adv_cfg = ecmd->advertising | ADVERTISED_Autoneg;
 	} else {
-		int speed = ethtool_cmd_speed(ecmd);
-
-		switch (speed + ecmd->duplex) {
-		case SPEED_10 + DUPLEX_HALF:
-			adv_cfg = ADVERTISED_10baseT_Half;
-			break;
-		case SPEED_10 + DUPLEX_FULL:
-			adv_cfg = ADVERTISED_10baseT_Full;
-			break;
-		case SPEED_100 + DUPLEX_HALF:
-			adv_cfg = ADVERTISED_100baseT_Half;
-			break;
-		case SPEED_100 + DUPLEX_FULL:
-			adv_cfg = ADVERTISED_100baseT_Full;
-			break;
-		default:
+		adv_cfg = alx_speed_to_ethadv(ethtool_cmd_speed(ecmd),
+					      ecmd->duplex);
+
+		if (!adv_cfg || adv_cfg == ADVERTISED_1000baseT_Full)
 			return -EINVAL;
-		}
 	}
 
 	hw->adv_cfg = adv_cfg;
diff --git a/drivers/net/ethernet/atheros/alx/hw.c b/drivers/net/ethernet/atheros/alx/hw.c
index dc71cfb..2a2bbe9 100644
--- a/drivers/net/ethernet/atheros/alx/hw.c
+++ b/drivers/net/ethernet/atheros/alx/hw.c
@@ -624,12 +624,12 @@ void alx_start_mac(struct alx_hw *hw)
 	alx_write_mem32(hw, ALX_TXQ0, txq | ALX_TXQ0_EN);
 
 	mac = hw->rx_ctrl;
-	if (hw->link_speed % 10 == DUPLEX_FULL)
+	if (hw->duplex == DUPLEX_FULL)
 		mac |= ALX_MAC_CTRL_FULLD;
 	else
 		mac &= ~ALX_MAC_CTRL_FULLD;
 	ALX_SET_FIELD(mac, ALX_MAC_CTRL_SPEED,
-		      hw->link_speed >= SPEED_1000 ? ALX_MAC_CTRL_SPEED_1000 :
+		      hw->link_speed == SPEED_1000 ? ALX_MAC_CTRL_SPEED_1000 :
 						     ALX_MAC_CTRL_SPEED_10_100);
 	mac |= ALX_MAC_CTRL_TX_EN | ALX_MAC_CTRL_RX_EN;
 	hw->rx_ctrl = mac;
@@ -790,28 +790,22 @@ void alx_post_phy_link(struct alx_hw *hw)
 	u16 phy_val, len, agc;
 	u8 revid = alx_hw_revision(hw);
 	bool adj_th = revid == ALX_REV_B0;
-	int speed;
-
-	if (hw->link_speed == SPEED_UNKNOWN)
-		speed = SPEED_UNKNOWN;
-	else
-		speed = hw->link_speed - hw->link_speed % 10;
 
 	if (revid != ALX_REV_B0 && !alx_is_rev_a(revid))
 		return;
 
 	/* 1000BT/AZ, wrong cable length */
-	if (speed != SPEED_UNKNOWN) {
+	if (hw->link_speed != SPEED_UNKNOWN) {
 		alx_read_phy_ext(hw, ALX_MIIEXT_PCS, ALX_MIIEXT_CLDCTRL6,
 				 &phy_val);
 		len = ALX_GET_FIELD(phy_val, ALX_CLDCTRL6_CAB_LEN);
 		alx_read_phy_dbg(hw, ALX_MIIDBG_AGC, &phy_val);
 		agc = ALX_GET_FIELD(phy_val, ALX_AGC_2_VGA);
 
-		if ((speed == SPEED_1000 &&
+		if ((hw->link_speed == SPEED_1000 &&
 		     (len > ALX_CLDCTRL6_CAB_LEN_SHORT1G ||
 		      (len == 0 && agc > ALX_AGC_LONG1G_LIMT))) ||
-		    (speed == SPEED_100 &&
+		    (hw->link_speed == SPEED_100 &&
 		     (len > ALX_CLDCTRL6_CAB_LEN_SHORT100M ||
 		      (len == 0 && agc > ALX_AGC_LONG100M_LIMT)))) {
 			alx_write_phy_dbg(hw, ALX_MIIDBG_AZ_ANADECT,
@@ -831,10 +825,10 @@ void alx_post_phy_link(struct alx_hw *hw)
 
 		/* threshold adjust */
 		if (adj_th && hw->lnk_patch) {
-			if (speed == SPEED_100) {
+			if (hw->link_speed == SPEED_100) {
 				alx_write_phy_dbg(hw, ALX_MIIDBG_MSE16DB,
 						  ALX_MSE16DB_UP);
-			} else if (speed == SPEED_1000) {
+			} else if (hw->link_speed == SPEED_1000) {
 				/*
 				 * Giga link threshold, raise the tolerance of
 				 * noise 50%
@@ -869,7 +863,7 @@ void alx_post_phy_link(struct alx_hw *hw)
  *    1. phy link must be established before calling this function
  *    2. wol option (pattern,magic,link,etc.) is configed before call it.
  */
-int alx_pre_suspend(struct alx_hw *hw, int speed)
+int alx_pre_suspend(struct alx_hw *hw, int speed, u8 duplex)
 {
 	u32 master, mac, phy, val;
 	int err = 0;
@@ -897,9 +891,9 @@ int alx_pre_suspend(struct alx_hw *hw, int speed)
 			mac |= ALX_MAC_CTRL_RX_EN | ALX_MAC_CTRL_BRD_EN;
 		if (hw->sleep_ctrl & ALX_SLEEP_CIFS)
 			mac |= ALX_MAC_CTRL_TX_EN;
-		if (speed % 10 == DUPLEX_FULL)
+		if (duplex == DUPLEX_FULL)
 			mac |= ALX_MAC_CTRL_FULLD;
-		if (speed >= SPEED_1000)
+		if (speed == SPEED_1000)
 			ALX_SET_FIELD(mac, ALX_MAC_CTRL_SPEED,
 				      ALX_MAC_CTRL_SPEED_1000);
 		phy |= ALX_PHY_CTRL_DSPRST_OUT;
@@ -938,7 +932,7 @@ bool alx_phy_configured(struct alx_hw *hw)
 	return cfg == hw_cfg;
 }
 
-int alx_get_phy_link(struct alx_hw *hw, int *speed)
+int alx_read_phy_link(struct alx_hw *hw)
 {
 	struct pci_dev *pdev = hw->pdev;
 	u16 bmsr, giga;
@@ -953,7 +947,8 @@ int alx_get_phy_link(struct alx_hw *hw, int *speed)
 		return err;
 
 	if (!(bmsr & BMSR_LSTATUS)) {
-		*speed = SPEED_UNKNOWN;
+		hw->link_speed = SPEED_UNKNOWN;
+		hw->duplex = DUPLEX_UNKNOWN;
 		return 0;
 	}
 
@@ -967,20 +962,20 @@ int alx_get_phy_link(struct alx_hw *hw, int *speed)
 
 	switch (giga & ALX_GIGA_PSSR_SPEED) {
 	case ALX_GIGA_PSSR_1000MBS:
-		*speed = SPEED_1000;
+		hw->link_speed = SPEED_1000;
 		break;
 	case ALX_GIGA_PSSR_100MBS:
-		*speed = SPEED_100;
+		hw->link_speed = SPEED_100;
 		break;
 	case ALX_GIGA_PSSR_10MBS:
-		*speed = SPEED_10;
+		hw->link_speed = SPEED_10;
 		break;
 	default:
 		goto wrong_speed;
 	}
 
-	*speed += (giga & ALX_GIGA_PSSR_DPLX) ? DUPLEX_FULL : DUPLEX_HALF;
-	return 1;
+	hw->duplex = (giga & ALX_GIGA_PSSR_DPLX) ? DUPLEX_FULL : DUPLEX_HALF;
+	return 0;
 
 wrong_speed:
 	dev_err(&pdev->dev, "invalid PHY speed/duplex: 0x%x\n", giga);
@@ -1126,34 +1121,16 @@ void alx_configure_basic(struct alx_hw *hw)
 	alx_write_mem32(hw, ALX_WRR, val);
 }
 
-static inline u32 alx_speed_to_ethadv(int speed)
-{
-	switch (speed) {
-	case SPEED_1000 + DUPLEX_FULL:
-		return ADVERTISED_1000baseT_Full;
-	case SPEED_100 + DUPLEX_FULL:
-		return ADVERTISED_100baseT_Full;
-	case SPEED_100 + DUPLEX_HALF:
-		return ADVERTISED_100baseT_Half;
-	case SPEED_10 + DUPLEX_FULL:
-		return ADVERTISED_10baseT_Full;
-	case SPEED_10 + DUPLEX_HALF:
-		return ADVERTISED_10baseT_Half;
-	default:
-		return 0;
-	}
-}
-
-int alx_select_powersaving_speed(struct alx_hw *hw, int *speed)
+int alx_select_powersaving_speed(struct alx_hw *hw, int *speed, u8 *duplex)
 {
-	int i, err, spd;
+	int i, err;
 	u16 lpa;
 
-	err = alx_get_phy_link(hw, &spd);
-	if (err < 0)
+	err = alx_read_phy_link(hw);
+	if (err)
 		return err;
 
-	if (spd == SPEED_UNKNOWN)
+	if (hw->link_speed == SPEED_UNKNOWN)
 		return 0;
 
 	err = alx_read_phy_reg(hw, MII_LPA, &lpa);
@@ -1161,46 +1138,47 @@ int alx_select_powersaving_speed(struct alx_hw *hw, int *speed)
 		return err;
 
 	if (!(lpa & LPA_LPACK)) {
-		*speed = spd;
+		*speed = hw->link_speed;
 		return 0;
 	}
 
-	if (lpa & LPA_10FULL)
-		*speed = SPEED_10 + DUPLEX_FULL;
-	else if (lpa & LPA_10HALF)
-		*speed = SPEED_10 + DUPLEX_HALF;
-	else if (lpa & LPA_100FULL)
-		*speed = SPEED_100 + DUPLEX_FULL;
-	else
-		*speed = SPEED_100 + DUPLEX_HALF;
-
-	if (*speed != spd) {
-		err = alx_write_phy_reg(hw, ALX_MII_IER, 0);
-		if (err)
-			return err;
-		err = alx_setup_speed_duplex(hw,
-					     alx_speed_to_ethadv(*speed) |
-					     ADVERTISED_Autoneg,
-					     ALX_FC_ANEG | ALX_FC_RX |
-					     ALX_FC_TX);
-		if (err)
-			return err;
+	if (lpa & LPA_10FULL) {
+		*speed = SPEED_10;
+		*duplex = DUPLEX_FULL;
+	} else if (lpa & LPA_10HALF) {
+		*speed = SPEED_10;
+		*duplex = DUPLEX_HALF;
+	} else if (lpa & LPA_100FULL) {
+		*speed = SPEED_100;
+		*duplex = DUPLEX_FULL;
+	} else {
+		*speed = SPEED_100;
+		*duplex = DUPLEX_HALF;
+	}
 
-		/* wait for linkup */
-		for (i = 0; i < ALX_MAX_SETUP_LNK_CYCLE; i++) {
-			int speed2;
+	if (*speed == hw->link_speed && *duplex == hw->duplex)
+		return 0;
+	err = alx_write_phy_reg(hw, ALX_MII_IER, 0);
+	if (err)
+		return err;
+	err = alx_setup_speed_duplex(hw, alx_speed_to_ethadv(*speed, *duplex) |
+					 ADVERTISED_Autoneg, ALX_FC_ANEG |
+					 ALX_FC_RX | ALX_FC_TX);
+	if (err)
+		return err;
 
-			msleep(100);
+	/* wait for linkup */
+	for (i = 0; i < ALX_MAX_SETUP_LNK_CYCLE; i++) {
+		msleep(100);
 
-			err = alx_get_phy_link(hw, &speed2);
-			if (err < 0)
-				return err;
-			if (speed2 != SPEED_UNKNOWN)
-				break;
-		}
-		if (i == ALX_MAX_SETUP_LNK_CYCLE)
-			return -ETIMEDOUT;
+		err = alx_read_phy_link(hw);
+		if (err < 0)
+			return err;
+		if (hw->link_speed != SPEED_UNKNOWN)
+			break;
 	}
+	if (i == ALX_MAX_SETUP_LNK_CYCLE)
+		return -ETIMEDOUT;
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
index 65e723d..a60e35c 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -412,10 +412,11 @@ struct alx_hw {
 	u32 smb_timer;
 	/* SPEED_* + DUPLEX_*, SPEED_UNKNOWN if link is down */
 	int link_speed;
+	u8 duplex;
 
 	/* auto-neg advertisement or force mode config */
-	u32 adv_cfg;
 	u8 flowctrl;
+	u32 adv_cfg;
 
 	u32 sleep_ctrl;
 
@@ -478,12 +479,12 @@ void alx_reset_pcie(struct alx_hw *hw);
 void alx_enable_aspm(struct alx_hw *hw, bool l0s_en, bool l1_en);
 int alx_setup_speed_duplex(struct alx_hw *hw, u32 ethadv, u8 flowctrl);
 void alx_post_phy_link(struct alx_hw *hw);
-int alx_pre_suspend(struct alx_hw *hw, int speed);
+int alx_pre_suspend(struct alx_hw *hw, int speed, u8 duplex);
 int alx_read_phy_reg(struct alx_hw *hw, u16 reg, u16 *phy_data);
 int alx_write_phy_reg(struct alx_hw *hw, u16 reg, u16 phy_data);
 int alx_read_phy_ext(struct alx_hw *hw, u8 dev, u16 reg, u16 *pdata);
 int alx_write_phy_ext(struct alx_hw *hw, u8 dev, u16 reg, u16 data);
-int alx_get_phy_link(struct alx_hw *hw, int *speed);
+int alx_read_phy_link(struct alx_hw *hw);
 int alx_clear_phy_intr(struct alx_hw *hw);
 int alx_config_wol(struct alx_hw *hw);
 void alx_cfg_mac_flowcontrol(struct alx_hw *hw, u8 fc);
@@ -493,7 +494,22 @@ void alx_set_macaddr(struct alx_hw *hw, const u8 *addr);
 bool alx_phy_configured(struct alx_hw *hw);
 void alx_configure_basic(struct alx_hw *hw);
 void alx_disable_rss(struct alx_hw *hw);
-int alx_select_powersaving_speed(struct alx_hw *hw, int *speed);
+int alx_select_powersaving_speed(struct alx_hw *hw, int *speed, u8 *duplex);
 bool alx_get_phy_info(struct alx_hw *hw);
 
+static inline u32 alx_speed_to_ethadv(int speed, u8 duplex)
+{
+	if (speed == SPEED_1000 && duplex == DUPLEX_FULL)
+		return ADVERTISED_1000baseT_Full;
+	if (speed == SPEED_100 && duplex == DUPLEX_FULL)
+		return ADVERTISED_100baseT_Full;
+	if (speed == SPEED_100 && duplex== DUPLEX_HALF)
+		return ADVERTISED_100baseT_Half;
+	if (speed == SPEED_10 && duplex == DUPLEX_FULL)
+		return ADVERTISED_10baseT_Full;
+	if (speed == SPEED_10 && duplex == DUPLEX_HALF)
+		return ADVERTISED_10baseT_Half;
+	return 0;
+}
+
 #endif
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index 418de8b..148b4b9 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -712,6 +712,7 @@ static int alx_init_sw(struct alx_priv *alx)
 	hw->dma_chnl = hw->max_dma_chnl;
 	hw->ith_tpd = alx->tx_ringsz / 3;
 	hw->link_speed = SPEED_UNKNOWN;
+	hw->duplex = DUPLEX_UNKNOWN;
 	hw->adv_cfg = ADVERTISED_Autoneg |
 		      ADVERTISED_10baseT_Half |
 		      ADVERTISED_10baseT_Full |
@@ -758,6 +759,7 @@ static void alx_halt(struct alx_priv *alx)
 
 	alx_netif_stop(alx);
 	hw->link_speed = SPEED_UNKNOWN;
+	hw->duplex = DUPLEX_UNKNOWN;
 
 	alx_reset_mac(hw);
 
@@ -869,18 +871,18 @@ static void __alx_stop(struct alx_priv *alx)
 	alx_free_rings(alx);
 }
 
-static const char *alx_speed_desc(u16 speed)
+static const char *alx_speed_desc(struct alx_hw *hw)
 {
-	switch (speed) {
-	case SPEED_1000 + DUPLEX_FULL:
+	switch (alx_speed_to_ethadv(hw->link_speed, hw->duplex)) {
+	case ADVERTISED_1000baseT_Full:
 		return "1 Gbps Full";
-	case SPEED_100 + DUPLEX_FULL:
+	case ADVERTISED_100baseT_Full:
 		return "100 Mbps Full";
-	case SPEED_100 + DUPLEX_HALF:
+	case ADVERTISED_100baseT_Half:
 		return "100 Mbps Half";
-	case SPEED_10 + DUPLEX_FULL:
+	case ADVERTISED_10baseT_Full:
 		return "10 Mbps Full";
-	case SPEED_10 + DUPLEX_HALF:
+	case ADVERTISED_10baseT_Half:
 		return "10 Mbps Half";
 	default:
 		return "Unknown speed";
@@ -891,7 +893,8 @@ static void alx_check_link(struct alx_priv *alx)
 {
 	struct alx_hw *hw = &alx->hw;
 	unsigned long flags;
-	int speed, old_speed;
+	int old_speed;
+	u8 old_duplex;
 	int err;
 
 	/* clear PHY internal interrupt status, otherwise the main
@@ -899,7 +902,9 @@ static void alx_check_link(struct alx_priv *alx)
 	 */
 	alx_clear_phy_intr(hw);
 
-	err = alx_get_phy_link(hw, &speed);
+	old_speed = hw->link_speed;
+	old_duplex = hw->duplex;
+	err = alx_read_phy_link(hw);
 	if (err < 0)
 		goto reset;
 
@@ -908,15 +913,12 @@ static void alx_check_link(struct alx_priv *alx)
 	alx_write_mem32(hw, ALX_IMR, alx->int_mask);
 	spin_unlock_irqrestore(&alx->irq_lock, flags);
 
-	old_speed = hw->link_speed;
-
-	if (old_speed == speed)
+	if (old_speed == hw->link_speed)
 		return;
-	hw->link_speed = speed;
 
-	if (speed != SPEED_UNKNOWN) {
+	if (hw->link_speed != SPEED_UNKNOWN) {
 		netif_info(alx, link, alx->dev,
-			   "NIC Up: %s\n", alx_speed_desc(speed));
+			   "NIC Up: %s\n", alx_speed_desc(hw));
 		alx_post_phy_link(hw);
 		alx_enable_aspm(hw, true, true);
 		alx_start_mac(hw);
@@ -965,6 +967,7 @@ static int __alx_shutdown(struct pci_dev *pdev, bool *wol_en)
 	struct net_device *netdev = alx->dev;
 	struct alx_hw *hw = &alx->hw;
 	int err, speed;
+	u8 duplex;
 
 	netif_device_detach(netdev);
 
@@ -977,13 +980,13 @@ static int __alx_shutdown(struct pci_dev *pdev, bool *wol_en)
 		return err;
 #endif
 
-	err = alx_select_powersaving_speed(hw, &speed);
+	err = alx_select_powersaving_speed(hw, &speed, &duplex);
 	if (err)
 		return err;
 	err = alx_clear_phy_intr(hw);
 	if (err)
 		return err;
-	err = alx_pre_suspend(hw, speed);
+	err = alx_pre_suspend(hw, speed, duplex);
 	if (err)
 		return err;
 	err = alx_config_wol(hw);
-- 
1.8.0

[PATCH 5/6] alx: fix MAC address alignment problem

[Johannes Berg]

In two places, parts of MAC addresses are used as u32/u16
values. This can cause alignment problems, use put_unaligned
and get_unaligned to fix this.

Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/ethernet/atheros/alx/hw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/atheros/alx/hw.c b/drivers/net/ethernet/atheros/alx/hw.c
index 2a2bbe9..6b34855 100644
--- a/drivers/net/ethernet/atheros/alx/hw.c
+++ b/drivers/net/ethernet/atheros/alx/hw.c
@@ -282,8 +282,8 @@ static bool alx_read_macaddr(struct alx_hw *hw, u8 *addr)
 	mac1 = alx_read_mem32(hw, ALX_STAD1);
 
 	/* addr should be big-endian */
-	*(__be32 *)(addr + 2) = cpu_to_be32(mac0);
-	*(__be16 *)addr = cpu_to_be16(mac1);
+	put_unaligned(cpu_to_be32(mac0), (__be32 *)(addr + 2));
+	put_unaligned(cpu_to_be16(mac1), (__be16 *)addr);
 
 	return is_valid_ether_addr(addr);
 }
@@ -326,9 +326,9 @@ void alx_set_macaddr(struct alx_hw *hw, const u8 *addr)
 	u32 val;
 
 	/* for example: 00-0B-6A-F6-00-DC * STAD0=6AF600DC, STAD1=000B */
-	val = be32_to_cpu(*(__be32 *)(addr + 2));
+	val = be32_to_cpu(get_unaligned((__be32 *)(addr + 2)));
 	alx_write_mem32(hw, ALX_STAD0, val);
-	val = be16_to_cpu(*(__be16 *)addr);
+	val = be16_to_cpu(get_unaligned((__be16 *)addr));
 	alx_write_mem32(hw, ALX_STAD1, val);
 }
 
-- 
1.8.0

[PATCH 6/6] alx: fix ethtool support code

[Johannes Berg]

A number of places treated features wrongly, listing not-supported
features instead of supported ones. Also, the get_drvinfo ethtool
callback isn't needed, and alx_get_pauseparam can be simplified.

Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/ethernet/atheros/alx/ethtool.c | 62 +++++++++++++-----------------
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 2170b61..0f4c79e 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -46,22 +46,35 @@
 #include "reg.h"
 #include "hw.h"
 
+static u32 alx_get_supported_speeds(struct alx_hw *hw)
+{
+	u32 supported = SUPPORTED_10baseT_Half |
+			SUPPORTED_10baseT_Full |
+			SUPPORTED_100baseT_Half |
+			SUPPORTED_100baseT_Full;
+
+	if (alx_hw_giga(hw))
+		supported |= SUPPORTED_1000baseT_Full;
+
+	BUILD_BUG_ON(SUPPORTED_10baseT_Half != ADVERTISED_10baseT_Half);
+	BUILD_BUG_ON(SUPPORTED_10baseT_Full != ADVERTISED_10baseT_Full);
+	BUILD_BUG_ON(SUPPORTED_100baseT_Half != ADVERTISED_100baseT_Half);
+	BUILD_BUG_ON(SUPPORTED_100baseT_Full != ADVERTISED_100baseT_Full);
+	BUILD_BUG_ON(SUPPORTED_1000baseT_Full != ADVERTISED_1000baseT_Full);
+
+	return supported;
+}
 
 static int alx_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 {
 	struct alx_priv *alx = netdev_priv(netdev);
 	struct alx_hw *hw = &alx->hw;
 
-	ecmd->supported = SUPPORTED_10baseT_Half |
-			  SUPPORTED_10baseT_Full |
-			  SUPPORTED_100baseT_Half |
-			  SUPPORTED_100baseT_Full |
-			  SUPPORTED_Autoneg |
+	ecmd->supported = SUPPORTED_Autoneg |
 			  SUPPORTED_TP |
 			  SUPPORTED_Pause |
 			  SUPPORTED_Asym_Pause;
-	if (alx_hw_giga(hw))
-		ecmd->supported |= SUPPORTED_1000baseT_Full;
+	ecmd->supported |= alx_get_supported_speeds(hw);
 
 	ecmd->advertising = ADVERTISED_TP;
 	if (hw->adv_cfg & ADVERTISED_Autoneg)
@@ -102,7 +115,7 @@ static int alx_set_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 	ASSERT_RTNL();
 
 	if (ecmd->autoneg == AUTONEG_ENABLE) {
-		if (ecmd->advertising & ADVERTISED_1000baseT_Half)
+		if (ecmd->advertising & ~alx_get_supported_speeds(hw))
 			return -EINVAL;
 		adv_cfg = ecmd->advertising | ADVERTISED_Autoneg;
 	} else {
@@ -123,21 +136,10 @@ static void alx_get_pauseparam(struct net_device *netdev,
 	struct alx_priv *alx = netdev_priv(netdev);
 	struct alx_hw *hw = &alx->hw;
 
-	if (hw->flowctrl & ALX_FC_ANEG &&
-	    hw->adv_cfg & ADVERTISED_Autoneg)
-		pause->autoneg = AUTONEG_ENABLE;
-	else
-		pause->autoneg = AUTONEG_DISABLE;
-
-	if (hw->flowctrl & ALX_FC_TX)
-		pause->tx_pause = 1;
-	else
-		pause->tx_pause = 0;
-
-	if (hw->flowctrl & ALX_FC_RX)
-		pause->rx_pause = 1;
-	else
-		pause->rx_pause = 0;
+	pause->autoneg = !!(hw->flowctrl & ALX_FC_ANEG &&
+			    hw->adv_cfg & ADVERTISED_Autoneg);
+	pause->tx_pause = !!(hw->flowctrl & ALX_FC_TX);
+	pause->rx_pause = !!(hw->flowctrl & ALX_FC_RX);
 }
 
 
@@ -216,8 +218,7 @@ static int alx_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	struct alx_priv *alx = netdev_priv(netdev);
 	struct alx_hw *hw = &alx->hw;
 
-	if (wol->wolopts & (WAKE_ARP | WAKE_MAGICSECURE |
-			    WAKE_UCAST | WAKE_BCAST | WAKE_MCAST))
+	if (wol->wolopts & ~(WAKE_MAGIC | WAKE_PHY))
 		return -EOPNOTSUPP;
 
 	hw->sleep_ctrl = 0;
@@ -232,22 +233,11 @@ static int alx_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	return 0;
 }
 
-static void alx_get_drvinfo(struct net_device *netdev,
-			    struct ethtool_drvinfo *drvinfo)
-{
-	struct alx_priv *alx = netdev_priv(netdev);
-
-	strlcpy(drvinfo->driver, alx_drv_name, sizeof(drvinfo->driver));
-	strlcpy(drvinfo->bus_info, pci_name(alx->hw.pdev),
-		sizeof(drvinfo->bus_info));
-}
-
 const 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_msglevel	= alx_get_msglevel,
 	.set_msglevel	= alx_set_msglevel,
 	.get_wol	= alx_get_wol,
-- 
1.8.0

RE: [PATCH 5/6] alx: fix MAC address alignment problem

[David Laight]

> In two places, parts of MAC addresses are used as u32/u16
> values. This can cause alignment problems, use put_unaligned
> and get_unaligned to fix this.

I suspect they are always 16bit aligned, so 16bit accesses
would be ok - rather than using byte accesses.
However I wouldn't know how to request this with the available
definitions....

	David

...
>  	/* addr should be big-endian */
> -	*(__be32 *)(addr + 2) = cpu_to_be32(mac0);
> -	*(__be16 *)addr = cpu_to_be16(mac1);
> +	put_unaligned(cpu_to_be32(mac0), (__be32 *)(addr + 2));
> +	put_unaligned(cpu_to_be16(mac1), (__be16 *)addr);
...
> -	val = be32_to_cpu(*(__be32 *)(addr + 2));
> +	val = be32_to_cpu(get_unaligned((__be32 *)(addr + 2)));
>  	alx_write_mem32(hw, ALX_STAD0, val);
> -	val = be16_to_cpu(*(__be16 *)addr);
> +	val = be16_to_cpu(get_unaligned((__be16 *)addr));
>  	alx_write_mem32(hw, ALX_STAD1, val);

Re: [PATCH 5/6] alx: fix MAC address alignment problem

[Johannes Berg]

On Fri, 2013-06-21 at 09:38 +0100, David Laight wrote:
> > In two places, parts of MAC addresses are used as u32/u16
> > values. This can cause alignment problems, use put_unaligned
> > and get_unaligned to fix this.
> 
> I suspect they are always 16bit aligned, so 16bit accesses
> would be ok - rather than using byte accesses.
> However I wouldn't know how to request this with the available
> definitions....

For at least one of them, there's no such guarantee.

johannes

Re: [PATCH 4/6] alx: separate link speed/duplex fields

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

On Fri, 2013-06-21 at 00:14 +0200, Johannes Berg wrote:
> As suggested by Ben Hutchings, use separate fields to track
> current link speed and duplex setting.
> 
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
>  drivers/net/ethernet/atheros/alx/ethtool.c |  35 ++------
>  drivers/net/ethernet/atheros/alx/hw.c      | 136 ++++++++++++-----------------
>  drivers/net/ethernet/atheros/alx/hw.h      |  24 ++++-
>  drivers/net/ethernet/atheros/alx/main.c    |  37 ++++----
>  4 files changed, 106 insertions(+), 126 deletions(-)
> 
> diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
> index 50a91d0..2170b61 100644
> --- a/drivers/net/ethernet/atheros/alx/ethtool.c
> +++ b/drivers/net/ethernet/atheros/alx/ethtool.c
> @@ -58,7 +58,8 @@ static int alx_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
>  			  SUPPORTED_100baseT_Full |
>  			  SUPPORTED_Autoneg |
>  			  SUPPORTED_TP |
> -			  SUPPORTED_Pause;
> +			  SUPPORTED_Pause |
> +			  SUPPORTED_Asym_Pause;
>  	if (alx_hw_giga(hw))
>  		ecmd->supported |= SUPPORTED_1000baseT_Full;
> 
[...]

This hunk belongs in patch 6.

Otherwise, I think this is fine, but I didn't look too closely.

Ben.

-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                                    A fail-safe circuit will destroy others.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 6/6] alx: fix ethtool support code

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Fri, 2013-06-21 at 00:14 +0200, Johannes Berg wrote:
> A number of places treated features wrongly, listing not-supported
> features instead of supported ones. Also, the get_drvinfo ethtool
> callback isn't needed, and alx_get_pauseparam can be simplified.
> 
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  drivers/net/ethernet/atheros/alx/ethtool.c | 62 +++++++++++++-----------------
>  1 file changed, 26 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
> index 2170b61..0f4c79e 100644
> --- a/drivers/net/ethernet/atheros/alx/ethtool.c
> +++ b/drivers/net/ethernet/atheros/alx/ethtool.c
> @@ -46,22 +46,35 @@
>  #include "reg.h"
>  #include "hw.h"
>  
> +static u32 alx_get_supported_speeds(struct alx_hw *hw)
> +{
> +	u32 supported = SUPPORTED_10baseT_Half |
> +			SUPPORTED_10baseT_Full |
> +			SUPPORTED_100baseT_Half |
> +			SUPPORTED_100baseT_Full;
> +
> +	if (alx_hw_giga(hw))
> +		supported |= SUPPORTED_1000baseT_Full;
> +
> +	BUILD_BUG_ON(SUPPORTED_10baseT_Half != ADVERTISED_10baseT_Half);
> +	BUILD_BUG_ON(SUPPORTED_10baseT_Full != ADVERTISED_10baseT_Full);
> +	BUILD_BUG_ON(SUPPORTED_100baseT_Half != ADVERTISED_100baseT_Half);
> +	BUILD_BUG_ON(SUPPORTED_100baseT_Full != ADVERTISED_100baseT_Full);
> +	BUILD_BUG_ON(SUPPORTED_1000baseT_Full != ADVERTISED_1000baseT_Full);
> +
> +	return supported;
> +}
[...]
> @@ -102,7 +115,7 @@ static int alx_set_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
>  	ASSERT_RTNL();
>  
>  	if (ecmd->autoneg == AUTONEG_ENABLE) {
> -		if (ecmd->advertising & ADVERTISED_1000baseT_Half)
> +		if (ecmd->advertising & ~alx_get_supported_speeds(hw))
[...]

This means that Pause/Asym_Pause advertising can't be directly
controlled.  It's a pain to implement that properly, though, so I
suppose this can be deferred.

Ben.

-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                                    A fail-safe circuit will destroy others.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 4/6] alx: separate link speed/duplex fields

[Johannes Berg]

On Fri, 2013-06-21 at 13:37 +0100, Ben Hutchings wrote:
> On Fri, 2013-06-21 at 00:14 +0200, Johannes Berg wrote:
> > As suggested by Ben Hutchings, use separate fields to track
> > current link speed and duplex setting.
> > 
> > Reported-by: Ben Hutchings <ben@decadent.org.uk>
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > ---
> >  drivers/net/ethernet/atheros/alx/ethtool.c |  35 ++------
> >  drivers/net/ethernet/atheros/alx/hw.c      | 136 ++++++++++++-----------------
> >  drivers/net/ethernet/atheros/alx/hw.h      |  24 ++++-
> >  drivers/net/ethernet/atheros/alx/main.c    |  37 ++++----
> >  4 files changed, 106 insertions(+), 126 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
> > index 50a91d0..2170b61 100644
> > --- a/drivers/net/ethernet/atheros/alx/ethtool.c
> > +++ b/drivers/net/ethernet/atheros/alx/ethtool.c
> > @@ -58,7 +58,8 @@ static int alx_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
> >  			  SUPPORTED_100baseT_Full |
> >  			  SUPPORTED_Autoneg |
> >  			  SUPPORTED_TP |
> > -			  SUPPORTED_Pause;
> > +			  SUPPORTED_Pause |
> > +			  SUPPORTED_Asym_Pause;
> >  	if (alx_hw_giga(hw))
> >  		ecmd->supported |= SUPPORTED_1000baseT_Full;
> > 
> [...]
> 
> This hunk belongs in patch 6.

Good catch, thanks. I'll resend 4 and 6 (tomorrow or so).

johannes

Re: [PATCH 6/6] alx: fix ethtool support code

[Johannes Berg]

> > +	u32 supported = SUPPORTED_10baseT_Half |
> > +			SUPPORTED_10baseT_Full |
> > +			SUPPORTED_100baseT_Half |
> > +			SUPPORTED_100baseT_Full;
> > +
> > +	if (alx_hw_giga(hw))
> > +		supported |= SUPPORTED_1000baseT_Full;

> >  	if (ecmd->autoneg == AUTONEG_ENABLE) {
> > -		if (ecmd->advertising & ADVERTISED_1000baseT_Half)
> > +		if (ecmd->advertising & ~alx_get_supported_speeds(hw))
> [...]
> 
> This means that Pause/Asym_Pause advertising can't be directly
> controlled.  

True.

> It's a pain to implement that properly, though, so I
> suppose this can be deferred.

I have no idea how I would do it with this chip. I have no support from
the manufacturer other than their driver, and to be honest I don't have
a huge interest in adding these features, I have no need for them anyway
since my machine is connected to a gbit/full duplex switch :-)

My main concern was getting a driver into mainline, maybe/hopefully the
Qualcomm folks are interested in adding more features to it.

johannes