[PATCH] net: phy: sfp: correct store of detected link modes

[PATCH] net: phy: sfp: correct store of detected link modes

[Baruch Siach]

The link modes that sfp_parse_support() detects are stored in the
'modes' bitmap. There is no reason to make an exception for 1000Base-PX
or 1000Base-BX10.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/net/phy/sfp-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 83060fb349f4..ad9db652874d 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -162,7 +162,7 @@ void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
 	/* 1000Base-PX or 1000Base-BX10 */
 	if ((id->base.e_base_px || id->base.e_base_bx10) &&
 	    br_min <= 1300 && br_max >= 1200)
-		phylink_set(support, 1000baseX_Full);
+		phylink_set(modes, 1000baseX_Full);
 
 	/* For active or passive cables, select the link modes
 	 * based on the bit rates and the cable compliance bytes.
-- 
2.19.2

Re: [PATCH] net: phy: sfp: correct store of detected link modes

[Russell King - ARM Linux]

On Thu, Nov 29, 2018 at 12:40:11PM +0200, Baruch Siach wrote:
> The link modes that sfp_parse_support() detects are stored in the
> 'modes' bitmap. There is no reason to make an exception for 1000Base-PX
> or 1000Base-BX10.

I think you may be carrying some local patch, have an incorrect merge,
or maybe there's a patch in -next which changed this.

Mainline has:

        if (bitmap_empty(modes, __ETHTOOL_LINK_MODE_MASK_NBITS)) {
                /* If the encoding and bit rate allows 1000baseX */
                if (id->base.encoding == SFP_ENCODING_8B10B && br_nom &&
                    br_min <= 1300 && br_max >= 1200)
                        phylink_set(modes, 1000baseX_Full);
        }

but your patch changes that phylink_set() from:

                        phylink_set(support, 1000baseX_Full);

to:

                        phylink_set(modes, 1000baseX_Full);

which in the context of what's in mainline doesn't make sense.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phy: sfp: correct store of detected link modes

[Baruch Siach]

Hi Russell,

Russell King - ARM Linux writes:
> On Thu, Nov 29, 2018 at 12:40:11PM +0200, Baruch Siach wrote:
>> The link modes that sfp_parse_support() detects are stored in the
>> 'modes' bitmap. There is no reason to make an exception for 1000Base-PX
>> or 1000Base-BX10.
>
> I think you may be carrying some local patch, have an incorrect merge,
> or maybe there's a patch in -next which changed this.
>
> Mainline has:
>
>         if (bitmap_empty(modes, __ETHTOOL_LINK_MODE_MASK_NBITS)) {
>                 /* If the encoding and bit rate allows 1000baseX */
>                 if (id->base.encoding == SFP_ENCODING_8B10B && br_nom &&
>                     br_min <= 1300 && br_max >= 1200)
>                         phylink_set(modes, 1000baseX_Full);
>         }
>
> but your patch changes that phylink_set() from:
>
>                         phylink_set(support, 1000baseX_Full);
>
> to:
>
>                         phylink_set(modes, 1000baseX_Full);
>
> which in the context of what's in mainline doesn't make sense.

The code that this patch touches is at line 165 in current mainline as
of commit 60b548237fe:

    162         /* 1000Base-PX or 1000Base-BX10 */
    163         if ((id->base.e_base_px || id->base.e_base_bx10) &&
    164             br_min <= 1300 && br_max >= 1200)
    165                 phylink_set(support, 1000baseX_Full);

net-next as of e561bb29b6 carries no change in this file, as far as I
can see.

The first condition in the code snippet you cited above might
incorrectly evaluate as true because the PX and BX10 modes are currently
not reflected in the 'modes' bitmap. This patch should fix that. The
final set of modes would be the same regardless of this patch, but it's
still worth fixing for consistency, I think.

baruch

Re: [PATCH] net: phy: sfp: correct store of detected link modes

[Russell King - ARM Linux]

On Thu, Nov 29, 2018 at 02:30:53PM +0200, Baruch Siach wrote:
> Hi Russell,
> 
> Russell King - ARM Linux writes:
> > On Thu, Nov 29, 2018 at 12:40:11PM +0200, Baruch Siach wrote:
> >> The link modes that sfp_parse_support() detects are stored in the
> >> 'modes' bitmap. There is no reason to make an exception for 1000Base-PX
> >> or 1000Base-BX10.
> >
> > I think you may be carrying some local patch, have an incorrect merge,
> > or maybe there's a patch in -next which changed this.
> >
> > Mainline has:
> >
> >         if (bitmap_empty(modes, __ETHTOOL_LINK_MODE_MASK_NBITS)) {
> >                 /* If the encoding and bit rate allows 1000baseX */
> >                 if (id->base.encoding == SFP_ENCODING_8B10B && br_nom &&
> >                     br_min <= 1300 && br_max >= 1200)
> >                         phylink_set(modes, 1000baseX_Full);
> >         }
> >
> > but your patch changes that phylink_set() from:
> >
> >                         phylink_set(support, 1000baseX_Full);
> >
> > to:
> >
> >                         phylink_set(modes, 1000baseX_Full);
> >
> > which in the context of what's in mainline doesn't make sense.
> 
> The code that this patch touches is at line 165 in current mainline as
> of commit 60b548237fe:
> 
>     162         /* 1000Base-PX or 1000Base-BX10 */
>     163         if ((id->base.e_base_px || id->base.e_base_bx10) &&
>     164             br_min <= 1300 && br_max >= 1200)
>     165                 phylink_set(support, 1000baseX_Full);
> 
> net-next as of e561bb29b6 carries no change in this file, as far as I
> can see.

Ah, sorry, I was looking further down.  Yes, your change is correct -
it was missed in 03145864bd0fcac29e33442f39d67d4f28b0777c, so it needs
a fixes tag for that commit.  Probably missed by having patches hanging
around for soo long.

Fixes: 03145864bd0f ("sfp: support 1G BiDi (eg, FiberStore SFP-GE-BX) modules")
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] net: phy: sfp: correct store of detected link modes

[David Miller]

From: Baruch Siach <baruch@tkos.co.il>
Date: Thu, 29 Nov 2018 12:40:11 +0200

> The link modes that sfp_parse_support() detects are stored in the
> 'modes' bitmap. There is no reason to make an exception for 1000Base-PX
> or 1000Base-BX10.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Applied and queued up for -stable.