Re: [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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!
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!