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

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

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 <maxime.chevallier@bootlin.com>
>> Cc: Antoine Tenart <antoine.tenart@bootlin.com>
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>> 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