net: phy: Introduced the PHY_AN_PENDING state
Subsystems (3)
RFC PATCH 1/1 net: phy: Introduced the PHY_AN_PENDING state
The PHY_AN_PENDING state is put as a gate to enter the PHY_AN state where it will wait for any uncomplete auto-negotiation session to finish before starting a new one.
This extra state could be used to workaround some auto-negotation issues from certain vendors.
an_pending_timeout module parameter is used to enable the AN_PENDING transition state. Set it to 0 to disable AN_PENDING state transition, set it to any non-zero value to specify the timeout period for PHY_AN_PENDING state in second. The default value is 0.
an_pending_guard module parameter serves as a guard band to delay the auto-negotiation firing after the previous auto-negotiation finish.
Conflicts:
drivers/net/phy/phy.c
---
We observed failure in the ethernet link operation when our board pairs with some network switch model. The problem happens when an auto-negotiation is started around the time the previous auto-negotiation complete. We believe this might be an interoperatibility issue between the PHYs but we need a short-term solution in software to workaround the issue.
We found that we are able to avoid from hitting the problem by waiting any pending auto-negotiation to complete before starting a new one and this patch is designed to serve the purpose.
A PHY_AN_PENDING state is introduced and it will act as a gate to enter the PHY_AN state. This state will check for auto-negotiation completion or timeout after an_pending_timeout period, then it will wait for an_pending_guard before triggering another auto-negotiation.
The following diagram shows the timing diagram
an_pending_timeout an_pending_guard
V V auto-nego
|--------------------------------->|....................|
^
auto-negotiation complete/timeoutWe do not have plan to submit this patch upstream (unless the community feels this patch is useful in general) but we would like to seek for feedback or advice if this patch could introduce new problems.
--- drivers/net/phy/phy.c | 44 +++++++++++++++++++++++++++++++++++++++++++- include/linux/phy.h | 3 ++- 2 files changed, 45 insertions(+), 2 deletions(-)
drivers/net/phy/phy.c +43 -1
@@ -38,6 +38,16 @@#include <asm/irq.h> +static unsigned int an_pending_timeout; +module_param (an_pending_timeout, uint,0644 ); +MODULE_PARM_DESC (an_pending_timeout, +"Timeout period for PHY_AN_PENDING state in second. 0 to disable PHY_AN_PENDING state (default)" ); + +static unsigned int an_pending_guard; +module_param (an_pending_guard, uint,0644 ); +MODULE_PARM_DESC (an_pending_guard, +"Guard band period before firing auto-negotiation from PHY_AN_PENDING state in second. Default to 0" ); +static const char *phy_speed_to_str (int speed) {switch (speed) {
@@ -82,7 +92,6 @@ static const char *phy_state_to_str(enum phy_state st)return NULL ; } -/** * phy_print_status - Convenience function to print out the current phy status * @phydev: the phy_device struct
@@ -485,6 +494,18 @@ int phy_start_aneg(struct phy_device *phydev)/* Invalidate LP advertising flags */ phydev->lp_advertising =0 ; +if (an_pending_timeout) { +switch (phydev->state) { +case PHY_AN_PENDING : +case PHY_HALTED : +break ; +default : + phydev->state =PHY_AN_PENDING ; + phydev->link_timeout = an_pending_timeout; + goto out_unlock; + } + + } err = phydev->drv->config_aneg (phydev);if (err <0 )
@@ -831,6 +852,27 @@ void phy_state_machine(struct work_struct *work) phydev->link_timeout =PHY_AN_TIMEOUT ;break ; +case PHY_AN_PENDING : +/* Check if negotiation is done. Break if there's an error */ + err =phy_aneg_done (phydev); +if (err <0 ) +break ; + +/* If AN is done, we'll proceed with the real aneg triggering */ +if (err >0 ) { +if (phydev->link_timeout >0 ) + phydev->link_timeout = -(an_pending_guard); +else if (phydev->link_timeout <0 ) + phydev->link_timeout++; + }else + phydev->link_timeout--; + +if (0 == phydev->link_timeout) { + needs_aneg = true; + + phydev->link_timeout =PHY_AN_TIMEOUT ; + } +break ;case PHY_AN : err =phy_read_status (phydev);if (err <0 )
include/linux/phy.h +2 -1
@@ -308,7 +308,8 @@ enum phy_state {PHY_FORCING ,PHY_CHANGELINK ,PHY_HALTED , - PHY_RESUMING +PHY_RESUMING , + PHY_AN_PENDING };/**
Signature · 1 line
1.7.7.6
Quoted 6 lines · Keng Soon Cheah
The
PHY_AN_PENDINGstate is put as a gate to enter thePHY_ANstate where it will wait for any uncomplete auto-negotiation session to finish before starting a new one.This extra state could be used to workaround some auto-negotation issues from certain vendors.
The typical way to work around these problems are to fix them at the PHY driver level, see below.
Quoted 23 lines
an_pending_timeoutmodule parameter is used to enable theAN_PENDINGtransition state. Set it to 0 to disableAN_PENDINGstate transition, set it to any non-zero value to specify the timeout period forPHY_AN_PENDINGstate in second. The default value is 0.
an_pending_guardmodule parameter serves as a guard band to delay the auto-negotiation firing after the previous auto-negotiation finish.Signed-off-by Keng Soon Cheah <[email protected]>Conflicts:
drivers/net/phy/phy.c
---
We observed failure in the ethernet link operation when our board pairs with some network switch model. The problem happens when an auto-negotiation is started around the time the previous auto-negotiation complete. We believe this might be an interoperatibility issue between the PHYs but we need a short-term solution in software to workaround the issue.
Quoted 3 lines
We found that we are able to avoid from hitting the problem by waiting any pending auto-negotiation to complete before starting a new one and this patch is designed to serve the purpose.
That sounds like a bug in the PHY state machine and/or the PHY driver if you are allowed to restart auto-negotiation while one is pending. Now that the PHY state machine has debug prints built-in, could you capture a trace of this failing case?
Is this observed with the generic PHY driver or a custom PHY driver?
Quoted 12 lines
A
PHY_AN_PENDINGstate is introduced and it will act as a gate to enter thePHY_ANstate. This state will check for auto-negotiation completion or timeout afteran_pending_timeoutperiod, then it will wait foran_pending_guardbefore triggering another auto-negotiation.The following diagram shows the timing diagram
an_pending_timeoutan_pending_guard
V V auto-nego
|--------------------------------->|....................|
Quoted 6 lines
^ auto-negotiation complete/timeoutWe do not have plan to submit this patch upstream (unless the community feels this patch is useful in general) but we would like to seek for feedback or advice if this patch could introduce new problems.
As usual with state machines, introducing a new state needs to be carefully done in order to make sure that all transitions are correct, so far I would rather work on finding the root cause/extending the timeout and/or making it configurable on a PHY-driver basis rather than having this additional state which is more error prone.
Thanks!
Quoted 97 lines
--- drivers/net/phy/phy.c | 44 +++++++++++++++++++++++++++++++++++++++++++- include/linux/phy.h | 3 ++- 2 files changed, 45 insertions(+), 2 deletions(-)drivers/net/phy/phy.c +43
-1@@ -38,6 +38,16 @@#include <asm/irq.h> +static unsigned int an_pending_timeout; +module_param (an_pending_timeout, uint,0644 ); +MODULE_PARM_DESC (an_pending_timeout, +"Timeout period for PHY_AN_PENDING state in second. 0 to disable PHY_AN_PENDING state (default)" ); + +static unsigned int an_pending_guard; +module_param (an_pending_guard, uint,0644 ); +MODULE_PARM_DESC (an_pending_guard, +"Guard band period before firing auto-negotiation from PHY_AN_PENDING state in second. Default to 0" ); +static const char *phy_speed_to_str (int speed) {switch (speed) {@@ -82,7 +92,6 @@ static const char *phy_state_to_str(enum phy_state st)return NULL ; } -/** * phy_print_status - Convenience function to print out the current phy status * @phydev: the phy_device struct @@ -485,6 +494,18 @@ int phy_start_aneg(struct phy_device *phydev)/* Invalidate LP advertising flags */ phydev->lp_advertising =0 ; +if (an_pending_timeout) { +switch (phydev->state) { +case PHY_AN_PENDING : +case PHY_HALTED : +break ; +default : + phydev->state =PHY_AN_PENDING ; + phydev->link_timeout = an_pending_timeout; + goto out_unlock; + } + + } err = phydev->drv->config_aneg (phydev);if (err <0 )@@ -831,6 +852,27 @@ void phy_state_machine(struct work_struct *work) phydev->link_timeout =PHY_AN_TIMEOUT ;break ; +case PHY_AN_PENDING : +/* Check if negotiation is done. Break if there's an error */ + err =phy_aneg_done (phydev); +if (err <0 ) +break ; + +/* If AN is done, we'll proceed with the real aneg triggering */ +if (err >0 ) { +if (phydev->link_timeout >0 ) + phydev->link_timeout = -(an_pending_guard); +else if (phydev->link_timeout <0 ) + phydev->link_timeout++; + }else + phydev->link_timeout--; + +if (0 == phydev->link_timeout) { + needs_aneg = true; + + phydev->link_timeout =PHY_AN_TIMEOUT ; + } +break ;case PHY_AN : err =phy_read_status (phydev);if (err <0 )include/linux/phy.h +2
-1@@ -308,7 +308,8 @@ enum phy_state {PHY_FORCING ,PHY_CHANGELINK ,PHY_HALTED , - PHY_RESUMING +PHY_RESUMING , + PHY_AN_PENDING };/**
Signature · 1 line
Florian