Subject: Re: [RFC PATCH] net: mvpp2: fix detection of 10G SFP modules

From: Baruch Siach <[email-masked]>
Date: Thu, 29 Nov 2018 21:48:32 +0200

Hi Florian,

Florian Fainelli writes:
> On 11/29/2018 4:49 AM, Baruch Siach wrote:
>> The mvpp2_phylink_validate() relies on the interface field of
>> phylink_link_state to determine valid link modes. However, when called
>> from phylink_sfp_module_insert() this field in not initialized. The
>> default switch case then excludes 10G link modes. This allows 10G SFP
>> modules that are detected correctly to be configured at max rate of
>> 2.5G.
>>
>> Catch the uninitialized PHY mode case, and allow 10G rates.
>>
>> Cc: Maxime Chevallier <[email-masked]>
>> Cc: Antoine Tenart <[email-masked]>
>> Signed-off-by: Baruch Siach <[email-masked]>
>> ---
>> Is that the right fix?
>
> It would be a bit surprising that this is the right fix, you would
> expect validate to be called once everything has been parsed
> successfully from the SFP, is not that the case here? If not, can you
> find out what happens?

This is the code from phylink_sfp_module_insert() at
drivers/net/phy/phylink.c:

        __ETHTOOL_DECLARE_LINK_MODE_MASK(support) = { 0, };
        struct phylink_link_state config;
        ...

        sfp_parse_support(pl->sfp_bus, id, support);
        port = sfp_parse_port(pl->sfp_bus, id, support);

        memset(&config, 0, sizeof(config));
        linkmode_copy(config.advertising, support);
        config.interface = PHY_INTERFACE_MODE_NA;
        /* ... more 'config' fields initialization ... */
        ret = phylink_validate(pl, support, &config);

The 'interface' field is not detected at this stage. I think it is not
meant to represent the detected information, but the actual
configuration.

baruch