[PATCH net-next] net: sfp: add some quirks for FreeTel direct attach modules

[PATCH net-next] net: sfp: add some quirks for FreeTel direct attach modules

[Marek Behún]

FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from
their EEPROM. They report correct nominal bitrate of 10300 MBd, but do
not report sfp_ct_passive nor sfp_ct_active in their ERPROM.

These modules can also operate at 1000baseX and 2500baseX.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 6900c68260e0..f021709bedcc 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
 	phylink_set(modes, 2500baseX_Full);
 }
 
+static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id,
+					unsigned long *modes)
+{
+	phylink_set(modes, 10000baseCR_Full);
+	phylink_set(modes, 2500baseX_Full);
+	phylink_set(modes, 1000baseX_Full);
+}
+
 static const struct sfp_quirk sfp_quirks[] = {
 	{
 		// Alcatel Lucent G-010S-P can operate at 2500base-X, but
@@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = {
 		.vendor = "HUAWEI",
 		.part = "MA5671A",
 		.modes = sfp_quirk_2500basex,
+	}, {
+		// FreeTel P.C30.2 is a SFP+ direct attach that can operate at
+		// at 1000baseX, 2500baseX and 10000baseCR, but may report none
+		// of these in their EEPROM
+		.vendor = "FreeTel",
+		.part = "P.C30.2",
+		.modes = sfp_quirk_direct_attach_10g,
+	}, {
+		// same as previous
+		.vendor = "FreeTel",
+		.part = "P.C30.3",
+		.modes = sfp_quirk_direct_attach_10g,
 	},
 };
 
-- 
2.24.1

Re: [PATCH net-next] net: sfp: add some quirks for FreeTel direct attach modules

[Russell King - ARM Linux admin]

On Fri, May 08, 2020 at 05:30:06PM +0300, Denis Kirjanov wrote:
> On Thursday, May 7, 2020, Marek Behún <marek.behun@nic.cz> wrote:
> 
> > FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from
> > their EEPROM. They report correct nominal bitrate of 10300 MBd, but do
> > not report sfp_ct_passive nor sfp_ct_active in their ERPROM.
> >
> > These modules can also operate at 1000baseX and 2500baseX.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > Cc: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> > index 6900c68260e0..f021709bedcc 100644
> > --- a/drivers/net/phy/sfp-bus.c
> > +++ b/drivers/net/phy/sfp-bus.c
> > @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct
> > sfp_eeprom_id *id,
> >         phylink_set(modes, 2500baseX_Full);
> >  }
> >
> > +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id,
> > +                                       unsigned long *modes)
> > +{
> > +       phylink_set(modes, 10000baseCR_Full);
> > +       phylink_set(modes, 2500baseX_Full);
> > +       phylink_set(modes, 1000baseX_Full);
> > +}
> > +
> >  static const struct sfp_quirk sfp_quirks[] = {
> >         {
> >                 // Alcatel Lucent G-010S-P can operate at 2500base-X, but
> > @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = {
> >                 .vendor = "HUAWEI",
> >                 .part = "MA5671A",
> >                 .modes = sfp_quirk_2500basex,
> > +       }, {
> > +               // FreeTel P.C30.2 is a SFP+ direct attach that can
> > operate at
> > +               // at 1000baseX, 2500baseX and 10000baseCR, but may report
> > none
> > +               // of these in their EEPROM
> 
> 
>  Hi Marek,
> 
> The comment style above is not what Linux kernel uses

The comment style is per file - keeping consistency within a file is
far more important than rigid conformance to some coding style. That
is, unless someone is prepared to convert said file.  The commenting
style for these entries is to use "//" style.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next] net: sfp: add some quirks for FreeTel direct attach modules

[Russell King - ARM Linux admin]

On Thu, May 07, 2020 at 03:21:35PM +0200, Marek Behún wrote:
> FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from
> their EEPROM. They report correct nominal bitrate of 10300 MBd, but do
> not report sfp_ct_passive nor sfp_ct_active in their ERPROM.
> 
> These modules can also operate at 1000baseX and 2500baseX.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> index 6900c68260e0..f021709bedcc 100644
> --- a/drivers/net/phy/sfp-bus.c
> +++ b/drivers/net/phy/sfp-bus.c
> @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
>  	phylink_set(modes, 2500baseX_Full);
>  }
>  
> +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id,
> +					unsigned long *modes)
> +{
> +	phylink_set(modes, 10000baseCR_Full);
> +	phylink_set(modes, 2500baseX_Full);
> +	phylink_set(modes, 1000baseX_Full);
> +}
> +
>  static const struct sfp_quirk sfp_quirks[] = {
>  	{
>  		// Alcatel Lucent G-010S-P can operate at 2500base-X, but
> @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = {
>  		.vendor = "HUAWEI",
>  		.part = "MA5671A",
>  		.modes = sfp_quirk_2500basex,
> +	}, {
> +		// FreeTel P.C30.2 is a SFP+ direct attach that can operate at
> +		// at 1000baseX, 2500baseX and 10000baseCR, but may report none
> +		// of these in their EEPROM
> +		.vendor = "FreeTel",
> +		.part = "P.C30.2",
> +		.modes = sfp_quirk_direct_attach_10g,
> +	}, {
> +		// same as previous
> +		.vendor = "FreeTel",
> +		.part = "P.C30.3",
> +		.modes = sfp_quirk_direct_attach_10g,

Looking at the EEPROM capabilities, it seems that these modules give
either:

Transceiver codes     : 0x01 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00
Transceiver type      : Infiniband: 1X Copper Passive
Transceiver type      : Passive Cable
Transceiver type      : FC: Twin Axial Pair (TW)
Encoding              : 0x06 (64B/66B)
BR, Nominal           : 10300MBd
Passive Cu cmplnce.   : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only]
BR margin, max        : 0%
BR margin, min        : 0%

or:

Transceiver codes     : 0x00 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00
Transceiver type      : Passive Cable
Transceiver type      : FC: Twin Axial Pair (TW)
Encoding              : 0x06 (64B/66B)
BR, Nominal           : 10300MBd
Passive Cu cmplnce.   : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only]
BR margin, max        : 0%
BR margin, min        : 0%

These give ethtool capability mask of 000,00000600,0000e040, which
is:

	2500baseX (bit 15)
	1000baseX (bit 41)
	10000baseCR (bit 42)

10000baseCR, 2500baseX and 1000baseX comes from:

        if ((id->base.sfp_ct_passive || id->base.sfp_ct_active) && br_nom) {
                /* This may look odd, but some manufacturers use 12000MBd */
                if (br_min <= 12000 && br_max >= 10300)
                        phylink_set(modes, 10000baseCR_Full);
                if (br_min <= 3200 && br_max >= 3100)
                        phylink_set(modes, 2500baseX_Full);
                if (br_min <= 1300 && br_max >= 1200)
                        phylink_set(modes, 1000baseX_Full);

since id->base.sfp_ct_passive is true, and br_nom = br_max = 10300 and
br_min = 0.

10000baseCR will also come from:

        if (id->base.sfp_ct_passive) {
                if (id->base.passive.sff8431_app_e)
                        phylink_set(modes, 10000baseCR_Full);
        }

You claimed in your patch description that sfp_ct_passive is not set,
but the EEPROM dumps contain:

	Transceiver type      : Passive Cable

which is correctly parsed by the kernel.

So, I'm rather confused, and I don't see why this patch is needed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next] net: sfp: add some quirks for FreeTel direct attach modules

[Marek Behun]

On Fri, 8 May 2020 16:28:44 +0100
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> On Thu, May 07, 2020 at 03:21:35PM +0200, Marek Behún wrote:
> > FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from
> > their EEPROM. They report correct nominal bitrate of 10300 MBd, but do
> > not report sfp_ct_passive nor sfp_ct_active in their ERPROM.
> > 
> > These modules can also operate at 1000baseX and 2500baseX.
> > 
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > Cc: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> > index 6900c68260e0..f021709bedcc 100644
> > --- a/drivers/net/phy/sfp-bus.c
> > +++ b/drivers/net/phy/sfp-bus.c
> > @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
> >  	phylink_set(modes, 2500baseX_Full);
> >  }
> >  
> > +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id,
> > +					unsigned long *modes)
> > +{
> > +	phylink_set(modes, 10000baseCR_Full);
> > +	phylink_set(modes, 2500baseX_Full);
> > +	phylink_set(modes, 1000baseX_Full);
> > +}
> > +
> >  static const struct sfp_quirk sfp_quirks[] = {
> >  	{
> >  		// Alcatel Lucent G-010S-P can operate at 2500base-X, but
> > @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = {
> >  		.vendor = "HUAWEI",
> >  		.part = "MA5671A",
> >  		.modes = sfp_quirk_2500basex,
> > +	}, {
> > +		// FreeTel P.C30.2 is a SFP+ direct attach that can operate at
> > +		// at 1000baseX, 2500baseX and 10000baseCR, but may report none
> > +		// of these in their EEPROM
> > +		.vendor = "FreeTel",
> > +		.part = "P.C30.2",
> > +		.modes = sfp_quirk_direct_attach_10g,
> > +	}, {
> > +		// same as previous
> > +		.vendor = "FreeTel",
> > +		.part = "P.C30.3",
> > +		.modes = sfp_quirk_direct_attach_10g,  
> 
> Looking at the EEPROM capabilities, it seems that these modules give
> either:
> 
> Transceiver codes     : 0x01 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00
> Transceiver type      : Infiniband: 1X Copper Passive
> Transceiver type      : Passive Cable
> Transceiver type      : FC: Twin Axial Pair (TW)
> Encoding              : 0x06 (64B/66B)
> BR, Nominal           : 10300MBd
> Passive Cu cmplnce.   : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only]
> BR margin, max        : 0%
> BR margin, min        : 0%
> 
> or:
> 
> Transceiver codes     : 0x00 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00
> Transceiver type      : Passive Cable
> Transceiver type      : FC: Twin Axial Pair (TW)
> Encoding              : 0x06 (64B/66B)
> BR, Nominal           : 10300MBd
> Passive Cu cmplnce.   : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only]
> BR margin, max        : 0%
> BR margin, min        : 0%
> 
> These give ethtool capability mask of 000,00000600,0000e040, which
> is:
> 
> 	2500baseX (bit 15)
> 	1000baseX (bit 41)
> 	10000baseCR (bit 42)
> 
> 10000baseCR, 2500baseX and 1000baseX comes from:
> 
>         if ((id->base.sfp_ct_passive || id->base.sfp_ct_active) && br_nom) {
>                 /* This may look odd, but some manufacturers use 12000MBd */
>                 if (br_min <= 12000 && br_max >= 10300)
>                         phylink_set(modes, 10000baseCR_Full);
>                 if (br_min <= 3200 && br_max >= 3100)
>                         phylink_set(modes, 2500baseX_Full);
>                 if (br_min <= 1300 && br_max >= 1200)
>                         phylink_set(modes, 1000baseX_Full);
> 
> since id->base.sfp_ct_passive is true, and br_nom = br_max = 10300 and
> br_min = 0.
> 
> 10000baseCR will also come from:
> 
>         if (id->base.sfp_ct_passive) {
>                 if (id->base.passive.sff8431_app_e)
>                         phylink_set(modes, 10000baseCR_Full);
>         }
> 
> You claimed in your patch description that sfp_ct_passive is not set,
> but the EEPROM dumps contain:
> 
> 	Transceiver type      : Passive Cable
> 
> which is correctly parsed by the kernel.
> 
> So, I'm rather confused, and I don't see why this patch is needed.
> 

Russell,

something is wrong here, and it is my bad. I hope I didn't mix
the EEPROM images from when I was playing with the contents, but it
seems possible now :( I probably sent you modified images and lost the
original ones.

The thing I know for sure is that it did not work when I got the
cables and also that they had different contents inside - ie at least
one side of one cable did not report ct_passive nor ct_active. And I
think that they reported different things on each side.

I will try to get another such cable and return to this.

Marek

Re: [PATCH net-next] net: sfp: add some quirks for FreeTel direct attach modules

[Russell King - ARM Linux admin]

On Fri, May 08, 2020 at 06:27:06PM +0200, Marek Behun wrote:
> On Fri, 8 May 2020 16:28:44 +0100
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > On Thu, May 07, 2020 at 03:21:35PM +0200, Marek Behún wrote:
> > > FreeTel P.C30.2 and P.C30.3 may fail to report anything useful from
> > > their EEPROM. They report correct nominal bitrate of 10300 MBd, but do
> > > not report sfp_ct_passive nor sfp_ct_active in their ERPROM.
> > > 
> > > These modules can also operate at 1000baseX and 2500baseX.
> > > 
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > Cc: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  drivers/net/phy/sfp-bus.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> > > index 6900c68260e0..f021709bedcc 100644
> > > --- a/drivers/net/phy/sfp-bus.c
> > > +++ b/drivers/net/phy/sfp-bus.c
> > > @@ -44,6 +44,14 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
> > >  	phylink_set(modes, 2500baseX_Full);
> > >  }
> > >  
> > > +static void sfp_quirk_direct_attach_10g(const struct sfp_eeprom_id *id,
> > > +					unsigned long *modes)
> > > +{
> > > +	phylink_set(modes, 10000baseCR_Full);
> > > +	phylink_set(modes, 2500baseX_Full);
> > > +	phylink_set(modes, 1000baseX_Full);
> > > +}
> > > +
> > >  static const struct sfp_quirk sfp_quirks[] = {
> > >  	{
> > >  		// Alcatel Lucent G-010S-P can operate at 2500base-X, but
> > > @@ -63,6 +71,18 @@ static const struct sfp_quirk sfp_quirks[] = {
> > >  		.vendor = "HUAWEI",
> > >  		.part = "MA5671A",
> > >  		.modes = sfp_quirk_2500basex,
> > > +	}, {
> > > +		// FreeTel P.C30.2 is a SFP+ direct attach that can operate at
> > > +		// at 1000baseX, 2500baseX and 10000baseCR, but may report none
> > > +		// of these in their EEPROM
> > > +		.vendor = "FreeTel",
> > > +		.part = "P.C30.2",
> > > +		.modes = sfp_quirk_direct_attach_10g,
> > > +	}, {
> > > +		// same as previous
> > > +		.vendor = "FreeTel",
> > > +		.part = "P.C30.3",
> > > +		.modes = sfp_quirk_direct_attach_10g,  
> > 
> > Looking at the EEPROM capabilities, it seems that these modules give
> > either:
> > 
> > Transceiver codes     : 0x01 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00
> > Transceiver type      : Infiniband: 1X Copper Passive
> > Transceiver type      : Passive Cable
> > Transceiver type      : FC: Twin Axial Pair (TW)
> > Encoding              : 0x06 (64B/66B)
> > BR, Nominal           : 10300MBd
> > Passive Cu cmplnce.   : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only]
> > BR margin, max        : 0%
> > BR margin, min        : 0%
> > 
> > or:
> > 
> > Transceiver codes     : 0x00 0x00 0x00 0x00 0x00 0x04 0x80 0x00 0x00
> > Transceiver type      : Passive Cable
> > Transceiver type      : FC: Twin Axial Pair (TW)
> > Encoding              : 0x06 (64B/66B)
> > BR, Nominal           : 10300MBd
> > Passive Cu cmplnce.   : 0x01 (SFF-8431 appendix E) [SFF-8472 rev10.4 only]
> > BR margin, max        : 0%
> > BR margin, min        : 0%
> > 
> > These give ethtool capability mask of 000,00000600,0000e040, which
> > is:
> > 
> > 	2500baseX (bit 15)
> > 	1000baseX (bit 41)
> > 	10000baseCR (bit 42)
> > 
> > 10000baseCR, 2500baseX and 1000baseX comes from:
> > 
> >         if ((id->base.sfp_ct_passive || id->base.sfp_ct_active) && br_nom) {
> >                 /* This may look odd, but some manufacturers use 12000MBd */
> >                 if (br_min <= 12000 && br_max >= 10300)
> >                         phylink_set(modes, 10000baseCR_Full);
> >                 if (br_min <= 3200 && br_max >= 3100)
> >                         phylink_set(modes, 2500baseX_Full);
> >                 if (br_min <= 1300 && br_max >= 1200)
> >                         phylink_set(modes, 1000baseX_Full);
> > 
> > since id->base.sfp_ct_passive is true, and br_nom = br_max = 10300 and
> > br_min = 0.
> > 
> > 10000baseCR will also come from:
> > 
> >         if (id->base.sfp_ct_passive) {
> >                 if (id->base.passive.sff8431_app_e)
> >                         phylink_set(modes, 10000baseCR_Full);
> >         }
> > 
> > You claimed in your patch description that sfp_ct_passive is not set,
> > but the EEPROM dumps contain:
> > 
> > 	Transceiver type      : Passive Cable
> > 
> > which is correctly parsed by the kernel.
> > 
> > So, I'm rather confused, and I don't see why this patch is needed.
> > 
> 
> Russell,
> 
> something is wrong here, and it is my bad. I hope I didn't mix
> the EEPROM images from when I was playing with the contents, but it
> seems possible now :( I probably sent you modified images and lost the
> original ones.

Ah.

> The thing I know for sure is that it did not work when I got the
> cables and also that they had different contents inside - ie at least
> one side of one cable did not report ct_passive nor ct_active. And I
> think that they reported different things on each side.

One of your files did have a difference in the capabilities - it was
missing the Infiniband capability mentioned above.

> I will try to get another such cable and return to this.

Thanks.

One thing we could do is to augment the "guessing" towards the end of
sfp_parse_support() so that if we encounter a module indicating
support for 10300MBd and 64B/66B, we could set one of the 10G properties
so that at least we get some functionality from modules that report
no capabilities.  See:

        /* If we haven't discovered any modes that this module supports, try
         * the encoding and bitrate to determine supported modes. Some BiDi
         * modules (eg, 1310nm/1550nm) are not 1000BASE-BX compliant due to
         * the differing wavelengths, so do not set any transceiver bits.
         */
        if (bitmap_empty(modes, __ETHTOOL_LINK_MODE_MASK_NBITS)) {
                /* If the encoding and bit rate allows 1000baseX */
                if (id->base.encoding == SFF8024_ENCODING_8B10B && br_nom &&
                    br_min <= 1300 && br_max >= 1200)
                        phylink_set(modes, 1000baseX_Full);
        }

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up