[PATCH net-next 0/2] Add rudimentary SFP module quirk support

[PATCH net-next 0/2] Add rudimentary SFP module quirk support

[Russell King - ARM Linux admin]

The SFP module EEPROM describes the capabilities of the module, but
doesn't describe the host interface.  We have a certain amount of
guess-work to work out how to configure the host - which works most
of the time.

However, there are some (such as GPON) modules which are able to
support different host interfaces, such as 1000BASE-X and 2500BASE-X.
The module will switch between each mode until it achieves link with
the host.

There is no defined way to describe this in the SFP EEPROM, so we can
only recognise the module and handle it appropriately.  This series
adds the necessary recognition of the modules using a quirk system,
and tweaks the support mask to allow them to link with the host at
2500BASE-X, thereby allowing the user to achieve full line rate.

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

-- 
RMK's Patch system: https://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

[PATCH net-next 1/2] net: sfp: add support for module quirks

[Russell King]

Add support for applying module quirks to the list of supported
ethtool link modes.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp-bus.c | 54 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 2f826a303ad8..aebb6978e91a 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -10,6 +10,12 @@
 
 #include "sfp.h"
 
+struct sfp_quirk {
+	const char *vendor;
+	const char *part;
+	void (*modes)(const struct sfp_eeprom_id *id, unsigned long *modes);
+};
+
 /**
  * struct sfp_bus - internal representation of a sfp bus
  */
@@ -22,6 +28,7 @@ struct sfp_bus {
 	const struct sfp_socket_ops *socket_ops;
 	struct device *sfp_dev;
 	struct sfp *sfp;
+	const struct sfp_quirk *sfp_quirk;
 
 	const struct sfp_upstream_ops *upstream_ops;
 	void *upstream;
@@ -31,6 +38,46 @@ struct sfp_bus {
 	bool started;
 };
 
+static const struct sfp_quirk sfp_quirks[] = {
+};
+
+static size_t sfp_strlen(const char *str, size_t maxlen)
+{
+	size_t size, i;
+
+	/* Trailing characters should be filled with space chars */
+	for (i = 0, size = 0; i < maxlen; i++)
+		if (str[i] != ' ')
+			size = i + 1;
+
+	return size;
+}
+
+static bool sfp_match(const char *qs, const char *str, size_t len)
+{
+	if (!qs)
+		return true;
+	if (strlen(qs) != len)
+		return false;
+	return !strncmp(qs, str, len);
+}
+
+static const struct sfp_quirk *sfp_lookup_quirk(const struct sfp_eeprom_id *id)
+{
+	const struct sfp_quirk *q;
+	unsigned int i;
+	size_t vs, ps;
+
+	vs = sfp_strlen(id->base.vendor_name, ARRAY_SIZE(id->base.vendor_name));
+	ps = sfp_strlen(id->base.vendor_pn, ARRAY_SIZE(id->base.vendor_pn));
+
+	for (i = 0, q = sfp_quirks; i < ARRAY_SIZE(sfp_quirks); i++, q++)
+		if (sfp_match(q->vendor, id->base.vendor_name, vs) &&
+		    sfp_match(q->part, id->base.vendor_pn, ps))
+			return q;
+
+	return NULL;
+}
 /**
  * sfp_parse_port() - Parse the EEPROM base ID, setting the port type
  * @bus: a pointer to the &struct sfp_bus structure for the sfp module
@@ -245,6 +292,9 @@ void sfp_parse_support(struct sfp_bus *bus, const struct sfp_eeprom_id *id,
 			phylink_set(modes, 1000baseX_Full);
 	}
 
+	if (bus->sfp_quirk)
+		bus->sfp_quirk->modes(id, modes);
+
 	bitmap_or(support, support, modes, __ETHTOOL_LINK_MODE_MASK_NBITS);
 
 	phylink_set(support, Autoneg);
@@ -622,6 +672,8 @@ int sfp_module_insert(struct sfp_bus *bus, const struct sfp_eeprom_id *id)
 	const struct sfp_upstream_ops *ops = sfp_get_upstream_ops(bus);
 	int ret = 0;
 
+	bus->sfp_quirk = sfp_lookup_quirk(id);
+
 	if (ops && ops->module_insert)
 		ret = ops->module_insert(bus->upstream, id);
 
@@ -635,6 +687,8 @@ void sfp_module_remove(struct sfp_bus *bus)
 
 	if (ops && ops->module_remove)
 		ops->module_remove(bus->upstream);
+
+	bus->sfp_quirk = NULL;
 }
 EXPORT_SYMBOL_GPL(sfp_module_remove);
 
-- 
2.20.1

Re: [PATCH net-next 1/2] net: sfp: add support for module quirks

[Andrew Lunn]

> +static size_t sfp_strlen(const char *str, size_t maxlen)
> +{
> +	size_t size, i;
> +
> +	/* Trailing characters should be filled with space chars */
> +	for (i = 0, size = 0; i < maxlen; i++)
> +		if (str[i] != ' ')
> +			size = i + 1;

Hi Russell

Is this space padding part of the standard? What's the bet there are
some with a NULL character?

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 1/2] net: sfp: add support for module quirks

[Russell King - ARM Linux admin]

On Thu, Nov 21, 2019 at 03:24:52AM +0100, Andrew Lunn wrote:
> > +static size_t sfp_strlen(const char *str, size_t maxlen)
> > +{
> > +	size_t size, i;
> > +
> > +	/* Trailing characters should be filled with space chars */
> > +	for (i = 0, size = 0; i < maxlen; i++)
> > +		if (str[i] != ' ')
> > +			size = i + 1;
> 
> Hi Russell
> 
> Is this space padding part of the standard? What's the bet there are
> some with a NULL character?

Yes, it's specified.  I haven't yet seen a module not space-pad.

-- 
RMK's Patch system: https://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: [PATCH net-next 1/2] net: sfp: add support for module quirks

[Florian Fainelli]

On 11/20/2019 3:42 AM, Russell King wrote:
> Add support for applying module quirks to the list of supported
> ethtool link modes.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 2/2] net: sfp: add some quirks for GPON modules

[Russell King]

Marc Micalizzi reports that Huawei MA5671A and Alcatel/Lucent G-010S-P
modules are capable of 2500base-X, but incorrectly report their
capabilities in the EEPROM.  It seems rather common that GPON modules
mis-report.

Let's fix these modules by adding some quirks.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp-bus.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index aebb6978e91a..97680414b5a9 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -38,7 +38,32 @@ struct sfp_bus {
 	bool started;
 };
 
+static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
+				unsigned long *modes)
+{
+	phylink_set(modes, 2500baseX_Full);
+}
+
 static const struct sfp_quirk sfp_quirks[] = {
+	{
+		// Alcatel Lucent G-010S-P can operate at 2500base-X, but
+		// incorrectly report 2500MBd NRZ in their EEPROM
+		.vendor = "ALCATELLUCENT",
+		.part = "G010SP",
+		.modes = sfp_quirk_2500basex,
+	}, {
+		// Alcatel Lucent G-010S-A can operate at 2500base-X, but
+		// report 3.2GBd NRZ in their EEPROM
+		.vendor = "ALCATELLUCENT",
+		.part = "3FE46541AA",
+		.modes = sfp_quirk_2500basex,
+	}, {
+		// Huawei MA5671A can operate at 2500base-X, but report 1.2GBd
+		// NRZ in their EEPROM
+		.vendor = "HUAWEI",
+		.part = "MA5671A",
+		.modes = sfp_quirk_2500basex,
+	},
 };
 
 static size_t sfp_strlen(const char *str, size_t maxlen)
-- 
2.20.1

Re: [PATCH net-next 2/2] net: sfp: add some quirks for GPON modules

[Jakub Kicinski]

On Wed, 20 Nov 2019 11:42:47 +0000, Russell King wrote:
>  static const struct sfp_quirk sfp_quirks[] = {
> +	{
> +		// Alcatel Lucent G-010S-P can operate at 2500base-X, but
> +		// incorrectly report 2500MBd NRZ in their EEPROM
> +		.vendor = "ALCATELLUCENT",
> +		.part = "G010SP",
> +		.modes = sfp_quirk_2500basex,
> +	}, {
> +		// Alcatel Lucent G-010S-A can operate at 2500base-X, but
> +		// report 3.2GBd NRZ in their EEPROM
> +		.vendor = "ALCATELLUCENT",
> +		.part = "3FE46541AA",
> +		.modes = sfp_quirk_2500basex,
> +	}, {
> +		// Huawei MA5671A can operate at 2500base-X, but report 1.2GBd
> +		// NRZ in their EEPROM
> +		.vendor = "HUAWEI",
> +		.part = "MA5671A",
> +		.modes = sfp_quirk_2500basex,
> +	},
>  };

nit: no C++ comment style?

Re: [PATCH net-next 2/2] net: sfp: add some quirks for GPON modules

[Russell King - ARM Linux admin]

On Wed, Nov 20, 2019 at 02:46:32PM -0800, Jakub Kicinski wrote:
> On Wed, 20 Nov 2019 11:42:47 +0000, Russell King wrote:
> >  static const struct sfp_quirk sfp_quirks[] = {
> > +	{
> > +		// Alcatel Lucent G-010S-P can operate at 2500base-X, but
> > +		// incorrectly report 2500MBd NRZ in their EEPROM
> > +		.vendor = "ALCATELLUCENT",
> > +		.part = "G010SP",
> > +		.modes = sfp_quirk_2500basex,
> > +	}, {
> > +		// Alcatel Lucent G-010S-A can operate at 2500base-X, but
> > +		// report 3.2GBd NRZ in their EEPROM
> > +		.vendor = "ALCATELLUCENT",
> > +		.part = "3FE46541AA",
> > +		.modes = sfp_quirk_2500basex,
> > +	}, {
> > +		// Huawei MA5671A can operate at 2500base-X, but report 1.2GBd
> > +		// NRZ in their EEPROM
> > +		.vendor = "HUAWEI",
> > +		.part = "MA5671A",
> > +		.modes = sfp_quirk_2500basex,
> > +	},
> >  };
> 
> nit: no C++ comment style?

Did you read Linus' opinions on commentry style during the discussion
over the SPDX tags?

https://lkml.org/lkml/2017/11/2/715
https://lkml.org/lkml/2017/11/25/133

It seems that Linus has decided to prefer // over /* */

-- 
RMK's Patch system: https://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: [PATCH net-next 2/2] net: sfp: add some quirks for GPON modules

[Jakub Kicinski]

On Thu, 21 Nov 2019 00:03:28 +0000, Russell King - ARM Linux admin
wrote:
> On Wed, Nov 20, 2019 at 02:46:32PM -0800, Jakub Kicinski wrote:
> > On Wed, 20 Nov 2019 11:42:47 +0000, Russell King wrote:  
> > >  static const struct sfp_quirk sfp_quirks[] = {
> > > +	{
> > > +		// Alcatel Lucent G-010S-P can operate at 2500base-X, but
> > > +		// incorrectly report 2500MBd NRZ in their EEPROM
> > > +		.vendor = "ALCATELLUCENT",
> > > +		.part = "G010SP",
> > > +		.modes = sfp_quirk_2500basex,
> > > +	}, {
> > > +		// Alcatel Lucent G-010S-A can operate at 2500base-X, but
> > > +		// report 3.2GBd NRZ in their EEPROM
> > > +		.vendor = "ALCATELLUCENT",
> > > +		.part = "3FE46541AA",
> > > +		.modes = sfp_quirk_2500basex,
> > > +	}, {
> > > +		// Huawei MA5671A can operate at 2500base-X, but report 1.2GBd
> > > +		// NRZ in their EEPROM
> > > +		.vendor = "HUAWEI",
> > > +		.part = "MA5671A",
> > > +		.modes = sfp_quirk_2500basex,
> > > +	},
> > >  };  
> > 
> > nit: no C++ comment style?  
> 
> Did you read Linus' opinions on commentry style during the discussion
> over the SPDX tags?
> 
> https://lkml.org/lkml/2017/11/2/715
> https://lkml.org/lkml/2017/11/25/133
> 
> It seems that Linus has decided to prefer // over /* */

Yeah, I remember that, I did:

$ git grep '// ' drivers/net/phy/ | grep -v SPDX
drivers/net/phy/microchip_t1.c:// Copyright (C) 2018 Microchip Technology

before I asked, and assumed since for the last two years they didn't
gain much popularity in this corner of the kernel it's worth asking if
this is intentional :)

Re: [PATCH net-next 2/2] net: sfp: add some quirks for GPON modules

[Andrew Lunn]

On Wed, Nov 20, 2019 at 11:42:47AM +0000, Russell King wrote:
> Marc Micalizzi reports that Huawei MA5671A and Alcatel/Lucent G-010S-P
> modules are capable of 2500base-X, but incorrectly report their
> capabilities in the EEPROM.  It seems rather common that GPON modules
> mis-report.
> 
> Let's fix these modules by adding some quirks.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 2/2] net: sfp: add some quirks for GPON modules

[Florian Fainelli]

On 11/20/2019 3:42 AM, Russell King wrote:
> Marc Micalizzi reports that Huawei MA5671A and Alcatel/Lucent G-010S-P
> modules are capable of 2500base-X, but incorrectly report their
> capabilities in the EEPROM.  It seems rather common that GPON modules
> mis-report.
> 
> Let's fix these modules by adding some quirks.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 0/2] Add rudimentary SFP module quirk support

[Andrew Lunn]

On Wed, Nov 20, 2019 at 11:39:00AM +0000, Russell King - ARM Linux admin wrote:
> The SFP module EEPROM describes the capabilities of the module, but
> doesn't describe the host interface.  We have a certain amount of
> guess-work to work out how to configure the host - which works most
> of the time.
> 
> However, there are some (such as GPON) modules which are able to
> support different host interfaces, such as 1000BASE-X and 2500BASE-X.
> The module will switch between each mode until it achieves link with
> the host.
> 
> There is no defined way to describe this in the SFP EEPROM, so we can
> only recognise the module and handle it appropriately.  This series
> adds the necessary recognition of the modules using a quirk system,
> and tweaks the support mask to allow them to link with the host at
> 2500BASE-X, thereby allowing the user to achieve full line rate.

Hi Russell

Did you consider making the Cotsworks checksum issue a quirk?

    Andrew

Re: [PATCH net-next 0/2] Add rudimentary SFP module quirk support

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Wed, 20 Nov 2019 11:39:00 +0000

> The SFP module EEPROM describes the capabilities of the module, but
> doesn't describe the host interface.  We have a certain amount of
> guess-work to work out how to configure the host - which works most
> of the time.
> 
> However, there are some (such as GPON) modules which are able to
> support different host interfaces, such as 1000BASE-X and 2500BASE-X.
> The module will switch between each mode until it achieves link with
> the host.
> 
> There is no defined way to describe this in the SFP EEPROM, so we can
> only recognise the module and handle it appropriately.  This series
> adds the necessary recognition of the modules using a quirk system,
> and tweaks the support mask to allow them to link with the host at
> 2500BASE-X, thereby allowing the user to achieve full line rate.

Series applied, thanks.