* [PATCH] net: phy: Correctly handle MII ioctl which changes autonegotiation.
@ 2014-11-10 19:18 Brian Hill
2014-11-11 4:17 ` Florian Fainelli
0 siblings, 1 reply; 5+ messages in thread
From: Brian Hill @ 2014-11-10 19:18 UTC (permalink / raw)
To: netdev
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.
Signed-off-by: Brian Hill <Brian@houston-radar.com>
---
drivers/net/phy/phy.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c94e2a2..ee9f0c9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] net: phy: Correctly handle MII ioctl which changes autonegotiation.
2014-11-10 19:18 [PATCH] net: phy: Correctly handle MII ioctl which changes autonegotiation Brian Hill
@ 2014-11-11 4:17 ` Florian Fainelli
0 siblings, 0 replies; 5+ messages in thread
From: Florian Fainelli @ 2014-11-11 4:17 UTC (permalink / raw)
To: Brian Hill; +Cc: netdev
Hi Brian,
2014-11-10 11:18 GMT-08:00 Brian Hill <brian@houston-radar.com>:
>
> 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.
Looks very good to me, just one minor thing I did not notice
initially. Once you fix that, feel free to resubmit with:
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>
> Signed-off-by: Brian Hill <Brian@houston-radar.com>
> ---
> drivers/net/phy/phy.c | 36 ++++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c94e2a2..ee9f0c9 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -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;
This should be a "bool autoneg = false;" here
>
> 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:
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Florian
^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <5462210F.7040306@houston-radar.com>]
* [PATCH] net: phy: Correctly handle MII ioctl which changes autonegotiation.
[not found] <5462210F.7040306@houston-radar.com>
@ 2014-11-11 14:53 ` Brian Hill
2014-11-11 20:21 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Brian Hill @ 2014-11-11 14:53 UTC (permalink / raw)
To: netdev
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.
Signed-off-by: Brian Hill <Brian@houston-radar.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/phy.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c94e2a2..ee9f0c9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] net: phy: Correctly handle MII ioctl which changes autonegotiation.
2014-11-11 14:53 ` Brian Hill
@ 2014-11-11 20:21 ` David Miller
2014-11-11 20:39 ` Brian Hill
0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2014-11-11 20:21 UTC (permalink / raw)
To: brian; +Cc: netdev
From: Brian Hill <brian@houston-radar.com>
Date: Tue, 11 Nov 2014 07:53:39 -0700
>
> 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.
>
> Signed-off-by: Brian Hill <Brian@houston-radar.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/phy/phy.c | 36 ++++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c94e2a2..ee9f0c9 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -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'.
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] net: phy: Correctly handle MII ioctl which changes autonegotiation.
2014-11-11 20:21 ` David Miller
@ 2014-11-11 20:39 ` Brian Hill
0 siblings, 0 replies; 5+ messages in thread
From: Brian Hill @ 2014-11-11 20:39 UTC (permalink / raw)
To: netdev
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.
Signed-off-by: Brian Hill <Brian@houston-radar.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/phy.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c94e2a2..d40ca35 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-11 20:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-10 19:18 [PATCH] net: phy: Correctly handle MII ioctl which changes autonegotiation Brian Hill
2014-11-11 4:17 ` Florian Fainelli
[not found] <5462210F.7040306@houston-radar.com>
2014-11-11 14:53 ` Brian Hill
2014-11-11 20:21 ` David Miller
2014-11-11 20:39 ` Brian Hill
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).