| Message ID | 20210106153749.6748-3-pali@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| 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 Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM. > > Such combination of bits is meaningless so assume that LOS signal is not > implemented. > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > Signed-off-by: Pali Rohár <pali@kernel.org> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote: > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote: > > From: Russell King <rmk+kernel@armlinux.org.uk> > > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM. > > > > Such combination of bits is meaningless so assume that LOS signal is not > > implemented. > > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> I'd like to send this patch irrespective of discussion on the other patches - I already have it committed in my repository with a different description, but the patch content is the same. Are you happy if I transfer Andrew's r-b tag, and convert yours to an acked-by before I send it? I'd also like to add a patch that allows 2.5G if no other modes are found, but the bitrate specified by the module allows 2.5G speed - much like we do for 1G speeds.
On Sat, Jan 09, 2021 at 03:46:01PM +0000, Russell King - ARM Linux admin wrote: > On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote: > > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote: > > > From: Russell King <rmk+kernel@armlinux.org.uk> > > > > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM. > > > > > > Such combination of bits is meaningless so assume that LOS signal is not > > > implemented. > > > > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant. > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > I'd like to send this patch irrespective of discussion on the other > patches - I already have it committed in my repository with a different > description, but the patch content is the same. > > Are you happy if I transfer Andrew's r-b tag Hi Russell If it is the same contest, no problem. I can always NACK it later... Andrew
On Sat, Jan 09, 2021 at 04:54:22PM +0100, Andrew Lunn wrote: > On Sat, Jan 09, 2021 at 03:46:01PM +0000, Russell King - ARM Linux admin wrote: > > On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote: > > > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote: > > > > From: Russell King <rmk+kernel@armlinux.org.uk> > > > > > > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both > > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM. > > > > > > > > Such combination of bits is meaningless so assume that LOS signal is not > > > > implemented. > > > > > > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant. > > > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > I'd like to send this patch irrespective of discussion on the other > > patches - I already have it committed in my repository with a different > > description, but the patch content is the same. > > > > Are you happy if I transfer Andrew's r-b tag > > Hi Russell > > If it is the same contest, no problem. I can always NACK it later... The commit message is different: net: sfp: cope with SFPs that set both LOS normal and LOS inverted 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> As I say, the actual patch is the same.
On Saturday 09 January 2021 15:46:01 Russell King - ARM Linux admin wrote: > On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote: > > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote: > > > From: Russell King <rmk+kernel@armlinux.org.uk> > > > > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM. > > > > > > Such combination of bits is meaningless so assume that LOS signal is not > > > implemented. > > > > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant. > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > I'd like to send this patch irrespective of discussion on the other > patches - I already have it committed in my repository with a different > description, but the patch content is the same. > > Are you happy if I transfer Andrew's r-b tag, and convert yours to an > acked-by before I send it? > > I'd also like to add a patch that allows 2.5G if no other modes are > found, but the bitrate specified by the module allows 2.5G speed - much > like we do for 1G speeds. Russell, should I send a new version of patch series without this patch?
On Sat, Jan 09, 2021 at 08:14:47PM +0100, Pali Rohár wrote: > On Saturday 09 January 2021 15:46:01 Russell King - ARM Linux admin wrote: > > On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote: > > > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote: > > > > From: Russell King <rmk+kernel@armlinux.org.uk> > > > > > > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both > > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM. > > > > > > > > Such combination of bits is meaningless so assume that LOS signal is not > > > > implemented. > > > > > > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant. > > > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > I'd like to send this patch irrespective of discussion on the other > > patches - I already have it committed in my repository with a different > > description, but the patch content is the same. > > > > Are you happy if I transfer Andrew's r-b tag, and convert yours to an > > acked-by before I send it? > > > > I'd also like to add a patch that allows 2.5G if no other modes are > > found, but the bitrate specified by the module allows 2.5G speed - much > > like we do for 1G speeds. > > Russell, should I send a new version of patch series without this patch? I think there's some work to be done for patch 1, so I was thinking of sending this with another SFP patch. It's really a bug fix since the existing code doesn't behave very well if both bits are set - it will toggle state on every RX_LOS event received irrespective of the RX_LOS state.
On Saturday 09 January 2021 23:19:54 Russell King - ARM Linux admin wrote: > On Sat, Jan 09, 2021 at 08:14:47PM +0100, Pali Rohár wrote: > > On Saturday 09 January 2021 15:46:01 Russell King - ARM Linux admin wrote: > > > On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote: > > > > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote: > > > > > From: Russell King <rmk+kernel@armlinux.org.uk> > > > > > > > > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both > > > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM. > > > > > > > > > > Such combination of bits is meaningless so assume that LOS signal is not > > > > > implemented. > > > > > > > > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant. > > > > > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > > > I'd like to send this patch irrespective of discussion on the other > > > patches - I already have it committed in my repository with a different > > > description, but the patch content is the same. > > > > > > Are you happy if I transfer Andrew's r-b tag, and convert yours to an > > > acked-by before I send it? > > > > > > I'd also like to add a patch that allows 2.5G if no other modes are > > > found, but the bitrate specified by the module allows 2.5G speed - much > > > like we do for 1G speeds. > > > > Russell, should I send a new version of patch series without this patch? > > I think there's some work to be done for patch 1, so I was thinking of > sending this with another SFP patch. It's really a bug fix since the > existing code doesn't behave very well if both bits are set - it will > toggle state on every RX_LOS event received irrespective of the RX_LOS > state. Ok! So I will fix what is needed for patch 1, send it with patch 3 as next version and let patch 2 to you.
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index c0a891cdcd73..15fb8f7dfe5b 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -1488,15 +1488,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); @@ -1506,18 +1510,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)