| Message ID | E1kyYQa-0004iR-CU@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: sfp: cope with SFPs that set both LOS normal and LOS inverted | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: linux@armlinux.org.uk |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 55 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sun, Jan 10, 2021 at 10:58:32AM +0000, Russell King wrote: > The SFP MSA defines two option bits in byte 65 to indicate how the > Rx_LOS signal on SFP pin 8 behaves: > > bit 2 - Loss of Signal implemented, signal inverted from standard > definition in SFP MSA (often called "Signal Detect"). > bit 1 - Loss of Signal implemented, signal as defined in SFP MSA > (often called "Rx_LOS"). > > Clearly, setting both bits results in a meaningless situation: it would > mean that LOS is implemented in both the normal sense (1 = signal loss) > and inverted sense (0 = signal loss). > > Unfortunately, there are modules out there which set both bits, which > will be initially interpret as "inverted" sense, and then, if the LOS > signal changes state, we will toggle between LINK_UP and WAIT_LOS > states. > > Change our LOS handling to give well defined behaviour: only interpret > these bits as meaningful if exactly one is set, otherwise treat it as > if LOS is not implemented. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Sun, 10 Jan 2021 17:48:15 +0100 Andrew Lunn wrote: > On Sun, Jan 10, 2021 at 10:58:32AM +0000, Russell King wrote: > > The SFP MSA defines two option bits in byte 65 to indicate how the > > Rx_LOS signal on SFP pin 8 behaves: > > > > bit 2 - Loss of Signal implemented, signal inverted from standard > > definition in SFP MSA (often called "Signal Detect"). > > bit 1 - Loss of Signal implemented, signal as defined in SFP MSA > > (often called "Rx_LOS"). > > > > Clearly, setting both bits results in a meaningless situation: it would > > mean that LOS is implemented in both the normal sense (1 = signal loss) > > and inverted sense (0 = signal loss). > > > > Unfortunately, there are modules out there which set both bits, which > > will be initially interpret as "inverted" sense, and then, if the LOS > > signal changes state, we will toggle between LINK_UP and WAIT_LOS > > states. > > > > Change our LOS handling to give well defined behaviour: only interpret > > these bits as meaningful if exactly one is set, otherwise treat it as > > if LOS is not implemented. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Applied, thanks!
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index 374351de2063..b2a5ed6915fa 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -1534,15 +1534,19 @@ static void sfp_sm_link_down(struct sfp *sfp) static void sfp_sm_link_check_los(struct sfp *sfp) { - unsigned int los = sfp->state & SFP_F_LOS; + const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED); + const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL); + __be16 los_options = sfp->id.ext.options & (los_inverted | los_normal); + bool los = false; /* If neither SFP_OPTIONS_LOS_INVERTED nor SFP_OPTIONS_LOS_NORMAL - * are set, we assume that no LOS signal is available. + * are set, we assume that no LOS signal is available. If both are + * set, we assume LOS is not implemented (and is meaningless.) */ - if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED)) - los ^= SFP_F_LOS; - else if (!(sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL))) - los = 0; + if (los_options == los_inverted) + los = !(sfp->state & SFP_F_LOS); + else if (los_options == los_normal) + los = !!(sfp->state & SFP_F_LOS); if (los) sfp_sm_next(sfp, SFP_S_WAIT_LOS, 0); @@ -1552,18 +1556,22 @@ static void sfp_sm_link_check_los(struct sfp *sfp) static bool sfp_los_event_active(struct sfp *sfp, unsigned int event) { - return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) && - event == SFP_E_LOS_LOW) || - (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) && - event == SFP_E_LOS_HIGH); + const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED); + const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL); + __be16 los_options = sfp->id.ext.options & (los_inverted | los_normal); + + return (los_options == los_inverted && event == SFP_E_LOS_LOW) || + (los_options == los_normal && event == SFP_E_LOS_HIGH); } static bool sfp_los_event_inactive(struct sfp *sfp, unsigned int event) { - return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) && - event == SFP_E_LOS_HIGH) || - (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) && - event == SFP_E_LOS_LOW); + const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED); + const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL); + __be16 los_options = sfp->id.ext.options & (los_inverted | los_normal); + + return (los_options == los_inverted && event == SFP_E_LOS_HIGH) || + (los_options == los_normal && event == SFP_E_LOS_LOW); } static void sfp_sm_fault(struct sfp *sfp, unsigned int next_state, bool warn)
The SFP MSA defines two option bits in byte 65 to indicate how the Rx_LOS signal on SFP pin 8 behaves: bit 2 - Loss of Signal implemented, signal inverted from standard definition in SFP MSA (often called "Signal Detect"). bit 1 - Loss of Signal implemented, signal as defined in SFP MSA (often called "Rx_LOS"). Clearly, setting both bits results in a meaningless situation: it would mean that LOS is implemented in both the normal sense (1 = signal loss) and inverted sense (0 = signal loss). Unfortunately, there are modules out there which set both bits, which will be initially interpret as "inverted" sense, and then, if the LOS signal changes state, we will toggle between LINK_UP and WAIT_LOS states. Change our LOS handling to give well defined behaviour: only interpret these bits as meaningful if exactly one is set, otherwise treat it as if LOS is not implemented. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-)