[PATCH net-next 0/2] Add support for VSOL V2801F/CarlitoxxPro CPGOS03 GPON module

[PATCH net-next 0/2] Add support for VSOL V2801F/CarlitoxxPro CPGOS03 GPON module

[Russell King - ARM Linux admin]

Hi,

This patch set adds support for the V2801F / CarlitoxxPro module. This
requires two changes:

1) the module only supports single byte reads to the ID EEPROM,
   while we need to still permit sequential reads to the diagnostics
   EEPROM for atomicity reasons.

2) we need to relax the encoding check when we have no reported
   capabilities to allow 1000base-X based on the module bitrate.

Thanks to Pali Rohár for responsive testing over the last two days.

 drivers/net/phy/sfp-bus.c | 11 ++++-----
 drivers/net/phy/sfp.c     | 63 +++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 63 insertions(+), 11 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/2] net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround

[Russell King - ARM Linux admin]

... and I just got another email from Pali Rohár about an hour after
giving me a Tested-by for this patch, saying...

Just to note I applied following fixup change to your patches for
fixing compile errors, there is missing "struct" keyword...

So, v2 on its way.

On Fri, Dec 04, 2020 at 02:35:22PM +0000, Russell King wrote:
> Add a workaround for the detection of VSOL V2801F / CarlitoxxPro
> CPGOS03-0490 v2.0 GPON module which CarlitoxxPro states needs single
> byte I2C reads to the EEPROM.
> 
> Pali Rohár reports that he also has a CarlitoxxPro-based V2801F module,
> which reports a manufacturer of "OEM". This manufacturer can't be
> matched as it appears in many different modules, so also match the part
> number too.
> 
> Reported-by: Thomas Schreiber <tschreibe@gmail.com>
> Reported-by: Pali Rohár <pali@kernel.org>
> Tested-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/sfp.c | 63 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 34aa196b7465..0b26495973ff 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -219,6 +219,7 @@ struct sfp {
>  	struct sfp_bus *sfp_bus;
>  	struct phy_device *mod_phy;
>  	const struct sff_data *type;
> +	size_t i2c_block_size;
>  	u32 max_power_mW;
>  
>  	unsigned int (*get_state)(struct sfp *);
> @@ -335,10 +336,19 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>  			size_t len)
>  {
>  	struct i2c_msg msgs[2];
> -	u8 bus_addr = a2 ? 0x51 : 0x50;
> +	size_t block_size;
>  	size_t this_len;
> +	u8 bus_addr;
>  	int ret;
>  
> +	if (a2) {
> +		block_size = 16;
> +		bus_addr = 0x51;
> +	} else {
> +		block_size = sfp->i2c_block_size;
> +		bus_addr = 0x50;
> +	}
> +
>  	msgs[0].addr = bus_addr;
>  	msgs[0].flags = 0;
>  	msgs[0].len = 1;
> @@ -350,8 +360,8 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>  
>  	while (len) {
>  		this_len = len;
> -		if (this_len > 16)
> -			this_len = 16;
> +		if (this_len > block_size)
> +			this_len = block_size;
>  
>  		msgs[1].len = this_len;
>  
> @@ -1632,6 +1642,28 @@ static int sfp_sm_mod_hpower(struct sfp *sfp, bool enable)
>  	return 0;
>  }
>  
> +/* Some modules (Nokia 3FE46541AA) lock up if byte 0x51 is read as a
> + * single read. Switch back to reading 16 byte blocks unless we have
> + * a CarlitoxxPro module (rebranded VSOL V2801F). Even more annoyingly,
> + * some VSOL V2801F have the vendor name changed to OEM.
> + */
> +static int sfp_quirk_i2c_block_size(const sfp_eeprom_base *base)
> +{
> +	if (!memcmp(base->vendor_name, "VSOL            ", 16))
> +		return 1;
> +	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
> +	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
> +		return 1;
> +
> +	/* Some modules can't cope with long reads */
> +	return 16;
> +}
> +
> +static void sfp_quirks_base(struct sfp *sfp, const sfp_eeprom_base *base)
> +{
> +	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
> +}
> +
>  static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)
>  {
>  	u8 check;
> @@ -1673,14 +1705,20 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
>  	u8 check;
>  	int ret;
>  
> -	ret = sfp_read(sfp, false, 0, &id, sizeof(id));
> +	/* Some modules (CarlitoxxPro CPGOS03-0490) do not support multibyte
> +	 * reads from the EEPROM, so start by reading the base identifying
> +	 * information one byte at a time.
> +	 */
> +	sfp->i2c_block_size = 1;
> +
> +	ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base));
>  	if (ret < 0) {
>  		if (report)
>  			dev_err(sfp->dev, "failed to read EEPROM: %d\n", ret);
>  		return -EAGAIN;
>  	}
>  
> -	if (ret != sizeof(id)) {
> +	if (ret != sizeof(id.base)) {
>  		dev_err(sfp->dev, "EEPROM short read: %d\n", ret);
>  		return -EAGAIN;
>  	}
> @@ -1719,6 +1757,21 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
>  		}
>  	}
>  
> +	/* Apply any early module-specific quirks */
> +	sfp_quirks_base(sfp, &id.base);
> +
> +	ret = sfp_read(sfp, false, SFP_CC_BASE + 1, &id.ext, sizeof(id.ext));
> +	if (ret < 0) {
> +		if (report)
> +			dev_err(sfp->dev, "failed to read EEPROM: %d\n", ret);
> +		return -EAGAIN;
> +	}
> +
> +	if (ret != sizeof(id.ext)) {
> +		dev_err(sfp->dev, "EEPROM short read: %d\n", ret);
> +		return -EAGAIN;
> +	}
> +
>  	check = sfp_check(&id.ext, sizeof(id.ext) - 1);
>  	if (check != id.ext.cc_ext) {
>  		if (cotsworks) {
> -- 
> 2.20.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/2] net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround

[Andrew Lunn]

> +/* Some modules (Nokia 3FE46541AA) lock up if byte 0x51 is read as a
> + * single read. Switch back to reading 16 byte blocks unless we have
> + * a CarlitoxxPro module (rebranded VSOL V2801F). Even more annoyingly,
> + * some VSOL V2801F have the vendor name changed to OEM.

Hi Russell

I guess it is more likely, the vendor name has not been changed _from_
OEM.

Otherwise, this looks good.

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

    Andrew

Re: [PATCH net-next 2/2] net: sfp: relax bitrate-derived mode check

[Andrew Lunn]

On Fri, Dec 04, 2020 at 02:35:27PM +0000, Russell King wrote:
> Do not check the encoding when deriving 1000BASE-X from the bitrate
> when no other modes are discovered. Some GPON modules (VSOL V2801F
> and CarlitoxxPro CPGOS03-0490 v2.0) indicate NRZ encoding with a
> 1200Mbaud bitrate, but should be driven with 1000BASE-X on the host
> side.

Seems like somebody could make a nice side line writing SFP EEPROM
validation tools. There obviously are none in widespread use!

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

    Andrew

Re: [PATCH net-next 2/2] net: sfp: relax bitrate-derived mode check

[Russell King - ARM Linux admin]

On Fri, Dec 04, 2020 at 04:38:50PM +0100, Andrew Lunn wrote:
> On Fri, Dec 04, 2020 at 02:35:27PM +0000, Russell King wrote:
> > Do not check the encoding when deriving 1000BASE-X from the bitrate
> > when no other modes are discovered. Some GPON modules (VSOL V2801F
> > and CarlitoxxPro CPGOS03-0490 v2.0) indicate NRZ encoding with a
> > 1200Mbaud bitrate, but should be driven with 1000BASE-X on the host
> > side.
> 
> Seems like somebody could make a nice side line writing SFP EEPROM
> validation tools. There obviously are none in widespread use!

Definitely. Here's an example of another module:

  Identifier                                : 0x02 (module soldered to motherboard)

This is incorrect, it's a plug-in module.

  Transceiver codes                         : 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x00
  Transceiver type                          : 10G Ethernet: 10G Base-ER [SFF-8472 rev10.4 onwards]
  Transceiver type                          : 10G Ethernet: 10G Base-LRM
  Transceiver type                          : 10G Ethernet: 10G Base-LR
  Transceiver type                          : 10G Ethernet: 10G Base-SR
  Transceiver type                          : Infiniband: 1X SX
  Transceiver type                          : Infiniband: 1X LX
  Transceiver type                          : Infiniband: 1X Copper Active        Transceiver type                          : Infiniband: 1X Copper Passive
  Transceiver type                          : ESCON: ESCON MMF, 1310nm LED        Transceiver type                          : ESCON: ESCON SMF, 1310nm Laser
  Transceiver type                          : SONET: OC-192, short reach
  Transceiver type                          : SONET: SONET reach specifier bit 1
  Transceiver type                          : SONET: SONET reach specifier        Transceiver type                          : SONET: SONET reach specifier bit 2
  Transceiver type                          : SONET: OC-48, long reach
  Transceiver type                          : SONET: OC-48, intermediate reach
  Transceiver type                          : SONET: OC-48, short reach
  Transceiver type                          : SONET: OC-12, single mode, long reach
  Transceiver type                          : SONET: OC-12, single mode, inter. reach
  Transceiver type                          : SONET: OC-12, short reach
  Transceiver type                          : SONET: OC-3, single mode, long reach
  Transceiver type                          : SONET: OC-3, single mode, inter. reach
  Transceiver type                          : SONET: OC-3, short reach
  Transceiver type                          : Ethernet: BASE-PX
  Transceiver type                          : Ethernet: BASE-BX10
  Transceiver type                          : Ethernet: 100BASE-FX
  Transceiver type                          : Ethernet: 100BASE-LX/LX10
  Transceiver type                          : Ethernet: 1000BASE-T
  Transceiver type                          : Ethernet: 1000BASE-CX
  Transceiver type                          : Ethernet: 1000BASE-LX
  Transceiver type                          : Ethernet: 1000BASE-SX
  Transceiver type                          : FC: very long distance (V)
  Transceiver type                          : FC: short distance (S)
  Transceiver type                          : FC: intermediate distance (I)
  Transceiver type                          : FC: long distance (L)
  Transceiver type                          : FC: medium distance (M)
  Transceiver type                          : FC: Shortwave laser, linear
Rx (SA)
  Transceiver type                          : FC: Longwave laser (LC)
  Transceiver type                          : FC: Electrical inter-enclosure (EL)
  Transceiver type                          : FC: Electrical intra-enclosure (EL)
  Transceiver type                          : FC: Shortwave laser w/o OFC
(SN)
  Transceiver type                          : FC: Shortwave laser with OFC (SL)
  Transceiver type                          : FC: Longwave laser (LL)
  Transceiver type                          : Active Cable
  Transceiver type                          : Passive Cable
  Transceiver type                          : FC: Copper FC-BaseT
  Transceiver type                          : FC: Twin Axial Pair (TW)
  Transceiver type                          : FC: Twisted Pair (TP)
  Transceiver type                          : FC: Miniature Coax (MI)
  Transceiver type                          : FC: Video Coax (TV)
  Transceiver type                          : FC: Multimode, 62.5um (M6)
  Transceiver type                          : FC: Multimode, 50um (M5)
  Transceiver type                          : FC: Single Mode (SM)
  Transceiver type                          : FC: 1200 MBytes/sec
  Transceiver type                          : FC: 800 MBytes/sec
  Transceiver type                          : FC: 400 MBytes/sec
  Transceiver type                          : FC: 200 MBytes/sec
  Transceiver type                          : FC: 100 MBytes/sec

This, of course, _really_ messes up our current EEPROM parsing code.
I can only think that this SFP module is very very large externally
to support all those different connectors for all those capabilities!

I think it's safe to assume all SFP GPON modules are broken in one
way or another, and sadly no manufacturer cares one iota what they
stuff into their EEPROM. So far, every GPON module I've heard of has
had some problem or another. This is a really sad state of affairs.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] Add support for VSOL V2801F/CarlitoxxPro CPGOS03 GPON module

[Russell King - ARM Linux admin]

Jakub,

What's happening with these patches?

If I'm getting the patchwork URLs correct, these patches seem not to
appear in patchwork. They both went to netdev as normal and where
accepted by vger from what I can see.

Thanks.

On Fri, Dec 04, 2020 at 02:34:52PM +0000, Russell King - ARM Linux admin wrote:
> Hi,
> 
> This patch set adds support for the V2801F / CarlitoxxPro module. This
> requires two changes:
> 
> 1) the module only supports single byte reads to the ID EEPROM,
>    while we need to still permit sequential reads to the diagnostics
>    EEPROM for atomicity reasons.
> 
> 2) we need to relax the encoding check when we have no reported
>    capabilities to allow 1000base-X based on the module bitrate.
> 
> Thanks to Pali Rohár for responsive testing over the last two days.
> 
>  drivers/net/phy/sfp-bus.c | 11 ++++-----
>  drivers/net/phy/sfp.c     | 63 +++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 63 insertions(+), 11 deletions(-)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 0/2] Add support for VSOL V2801F/CarlitoxxPro CPGOS03 GPON module

[Russell King - ARM Linux admin]

Oh, it's because Pali Rohar's name contains UTF-8 and I can't
encode that prior to sending the actual patches themselves.

Sorry, I'm going to have to have to limit to ASCII only in the
header lines when mailing list patches - scripting the encoding
for UTF-8 characters in shell is far from simple.

On Wed, Dec 09, 2020 at 11:11:09AM +0000, Russell King - ARM Linux admin wrote:
> Jakub,
> 
> What's happening with these patches?
> 
> If I'm getting the patchwork URLs correct, these patches seem not to
> appear in patchwork. They both went to netdev as normal and where
> accepted by vger from what I can see.
> 
> Thanks.
> 
> On Fri, Dec 04, 2020 at 02:34:52PM +0000, Russell King - ARM Linux admin wrote:
> > Hi,
> > 
> > This patch set adds support for the V2801F / CarlitoxxPro module. This
> > requires two changes:
> > 
> > 1) the module only supports single byte reads to the ID EEPROM,
> >    while we need to still permit sequential reads to the diagnostics
> >    EEPROM for atomicity reasons.
> > 
> > 2) we need to relax the encoding check when we have no reported
> >    capabilities to allow 1000base-X based on the module bitrate.
> > 
> > Thanks to Pali Rohár for responsive testing over the last two days.
> > 
> >  drivers/net/phy/sfp-bus.c | 11 ++++-----
> >  drivers/net/phy/sfp.c     | 63 +++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 63 insertions(+), 11 deletions(-)
> > 
> > -- 
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/2] net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround

[Russell King - ARM Linux admin]

On Fri, Dec 04, 2020 at 04:31:52PM +0100, Andrew Lunn wrote:
> > +/* Some modules (Nokia 3FE46541AA) lock up if byte 0x51 is read as a
> > + * single read. Switch back to reading 16 byte blocks unless we have
> > + * a CarlitoxxPro module (rebranded VSOL V2801F). Even more annoyingly,
> > + * some VSOL V2801F have the vendor name changed to OEM.
> 
> Hi Russell
> 
> I guess it is more likely, the vendor name has not been changed _from_
> OEM.

That is very unclear what happened: it seems older dated modules used
VSOL and later ones use OEM, so I think the way I describe it is
likely accurate from what we can observe.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 1/2] net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround

[Russell King]

Add a workaround for the detection of VSOL V2801F / CarlitoxxPro
CPGOS03-0490 v2.0 GPON module which CarlitoxxPro states needs single
byte I2C reads to the EEPROM.

Pali Rohár reports that he also has a CarlitoxxPro-based V2801F module,
which reports a manufacturer of "OEM". This manufacturer can't be
matched as it appears in many different modules, so also match the part
number too.

Reported-by: Thomas Schreiber <tschreibe@gmail.com>
Reported-by: Pali Rohár <pali@kernel.org>
Tested-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/sfp.c | 63 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 34aa196b7465..91d74c1a920a 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -219,6 +219,7 @@ struct sfp {
 	struct sfp_bus *sfp_bus;
 	struct phy_device *mod_phy;
 	const struct sff_data *type;
+	size_t i2c_block_size;
 	u32 max_power_mW;
 
 	unsigned int (*get_state)(struct sfp *);
@@ -335,10 +336,19 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
 			size_t len)
 {
 	struct i2c_msg msgs[2];
-	u8 bus_addr = a2 ? 0x51 : 0x50;
+	size_t block_size;
 	size_t this_len;
+	u8 bus_addr;
 	int ret;
 
+	if (a2) {
+		block_size = 16;
+		bus_addr = 0x51;
+	} else {
+		block_size = sfp->i2c_block_size;
+		bus_addr = 0x50;
+	}
+
 	msgs[0].addr = bus_addr;
 	msgs[0].flags = 0;
 	msgs[0].len = 1;
@@ -350,8 +360,8 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
 
 	while (len) {
 		this_len = len;
-		if (this_len > 16)
-			this_len = 16;
+		if (this_len > block_size)
+			this_len = block_size;
 
 		msgs[1].len = this_len;
 
@@ -1632,6 +1642,28 @@ static int sfp_sm_mod_hpower(struct sfp *sfp, bool enable)
 	return 0;
 }
 
+/* Some modules (Nokia 3FE46541AA) lock up if byte 0x51 is read as a
+ * single read. Switch back to reading 16 byte blocks unless we have
+ * a CarlitoxxPro module (rebranded VSOL V2801F). Even more annoyingly,
+ * some VSOL V2801F have the vendor name changed to OEM.
+ */
+static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
+{
+	if (!memcmp(base->vendor_name, "VSOL            ", 16))
+		return 1;
+	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
+	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
+		return 1;
+
+	/* Some modules can't cope with long reads */
+	return 16;
+}
+
+static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base *base)
+{
+	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
+}
+
 static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)
 {
 	u8 check;
@@ -1673,14 +1705,20 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	u8 check;
 	int ret;
 
-	ret = sfp_read(sfp, false, 0, &id, sizeof(id));
+	/* Some modules (CarlitoxxPro CPGOS03-0490) do not support multibyte
+	 * reads from the EEPROM, so start by reading the base identifying
+	 * information one byte at a time.
+	 */
+	sfp->i2c_block_size = 1;
+
+	ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base));
 	if (ret < 0) {
 		if (report)
 			dev_err(sfp->dev, "failed to read EEPROM: %d\n", ret);
 		return -EAGAIN;
 	}
 
-	if (ret != sizeof(id)) {
+	if (ret != sizeof(id.base)) {
 		dev_err(sfp->dev, "EEPROM short read: %d\n", ret);
 		return -EAGAIN;
 	}
@@ -1719,6 +1757,21 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 		}
 	}
 
+	/* Apply any early module-specific quirks */
+	sfp_quirks_base(sfp, &id.base);
+
+	ret = sfp_read(sfp, false, SFP_CC_BASE + 1, &id.ext, sizeof(id.ext));
+	if (ret < 0) {
+		if (report)
+			dev_err(sfp->dev, "failed to read EEPROM: %d\n", ret);
+		return -EAGAIN;
+	}
+
+	if (ret != sizeof(id.ext)) {
+		dev_err(sfp->dev, "EEPROM short read: %d\n", ret);
+		return -EAGAIN;
+	}
+
 	check = sfp_check(&id.ext, sizeof(id.ext) - 1);
 	if (check != id.ext.cc_ext) {
 		if (cotsworks) {
-- 
2.20.1