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

[Baruch Siach <baruch@tkos.co.il>]

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