| Message ID | 20201230154755.14746-4-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, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote: > 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. > > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> No, this is not co-developed. The patch content is exactly what _I_ sent you, only the commit description is your own. > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++-------------- > 1 file changed, 22 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index 73f3ecf15260..d47485ed239c 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -1475,15 +1475,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); > @@ -1493,18 +1497,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) > -- > 2.20.1 > >
On Wednesday 30 December 2020 16:13:10 Russell King - ARM Linux admin wrote: > On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote: > > 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. > > > > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > No, this is not co-developed. The patch content is exactly what _I_ > sent you, only the commit description is your own. Sorry, in this case I misunderstood usage of this Co-developed-by tag. I will remove it in next iteration of patches. > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++-------------- > > 1 file changed, 22 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > > index 73f3ecf15260..d47485ed239c 100644 > > --- a/drivers/net/phy/sfp.c > > +++ b/drivers/net/phy/sfp.c > > @@ -1475,15 +1475,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); > > @@ -1493,18 +1497,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) > > -- > > 2.20.1 > > > > > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Dec 30, 2020 at 05:57:58PM +0100, Pali Rohár wrote: > On Wednesday 30 December 2020 16:13:10 Russell King - ARM Linux admin wrote: > > On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote: > > > 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. > > > > > > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > No, this is not co-developed. The patch content is exactly what _I_ > > sent you, only the commit description is your own. > > Sorry, in this case I misunderstood usage of this Co-developed-by tag. > I will remove it in next iteration of patches. You need to mark me as the author of the code at the very least... > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > > --- > > > drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++-------------- > > > 1 file changed, 22 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > > > index 73f3ecf15260..d47485ed239c 100644 > > > --- a/drivers/net/phy/sfp.c > > > +++ b/drivers/net/phy/sfp.c > > > @@ -1475,15 +1475,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); > > > @@ -1493,18 +1497,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) > > > -- > > > 2.20.1 > > > > > > > > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
On Wed, Dec 30, 2020 at 05:06:23PM +0000, Russell King - ARM Linux admin wrote: > On Wed, Dec 30, 2020 at 05:57:58PM +0100, Pali Rohár wrote: > > On Wednesday 30 December 2020 16:13:10 Russell King - ARM Linux admin wrote: > > > On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote: > > > > 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. > > > > > > > > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > > > No, this is not co-developed. The patch content is exactly what _I_ > > > sent you, only the commit description is your own. > > > > Sorry, in this case I misunderstood usage of this Co-developed-by tag. > > I will remove it in next iteration of patches. > > You need to mark me as the author of the code at the very least... Hi Pali You also need to keep your own Signed-off-by, since the patch is coming through you. So basically, git commit --am --author="Russell King <rmk+kernel@armlinux.org.uk>" and then two Signed-off-by: lines. Andrew
On Wednesday 30 December 2020 18:17:41 Andrew Lunn wrote: > On Wed, Dec 30, 2020 at 05:06:23PM +0000, Russell King - ARM Linux admin wrote: > > On Wed, Dec 30, 2020 at 05:57:58PM +0100, Pali Rohár wrote: > > > On Wednesday 30 December 2020 16:13:10 Russell King - ARM Linux admin wrote: > > > > On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote: > > > > > 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. > > > > > > > > > > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > > > > > No, this is not co-developed. The patch content is exactly what _I_ > > > > sent you, only the commit description is your own. > > > > > > Sorry, in this case I misunderstood usage of this Co-developed-by tag. > > > I will remove it in next iteration of patches. > > > > You need to mark me as the author of the code at the very least... > > Hi Pali > > You also need to keep your own Signed-off-by, since the patch is > coming through you. > > So basically, git commit --am --author="Russell King <rmk+kernel@armlinux.org.uk>" > and then two Signed-off-by: lines. Got it, thank you!
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index 73f3ecf15260..d47485ed239c 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -1475,15 +1475,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); @@ -1493,18 +1497,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)