[PATCH RFC] phylib: fix forced mode misbehaviour for aneg off case
When user disabled autonegotiation via ethtool, and no link is detected,
phylib will place phy into forcing mode, and then will start calling
phy_force_reduction(). This will break user expectations. For example,
user asks for fixed speed 1000:
ethtool -s eth0 autoneg off speed 1000
Without link attached what will actually happen is:
Trying 100/FULL
Trying 100/HALF
Trying 10/FULL
Trying 10/HALF
...
This patch implements software autonegotiation that is equivalent to
current behaviour, but enabled only when hardware autonegotiation was
enabled and failed afterwards. With aneg disabled, phylib will not try
other link setups.
Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
---
This is of course post-2.6.25 material and highly RFC, as it changes
current behaviour. Please review carefully.
Thanks.
drivers/net/phy/phy.c | 19 ---
include/linux/ethtool.h |4
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 12fccb1..35ad91f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -264,7 +264,8 @@ void phy_sanitize_settings(struct phy_device *phydev)
int idx;
/* Sanitize settings based on PHY capabilities */
- if ((features SUPPORTED_Autoneg) == 0)
+ if ((features SUPPORTED_Autoneg) == 0
+ AUTONEG_SOFT != phydev-autoneg)
phydev-autoneg = AUTONEG_DISABLE;
idx = phy_find_valid(phy_find_setting(phydev-speed, phydev-duplex),
@@ -297,13 +298,15 @@ int phy_ethtool_sset(struct phy_device *phydev, struct
ethtool_cmd *cmd)
cmd-advertising = phydev-supported;
/* Verify the settings we care about. */
- if (cmd-autoneg != AUTONEG_ENABLE cmd-autoneg != AUTONEG_DISABLE)
+ if (cmd-autoneg != AUTONEG_ENABLE
+ cmd-autoneg != AUTONEG_DISABLE
+ cmd-autoneg != AUTONEG_SOFT)
return -EINVAL;
if (cmd-autoneg == AUTONEG_ENABLE cmd-advertising == 0)
return -EINVAL;
- if (cmd-autoneg == AUTONEG_DISABLE
+ if ((cmd-autoneg == AUTONEG_DISABLE || cmd-autoneg == AUTONEG_SOFT)
((cmd-speed != SPEED_1000
cmd-speed != SPEED_100
cmd-speed != SPEED_10)
@@ -433,7 +436,8 @@ int phy_start_aneg(struct phy_device *phydev)
mutex_lock(phydev-lock);
- if (AUTONEG_DISABLE == phydev-autoneg)
+ if (AUTONEG_DISABLE == phydev-autoneg ||
+ AUTONEG_SOFT == phydev-autoneg)
phy_sanitize_settings(phydev);
err = phydev-drv-config_aneg(phydev);
@@ -447,7 +451,8 @@ int phy_start_aneg(struct phy_device *phydev)
phydev-link_timeout = PHY_AN_TIMEOUT;
} else {
phydev-state = PHY_FORCING;
- phydev-link_timeout = PHY_FORCE_TIMEOUT;
+ if (AUTONEG_SOFT == phydev-autoneg)
+ phydev-link_timeout = PHY_FORCE_TIMEOUT;
}
}
@@ -875,7 +880,7 @@ static void phy_state_machine(struct work_struct *work)
phydev-speed = settings[idx].speed;
phydev-duplex = settings[idx].duplex;
- phydev-autoneg = AUTONEG_DISABLE;
+ phydev-autoneg = AUTONEG_SOFT;
pr_info(Trying %d/%s\n, phydev-speed,
DUPLEX_FULL ==
@@ -904,7 +909,7 @@ static void phy_state_machine(struct work_struct *work)
if (phydev-link) {
phydev-state = PHY_RUNNING;
netif_carrier_on(phydev-attached_dev);
- } else {
+ } else if (AUTONEG_SOFT == phydev-autoneg) {
if (0 == phydev-link_timeout--) {
phy_force_reduction(phydev);
needs_aneg = 1;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index fcbe8b6..446f78b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -517,6 +517,10 @@ struct ethtool_ops {
*/
#define AUTONEG_DISABLE0x00
#define AUTONEG_ENABLE 0x01
+/* Software autonegotiation: will try several link variants in this
+ * order -- 1000/FULL, 1000/HALF, 100/FULL, 100/HALF, 10/FULL, 10/HALF.
+ */
+#define AUTONEG_SOFT 0x02
/* Wake-On-Lan options. */
#define WAKE_PHY (1 0)
--
1.5.2.2
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] phylib: fix forced mode misbehaviour for aneg off case
On Feb 22, 2008, at 09:55, Anton Vorontsov wrote:
When user disabled autonegotiation via ethtool, and no link is
detected,
phylib will place phy into forcing mode, and then will start calling
phy_force_reduction(). This will break user expectations. For example,
user asks for fixed speed 1000:
ethtool -s eth0 autoneg off speed 1000
Without link attached what will actually happen is:
Trying 100/FULL
Trying 100/HALF
Trying 10/FULL
Trying 10/HALF
...
The intent of phy_force_reduction() was to provide a fallback in case
the user unknowingly selects a speed that is not possible with the
current link partner. For instance, if you try to select gigabit on
a 100MB link, it wouldn't work, but because of the way the code was
designed, the phylib will find a link configuration that works.
However, I agree that it's not ideal to have the phylib spending a
lot of time looking for a link if there's not one there. On the
other hand, why is the user trying to force the link to a certain
speed if there's no link?
I'm not really opposed to it, though.
This patch implements software autonegotiation that is equivalent to
current behaviour, but enabled only when hardware autonegotiation was
enabled and failed afterwards. With aneg disabled, phylib will not try
other link setups.
Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
---
@@ -447,7 +451,8 @@ int phy_start_aneg(struct phy_device *phydev)
phydev-link_timeout = PHY_AN_TIMEOUT;
} else {
phydev-state = PHY_FORCING;
- phydev-link_timeout = PHY_FORCE_TIMEOUT;
+ if (AUTONEG_SOFT == phydev-autoneg)
+ phydev-link_timeout = PHY_FORCE_TIMEOUT;
}
}
I'm worried that phydev-link_timeout may end up being left in an
unknown state here. Are you expecting it to be 0? If so, I think it
would be best to set it to 0 in an if clause.
@@ -904,7 +909,7 @@ static void phy_state_machine(struct
work_struct *work)
if (phydev-link) {
phydev-state = PHY_RUNNING;
netif_carrier_on(phydev-attached_dev);
- } else {
+ } else if (AUTONEG_SOFT == phydev-autoneg) {
if (0 == phydev-link_timeout--) {
phy_force_reduction(phydev);
needs_aneg = 1;
Especially since this will, I believe, leave link_timeout at -1
Andy
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] phylib: fix forced mode misbehaviour for aneg off case
On Fri, Feb 22, 2008 at 11:40:04AM -0600, Andy Fleming wrote:
On Feb 22, 2008, at 09:55, Anton Vorontsov wrote:
When user disabled autonegotiation via ethtool, and no link is
detected,
phylib will place phy into forcing mode, and then will start calling
phy_force_reduction(). This will break user expectations. For example,
user asks for fixed speed 1000:
ethtool -s eth0 autoneg off speed 1000
Without link attached what will actually happen is:
Trying 100/FULL
Trying 100/HALF
Trying 10/FULL
Trying 10/HALF
...
The intent of phy_force_reduction() was to provide a fallback in case
the user unknowingly selects a speed that is not possible with the
current link partner. For instance, if you try to select gigabit on
a 100MB link, it wouldn't work, but because of the way the code was
designed, the phylib will find a link configuration that works.
Yup, with this patch phylib will not able to find suitable speed for
PHYs without hw angeg capability. The question is: do we have such
hardware or this feature was actually unused and we'll not break
anything.
We can think out something for this case, but it will be still
incompatible with old behaviour. For example, for such setups
we might introduce kernel command line option, specifying
softaneg=eth0, that will force softaneg for PHYs without hw aneg
capability.
However, I agree that it's not ideal to have the phylib spending a
lot of time looking for a link if there's not one there. On the
other hand, why is the user trying to force the link to a certain
speed if there's no link?
To set up fixed speed for the link, thus to not re-setup it when link
is gone down... This is how all drivers [not using phylib] are
behaving.
I'm not really opposed to it, though.
This patch implements software autonegotiation that is equivalent to
current behaviour, but enabled only when hardware autonegotiation was
enabled and failed afterwards. With aneg disabled, phylib will not try
other link setups.
Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
---
@@ -447,7 +451,8 @@ int phy_start_aneg(struct phy_device *phydev)
phydev-link_timeout = PHY_AN_TIMEOUT;
} else {
phydev-state = PHY_FORCING;
-phydev-link_timeout = PHY_FORCE_TIMEOUT;
+if (AUTONEG_SOFT == phydev-autoneg)
+phydev-link_timeout = PHY_FORCE_TIMEOUT;
}
}
I'm worried that phydev-link_timeout may end up being left in an
unknown state here. Are you expecting it to be 0? If so, I think it
would be best to set it to 0 in an if clause.
Um.. I though about it when I wrote this, and to me it seems we
really don't use link_timeout with AUTONEG_DISABLED...
We use it for PHY_AN, PHY_FORCING AUTONEG_SOFT, and
PHY_RESUMING AUTONEG_ENABLED.
But as a matter of safety, I probably indeed add link_timeout
zeroing...
Thanks,
--
Anton Vorontsov
email: [EMAIL PROTECTED]
backup email: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
