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

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

[Baruch Siach]

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?
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 7a37a37e3fb3..eb1dc8abc359 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -4384,6 +4384,7 @@ static void mvpp2_phylink_validate(struct net_device *dev,
 
 	switch (state->interface) {
 	case PHY_INTERFACE_MODE_10GKR:
+	case PHY_INTERFACE_MODE_NA:
 		phylink_set(mask, 10000baseCR_Full);
 		phylink_set(mask, 10000baseSR_Full);
 		phylink_set(mask, 10000baseLR_Full);
-- 
2.19.2

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

[Florian Fainelli]

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?
-- 
Florian

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

[Baruch Siach]

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

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

[Russell King - ARM Linux]

On Thu, Nov 29, 2018 at 11:31:23AM -0800, Florian Fainelli wrote:
> 
> 
> 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?

Two calls are made - the first with PHY_INTERFACE_MODE_NA to
determine what the advertising link mode may be, and then again
once the interface mode has been selected from the advertising mask.

Why?

Consider a 4.3Mbps fiberchannel SFP plugged into a 1G-only MAC.
If we did it as a single pass, we would end up passing an
interface mode of 2500BASEX first time around which is illogical.

-- 
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: [RFC PATCH] net: mvpp2: fix detection of 10G SFP modules

[Baruch Siach]

Hi Russell,

On Thu, Nov 29, 2018 at 10:00:43PM +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 29, 2018 at 11:31:23AM -0800, Florian Fainelli wrote:
> > 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?
> 
> Two calls are made - the first with PHY_INTERFACE_MODE_NA to
> determine what the advertising link mode may be, and then again
> once the interface mode has been selected from the advertising mask.
> 
> Why?
> 
> Consider a 4.3Mbps fiberchannel SFP plugged into a 1G-only MAC.
> If we did it as a single pass, we would end up passing an
> interface mode of 2500BASEX first time around which is illogical.

So you consider this to be the right fix, right?

I should have added:

Fixes: d97c9f4ab000b ("net: mvpp2: 1000baseX support")

Antoine, is this fix OK with you?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

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

[Russell King - ARM Linux]

On Tue, Dec 04, 2018 at 12:19:54PM +0200, Baruch Siach wrote:
> Hi Russell,
> 
> On Thu, Nov 29, 2018 at 10:00:43PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Nov 29, 2018 at 11:31:23AM -0800, Florian Fainelli wrote:
> > > 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?
> > 
> > Two calls are made - the first with PHY_INTERFACE_MODE_NA to
> > determine what the advertising link mode may be, and then again
> > once the interface mode has been selected from the advertising mask.
> > 
> > Why?
> > 
> > Consider a 4.3Mbps fiberchannel SFP plugged into a 1G-only MAC.
> > If we did it as a single pass, we would end up passing an
> > interface mode of 2500BASEX first time around which is illogical.
> 
> So you consider this to be the right fix, right?

Yes, but there is another bug lurking here - the handling of invalid
interface modes is not correct.  Please see mvneta.c as an example -
interface modes that are not supported by the MAC (apart from the NA
mode) end up with the supported mask completely cleared.

-- 
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: [RFC PATCH] net: mvpp2: fix detection of 10G SFP modules

[Baruch Siach]

Hi Russell,

On Tue, Dec 04, 2018 at 10:27:01AM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 04, 2018 at 12:19:54PM +0200, Baruch Siach wrote:
> > On Thu, Nov 29, 2018 at 10:00:43PM +0000, Russell King - ARM Linux wrote:
> > > On Thu, Nov 29, 2018 at 11:31:23AM -0800, Florian Fainelli wrote:
> > > > 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?
> > > 
> > > Two calls are made - the first with PHY_INTERFACE_MODE_NA to
> > > determine what the advertising link mode may be, and then again
> > > once the interface mode has been selected from the advertising mask.
> > > 
> > > Why?
> > > 
> > > Consider a 4.3Mbps fiberchannel SFP plugged into a 1G-only MAC.
> > > If we did it as a single pass, we would end up passing an
> > > interface mode of 2500BASEX first time around which is illogical.
> > 
> > So you consider this to be the right fix, right?
> 
> Yes, but there is another bug lurking here - the handling of invalid
> interface modes is not correct.  Please see mvneta.c as an example -
> interface modes that are not supported by the MAC (apart from the NA
> mode) end up with the supported mask completely cleared.

I'll take this as an ack. Thanks for reviewing.

I plan to resend this patch with your ack and a proper Fixes tag. I'll then 
add another patch to properly handle the invalid mode case.

Thanks,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -