[PATCH net-next] net: phy: sfp: handle cases where neither BR,min nor BR,max is given

[PATCH net-next] net: phy: sfp: handle cases where neither BR,min nor BR,max is given

[Antoine Tenart]

When computing the bitrate using values read from an SFP module EEPROM,
we use the nominal BR plus BR,min and BR,max to determine the
boundaries. But in some cases BR,min and BR,max aren't provided, which
led the SFP code to end up having the nominal value for both the minimum
and maximum bitrate values. When using a passive cable, the nominal
value should be used as the maximum one, and there is no minimum one
so we should use 0.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---

Hi Russell,

I'm not completely sure about this patch as this case is not really
specified in the specification. But the issue is there, and I've discuss
this with others. It seemed logical (at least to us :)) to use the
BR,nominal values as br_max and 0 as br_min when using a passive cable
which only provides BR,nominal as this would be the highest rate at
which the cable could work. And because it's passive, there should be no
issues using it at a lower rate.

I've tested this with one passive cable which only reports its
BR,nominal (which was 10300) while it could be used when using 1000baseX
or 2500baseX modes.

Thanks,
Antoine

 drivers/net/phy/sfp-bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index fd6c23f69c2f..d437f4f5ed52 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -132,6 +132,13 @@ void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
 			br_max = br_nom + br_nom * id->ext.br_min / 100;
 			br_min = br_nom - br_nom * id->ext.br_min / 100;
 		}
+
+		/* When using passive cables, in case neither BR,min nor BR,max
+		 * are specified, set br_min to 0 as the nominal value is then
+		 * used as the maximum.
+		 */
+		if (br_min == br_max && id->base.sfp_ct_passive)
+			br_min = 0;
 	}
 
 	/* Set ethtool support from the compliance fields. */
-- 
2.17.0

Re: [PATCH net-next] net: phy: sfp: handle cases where neither BR,min nor BR,max is given

[Florian Fainelli]

On May 4, 2018 8:21:03 AM PDT, Antoine Tenart <antoine.tenart@bootlin.com> wrote:
>When computing the bitrate using values read from an SFP module EEPROM,
>we use the nominal BR plus BR,min and BR,max to determine the
>boundaries. But in some cases BR,min and BR,max aren't provided, which
>led the SFP code to end up having the nominal value for both the
>minimum
>and maximum bitrate values. When using a passive cable, the nominal
>value should be used as the maximum one, and there is no minimum one
>so we should use 0.
>
>Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
>---
>
>Hi Russell,
>
>I'm not completely sure about this patch as this case is not really
>specified in the specification. But the issue is there, and I've
>discuss
>this with others. It seemed logical (at least to us :)) to use the
>BR,nominal values as br_max and 0 as br_min when using a passive cable
>which only provides BR,nominal as this would be the highest rate at
>which the cable could work. And because it's passive, there should be
>no
>issues using it at a lower rate.
>
>I've tested this with one passive cable which only reports its
>BR,nominal (which was 10300) while it could be used when using
>1000baseX
>or 2500baseX modes.

Which SFP modules (vendor and model) exposed this out of curiosity? Russell and I already saw the Cotsworks modules having so e issues with checksums, so building a table of quirks would help. Thanks!

-- 
Florian

Re: [PATCH net-next] net: phy: sfp: handle cases where neither BR,min nor BR,max is given

[Russell King - ARM Linux]

On Sat, May 05, 2018 at 01:35:34PM -0700, Florian Fainelli wrote:
> On May 4, 2018 8:21:03 AM PDT, Antoine Tenart <antoine.tenart@bootlin.com> wrote:
> >When computing the bitrate using values read from an SFP module EEPROM,
> >we use the nominal BR plus BR,min and BR,max to determine the
> >boundaries. But in some cases BR,min and BR,max aren't provided, which
> >led the SFP code to end up having the nominal value for both the
> >minimum
> >and maximum bitrate values. When using a passive cable, the nominal
> >value should be used as the maximum one, and there is no minimum one
> >so we should use 0.
> >
> >Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> >---
> >
> >Hi Russell,
> >
> >I'm not completely sure about this patch as this case is not really
> >specified in the specification. But the issue is there, and I've
> >discuss
> >this with others. It seemed logical (at least to us :)) to use the
> >BR,nominal values as br_max and 0 as br_min when using a passive cable
> >which only provides BR,nominal as this would be the highest rate at
> >which the cable could work. And because it's passive, there should be
> >no
> >issues using it at a lower rate.
> >
> >I've tested this with one passive cable which only reports its
> >BR,nominal (which was 10300) while it could be used when using
> >1000baseX
> >or 2500baseX modes.
> 
> Which SFP modules (vendor and model) exposed this out of curiosity?
> Russell and I already saw the Cotsworks modules having so e issues
> with checksums, so building a table of quirks would help. Thanks!

I think this is just manufacturers being lazy with their EEPROM
contents - looking around, most passive cables are specified to be
"up to" some figure, and that's definitely what's specified by the
SFP+ specification by way of the high-pass pole requirement of the
coupling capacitors.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH net-next] net: phy: sfp: handle cases where neither BR,min nor BR,max is given

[Russell King - ARM Linux]

On Fri, May 04, 2018 at 05:21:03PM +0200, Antoine Tenart wrote:
> When computing the bitrate using values read from an SFP module EEPROM,
> we use the nominal BR plus BR,min and BR,max to determine the
> boundaries. But in some cases BR,min and BR,max aren't provided, which
> led the SFP code to end up having the nominal value for both the minimum
> and maximum bitrate values. When using a passive cable, the nominal
> value should be used as the maximum one, and there is no minimum one
> so we should use 0.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
> 
> Hi Russell,
> 
> I'm not completely sure about this patch as this case is not really
> specified in the specification. But the issue is there, and I've discuss
> this with others. It seemed logical (at least to us :)) to use the
> BR,nominal values as br_max and 0 as br_min when using a passive cable
> which only provides BR,nominal as this would be the highest rate at
> which the cable could work. And because it's passive, there should be no
> issues using it at a lower rate.
> 
> I've tested this with one passive cable which only reports its
> BR,nominal (which was 10300) while it could be used when using 1000baseX
> or 2500baseX modes.

The electronic engineer in me says that using zero isn't really valid
because there are coupling capacitors in the SFP module that block DC.
These blocking capacitors are required by the SFP+ specs to have a high
pass pole of between 20kHz and 100kHz - in other words, frequencies
below this are attenuated by the coupling capacitors.  The relationship
between this and the bit rate will be a function of the encoding, so we
can't come to a definitive figure without some math (and I want to be
lazy about that!)

Practically, we're talking about SerDes Ethernet, where the bit rate is
no lower than 100Mbps [*], which will always have a frequency well above
this cut-off.  So, I don't have any problem with your approach to
setting the minimum to zero.  Therefore,

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Please send me the EEPROM dump using:

ethtool -m <ethernetdevice> raw on > foo.bin

so I can add it to my database for future testing and validation.

Thanks.

* - 10Mbps SGMII is 1Gbps SGMII with each bit repeated 100 times.
    100Mbps SGMII is 1Gbps SGMII with each bit repeated 10 times.
    There is a capability bits for transceivers supporting
    100base-FX/LX but no one has tested those yet.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH net-next] net: phy: sfp: handle cases where neither BR,min nor BR,max is given

[Antoine Tenart]

Hi Russell,

On Tue, May 08, 2018 at 01:30:26PM +0100, Russell King - ARM Linux wrote:
> 
> The electronic engineer in me says that using zero isn't really valid
> because there are coupling capacitors in the SFP module that block DC.
> These blocking capacitors are required by the SFP+ specs to have a high
> pass pole of between 20kHz and 100kHz - in other words, frequencies
> below this are attenuated by the coupling capacitors.  The relationship
> between this and the bit rate will be a function of the encoding, so we
> can't come to a definitive figure without some math (and I want to be
> lazy about that!)
> 
> Practically, we're talking about SerDes Ethernet, where the bit rate is
> no lower than 100Mbps [*], which will always have a frequency well above
> this cut-off.  So, I don't have any problem with your approach to
> setting the minimum to zero.  Therefore,
> 
> Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks for looking into it!
Antoine


-- 
Antoine Ténart, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next] net: phy: sfp: handle cases where neither BR,min nor BR,max is given

[David Miller]

From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Fri,  4 May 2018 17:21:03 +0200

> When computing the bitrate using values read from an SFP module EEPROM,
> we use the nominal BR plus BR,min and BR,max to determine the
> boundaries. But in some cases BR,min and BR,max aren't provided, which
> led the SFP code to end up having the nominal value for both the minimum
> and maximum bitrate values. When using a passive cable, the nominal
> value should be used as the maximum one, and there is no minimum one
> so we should use 0.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>

Applied, thank you.