Skip to contentSkip to conversation

net: phy: Correctly handle MII ioctl which changes autonegotiation.

PATCH In review Brian Hill 2 participants 3 messages Nov 11, 2014 1 message not in the archive

Subsystems (3)
lore
Share & tools
lore

Brian Hill

author
#1 lore raw

PATCH 1/1 net: phy: Correctly handle MII ioctl which changes autonegotiation.

In reply to a message not in the archive

When advertised capabilities are changed with mii-tool, such as:
mii-tool -A 10baseT
the existing handler has two errors.

- An actual PHY register value is provided by mii-tool, and this must be mapped to internal state with mii_adv_to_ethtool_adv_t().
- The PHY state machine needs to be told that autonegotiation has again been performed. If not, the MAC will not be notified of the new link speed and duplex, resulting in a possible config mismatch.

---
 drivers/net/phy/phy.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)
drivers/net/phy/phy.c +24 -12
index c94e2a2..ee9f0c9 100644
@@ -352,6 +352,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 {
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
 	u16 val = mii_data->val_in;
+	int change_autoneg = 0;
 
 	switch (cmd) {
 	case SIOCGMIIPHY:
@@ -367,22 +368,29 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		if (mii_data->phy_id == phydev->addr) {
 			switch (mii_data->reg_num) {
 			case MII_BMCR:
-				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0)
+				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
+					if (phydev->autoneg == AUTONEG_ENABLE)
+						change_autoneg = 1;
 					phydev->autoneg = AUTONEG_DISABLE;
-				else
+					if (val & BMCR_FULLDPLX)
+						phydev->duplex = DUPLEX_FULL;
+					else
+						phydev->duplex = DUPLEX_HALF;
+					if (val & BMCR_SPEED1000)
+						phydev->speed = SPEED_1000;
+					else if (val & BMCR_SPEED100)
+						phydev->speed = SPEED_100;
+					else phydev->speed = SPEED_10;
+				}
+				else {
+					if (phydev->autoneg == AUTONEG_DISABLE)
+						change_autoneg = 1;
 					phydev->autoneg = AUTONEG_ENABLE;
-				if (!phydev->autoneg && (val & BMCR_FULLDPLX))
-					phydev->duplex = DUPLEX_FULL;
-				else
-					phydev->duplex = DUPLEX_HALF;
-				if (!phydev->autoneg && (val & BMCR_SPEED1000))
-					phydev->speed = SPEED_1000;
-				else if (!phydev->autoneg &&
-					 (val & BMCR_SPEED100))
-					phydev->speed = SPEED_100;
+				}
 				break;
 			case MII_ADVERTISE:
-				phydev->advertising = val;
+				phydev->advertising = mii_adv_to_ethtool_adv_t(val);
+				change_autoneg = 1;
 				break;
 			default:
 				/* do nothing */
@@ -396,6 +404,10 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		if (mii_data->reg_num == MII_BMCR &&
 		    val & BMCR_RESET)
 			return phy_init_hw(phydev);
+
+		if (change_autoneg)
+			return phy_start_aneg(phydev);
+
 		return 0;
 
 	case SIOCSHWTSTAMP:
Signature · 1 line
1.7.9.5
maintainer
#2 lore raw

From: Brian Hill <[email protected]>
Date: Tue, 11 Nov 2014 07:53:39 -0700

Quoted 23 lines

When advertised capabilities are changed with mii-tool, such as:
mii-tool -A 10baseT
the existing handler has two errors.

- An actual PHY register value is provided by mii-tool, and this must be mapped to internal state with mii_adv_to_ethtool_adv_t().
- The PHY state machine needs to be told that autonegotiation has again been performed. If not, the MAC will not be notified of the new link speed and duplex, resulting in a possible config mismatch.

---
 drivers/net/phy/phy.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)
drivers/net/phy/phy.c +0 -0
index c94e2a2..ee9f0c9 100644

drivers/net/phy/phy.c int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd) · line 355

Quoted 4 lines
drivers/net/phy/phy.c +1 -0
@@ -352,6 +352,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 {
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
 	u16 val = mii_data->val_in;
+	int change_autoneg = 0;

As Florian asked, please use bool/true/false for 'change_autoneg'.

author
#3 lore raw

PATCH 1/1 net: phy: Correctly handle MII ioctl which changes autonegotiation.

When advertised capabilities are changed with mii-tool, such as:
mii-tool -A 10baseT
the existing handler has two errors.

- An actual PHY register value is provided by mii-tool, and this must be mapped to internal state with mii_adv_to_ethtool_adv_t().
- The PHY state machine needs to be told that autonegotiation has again been performed. If not, the MAC will not be notified of the new link speed and duplex, resulting in a possible config mismatch.

---
 drivers/net/phy/phy.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)
drivers/net/phy/phy.c +24 -12
index c94e2a2..d40ca35 100644
@@ -352,6 +352,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 {
 	struct mii_ioctl_data *mii_data = if_mii(ifr);
 	u16 val = mii_data->val_in;
+	bool change_autoneg = false;
 
 	switch (cmd) {
 	case SIOCGMIIPHY:
@@ -367,22 +368,29 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		if (mii_data->phy_id == phydev->addr) {
 			switch (mii_data->reg_num) {
 			case MII_BMCR:
-				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0)
+				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
+					if (phydev->autoneg == AUTONEG_ENABLE)
+						change_autoneg = true;
 					phydev->autoneg = AUTONEG_DISABLE;
-				else
+					if (val & BMCR_FULLDPLX)
+						phydev->duplex = DUPLEX_FULL;
+					else
+						phydev->duplex = DUPLEX_HALF;
+					if (val & BMCR_SPEED1000)
+						phydev->speed = SPEED_1000;
+					else if (val & BMCR_SPEED100)
+						phydev->speed = SPEED_100;
+					else phydev->speed = SPEED_10;
+				}
+				else {
+					if (phydev->autoneg == AUTONEG_DISABLE)
+						change_autoneg = true;
 					phydev->autoneg = AUTONEG_ENABLE;
-				if (!phydev->autoneg && (val & BMCR_FULLDPLX))
-					phydev->duplex = DUPLEX_FULL;
-				else
-					phydev->duplex = DUPLEX_HALF;
-				if (!phydev->autoneg && (val & BMCR_SPEED1000))
-					phydev->speed = SPEED_1000;
-				else if (!phydev->autoneg &&
-					 (val & BMCR_SPEED100))
-					phydev->speed = SPEED_100;
+				}
 				break;
 			case MII_ADVERTISE:
-				phydev->advertising = val;
+				phydev->advertising = mii_adv_to_ethtool_adv_t(val);
+				change_autoneg = true;
 				break;
 			default:
 				/* do nothing */
@@ -396,6 +404,10 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		if (mii_data->reg_num == MII_BMCR &&
 		    val & BMCR_RESET)
 			return phy_init_hw(phydev);
+
+		if (change_autoneg)
+			return phy_start_aneg(phydev);
+
 		return 0;
 
 	case SIOCSHWTSTAMP:
Signature · 1 line
1.7.9.5