From mboxrd@z Thu Jan  1 00:00:00 1970
From: Baruch Siach <baruch@tkos.co.il>
Subject: Re: [RFC PATCH] net: mvpp2: fix detection of 10G SFP modules
Date: Thu, 29 Nov 2018 21:48:32 +0200
Message-ID: <87efb37jhr.fsf@tkos.co.il>
References: <2c150415f8a820aed0f113e0884417bc977577d3.1543495790.git.baruch@tkos.co.il> <cd79f999-f2f5-8cd4-2d2a-3b6ef9a84583@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain
Cc: Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org,
        Maxime Chevallier <maxime.chevallier@bootlin.com>,
        Antoine Tenart <antoine.tenart@bootlin.com>
To: Florian Fainelli <f.fainelli@gmail.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from guitar.tcltek.co.il ([192.115.133.116]:49353 "EHLO
        mx.tkos.co.il" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1725883AbeK3GzH (ORCPT <rfc822;netdev@vger.kernel.org>);
        Fri, 30 Nov 2018 01:55:07 -0500
In-reply-to: <cd79f999-f2f5-8cd4-2d2a-3b6ef9a84583@gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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