netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
@ 2020-12-30 15:47 Pali Rohár
  2020-12-30 15:47 ` [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
                   ` (6 more replies)
  0 siblings, 7 replies; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 15:47 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún
  Cc: netdev, linux-kernel

This patch series add generic workaround for reading EEPROM content from
broken GPON SFP modules based on Realtek RTL8672/RTL9601C chips and add
another workarounds for GPON SFP module Ubiquiti U-Fiber Instant.

GPON SFP modules based on Realtek RTL8672/RTL9601C chips do not have a
real EEPROM but rather EEPROM emulator which is broken and needs special
hack for reading its content.

SFP module detection is done based on EEPROM content. But we obviously
cannot read EEPROM correctly if we do not know what type of connected
SFP module... And to have this chicken and egg problem more complicated,
GPON vendors generally put garbage into their EEPROM content so even
with knowing EEPROM content we do not know what kind of broken SFP is
connected... Workaround for Realtek RTL8672/RTL9601C based modules is
therefore done based on broken EEPROM reading characteristic.

This patch series also available in my git branch sfp-rtl8672:
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=sfp-rtl8672

Pali Rohár (4):
  net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  net: sfp: allow to use also SFP modules which are detected as SFF
  net: sfp: assume that LOS is not implemented if both LOS normal and
    inverted is set
  net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant

 drivers/net/phy/sfp-bus.c |  15 +++++
 drivers/net/phy/sfp.c     | 117 ++++++++++++++++++++++----------------
 2 files changed, 83 insertions(+), 49 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 78+ messages in thread

* [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
@ 2020-12-30 15:47 ` Pali Rohár
  2020-12-30 16:10   ` Russell King - ARM Linux admin
  2020-12-30 15:47 ` [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF Pali Rohár
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 15:47 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún
  Cc: netdev, linux-kernel

Workaround for GPON SFP modules based on VSOL V2801F brand was added in
commit 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490
v2.0 workaround"). But it works only for ids explicitly added to the list.
As there are more rebraded VSOL V2801F modules and OEM vendors are putting
into vendor name random strings we cannot build workaround based on ids.

Moreover issue which that commit tried to workaround is generic not only to
VSOL based modules, but rather to all GPON modules based on Realtek RTL8672
and RTL9601C chips.

They can be found for example in following GPON modules:
* V-SOL V2801F
* C-Data FD511GX-RM0
* OPTON GP801R
* BAUDCOM BD-1234-SFM
* CPGOS03-0490 v2.0
* Ubiquiti U-Fiber Instant
* EXOT EGS1

Those Realtek chips have broken EEPROM emulator which for N-byte read
operation returns just one byte of EEPROM data followed by N-1 zeros.

So introduce a new function sfp_id_needs_byte_io() which detects SFP
modules with these Realtek chips which have broken EEPROM emulator based on
N-1 zeros and switch to 1 byte EEPROM reading operation which workaround
this issue.

This patch fixes reading EEPROM content from SFP modules based on Realtek
RTL8672 and RTL9601C chips.

Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/net/phy/sfp.c | 78 ++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 91d74c1a920a..490e78a72dd6 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -336,19 +336,11 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
 			size_t len)
 {
 	struct i2c_msg msgs[2];
-	size_t block_size;
+	u8 bus_addr = a2 ? 0x51 : 0x50;
+	size_t block_size = sfp->i2c_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;
@@ -1642,26 +1634,30 @@ 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.
+/* GPON modules based on Realtek RTL8672 and RTL9601C chips (e.g. V-SOL
+ * V2801F, CarlitoxxPro CPGOS03-0490, Ubiquiti U-Fiber Instant, ...) do
+ * not support multibyte reads from the EEPROM. Each multi-byte read
+ * operation returns just one byte of EEPROM followed by zeros. There is
+ * no way to identify which modules are using Realtek RTL8672 and RTL9601C
+ * chips. Moreover every OEM of V-SOL V2801F module puts its own vendor
+ * name and vendor id into EEPROM, so there is even no way to detect if
+ * module is V-SOL V2801F. Therefore check for those zeros in the read
+ * data and then based on check switch to reading EEPROM to one byte
+ * at a time.
  */
-static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
+static bool sfp_id_needs_byte_io(struct sfp *sfp, void *buf, size_t len)
 {
-	if (!memcmp(base->vendor_name, "VSOL            ", 16))
-		return 1;
-	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
-	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
-		return 1;
+	size_t i, block_size = sfp->i2c_block_size;
 
-	/* Some modules can't cope with long reads */
-	return 16;
-}
+	/* Already using byte IO */
+	if (block_size == 1)
+		return false;
 
-static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base *base)
-{
-	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
+	for (i = 1; i < len; i += block_size) {
+		if (memchr_inv(buf + i, '\0', block_size - 1))
+			return false;
+	}
+	return true;
 }
 
 static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)
@@ -1705,11 +1701,7 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	u8 check;
 	int ret;
 
-	/* 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;
+	sfp->i2c_block_size = 16;
 
 	ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base));
 	if (ret < 0) {
@@ -1723,6 +1715,27 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 		return -EAGAIN;
 	}
 
+	if (sfp_id_needs_byte_io(sfp, &id.base, sizeof(id.base))) {
+		dev_info(sfp->dev,
+			 "Detected broken RTL8672/RTL9601C emulated EEPROM\n");
+		dev_info(sfp->dev,
+			 "Switching to reading EEPROM to one byte at a time\n");
+		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.base)) {
+			dev_err(sfp->dev, "EEPROM short read: %d\n", ret);
+			return -EAGAIN;
+		}
+	}
+
 	/* Cotsworks do not seem to update the checksums when they
 	 * do the final programming with the final module part number,
 	 * serial number and date code.
@@ -1757,9 +1770,6 @@ 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)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF
  2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
  2020-12-30 15:47 ` [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
@ 2020-12-30 15:47 ` Pali Rohár
  2020-12-30 16:11   ` Russell King - ARM Linux admin
  2020-12-30 15:47 ` [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 15:47 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún
  Cc: netdev, linux-kernel

Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set SFF phys_id
in their EEPROM. Kernel SFP subsystem currently does not allow to use
modules detected as SFF.

This change extends check for SFP modules so also those with SFF phys_id
are allowed. With this change also GPON SFP module Ubiquiti U-Fiber Instant
is recognized.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/net/phy/sfp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 490e78a72dd6..73f3ecf15260 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -273,7 +273,8 @@ static const struct sff_data sff_data = {
 
 static bool sfp_module_supported(const struct sfp_eeprom_id *id)
 {
-	return id->base.phys_id == SFF8024_ID_SFP &&
+	return (id->base.phys_id == SFF8024_ID_SFP ||
+		id->base.phys_id == SFF8024_ID_SFF_8472) &&
 	       id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP;
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
  2020-12-30 15:47 ` [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
  2020-12-30 15:47 ` [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF Pali Rohár
@ 2020-12-30 15:47 ` Pali Rohár
  2020-12-30 16:13   ` Russell King - ARM Linux admin
  2020-12-30 15:47 ` [PATCH 4/4] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 15:47 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún
  Cc: netdev, linux-kernel

Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.

Such combination of bits is meaningless so assume that LOS signal is not
implemented.

This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.

Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 73f3ecf15260..d47485ed239c 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1475,15 +1475,19 @@ static void sfp_sm_link_down(struct sfp *sfp)
 
 static void sfp_sm_link_check_los(struct sfp *sfp)
 {
-	unsigned int los = sfp->state & SFP_F_LOS;
+	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
+	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
+	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
+	bool los = false;
 
 	/* If neither SFP_OPTIONS_LOS_INVERTED nor SFP_OPTIONS_LOS_NORMAL
-	 * are set, we assume that no LOS signal is available.
+	 * are set, we assume that no LOS signal is available. If both are
+	 * set, we assume LOS is not implemented (and is meaningless.)
 	 */
-	if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED))
-		los ^= SFP_F_LOS;
-	else if (!(sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL)))
-		los = 0;
+	if (los_options == los_inverted)
+		los = !(sfp->state & SFP_F_LOS);
+	else if (los_options == los_normal)
+		los = !!(sfp->state & SFP_F_LOS);
 
 	if (los)
 		sfp_sm_next(sfp, SFP_S_WAIT_LOS, 0);
@@ -1493,18 +1497,22 @@ static void sfp_sm_link_check_los(struct sfp *sfp)
 
 static bool sfp_los_event_active(struct sfp *sfp, unsigned int event)
 {
-	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
-		event == SFP_E_LOS_LOW) ||
-	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
-		event == SFP_E_LOS_HIGH);
+	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
+	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
+	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
+
+	return (los_options == los_inverted && event == SFP_E_LOS_LOW) ||
+	       (los_options == los_normal && event == SFP_E_LOS_HIGH);
 }
 
 static bool sfp_los_event_inactive(struct sfp *sfp, unsigned int event)
 {
-	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
-		event == SFP_E_LOS_HIGH) ||
-	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
-		event == SFP_E_LOS_LOW);
+	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
+	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
+	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
+
+	return (los_options == los_inverted && event == SFP_E_LOS_HIGH) ||
+	       (los_options == los_normal && event == SFP_E_LOS_LOW);
 }
 
 static void sfp_sm_fault(struct sfp *sfp, unsigned int next_state, bool warn)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* [PATCH 4/4] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
  2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
                   ` (2 preceding siblings ...)
  2020-12-30 15:47 ` [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
@ 2020-12-30 15:47 ` Pali Rohár
  2021-01-06 15:37 ` [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 15:47 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún
  Cc: netdev, linux-kernel

SFP GPON module Ubiquiti U-Fiber Instant has in its EEPROM stored nonsense
information. It claims that support all transceiver types including 10G
Ethernet which is not truth. So clear all claimed modes and set only one
mode which module supports: 1000baseX_Full.

This change finally allows to detect and use SFP GPON module Ubiquiti
U-Fiber Instant on Linux system.

EEPROM content of this SFP module is (where XX is serial number):

00: 02 04 0b ff ff ff ff ff ff ff ff 03 0c 00 14 c8    ???........??.??
10: 00 00 00 00 55 42 4e 54 20 20 20 20 20 20 20 20    ....UBNT
20: 20 20 20 20 00 18 e8 29 55 46 2d 49 4e 53 54 41        .??)UF-INSTA
30: 4e 54 20 20 20 20 20 20 34 20 20 20 05 1e 00 36    NT      4   ??.6
40: 00 06 00 00 55 42 4e 54 XX XX XX XX XX XX XX XX    .?..UBNTXXXXXXXX
50: 20 20 20 20 31 34 30 31 32 33 20 20 60 80 02 41        140123  `??A

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/net/phy/sfp-bus.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 20b91f5dfc6e..4cf874fb5c5b 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -44,6 +44,17 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
 	phylink_set(modes, 2500baseX_Full);
 }
 
+static void sfp_quirk_ubnt_uf_instant(const struct sfp_eeprom_id *id,
+				      unsigned long *modes)
+{
+	/* Ubiquiti U-Fiber Instant module claims that support all transceiver
+	 * types including 10G Ethernet which is not truth. So clear all claimed
+	 * modes and set only one mode which module supports: 1000baseX_Full.
+	 */
+	phylink_zero(modes);
+	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 +74,10 @@ static const struct sfp_quirk sfp_quirks[] = {
 		.vendor = "HUAWEI",
 		.part = "MA5671A",
 		.modes = sfp_quirk_2500basex,
+	}, {
+		.vendor = "UBNT",
+		.part = "UF-INSTANT",
+		.modes = sfp_quirk_ubnt_uf_instant,
 	},
 };
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 15:47 ` [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
@ 2020-12-30 16:10   ` Russell King - ARM Linux admin
  2020-12-30 16:56     ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 16:10 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 04:47:52PM +0100, Pali Rohár wrote:
> Workaround for GPON SFP modules based on VSOL V2801F brand was added in
> commit 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490
> v2.0 workaround"). But it works only for ids explicitly added to the list.
> As there are more rebraded VSOL V2801F modules and OEM vendors are putting
> into vendor name random strings we cannot build workaround based on ids.
> 
> Moreover issue which that commit tried to workaround is generic not only to
> VSOL based modules, but rather to all GPON modules based on Realtek RTL8672
> and RTL9601C chips.
> 
> They can be found for example in following GPON modules:
> * V-SOL V2801F
> * C-Data FD511GX-RM0
> * OPTON GP801R
> * BAUDCOM BD-1234-SFM
> * CPGOS03-0490 v2.0
> * Ubiquiti U-Fiber Instant
> * EXOT EGS1
> 
> Those Realtek chips have broken EEPROM emulator which for N-byte read
> operation returns just one byte of EEPROM data followed by N-1 zeros.
> 
> So introduce a new function sfp_id_needs_byte_io() which detects SFP
> modules with these Realtek chips which have broken EEPROM emulator based on
> N-1 zeros and switch to 1 byte EEPROM reading operation which workaround
> this issue.
> 
> This patch fixes reading EEPROM content from SFP modules based on Realtek
> RTL8672 and RTL9601C chips.
> 
> Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
> Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/net/phy/sfp.c | 78 ++++++++++++++++++++++++-------------------
>  1 file changed, 44 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 91d74c1a920a..490e78a72dd6 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -336,19 +336,11 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
>  			size_t len)
>  {
>  	struct i2c_msg msgs[2];
> -	size_t block_size;
> +	u8 bus_addr = a2 ? 0x51 : 0x50;
> +	size_t block_size = sfp->i2c_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;
> -	}
> -

NAK. You are undoing something that is definitely needed. The
diagnostics must be read with sequential reads to be able to properly
read the 16-bit values.

The rest of the patch is fine; it's a shame the entire thing has
been spoilt by this extra addition that was not in the patch we had
been discussing off-list.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF
  2020-12-30 15:47 ` [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF Pali Rohár
@ 2020-12-30 16:11   ` Russell King - ARM Linux admin
  2020-12-30 17:06     ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 16:11 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 04:47:53PM +0100, Pali Rohár wrote:
> Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set SFF phys_id
> in their EEPROM. Kernel SFP subsystem currently does not allow to use
> modules detected as SFF.
> 
> This change extends check for SFP modules so also those with SFF phys_id
> are allowed. With this change also GPON SFP module Ubiquiti U-Fiber Instant
> is recognized.

I really don't want to do this for every single module out there.
It's likely that Ubiquiti do this crap as a vendor lock-in measure.
Let's make it specific to Ubiquiti modules _only_ until such time
that we know better.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2020-12-30 15:47 ` [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
@ 2020-12-30 16:13   ` Russell King - ARM Linux admin
  2020-12-30 16:57     ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 16:13 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote:
> Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> 
> Such combination of bits is meaningless so assume that LOS signal is not
> implemented.
> 
> This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> 
> Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

No, this is not co-developed. The patch content is exactly what _I_
sent you, only the commit description is your own.

> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 73f3ecf15260..d47485ed239c 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -1475,15 +1475,19 @@ static void sfp_sm_link_down(struct sfp *sfp)
>  
>  static void sfp_sm_link_check_los(struct sfp *sfp)
>  {
> -	unsigned int los = sfp->state & SFP_F_LOS;
> +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> +	bool los = false;
>  
>  	/* If neither SFP_OPTIONS_LOS_INVERTED nor SFP_OPTIONS_LOS_NORMAL
> -	 * are set, we assume that no LOS signal is available.
> +	 * are set, we assume that no LOS signal is available. If both are
> +	 * set, we assume LOS is not implemented (and is meaningless.)
>  	 */
> -	if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED))
> -		los ^= SFP_F_LOS;
> -	else if (!(sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL)))
> -		los = 0;
> +	if (los_options == los_inverted)
> +		los = !(sfp->state & SFP_F_LOS);
> +	else if (los_options == los_normal)
> +		los = !!(sfp->state & SFP_F_LOS);
>  
>  	if (los)
>  		sfp_sm_next(sfp, SFP_S_WAIT_LOS, 0);
> @@ -1493,18 +1497,22 @@ static void sfp_sm_link_check_los(struct sfp *sfp)
>  
>  static bool sfp_los_event_active(struct sfp *sfp, unsigned int event)
>  {
> -	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
> -		event == SFP_E_LOS_LOW) ||
> -	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
> -		event == SFP_E_LOS_HIGH);
> +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> +
> +	return (los_options == los_inverted && event == SFP_E_LOS_LOW) ||
> +	       (los_options == los_normal && event == SFP_E_LOS_HIGH);
>  }
>  
>  static bool sfp_los_event_inactive(struct sfp *sfp, unsigned int event)
>  {
> -	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
> -		event == SFP_E_LOS_HIGH) ||
> -	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
> -		event == SFP_E_LOS_LOW);
> +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> +
> +	return (los_options == los_inverted && event == SFP_E_LOS_HIGH) ||
> +	       (los_options == los_normal && event == SFP_E_LOS_LOW);
>  }
>  
>  static void sfp_sm_fault(struct sfp *sfp, unsigned int next_state, bool warn)
> -- 
> 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!

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 16:10   ` Russell King - ARM Linux admin
@ 2020-12-30 16:56     ` Pali Rohár
  2020-12-30 17:05       ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 16:56 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wednesday 30 December 2020 16:10:37 Russell King - ARM Linux admin wrote:
> On Wed, Dec 30, 2020 at 04:47:52PM +0100, Pali Rohár wrote:
> > Workaround for GPON SFP modules based on VSOL V2801F brand was added in
> > commit 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490
> > v2.0 workaround"). But it works only for ids explicitly added to the list.
> > As there are more rebraded VSOL V2801F modules and OEM vendors are putting
> > into vendor name random strings we cannot build workaround based on ids.
> > 
> > Moreover issue which that commit tried to workaround is generic not only to
> > VSOL based modules, but rather to all GPON modules based on Realtek RTL8672
> > and RTL9601C chips.
> > 
> > They can be found for example in following GPON modules:
> > * V-SOL V2801F
> > * C-Data FD511GX-RM0
> > * OPTON GP801R
> > * BAUDCOM BD-1234-SFM
> > * CPGOS03-0490 v2.0
> > * Ubiquiti U-Fiber Instant
> > * EXOT EGS1
> > 
> > Those Realtek chips have broken EEPROM emulator which for N-byte read
> > operation returns just one byte of EEPROM data followed by N-1 zeros.
> > 
> > So introduce a new function sfp_id_needs_byte_io() which detects SFP
> > modules with these Realtek chips which have broken EEPROM emulator based on
> > N-1 zeros and switch to 1 byte EEPROM reading operation which workaround
> > this issue.
> > 
> > This patch fixes reading EEPROM content from SFP modules based on Realtek
> > RTL8672 and RTL9601C chips.
> > 
> > Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
> > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  drivers/net/phy/sfp.c | 78 ++++++++++++++++++++++++-------------------
> >  1 file changed, 44 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> > index 91d74c1a920a..490e78a72dd6 100644
> > --- a/drivers/net/phy/sfp.c
> > +++ b/drivers/net/phy/sfp.c
> > @@ -336,19 +336,11 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
> >  			size_t len)
> >  {
> >  	struct i2c_msg msgs[2];
> > -	size_t block_size;
> > +	u8 bus_addr = a2 ? 0x51 : 0x50;
> > +	size_t block_size = sfp->i2c_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;
> > -	}
> > -
> 
> NAK. You are undoing something that is definitely needed. The
> diagnostics must be read with sequential reads to be able to properly
> read the 16-bit values.

This change is really required for those Realtek chips. I thought that
it is obvious that from *both* addresses 0x50 and 0x51 can be read only
one byte at the same time. Reading 2 bytes (for be16 value) cannot be
really done by one i2 transfer, it must be done in two.

Therefore I had to "undo" this change as it is breaking support for all
those Realtek SFPs, including VSOL. That is why I also put Fixes tag
into commit message as it is really fixing reading for VSOL SFP from
address 0x51.

I understand that you want to read __be16 val in sfp_hwmon_read_sensor()
function via one i2c transfer to have "consistent" value, but it is
impossible on these Realtek chips.

I have already tested that sfp_hwmon_read_sensor() function see just
zero on second byte when is trying to use 2-byte read via one i2c
transfer.

When it use two i2c transfers, one for low 8bits and one for high 8bits
then reading is working.

> The rest of the patch is fine; it's a shame the entire thing has
> been spoilt by this extra addition that was not in the patch we had
> been discussing off-list.

Sorry for that. I really thought that it is obvious that this change
needs to be undone and read must always use byte transfer if we detect
these broken Realtek chips.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2020-12-30 16:13   ` Russell King - ARM Linux admin
@ 2020-12-30 16:57     ` Pali Rohár
  2020-12-30 17:06       ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 16:57 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wednesday 30 December 2020 16:13:10 Russell King - ARM Linux admin wrote:
> On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote:
> > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > 
> > Such combination of bits is meaningless so assume that LOS signal is not
> > implemented.
> > 
> > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > 
> > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> No, this is not co-developed. The patch content is exactly what _I_
> sent you, only the commit description is your own.

Sorry, in this case I misunderstood usage of this Co-developed-by tag.
I will remove it in next iteration of patches.

> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++--------------
> >  1 file changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> > index 73f3ecf15260..d47485ed239c 100644
> > --- a/drivers/net/phy/sfp.c
> > +++ b/drivers/net/phy/sfp.c
> > @@ -1475,15 +1475,19 @@ static void sfp_sm_link_down(struct sfp *sfp)
> >  
> >  static void sfp_sm_link_check_los(struct sfp *sfp)
> >  {
> > -	unsigned int los = sfp->state & SFP_F_LOS;
> > +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> > +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> > +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> > +	bool los = false;
> >  
> >  	/* If neither SFP_OPTIONS_LOS_INVERTED nor SFP_OPTIONS_LOS_NORMAL
> > -	 * are set, we assume that no LOS signal is available.
> > +	 * are set, we assume that no LOS signal is available. If both are
> > +	 * set, we assume LOS is not implemented (and is meaningless.)
> >  	 */
> > -	if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED))
> > -		los ^= SFP_F_LOS;
> > -	else if (!(sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL)))
> > -		los = 0;
> > +	if (los_options == los_inverted)
> > +		los = !(sfp->state & SFP_F_LOS);
> > +	else if (los_options == los_normal)
> > +		los = !!(sfp->state & SFP_F_LOS);
> >  
> >  	if (los)
> >  		sfp_sm_next(sfp, SFP_S_WAIT_LOS, 0);
> > @@ -1493,18 +1497,22 @@ static void sfp_sm_link_check_los(struct sfp *sfp)
> >  
> >  static bool sfp_los_event_active(struct sfp *sfp, unsigned int event)
> >  {
> > -	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
> > -		event == SFP_E_LOS_LOW) ||
> > -	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
> > -		event == SFP_E_LOS_HIGH);
> > +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> > +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> > +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> > +
> > +	return (los_options == los_inverted && event == SFP_E_LOS_LOW) ||
> > +	       (los_options == los_normal && event == SFP_E_LOS_HIGH);
> >  }
> >  
> >  static bool sfp_los_event_inactive(struct sfp *sfp, unsigned int event)
> >  {
> > -	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
> > -		event == SFP_E_LOS_HIGH) ||
> > -	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
> > -		event == SFP_E_LOS_LOW);
> > +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> > +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> > +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> > +
> > +	return (los_options == los_inverted && event == SFP_E_LOS_HIGH) ||
> > +	       (los_options == los_normal && event == SFP_E_LOS_LOW);
> >  }
> >  
> >  static void sfp_sm_fault(struct sfp *sfp, unsigned int next_state, bool warn)
> > -- 
> > 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!

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 16:56     ` Pali Rohár
@ 2020-12-30 17:05       ` Russell King - ARM Linux admin
  2020-12-30 17:13         ` Andrew Lunn
  2020-12-30 17:31         ` Pali Rohár
  0 siblings, 2 replies; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 17:05 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 05:56:34PM +0100, Pali Rohár wrote:
> This change is really required for those Realtek chips. I thought that
> it is obvious that from *both* addresses 0x50 and 0x51 can be read only
> one byte at the same time. Reading 2 bytes (for be16 value) cannot be
> really done by one i2 transfer, it must be done in two.

Then these modules are even more broken than first throught, and
quite simply it is pointless supporting the diagnostics on them
because we can never read the values in an atomic way.

It's also a violation of the SFF-8472 that _requires_ multi-byte reads
to read these 16 byte values atomically. Reading them with individual
byte reads results in a non-atomic read, and the 16-bit value can not
be trusted to be correct.

That is really not optional, no matter what any manufacturer says - if
they claim the SFP MSAs allows it, they're quite simply talking out of
a donkey's backside and you should dispose of the module in biohazard
packaging. :)

So no, I hadn't understood this from your emails, and as I say above,
if this is the case, then we quite simply disable diagnostics on these
modules since they are _highly_ noncompliant.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2020-12-30 16:57     ` Pali Rohár
@ 2020-12-30 17:06       ` Russell King - ARM Linux admin
  2020-12-30 17:17         ` Andrew Lunn
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 17:06 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 05:57:58PM +0100, Pali Rohár wrote:
> On Wednesday 30 December 2020 16:13:10 Russell King - ARM Linux admin wrote:
> > On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote:
> > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > > 
> > > Such combination of bits is meaningless so assume that LOS signal is not
> > > implemented.
> > > 
> > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > > 
> > > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > No, this is not co-developed. The patch content is exactly what _I_
> > sent you, only the commit description is your own.
> 
> Sorry, in this case I misunderstood usage of this Co-developed-by tag.
> I will remove it in next iteration of patches.

You need to mark me as the author of the code at the very least...

> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >  drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++--------------
> > >  1 file changed, 22 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> > > index 73f3ecf15260..d47485ed239c 100644
> > > --- a/drivers/net/phy/sfp.c
> > > +++ b/drivers/net/phy/sfp.c
> > > @@ -1475,15 +1475,19 @@ static void sfp_sm_link_down(struct sfp *sfp)
> > >  
> > >  static void sfp_sm_link_check_los(struct sfp *sfp)
> > >  {
> > > -	unsigned int los = sfp->state & SFP_F_LOS;
> > > +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> > > +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> > > +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> > > +	bool los = false;
> > >  
> > >  	/* If neither SFP_OPTIONS_LOS_INVERTED nor SFP_OPTIONS_LOS_NORMAL
> > > -	 * are set, we assume that no LOS signal is available.
> > > +	 * are set, we assume that no LOS signal is available. If both are
> > > +	 * set, we assume LOS is not implemented (and is meaningless.)
> > >  	 */
> > > -	if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED))
> > > -		los ^= SFP_F_LOS;
> > > -	else if (!(sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL)))
> > > -		los = 0;
> > > +	if (los_options == los_inverted)
> > > +		los = !(sfp->state & SFP_F_LOS);
> > > +	else if (los_options == los_normal)
> > > +		los = !!(sfp->state & SFP_F_LOS);
> > >  
> > >  	if (los)
> > >  		sfp_sm_next(sfp, SFP_S_WAIT_LOS, 0);
> > > @@ -1493,18 +1497,22 @@ static void sfp_sm_link_check_los(struct sfp *sfp)
> > >  
> > >  static bool sfp_los_event_active(struct sfp *sfp, unsigned int event)
> > >  {
> > > -	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
> > > -		event == SFP_E_LOS_LOW) ||
> > > -	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
> > > -		event == SFP_E_LOS_HIGH);
> > > +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> > > +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> > > +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> > > +
> > > +	return (los_options == los_inverted && event == SFP_E_LOS_LOW) ||
> > > +	       (los_options == los_normal && event == SFP_E_LOS_HIGH);
> > >  }
> > >  
> > >  static bool sfp_los_event_inactive(struct sfp *sfp, unsigned int event)
> > >  {
> > > -	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
> > > -		event == SFP_E_LOS_HIGH) ||
> > > -	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
> > > -		event == SFP_E_LOS_LOW);
> > > +	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
> > > +	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
> > > +	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
> > > +
> > > +	return (los_options == los_inverted && event == SFP_E_LOS_HIGH) ||
> > > +	       (los_options == los_normal && event == SFP_E_LOS_LOW);
> > >  }
> > >  
> > >  static void sfp_sm_fault(struct sfp *sfp, unsigned int next_state, bool warn)
> > > -- 
> > > 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!
> 

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF
  2020-12-30 16:11   ` Russell King - ARM Linux admin
@ 2020-12-30 17:06     ` Pali Rohár
  2020-12-30 17:27       ` Marek Behún
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 17:06 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wednesday 30 December 2020 16:11:51 Russell King - ARM Linux admin wrote:
> On Wed, Dec 30, 2020 at 04:47:53PM +0100, Pali Rohár wrote:
> > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set SFF phys_id
> > in their EEPROM. Kernel SFP subsystem currently does not allow to use
> > modules detected as SFF.
> > 
> > This change extends check for SFP modules so also those with SFF phys_id
> > are allowed. With this change also GPON SFP module Ubiquiti U-Fiber Instant
> > is recognized.
> 
> I really don't want to do this for every single module out there.
> It's likely that Ubiquiti do this crap as a vendor lock-in measure.
> Let's make it specific to Ubiquiti modules _only_ until such time
> that we know better.

Ok. This module_supported() function is called in sfp_sm_mod_probe()
function. Current code is:

	/* Check whether we support this module */
	if (!sfp->type->module_supported(&id)) {
		dev_err(sfp->dev,
			"module is not supported - phys id 0x%02x 0x%02x\n",
			sfp->id.base.phys_id, sfp->id.base.phys_ext_id);
		return -EINVAL;
	}

Do you want to change code to something like this?

	/* Check whether we support this module */
	if (!sfp->type->module_supported(&id) &&
	    (memcmp(id.base.vendor_name, "UBNT            ", 16) ||
	     memcmp(id.base.vendor_pn, "UF-INSTANT      ", 16)))
		dev_err(sfp->dev,
			"module is not supported - phys id 0x%02x 0x%02x\n",
			sfp->id.base.phys_id, sfp->id.base.phys_ext_id);
		return -EINVAL;
	}

Or do you have a better idea how to skip that module_supported check for
this UBNT SFP?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 17:05       ` Russell King - ARM Linux admin
@ 2020-12-30 17:13         ` Andrew Lunn
  2020-12-30 17:43           ` Pali Rohár
  2020-12-30 17:31         ` Pali Rohár
  1 sibling, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2020-12-30 17:13 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Pali Rohár, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 05:05:46PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Dec 30, 2020 at 05:56:34PM +0100, Pali Rohár wrote:
> > This change is really required for those Realtek chips. I thought that
> > it is obvious that from *both* addresses 0x50 and 0x51 can be read only
> > one byte at the same time. Reading 2 bytes (for be16 value) cannot be
> > really done by one i2 transfer, it must be done in two.
> 
> Then these modules are even more broken than first throught, and
> quite simply it is pointless supporting the diagnostics on them
> because we can never read the values in an atomic way.
> 
> It's also a violation of the SFF-8472 that _requires_ multi-byte reads
> to read these 16 byte values atomically. Reading them with individual
> byte reads results in a non-atomic read, and the 16-bit value can not
> be trusted to be correct.

Hi Pali

I have to agree with Russell here. I would rather have no diagnostics
than untrustable diagnostics.

The only way this is going to be accepted is if the manufacture says
that reading the first byte of a word snapshots the second byte as
well in an atomic way and returns that snapshot on the second
read. But i highly doubt that happens, given how bad these SFPs are.

      Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2020-12-30 17:06       ` Russell King - ARM Linux admin
@ 2020-12-30 17:17         ` Andrew Lunn
  2020-12-30 17:32           ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2020-12-30 17:17 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Pali Rohár, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 05:06:23PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Dec 30, 2020 at 05:57:58PM +0100, Pali Rohár wrote:
> > On Wednesday 30 December 2020 16:13:10 Russell King - ARM Linux admin wrote:
> > > On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote:
> > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > > > 
> > > > Such combination of bits is meaningless so assume that LOS signal is not
> > > > implemented.
> > > > 
> > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > > > 
> > > > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > 
> > > No, this is not co-developed. The patch content is exactly what _I_
> > > sent you, only the commit description is your own.
> > 
> > Sorry, in this case I misunderstood usage of this Co-developed-by tag.
> > I will remove it in next iteration of patches.
> 
> You need to mark me as the author of the code at the very least...

Hi Pali

You also need to keep your own Signed-off-by, since the patch is
coming through you.

So basically, git commit --am --author="Russell King <rmk+kernel@armlinux.org.uk>"
and then two Signed-off-by: lines.

   Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF
  2020-12-30 17:06     ` Pali Rohár
@ 2020-12-30 17:27       ` Marek Behún
  2020-12-30 19:12         ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Marek Behún @ 2020-12-30 17:27 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, Andrew Lunn, Heiner Kallweit,
	David S. Miller, Jakub Kicinski, netdev, linux-kernel

On Wed, 30 Dec 2020 18:06:52 +0100
Pali Rohár <pali@kernel.org> wrote:

> 	if (!sfp->type->module_supported(&id) &&
> 	    (memcmp(id.base.vendor_name, "UBNT            ", 16) ||
> 	     memcmp(id.base.vendor_pn, "UF-INSTANT      ", 16)))

I would rather add a quirk member (bitfield) to the sfp structure and do
something like this

if (!sfp->type->module_supported(&id) &&
    !(sfp->quirks & SFP_QUIRK_BAD_PHYS_ID))

or maybe put this check into the module_supported method.

Marek

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 17:05       ` Russell King - ARM Linux admin
  2020-12-30 17:13         ` Andrew Lunn
@ 2020-12-30 17:31         ` Pali Rohár
  2020-12-30 19:18           ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 17:31 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wednesday 30 December 2020 17:05:46 Russell King - ARM Linux admin wrote:
> On Wed, Dec 30, 2020 at 05:56:34PM +0100, Pali Rohár wrote:
> > This change is really required for those Realtek chips. I thought that
> > it is obvious that from *both* addresses 0x50 and 0x51 can be read only
> > one byte at the same time. Reading 2 bytes (for be16 value) cannot be
> > really done by one i2 transfer, it must be done in two.
> 
> Then these modules are even more broken than first throught, and
> quite simply it is pointless supporting the diagnostics on them
> because we can never read the values in an atomic way.

They are broken in a way that neither holy water help them...

But from diagnostic 0x51 address we can read at least 8bit registers in
atomic way :-)

> It's also a violation of the SFF-8472 that _requires_ multi-byte reads
> to read these 16 byte values atomically. Reading them with individual
> byte reads results in a non-atomic read, and the 16-bit value can not
> be trusted to be correct.
> 
> That is really not optional, no matter what any manufacturer says - if
> they claim the SFP MSAs allows it, they're quite simply talking out of
> a donkey's backside and you should dispose of the module in biohazard
> packaging. :)
> 
> So no, I hadn't understood this from your emails, and as I say above,
> if this is the case, then we quite simply disable diagnostics on these
> modules since they are _highly_ noncompliant.

We have just two options:

Disable 2 (and more) bytes reads from 0x51 address and therefore disable
sfp_hwmon_read_sensor() function.

Or allow 2 bytes non-atomic reads and allow at least semi-correct values
for hwmon. I guess that upper 8bits would not change between two single
byte i2c transfers too much (when they are done immediately one by one).

But in any case we really need to set read operation for this eeprom to
one byte.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2020-12-30 17:17         ` Andrew Lunn
@ 2020-12-30 17:32           ` Pali Rohár
  0 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 17:32 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wednesday 30 December 2020 18:17:41 Andrew Lunn wrote:
> On Wed, Dec 30, 2020 at 05:06:23PM +0000, Russell King - ARM Linux admin wrote:
> > On Wed, Dec 30, 2020 at 05:57:58PM +0100, Pali Rohár wrote:
> > > On Wednesday 30 December 2020 16:13:10 Russell King - ARM Linux admin wrote:
> > > > On Wed, Dec 30, 2020 at 04:47:54PM +0100, Pali Rohár wrote:
> > > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > > > > 
> > > > > Such combination of bits is meaningless so assume that LOS signal is not
> > > > > implemented.
> > > > > 
> > > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > > > > 
> > > > > Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > 
> > > > No, this is not co-developed. The patch content is exactly what _I_
> > > > sent you, only the commit description is your own.
> > > 
> > > Sorry, in this case I misunderstood usage of this Co-developed-by tag.
> > > I will remove it in next iteration of patches.
> > 
> > You need to mark me as the author of the code at the very least...
> 
> Hi Pali
> 
> You also need to keep your own Signed-off-by, since the patch is
> coming through you.
> 
> So basically, git commit --am --author="Russell King <rmk+kernel@armlinux.org.uk>"
> and then two Signed-off-by: lines.

Got it, thank you!

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 17:13         ` Andrew Lunn
@ 2020-12-30 17:43           ` Pali Rohár
  2020-12-30 19:09             ` Russell King - ARM Linux admin
  2020-12-30 19:30             ` Andrew Lunn
  0 siblings, 2 replies; 78+ messages in thread
From: Pali Rohár @ 2020-12-30 17:43 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wednesday 30 December 2020 18:13:15 Andrew Lunn wrote:
> On Wed, Dec 30, 2020 at 05:05:46PM +0000, Russell King - ARM Linux admin wrote:
> > On Wed, Dec 30, 2020 at 05:56:34PM +0100, Pali Rohár wrote:
> > > This change is really required for those Realtek chips. I thought that
> > > it is obvious that from *both* addresses 0x50 and 0x51 can be read only
> > > one byte at the same time. Reading 2 bytes (for be16 value) cannot be
> > > really done by one i2 transfer, it must be done in two.
> > 
> > Then these modules are even more broken than first throught, and
> > quite simply it is pointless supporting the diagnostics on them
> > because we can never read the values in an atomic way.
> > 
> > It's also a violation of the SFF-8472 that _requires_ multi-byte reads
> > to read these 16 byte values atomically. Reading them with individual
> > byte reads results in a non-atomic read, and the 16-bit value can not
> > be trusted to be correct.
> 
> Hi Pali
> 
> I have to agree with Russell here. I would rather have no diagnostics
> than untrustable diagnostics.

Ok!

So should we completely skip hwmon_device_register_with_info() call
if (i2c_block_size < 2) ?

> The only way this is going to be accepted is if the manufacture says
> that reading the first byte of a word snapshots the second byte as
> well in an atomic way and returns that snapshot on the second
> read. But i highly doubt that happens, given how bad these SFPs are.

I do not think that manufacture says something. I think that they even
do not know that their Realtek chips are completely broken.

I can imagine that vendor just says: it is working in our branded boxes
with SFP cages and if it does not work in your kernel then problem is
with your custom kernel and we do not care about 3rd parties.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 17:43           ` Pali Rohár
@ 2020-12-30 19:09             ` Russell King - ARM Linux admin
  2020-12-30 19:49               ` Andrew Lunn
  2020-12-31 12:14               ` Pali Rohár
  2020-12-30 19:30             ` Andrew Lunn
  1 sibling, 2 replies; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 19:09 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 06:43:07PM +0100, Pali Rohár wrote:
> On Wednesday 30 December 2020 18:13:15 Andrew Lunn wrote:
> > Hi Pali
> > 
> > I have to agree with Russell here. I would rather have no diagnostics
> > than untrustable diagnostics.
> 
> Ok!
> 
> So should we completely skip hwmon_device_register_with_info() call
> if (i2c_block_size < 2) ?

I don't think that alone is sufficient - there's also the matter of
ethtool -m which will dump that information as well, and we don't want
to offer it to userspace in an unreliable form.

For reference, here is what SFF-8472 which defines the diagnostics, says
about this:

  To guarantee coherency of the diagnostic monitoring data, the host is
  required to retrieve any multi-byte fields from the diagnostic
  monitoring data structure (IE: Rx Power MSB - byte 104 in A2h, Rx Power
  LSB - byte 105 in A2h) by the use of a single two-byte read sequence
  across the two-wire interface interface.

  The transceiver is required to ensure that any multi-byte fields which
  are updated with diagnostic monitoring data (e.g. Rx Power MSB - byte
  104 in A2h, Rx Power LSB - byte 105 in A2h) must have this update done
  in a fashion which guarantees coherency and consistency of the data. In
  other words, the update of a multi-byte field by the transceiver must
  not occur such that a partially updated multi-byte field can be
  transferred to the host. Also, the transceiver shall not update a
  multi-byte field within the structure during the transfer of that
  multi-byte field to the host, such that partially updated data would be
  transferred to the host.

The first paragraph is extremely definitive in how these fields shall
be read atomically - by a _single_ two-byte read sequence. From what
you are telling us, these modules do not support that. Therefore, by
definition, they do *not* support proper and reliable reporting of
diagnostic data, and are non-conformant with the SFP MSAs.

So, they are basically broken, and the diagnostics can't be used to
retrieve data that can be said to be useful.

> I do not think that manufacture says something. I think that they even
> do not know that their Realtek chips are completely broken.

Oh, they do know. I had a response from CarlitoxxPro passed to me, which
was:

  That is a behavior related on how your router/switch try to read the
  EEPROM, as described in the datasheet of the GPON ONU SFP, the EEPROM
  can be read in Sequential Single-Byte mode, not in Multi-byte mode as
  you router do, basically, your router is trying to read the full a0h
  table in a single call, and retrieve a null response. that is normal
  and not affect the operations of the GPON ONU SFP, because these
  values are informational only. so the Software for your router should
  be able to read in Single-Byte mode to read the content of the EEPROM
  in concordance to SFF-8431

which totally misses the point that it is /not/ up to the module to
choose whether multi-byte reads are supported or not. If they bothered
to gain a proper understanding of the MSAs, they would have noticed that
the device on 0xA0 is required to behave as an Atmel AT24Cxx EEPROM.
The following from INF-8074i, which is the very first definition of the
SFP form factor modules:

  The SFP serial ID provides access to sophisticated identification
  information that describes the transceiver's capabilities, standard
  interfaces, manufacturer, and other information. The serial interface
  uses the 2-wire serial CMOS E2PROM protocol defined for the ATMEL
  AT24C01A/02/04 family of components.

As they took less than one working day to provide the above response, I
suspect they know full well that there's a problem - and it likely
affects other "routers" as well.

They're also confused about their SFF specifications. SFF-8431 is: "SFP+
10 Gb/s and Low Speed Electrical Interface" which is not the correct
specification for a 1Gbps module.

> I can imagine that vendor just says: it is working in our branded boxes
> with SFP cages and if it does not work in your kernel then problem is
> with your custom kernel and we do not care about 3rd parties.

Which shows why it's pointless producing an EEPROM validation tool that
runs under Linux (as has been your suggestion). They won't use it,
since their testing only goes as far as "does it work in our product?"

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF
  2020-12-30 17:27       ` Marek Behún
@ 2020-12-30 19:12         ` Russell King - ARM Linux admin
  2020-12-31 13:52           ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 19:12 UTC (permalink / raw)
  To: Marek Behún
  Cc: Pali Rohár, Andrew Lunn, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, netdev, linux-kernel

On Wed, Dec 30, 2020 at 06:27:07PM +0100, Marek Behún wrote:
> On Wed, 30 Dec 2020 18:06:52 +0100
> Pali Rohár <pali@kernel.org> wrote:
> 
> > 	if (!sfp->type->module_supported(&id) &&
> > 	    (memcmp(id.base.vendor_name, "UBNT            ", 16) ||
> > 	     memcmp(id.base.vendor_pn, "UF-INSTANT      ", 16)))
> 
> I would rather add a quirk member (bitfield) to the sfp structure and do
> something like this
> 
> if (!sfp->type->module_supported(&id) &&
>     !(sfp->quirks & SFP_QUIRK_BAD_PHYS_ID))
> 
> or maybe put this check into the module_supported method.

Sorry, definitely not. If you've ever looked at the SDHCI driver with
its multiple "quirks" bitfields, doing this is a recipe for creating
a very horrid hard to understand mess.

What you suggest just results in yet more complexity.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 17:31         ` Pali Rohár
@ 2020-12-30 19:18           ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 19:18 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 06:31:52PM +0100, Pali Rohár wrote:
> On Wednesday 30 December 2020 17:05:46 Russell King - ARM Linux admin wrote:
> > On Wed, Dec 30, 2020 at 05:56:34PM +0100, Pali Rohár wrote:
> > > This change is really required for those Realtek chips. I thought that
> > > it is obvious that from *both* addresses 0x50 and 0x51 can be read only
> > > one byte at the same time. Reading 2 bytes (for be16 value) cannot be
> > > really done by one i2 transfer, it must be done in two.
> > 
> > Then these modules are even more broken than first throught, and
> > quite simply it is pointless supporting the diagnostics on them
> > because we can never read the values in an atomic way.
> 
> They are broken in a way that neither holy water help them...
> 
> But from diagnostic 0x51 address we can read at least 8bit registers in
> atomic way :-)

... which doesn't fit the requirements.

> > It's also a violation of the SFF-8472 that _requires_ multi-byte reads
> > to read these 16 byte values atomically. Reading them with individual
> > byte reads results in a non-atomic read, and the 16-bit value can not
> > be trusted to be correct.
> > 
> > That is really not optional, no matter what any manufacturer says - if
> > they claim the SFP MSAs allows it, they're quite simply talking out of
> > a donkey's backside and you should dispose of the module in biohazard
> > packaging. :)
> > 
> > So no, I hadn't understood this from your emails, and as I say above,
> > if this is the case, then we quite simply disable diagnostics on these
> > modules since they are _highly_ noncompliant.
> 
> We have just two options:
> 
> Disable 2 (and more) bytes reads from 0x51 address and therefore disable
> sfp_hwmon_read_sensor() function.
> 
> Or allow 2 bytes non-atomic reads and allow at least semi-correct values
> for hwmon. I guess that upper 8bits would not change between two single
> byte i2c transfers too much (when they are done immediately one by one).

So when you read the temperature, and the MSB reads as the next higher
value than the LSB, causing an error of 256, or vice versa causing an
error of -256, which when scaled according to the factors causes a big
error, that's acceptable.

No, it isn't. If the data can't be read reliably, the data is useless.

Consider a system that implements userspace monitoring for modules and
checks the current values against pre-set thresholds - it suddenly gets
a value that is outside of its alarm threshold due to this. It raises a
false alarm. This is not good.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 17:43           ` Pali Rohár
  2020-12-30 19:09             ` Russell King - ARM Linux admin
@ 2020-12-30 19:30             ` Andrew Lunn
  2020-12-30 19:39               ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2020-12-30 19:30 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

> Ok!
> 
> So should we completely skip hwmon_device_register_with_info() call
> if (i2c_block_size < 2) ?

Yep. That would be a nice simple test.

But does ethtool -m still export the second page? That is also
unreliable.

	Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 19:30             ` Andrew Lunn
@ 2020-12-30 19:39               ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2020-12-30 19:39 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Pali Rohár, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wed, Dec 30, 2020 at 08:30:52PM +0100, Andrew Lunn wrote:
> > Ok!
> > 
> > So should we completely skip hwmon_device_register_with_info() call
> > if (i2c_block_size < 2) ?
> 
> Yep. That would be a nice simple test.
> 
> But does ethtool -m still export the second page? That is also
> unreliable.

It will do, but we need to check how ethtool decides to request/dump
that information - we probably need to make sfp_module_info()
report that it's a ETH_MODULE_SFF_8079 style EEPROM, not the
ETH_MODULE_SFF_8472 style.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 19:09             ` Russell King - ARM Linux admin
@ 2020-12-30 19:49               ` Andrew Lunn
  2020-12-31 12:14               ` Pali Rohár
  1 sibling, 0 replies; 78+ messages in thread
From: Andrew Lunn @ 2020-12-30 19:49 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Pali Rohár, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

> Which shows why it's pointless producing an EEPROM validation tool that
> runs under Linux (as has been your suggestion). They won't use it,
> since their testing only goes as far as "does it work in our product?"

Yes, i would need SNIA to push for conformance testing, hold
plug-testing events, and marketing that only devices which pass the
conformance test are allowed to use the SNIA logo, etc.

They do seem to have conformance testing for some of their storage
standards, but nothing for SFPs.

	   Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-30 19:09             ` Russell King - ARM Linux admin
  2020-12-30 19:49               ` Andrew Lunn
@ 2020-12-31 12:14               ` Pali Rohár
  2020-12-31 15:09                 ` Andrew Lunn
  2020-12-31 15:30                 ` Andrew Lunn
  1 sibling, 2 replies; 78+ messages in thread
From: Pali Rohár @ 2020-12-31 12:14 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Wednesday 30 December 2020 19:09:58 Russell King - ARM Linux admin wrote:
> On Wed, Dec 30, 2020 at 06:43:07PM +0100, Pali Rohár wrote:
> > On Wednesday 30 December 2020 18:13:15 Andrew Lunn wrote:
> > > Hi Pali
> > > 
> > > I have to agree with Russell here. I would rather have no diagnostics
> > > than untrustable diagnostics.
> > 
> > Ok!
> > 
> > So should we completely skip hwmon_device_register_with_info() call
> > if (i2c_block_size < 2) ?
> 
> I don't think that alone is sufficient - there's also the matter of
> ethtool -m which will dump that information as well, and we don't want
> to offer it to userspace in an unreliable form.

Any idea/preference how to disable access to these registers?

> For reference, here is what SFF-8472 which defines the diagnostics, says
> about this:
> 
>   To guarantee coherency of the diagnostic monitoring data, the host is
>   required to retrieve any multi-byte fields from the diagnostic
>   monitoring data structure (IE: Rx Power MSB - byte 104 in A2h, Rx Power
>   LSB - byte 105 in A2h) by the use of a single two-byte read sequence
>   across the two-wire interface interface.
> 
>   The transceiver is required to ensure that any multi-byte fields which
>   are updated with diagnostic monitoring data (e.g. Rx Power MSB - byte
>   104 in A2h, Rx Power LSB - byte 105 in A2h) must have this update done
>   in a fashion which guarantees coherency and consistency of the data. In
>   other words, the update of a multi-byte field by the transceiver must
>   not occur such that a partially updated multi-byte field can be
>   transferred to the host. Also, the transceiver shall not update a
>   multi-byte field within the structure during the transfer of that
>   multi-byte field to the host, such that partially updated data would be
>   transferred to the host.
> 
> The first paragraph is extremely definitive in how these fields shall
> be read atomically - by a _single_ two-byte read sequence. From what
> you are telling us, these modules do not support that. Therefore, by
> definition, they do *not* support proper and reliable reporting of
> diagnostic data, and are non-conformant with the SFP MSAs.
> 
> So, they are basically broken, and the diagnostics can't be used to
> retrieve data that can be said to be useful.

I agree they are broken. We really should disable access to those 16bit
registers.

Anyway here is "datasheet" to some CarlitoxxPro SFP:
https://www.docdroid.net/hRsJ560/cpgos03-0490v2-datasheet-10-pdf

And on page 10 is written:

    The I2C system can support the mode of random address / single
    byteread which conform to SFF-8431.

Which seems to be wrong.

> > I do not think that manufacture says something. I think that they even
> > do not know that their Realtek chips are completely broken.
> 
> Oh, they do know. I had a response from CarlitoxxPro passed to me, which
> was:

Could you give me contact address?

Anyway, we would rather should contact Realtek chip division, real
manufacture. Not CarlitoxxPro rebrander which can just "buy product"
from Realtek reseller and put its logo on stick (and maybe configuration
like sn, mac address, password and enable/disable bit for web/telnet).

>   That is a behavior related on how your router/switch try to read the
>   EEPROM, as described in the datasheet of the GPON ONU SFP, the EEPROM
>   can be read in Sequential Single-Byte mode, not in Multi-byte mode as
>   you router do, basically, your router is trying to read the full a0h
>   table in a single call, and retrieve a null response. that is normal
>   and not affect the operations of the GPON ONU SFP, because these
>   values are informational only. so the Software for your router should
>   be able to read in Single-Byte mode to read the content of the EEPROM
>   in concordance to SFF-8431
> 
> which totally misses the point that it is /not/ up to the module to
> choose whether multi-byte reads are supported or not. If they bothered
> to gain a proper understanding of the MSAs, they would have noticed that
> the device on 0xA0 is required to behave as an Atmel AT24Cxx EEPROM.
> The following from INF-8074i, which is the very first definition of the
> SFP form factor modules:
> 
>   The SFP serial ID provides access to sophisticated identification
>   information that describes the transceiver's capabilities, standard
>   interfaces, manufacturer, and other information. The serial interface
>   uses the 2-wire serial CMOS E2PROM protocol defined for the ATMEL
>   AT24C01A/02/04 family of components.
> 
> As they took less than one working day to provide the above response, I
> suspect they know full well that there's a problem - and it likely
> affects other "routers" as well.

As this issue is with all those Realtek chips I really think this issue
is in underlying Realtek chip and not in CarlitoxxPro rebranding. Seems
that they know about this issue and because it affects all GPON Realtek
SFPs with that chip that cannot do anything with it, so just wrote it
into "datasheet" and trying to find arguments "why behavior is correct"
even it is not truth.

> They're also confused about their SFF specifications. SFF-8431 is: "SFP+
> 10 Gb/s and Low Speed Electrical Interface" which is not the correct
> specification for a 1Gbps module.

Looks like "trying to find arguments why it is correct".

> > I can imagine that vendor just says: it is working in our branded boxes
> > with SFP cages and if it does not work in your kernel then problem is
> > with your custom kernel and we do not care about 3rd parties.
> 
> Which shows why it's pointless producing an EEPROM validation tool that
> runs under Linux (as has been your suggestion). They won't use it,
> since their testing only goes as far as "does it work in our product?"

At least users could do their own validation and for rewritable EEPROMs
they could be able to fix its content.

Anyway, if there is such tool then users could start creating database
of working and non-working SFPs where can be put result of this tool.

It could help people to decide which SFP they should buy and which not.
Sending page/database to manufactures with showing "do not buy this your
SFP, does not work and is broken" maybe change their state and start
doing validation if people stop buying their products or manufacture
name would be on unsupported black list.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF
  2020-12-30 19:12         ` Russell King - ARM Linux admin
@ 2020-12-31 13:52           ` Pali Rohár
  0 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2020-12-31 13:52 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Marek Behún, Andrew Lunn, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, netdev, linux-kernel

On Wednesday 30 December 2020 19:12:40 Russell King - ARM Linux admin wrote:
> On Wed, Dec 30, 2020 at 06:27:07PM +0100, Marek Behún wrote:
> > On Wed, 30 Dec 2020 18:06:52 +0100
> > Pali Rohár <pali@kernel.org> wrote:
> > 
> > > 	if (!sfp->type->module_supported(&id) &&
> > > 	    (memcmp(id.base.vendor_name, "UBNT            ", 16) ||
> > > 	     memcmp(id.base.vendor_pn, "UF-INSTANT      ", 16)))
> > 
> > I would rather add a quirk member (bitfield) to the sfp structure and do
> > something like this
> > 
> > if (!sfp->type->module_supported(&id) &&
> >     !(sfp->quirks & SFP_QUIRK_BAD_PHYS_ID))
> > 
> > or maybe put this check into the module_supported method.
> 
> Sorry, definitely not. If you've ever looked at the SDHCI driver with
> its multiple "quirks" bitfields, doing this is a recipe for creating
> a very horrid hard to understand mess.
> 
> What you suggest just results in yet more complexity.

Should I rather put this vendor name/pn check into the
sfp_module_supported() function?

static bool sfp_module_supported(const struct sfp_eeprom_id *id)
{
	if (id->base.phys_id == SFF8024_ID_SFP &&
	    id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP)
		return true;

	if (id->base.phys_id == SFF8024_ID_SFF_8472 &&
	    id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP &&
	    !memcmp(id->base.vendor_name, "UBNT            ", 16) &&
	    !memcmp(id->base.vendor_pn, "UF-INSTANT      ", 16))
		return true;

	return false;
}

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-31 12:14               ` Pali Rohár
@ 2020-12-31 15:09                 ` Andrew Lunn
  2020-12-31 15:40                   ` Pali Rohár
  2020-12-31 15:30                 ` Andrew Lunn
  1 sibling, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2020-12-31 15:09 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Thu, Dec 31, 2020 at 01:14:10PM +0100, Pali Rohár wrote:
> On Wednesday 30 December 2020 19:09:58 Russell King - ARM Linux admin wrote:
> > On Wed, Dec 30, 2020 at 06:43:07PM +0100, Pali Rohár wrote:
> > > On Wednesday 30 December 2020 18:13:15 Andrew Lunn wrote:
> > > > Hi Pali
> > > > 
> > > > I have to agree with Russell here. I would rather have no diagnostics
> > > > than untrustable diagnostics.
> > > 
> > > Ok!
> > > 
> > > So should we completely skip hwmon_device_register_with_info() call
> > > if (i2c_block_size < 2) ?
> > 
> > I don't think that alone is sufficient - there's also the matter of
> > ethtool -m which will dump that information as well, and we don't want
> > to offer it to userspace in an unreliable form.
> 
> Any idea/preference how to disable access to these registers?
> 
> > For reference, here is what SFF-8472 which defines the diagnostics, says
> > about this:
> > 
> >   To guarantee coherency of the diagnostic monitoring data, the host is
> >   required to retrieve any multi-byte fields from the diagnostic
> >   monitoring data structure (IE: Rx Power MSB - byte 104 in A2h, Rx Power
> >   LSB - byte 105 in A2h) by the use of a single two-byte read sequence
> >   across the two-wire interface interface.
> > 
> >   The transceiver is required to ensure that any multi-byte fields which
> >   are updated with diagnostic monitoring data (e.g. Rx Power MSB - byte
> >   104 in A2h, Rx Power LSB - byte 105 in A2h) must have this update done
> >   in a fashion which guarantees coherency and consistency of the data. In
> >   other words, the update of a multi-byte field by the transceiver must
> >   not occur such that a partially updated multi-byte field can be
> >   transferred to the host. Also, the transceiver shall not update a
> >   multi-byte field within the structure during the transfer of that
> >   multi-byte field to the host, such that partially updated data would be
> >   transferred to the host.
> > 
> > The first paragraph is extremely definitive in how these fields shall
> > be read atomically - by a _single_ two-byte read sequence. From what
> > you are telling us, these modules do not support that. Therefore, by
> > definition, they do *not* support proper and reliable reporting of
> > diagnostic data, and are non-conformant with the SFP MSAs.
> > 
> > So, they are basically broken, and the diagnostics can't be used to
> > retrieve data that can be said to be useful.
> 
> I agree they are broken. We really should disable access to those 16bit
> registers.
> 
> Anyway here is "datasheet" to some CarlitoxxPro SFP:
> https://www.docdroid.net/hRsJ560/cpgos03-0490v2-datasheet-10-pdf
> 
> And on page 10 is written:
> 
>     The I2C system can support the mode of random address / single
>     byteread which conform to SFF-8431.
> 
> Which seems to be wrong.

Searching around, i found:

http://read.pudn.com/downloads776/doc/3073304/RTL9601B-CG_Datasheet.pdf

It has two i2c busses, a master and a slave. The master bus can do
multi-byte transfers. The slave bus description says nothing in words
about multi-byte transfers, but the diagram shows only single byte
transfers.

So any SFP based around this device is broken.

The silly thing is, it is reading/writing from a shadow SRAM. The CPU
is not directly involved in an I2C transaction. So it could easily
read multiple bytes from the SRAM and return them. But it would still
need a synchronisation method to handle writes from the CPU to the
SRAM, in order to make these word values safe.

      Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-31 12:14               ` Pali Rohár
  2020-12-31 15:09                 ` Andrew Lunn
@ 2020-12-31 15:30                 ` Andrew Lunn
  2020-12-31 17:00                   ` Pali Rohár
  1 sibling, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2020-12-31 15:30 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Thu, Dec 31, 2020 at 01:14:10PM +0100, Pali Rohár wrote:
> On Wednesday 30 December 2020 19:09:58 Russell King - ARM Linux admin wrote:
> > On Wed, Dec 30, 2020 at 06:43:07PM +0100, Pali Rohár wrote:
> > > On Wednesday 30 December 2020 18:13:15 Andrew Lunn wrote:
> > > > Hi Pali
> > > > 
> > > > I have to agree with Russell here. I would rather have no diagnostics
> > > > than untrustable diagnostics.
> > > 
> > > Ok!
> > > 
> > > So should we completely skip hwmon_device_register_with_info() call
> > > if (i2c_block_size < 2) ?
> > 
> > I don't think that alone is sufficient - there's also the matter of
> > ethtool -m which will dump that information as well, and we don't want
> > to offer it to userspace in an unreliable form.
> 
> Any idea/preference how to disable access to these registers?

Page A0, byte 92:

"Diagnostic Monitoring Type" is a 1 byte field with 8 single bit
indicators describing how diagnostic monitoring is implemented in the
particular transceiver.

Note that if bit 6, address 92 is set indicating that digital
diagnostic monitoring has been implemented, received power
monitoring, transmitted power monitoring, bias current monitoring,
supply voltage monitoring and temperature monitoring must all be
implemented. Additionally, alarm and warning thresholds must be
written as specified in this document at locations 00 to 55 on
two-wire serial address 1010001X (A2h) (see Table 8-5).

Unfortunately, we cannot simply set sfp->id.ext.diagmon to false,
because it can also be used to indicate power, software reading of
TX_DISABLE, LOS, etc. These are all single bytes, so could be returned
correctly, assuming they have been implemented according to the spec.

Looking at sfp_module_info(), adding a check for i2c_block_size < 2
when determining what length to return. ethtool should do the right
thing, know that the second page has not been returned to user space.

	Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-31 15:09                 ` Andrew Lunn
@ 2020-12-31 15:40                   ` Pali Rohár
  0 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2020-12-31 15:40 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Thursday 31 December 2020 16:09:25 Andrew Lunn wrote:
> On Thu, Dec 31, 2020 at 01:14:10PM +0100, Pali Rohár wrote:
> > On Wednesday 30 December 2020 19:09:58 Russell King - ARM Linux admin wrote:
> > > On Wed, Dec 30, 2020 at 06:43:07PM +0100, Pali Rohár wrote:
> > > > On Wednesday 30 December 2020 18:13:15 Andrew Lunn wrote:
> > > > > Hi Pali
> > > > > 
> > > > > I have to agree with Russell here. I would rather have no diagnostics
> > > > > than untrustable diagnostics.
> > > > 
> > > > Ok!
> > > > 
> > > > So should we completely skip hwmon_device_register_with_info() call
> > > > if (i2c_block_size < 2) ?
> > > 
> > > I don't think that alone is sufficient - there's also the matter of
> > > ethtool -m which will dump that information as well, and we don't want
> > > to offer it to userspace in an unreliable form.
> > 
> > Any idea/preference how to disable access to these registers?
> > 
> > > For reference, here is what SFF-8472 which defines the diagnostics, says
> > > about this:
> > > 
> > >   To guarantee coherency of the diagnostic monitoring data, the host is
> > >   required to retrieve any multi-byte fields from the diagnostic
> > >   monitoring data structure (IE: Rx Power MSB - byte 104 in A2h, Rx Power
> > >   LSB - byte 105 in A2h) by the use of a single two-byte read sequence
> > >   across the two-wire interface interface.
> > > 
> > >   The transceiver is required to ensure that any multi-byte fields which
> > >   are updated with diagnostic monitoring data (e.g. Rx Power MSB - byte
> > >   104 in A2h, Rx Power LSB - byte 105 in A2h) must have this update done
> > >   in a fashion which guarantees coherency and consistency of the data. In
> > >   other words, the update of a multi-byte field by the transceiver must
> > >   not occur such that a partially updated multi-byte field can be
> > >   transferred to the host. Also, the transceiver shall not update a
> > >   multi-byte field within the structure during the transfer of that
> > >   multi-byte field to the host, such that partially updated data would be
> > >   transferred to the host.
> > > 
> > > The first paragraph is extremely definitive in how these fields shall
> > > be read atomically - by a _single_ two-byte read sequence. From what
> > > you are telling us, these modules do not support that. Therefore, by
> > > definition, they do *not* support proper and reliable reporting of
> > > diagnostic data, and are non-conformant with the SFP MSAs.
> > > 
> > > So, they are basically broken, and the diagnostics can't be used to
> > > retrieve data that can be said to be useful.
> > 
> > I agree they are broken. We really should disable access to those 16bit
> > registers.
> > 
> > Anyway here is "datasheet" to some CarlitoxxPro SFP:
> > https://www.docdroid.net/hRsJ560/cpgos03-0490v2-datasheet-10-pdf
> > 
> > And on page 10 is written:
> > 
> >     The I2C system can support the mode of random address / single
> >     byteread which conform to SFF-8431.
> > 
> > Which seems to be wrong.
> 
> Searching around, i found:
> 
> http://read.pudn.com/downloads776/doc/3073304/RTL9601B-CG_Datasheet.pdf
> 
> It has two i2c busses, a master and a slave. The master bus can do
> multi-byte transfers. The slave bus description says nothing in words
> about multi-byte transfers, but the diagram shows only single byte
> transfers.

Yes. Only i2c slave is used for communication with external entity and
diagrams clearly shows that only single byte i2c transfers are
supported.

> So any SFP based around this device is broken.

Exactly. That is why I send this patch in a way that try to detect these
RTL chips and not particular vendors who create product on top of this
chip.

All SFP modules with this RTL9601B chip are broken and cannot be fixed.

Re-branders and OEM vendors like CarlitoxxPro or UBNT should stop saying
that they are compliant to SFP/SFF standards, because based on above
descriptions it is not truth.

> The silly thing is, it is reading/writing from a shadow SRAM. The CPU
> is not directly involved in an I2C transaction. So it could easily
> read multiple bytes from the SRAM and return them. But it would still
> need a synchronisation method to handle writes from the CPU to the
> SRAM, in order to make these word values safe.

But there is a still issue how to read these values from SRAM outside of
SFP module via i2c. And with current one-byte transfers of that i2c
slave on RTL9601B it is impossible via current SFP diagnostic API
specification.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-31 15:30                 ` Andrew Lunn
@ 2020-12-31 17:00                   ` Pali Rohár
  2020-12-31 17:13                     ` Andrew Lunn
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2020-12-31 17:00 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Thursday 31 December 2020 16:30:33 Andrew Lunn wrote:
> On Thu, Dec 31, 2020 at 01:14:10PM +0100, Pali Rohár wrote:
> > On Wednesday 30 December 2020 19:09:58 Russell King - ARM Linux admin wrote:
> > > On Wed, Dec 30, 2020 at 06:43:07PM +0100, Pali Rohár wrote:
> > > > On Wednesday 30 December 2020 18:13:15 Andrew Lunn wrote:
> > > > > Hi Pali
> > > > > 
> > > > > I have to agree with Russell here. I would rather have no diagnostics
> > > > > than untrustable diagnostics.
> > > > 
> > > > Ok!
> > > > 
> > > > So should we completely skip hwmon_device_register_with_info() call
> > > > if (i2c_block_size < 2) ?
> > > 
> > > I don't think that alone is sufficient - there's also the matter of
> > > ethtool -m which will dump that information as well, and we don't want
> > > to offer it to userspace in an unreliable form.
> > 
> > Any idea/preference how to disable access to these registers?
> 
> Page A0, byte 92:
> 
> "Diagnostic Monitoring Type" is a 1 byte field with 8 single bit
> indicators describing how diagnostic monitoring is implemented in the
> particular transceiver.
> 
> Note that if bit 6, address 92 is set indicating that digital
> diagnostic monitoring has been implemented, received power
> monitoring, transmitted power monitoring, bias current monitoring,
> supply voltage monitoring and temperature monitoring must all be
> implemented. Additionally, alarm and warning thresholds must be
> written as specified in this document at locations 00 to 55 on
> two-wire serial address 1010001X (A2h) (see Table 8-5).
> 
> Unfortunately, we cannot simply set sfp->id.ext.diagmon to false,
> because it can also be used to indicate power, software reading of
> TX_DISABLE, LOS, etc. These are all single bytes, so could be returned
> correctly, assuming they have been implemented according to the spec.
> 
> Looking at sfp_module_info(), adding a check for i2c_block_size < 2
> when determining what length to return. ethtool should do the right
> thing, know that the second page has not been returned to user space.

But if we limit length of eeprom then userspace would not be able to
access those TX_DISABLE, LOS and other bits from byte 110 at address A2.

Problematic two-byte values are in byte range 96-109 at address A2.
Therefore before byte 110.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-31 17:00                   ` Pali Rohár
@ 2020-12-31 17:13                     ` Andrew Lunn
  2021-01-02  1:49                       ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2020-12-31 17:13 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

> > Looking at sfp_module_info(), adding a check for i2c_block_size < 2
> > when determining what length to return. ethtool should do the right
> > thing, know that the second page has not been returned to user space.
> 
> But if we limit length of eeprom then userspace would not be able to
> access those TX_DISABLE, LOS and other bits from byte 110 at address A2.

Have you tested these bits to see if they actually work? If they don't
work...

	 Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2020-12-31 17:13                     ` Andrew Lunn
@ 2021-01-02  1:49                       ` Pali Rohár
  2021-01-03  2:25                         ` Thomas Schreiber
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-02  1:49 UTC (permalink / raw)
  To: Andrew Lunn, Thomas Schreiber
  Cc: Russell King - ARM Linux admin, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Thursday 31 December 2020 18:13:38 Andrew Lunn wrote:
> > > Looking at sfp_module_info(), adding a check for i2c_block_size < 2
> > > when determining what length to return. ethtool should do the right
> > > thing, know that the second page has not been returned to user space.
> > 
> > But if we limit length of eeprom then userspace would not be able to
> > access those TX_DISABLE, LOS and other bits from byte 110 at address A2.
> 
> Have you tested these bits to see if they actually work? If they don't
> work...

On Ubiquiti module that LOS bit does not work.

I think that on CarlitoxxPro module LOS bit worked. But I cannot test it
right now as I do not have access to testing OLT unit.

Adding Thomas to loop. Can you check if CarlitoxxPro GPON ONT module
supports LOS or other bits at byte offset 110 at address A2?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-02  1:49                       ` Pali Rohár
@ 2021-01-03  2:25                         ` Thomas Schreiber
  2021-01-03  2:41                           ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Thomas Schreiber @ 2021-01-03  2:25 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Russell King - ARM Linux admin, Heiner Kallweit,
	David S. Miller, Jakub Kicinski, Marek Behún, netdev,
	linux-kernel

Hi Pali,
I have a CarlitoxxPro module and I reported an issue about RX_LOS pin
to the manufacturer.
It looks to me that the module asserts "inverted LOS" through EEPROM
but does not implement it. Consequently, the SFP state machine of my
host router stays in check los state and link is not set up for the
host interface.

Below is a dump of the module's EEPROM:

[root@clearfog-gt-8k ~]# ethtool -m eth0
Identifier                                : 0x03 (SFP)
Extended identifier                       : 0x04 (GBIC/SFP defined by
2-wire interface ID)
Connector                                 : 0x01 (SC)
Transceiver codes                         : 0x00 0x00 0x00 0x00 0x00
0x00 0x00 0x00 0x00
Encoding                                  : 0x03 (NRZ)
BR, Nominal                               : 1200MBd
Rate identifier                           : 0x00 (unspecified)
Length (SMF,km)                           : 20km
Length (SMF)                              : 20000m
Length (50um)                             : 0m
Length (62.5um)                           : 0m
Length (Copper)                           : 0m
Length (OM3)                              : 0m
Laser wavelength                          : 1310nm
Vendor name                               : VSOL
Vendor OUI                                : 00:00:00
Vendor PN                                 : V2801F
Vendor rev                                : 1.0
Option values                             : 0x00 0x1c
Option                                    : RX_LOS implemented, inverted
Option                                    : TX_FAULT implemented
Option                                    : TX_DISABLE implemented
BR margin, max                            : 0%
BR margin, min                            : 0%
Vendor SN                                 : CP202003180377
Date code                                 : 200408
Optical diagnostics support               : Yes
Laser bias current                        : 0.000 mA
Laser output power                        : 0.0000 mW / -inf dBm
Receiver signal average optical power     : 0.0000 mW / -inf dBm
Module temperature                        : 31.00 degrees C / 87.80 degrees F
Module voltage                            : 0.0000 V
Alarm/warning flags implemented           : Yes
Laser bias current high alarm             : Off
Laser bias current low alarm              : On
Laser bias current high warning           : Off
Laser bias current low warning            : Off
Laser output power high alarm             : Off
Laser output power low alarm              : On
Laser output power high warning           : Off
Laser output power low warning            : Off
Module temperature high alarm             : Off
Module temperature low alarm              : Off
Module temperature high warning           : Off
Module temperature low warning            : Off
Module voltage high alarm                 : Off
Module voltage low alarm                  : Off
Module voltage high warning               : Off
Module voltage low warning                : Off
Laser rx power high alarm                 : Off
Laser rx power low alarm                  : Off
Laser rx power high warning               : Off
Laser rx power low warning                : Off
Laser bias current high alarm threshold   : 74.752 mA
Laser bias current low alarm threshold    : 0.000 mA
Laser bias current high warning threshold : 0.000 mA
Laser bias current low warning threshold  : 0.000 mA
Laser output power high alarm threshold   : 0.0000 mW / -inf dBm
Laser output power low alarm threshold    : 0.0000 mW / -inf dBm
Laser output power high warning threshold : 0.0000 mW / -inf dBm
Laser output power low warning threshold  : 0.0000 mW / -inf dBm
Module temperature high alarm threshold   : 90.00 degrees C / 194.00 degrees F
Module temperature low alarm threshold    : 0.00 degrees C / 32.00 degrees F
Module temperature high warning threshold : 0.00 degrees C / 32.00 degrees F
Module temperature low warning threshold  : 0.00 degrees C / 32.00 degrees F
Module voltage high alarm threshold       : 0.0000 V
Module voltage low alarm threshold        : 0.0000 V
Module voltage high warning threshold     : 0.0000 V
Module voltage low warning threshold      : 0.0000 V
Laser rx power high alarm threshold       : 0.1536 mW / -8.14 dBm
Laser rx power low alarm threshold        : 0.0000 mW / -inf dBm
Laser rx power high warning threshold     : 0.0000 mW / -inf dBm
Laser rx power low warning threshold      : 0.0000 mW / -inf dBm


Le sam. 2 janv. 2021 à 02:49, Pali Rohár <pali@kernel.org> a écrit :
>
> On Thursday 31 December 2020 18:13:38 Andrew Lunn wrote:
> > > > Looking at sfp_module_info(), adding a check for i2c_block_size < 2
> > > > when determining what length to return. ethtool should do the right
> > > > thing, know that the second page has not been returned to user space.
> > >
> > > But if we limit length of eeprom then userspace would not be able to
> > > access those TX_DISABLE, LOS and other bits from byte 110 at address A2.
> >
> > Have you tested these bits to see if they actually work? If they don't
> > work...
>
> On Ubiquiti module that LOS bit does not work.
>
> I think that on CarlitoxxPro module LOS bit worked. But I cannot test it
> right now as I do not have access to testing OLT unit.
>
> Adding Thomas to loop. Can you check if CarlitoxxPro GPON ONT module
> supports LOS or other bits at byte offset 110 at address A2?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-03  2:25                         ` Thomas Schreiber
@ 2021-01-03  2:41                           ` Pali Rohár
  2021-01-06 14:55                             ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-03  2:41 UTC (permalink / raw)
  To: Thomas Schreiber
  Cc: Andrew Lunn, Russell King - ARM Linux admin, Heiner Kallweit,
	David S. Miller, Jakub Kicinski, Marek Behún, netdev,
	linux-kernel

Hello!

On Sunday 03 January 2021 03:25:23 Thomas Schreiber wrote:
> Hi Pali,
> I have a CarlitoxxPro module and I reported an issue about RX_LOS pin
> to the manufacturer.
> It looks to me that the module asserts "inverted LOS" through EEPROM
> but does not implement it.

So, it is broken :-( But I'm not surprised.

Anyway, I think you could be interested in this discussion about my
patch series, but I forgot to CC you on the first patch/cover letter.
You can read whole discussion on public archive available at:

https://lore.kernel.org/netdev/20201230154755.14746-1-pali@kernel.org/t/#u

If you have any comments, let me know so I can fix it for V2.

Those RTL8672/RTL9601C SFP are extremely broken and I do not think that
"rebrander" CarlitoxxPro would do anything with it.

> Consequently, the SFP state machine of my
> host router stays in check los state and link is not set up for the
> host interface.

So link does not work at all?

> Below is a dump of the module's EEPROM:
> 
> [root@clearfog-gt-8k ~]# ethtool -m eth0
> Identifier                                : 0x03 (SFP)
> Extended identifier                       : 0x04 (GBIC/SFP defined by
> 2-wire interface ID)
> Connector                                 : 0x01 (SC)
> Transceiver codes                         : 0x00 0x00 0x00 0x00 0x00
> 0x00 0x00 0x00 0x00
> Encoding                                  : 0x03 (NRZ)
> BR, Nominal                               : 1200MBd
> Rate identifier                           : 0x00 (unspecified)
> Length (SMF,km)                           : 20km
> Length (SMF)                              : 20000m
> Length (50um)                             : 0m
> Length (62.5um)                           : 0m
> Length (Copper)                           : 0m
> Length (OM3)                              : 0m
> Laser wavelength                          : 1310nm
> Vendor name                               : VSOL
> Vendor OUI                                : 00:00:00
> Vendor PN                                 : V2801F
> Vendor rev                                : 1.0
> Option values                             : 0x00 0x1c
> Option                                    : RX_LOS implemented, inverted
> Option                                    : TX_FAULT implemented
> Option                                    : TX_DISABLE implemented
> BR margin, max                            : 0%
> BR margin, min                            : 0%
> Vendor SN                                 : CP202003180377
> Date code                                 : 200408
> Optical diagnostics support               : Yes
> Laser bias current                        : 0.000 mA
> Laser output power                        : 0.0000 mW / -inf dBm
> Receiver signal average optical power     : 0.0000 mW / -inf dBm
> Module temperature                        : 31.00 degrees C / 87.80 degrees F
> Module voltage                            : 0.0000 V
> Alarm/warning flags implemented           : Yes
> Laser bias current high alarm             : Off
> Laser bias current low alarm              : On
> Laser bias current high warning           : Off
> Laser bias current low warning            : Off
> Laser output power high alarm             : Off
> Laser output power low alarm              : On
> Laser output power high warning           : Off
> Laser output power low warning            : Off
> Module temperature high alarm             : Off
> Module temperature low alarm              : Off
> Module temperature high warning           : Off
> Module temperature low warning            : Off
> Module voltage high alarm                 : Off
> Module voltage low alarm                  : Off
> Module voltage high warning               : Off
> Module voltage low warning                : Off
> Laser rx power high alarm                 : Off
> Laser rx power low alarm                  : Off
> Laser rx power high warning               : Off
> Laser rx power low warning                : Off
> Laser bias current high alarm threshold   : 74.752 mA
> Laser bias current low alarm threshold    : 0.000 mA
> Laser bias current high warning threshold : 0.000 mA
> Laser bias current low warning threshold  : 0.000 mA
> Laser output power high alarm threshold   : 0.0000 mW / -inf dBm
> Laser output power low alarm threshold    : 0.0000 mW / -inf dBm
> Laser output power high warning threshold : 0.0000 mW / -inf dBm
> Laser output power low warning threshold  : 0.0000 mW / -inf dBm
> Module temperature high alarm threshold   : 90.00 degrees C / 194.00 degrees F
> Module temperature low alarm threshold    : 0.00 degrees C / 32.00 degrees F
> Module temperature high warning threshold : 0.00 degrees C / 32.00 degrees F
> Module temperature low warning threshold  : 0.00 degrees C / 32.00 degrees F
> Module voltage high alarm threshold       : 0.0000 V
> Module voltage low alarm threshold        : 0.0000 V
> Module voltage high warning threshold     : 0.0000 V
> Module voltage low warning threshold      : 0.0000 V
> Laser rx power high alarm threshold       : 0.1536 mW / -8.14 dBm
> Laser rx power low alarm threshold        : 0.0000 mW / -inf dBm
> Laser rx power high warning threshold     : 0.0000 mW / -inf dBm
> Laser rx power low warning threshold      : 0.0000 mW / -inf dBm
> 
> 
> Le sam. 2 janv. 2021 à 02:49, Pali Rohár <pali@kernel.org> a écrit :
> >
> > On Thursday 31 December 2020 18:13:38 Andrew Lunn wrote:
> > > > > Looking at sfp_module_info(), adding a check for i2c_block_size < 2
> > > > > when determining what length to return. ethtool should do the right
> > > > > thing, know that the second page has not been returned to user space.
> > > >
> > > > But if we limit length of eeprom then userspace would not be able to
> > > > access those TX_DISABLE, LOS and other bits from byte 110 at address A2.
> > >
> > > Have you tested these bits to see if they actually work? If they don't
> > > work...
> >
> > On Ubiquiti module that LOS bit does not work.
> >
> > I think that on CarlitoxxPro module LOS bit worked. But I cannot test it
> > right now as I do not have access to testing OLT unit.
> >
> > Adding Thomas to loop. Can you check if CarlitoxxPro GPON ONT module
> > supports LOS or other bits at byte offset 110 at address A2?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-03  2:41                           ` Pali Rohár
@ 2021-01-06 14:55                             ` Pali Rohár
  2021-01-06 15:21                               ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-06 14:55 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Thomas Schreiber, Russell King - ARM Linux admin,
	Heiner Kallweit, David S. Miller, Jakub Kicinski,
	Marek Behún, netdev, linux-kernel

On Sunday 03 January 2021 03:41:32 Pali Rohár wrote:
> Hello!
> 
> On Sunday 03 January 2021 03:25:23 Thomas Schreiber wrote:
> > Hi Pali,
> > I have a CarlitoxxPro module and I reported an issue about RX_LOS pin
> > to the manufacturer.
> > It looks to me that the module asserts "inverted LOS" through EEPROM
> > but does not implement it.
> 
> So, it is broken :-( But I'm not surprised.
> 
> Anyway, I think you could be interested in this discussion about my
> patch series, but I forgot to CC you on the first patch/cover letter.
> You can read whole discussion on public archive available at:
> 
> https://lore.kernel.org/netdev/20201230154755.14746-1-pali@kernel.org/t/#u
> 
> If you have any comments, let me know so I can fix it for V2.
> 
> Those RTL8672/RTL9601C SFP are extremely broken and I do not think that
> "rebrander" CarlitoxxPro would do anything with it.
> 
> > Consequently, the SFP state machine of my
> > host router stays in check los state and link is not set up for the
> > host interface.
> 
> So link does not work at all?
> 
> > Below is a dump of the module's EEPROM:
> > 
> > [root@clearfog-gt-8k ~]# ethtool -m eth0
> > Identifier                                : 0x03 (SFP)
> > Extended identifier                       : 0x04 (GBIC/SFP defined by
> > 2-wire interface ID)
> > Connector                                 : 0x01 (SC)
> > Transceiver codes                         : 0x00 0x00 0x00 0x00 0x00
> > 0x00 0x00 0x00 0x00
> > Encoding                                  : 0x03 (NRZ)
> > BR, Nominal                               : 1200MBd
> > Rate identifier                           : 0x00 (unspecified)
> > Length (SMF,km)                           : 20km
> > Length (SMF)                              : 20000m
> > Length (50um)                             : 0m
> > Length (62.5um)                           : 0m
> > Length (Copper)                           : 0m
> > Length (OM3)                              : 0m
> > Laser wavelength                          : 1310nm
> > Vendor name                               : VSOL
> > Vendor OUI                                : 00:00:00
> > Vendor PN                                 : V2801F
> > Vendor rev                                : 1.0
> > Option values                             : 0x00 0x1c
> > Option                                    : RX_LOS implemented, inverted
> > Option                                    : TX_FAULT implemented
> > Option                                    : TX_DISABLE implemented
> > BR margin, max                            : 0%
> > BR margin, min                            : 0%
> > Vendor SN                                 : CP202003180377
> > Date code                                 : 200408
> > Optical diagnostics support               : Yes
> > Laser bias current                        : 0.000 mA
> > Laser output power                        : 0.0000 mW / -inf dBm
> > Receiver signal average optical power     : 0.0000 mW / -inf dBm
> > Module temperature                        : 31.00 degrees C / 87.80 degrees F
> > Module voltage                            : 0.0000 V
> > Alarm/warning flags implemented           : Yes
> > Laser bias current high alarm             : Off
> > Laser bias current low alarm              : On
> > Laser bias current high warning           : Off
> > Laser bias current low warning            : Off
> > Laser output power high alarm             : Off
> > Laser output power low alarm              : On
> > Laser output power high warning           : Off
> > Laser output power low warning            : Off
> > Module temperature high alarm             : Off
> > Module temperature low alarm              : Off
> > Module temperature high warning           : Off
> > Module temperature low warning            : Off
> > Module voltage high alarm                 : Off
> > Module voltage low alarm                  : Off
> > Module voltage high warning               : Off
> > Module voltage low warning                : Off
> > Laser rx power high alarm                 : Off
> > Laser rx power low alarm                  : Off
> > Laser rx power high warning               : Off
> > Laser rx power low warning                : Off
> > Laser bias current high alarm threshold   : 74.752 mA
> > Laser bias current low alarm threshold    : 0.000 mA
> > Laser bias current high warning threshold : 0.000 mA
> > Laser bias current low warning threshold  : 0.000 mA
> > Laser output power high alarm threshold   : 0.0000 mW / -inf dBm
> > Laser output power low alarm threshold    : 0.0000 mW / -inf dBm
> > Laser output power high warning threshold : 0.0000 mW / -inf dBm
> > Laser output power low warning threshold  : 0.0000 mW / -inf dBm
> > Module temperature high alarm threshold   : 90.00 degrees C / 194.00 degrees F
> > Module temperature low alarm threshold    : 0.00 degrees C / 32.00 degrees F
> > Module temperature high warning threshold : 0.00 degrees C / 32.00 degrees F
> > Module temperature low warning threshold  : 0.00 degrees C / 32.00 degrees F
> > Module voltage high alarm threshold       : 0.0000 V
> > Module voltage low alarm threshold        : 0.0000 V
> > Module voltage high warning threshold     : 0.0000 V
> > Module voltage low warning threshold      : 0.0000 V
> > Laser rx power high alarm threshold       : 0.1536 mW / -8.14 dBm
> > Laser rx power low alarm threshold        : 0.0000 mW / -inf dBm
> > Laser rx power high warning threshold     : 0.0000 mW / -inf dBm
> > Laser rx power low warning threshold      : 0.0000 mW / -inf dBm
> > 
> > 
> > Le sam. 2 janv. 2021 à 02:49, Pali Rohár <pali@kernel.org> a écrit :
> > >
> > > On Thursday 31 December 2020 18:13:38 Andrew Lunn wrote:
> > > > > > Looking at sfp_module_info(), adding a check for i2c_block_size < 2
> > > > > > when determining what length to return. ethtool should do the right
> > > > > > thing, know that the second page has not been returned to user space.
> > > > >
> > > > > But if we limit length of eeprom then userspace would not be able to
> > > > > access those TX_DISABLE, LOS and other bits from byte 110 at address A2.
> > > >
> > > > Have you tested these bits to see if they actually work? If they don't
> > > > work...
> > >
> > > On Ubiquiti module that LOS bit does not work.
> > >
> > > I think that on CarlitoxxPro module LOS bit worked. But I cannot test it
> > > right now as I do not have access to testing OLT unit.

On my tested CarlitoxxPro module is:

        Option values                             : 0x00 0x1c
        Option                                    : RX_LOS implemented, inverted
        Option                                    : TX_FAULT implemented
        Option                                    : TX_DISABLE implemented

When cable is disconnected then in EEPROM at position 0x16e is value
0x82. If I call 'ip link set eth1 up' then value changes to 0x02, module
itself has a link and I can connect to its internal telnet/webserver to
configure it.

When cable is connected but connection is not established by OLT then
this value is 0x80. If I call 'ip link set eth1 up' then value changes
to 0x00 and kernel does not see a link (no carrier).

So it seems that RX_LOS (bit 1 of 0x16e EEPROM) and also TX_DISABLE (bit
7 of 0x16e EEPROM) is implemented and working properly.

And therefore we should allow access to these bits.


I also tested UBNT module and result is:

        Option values                             : 0x00 0x06
        Option                                    : RX_LOS implemented
        Option                                    : RX_LOS implemented, inverted

Which means that those bits are not implemented.

Anyway I check position 0x16e and value on its value is randomly either
0x79 or 0xff independently of the state of the GPON module.

So it is really not implemented on UBNT.

> > > Adding Thomas to loop. Can you check if CarlitoxxPro GPON ONT module
> > > supports LOS or other bits at byte offset 110 at address A2?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-06 14:55                             ` Pali Rohár
@ 2021-01-06 15:21                               ` Russell King - ARM Linux admin
  2021-01-06 15:23                                 ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-06 15:21 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Thomas Schreiber, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wed, Jan 06, 2021 at 03:55:32PM +0100, Pali Rohár wrote:
> On my tested CarlitoxxPro module is:
> 
>         Option values                             : 0x00 0x1c
>         Option                                    : RX_LOS implemented, inverted
>         Option                                    : TX_FAULT implemented
>         Option                                    : TX_DISABLE implemented
> 
> When cable is disconnected then in EEPROM at position 0x16e is value
> 0x82. If I call 'ip link set eth1 up' then value changes to 0x02, module
> itself has a link and I can connect to its internal telnet/webserver to
> configure it.

Bit 7 reflects the TX_DISABLE pin state. Bit 1 reflects the RX_LOS pin
state. It isn't specified whether the inverted/non-inverted state is
reflected in bit 1 or not - the definition just says that bit 1 is
"Digital state of the RX_LOS Output Pin."

> I also tested UBNT module and result is:
> 
>         Option values                             : 0x00 0x06
>         Option                                    : RX_LOS implemented
>         Option                                    : RX_LOS implemented, inverted
> 
> Which means that those bits are not implemented.
> 
> Anyway I check position 0x16e and value on its value is randomly either
> 0x79 or 0xff independently of the state of the GPON module.
> 
> So it is really not implemented on UBNT.

There are enhanced options at offset 93 which tell you which of the
offset 110 signals are implemented.

We already have support for these, but only when the corresponding
GPIOs on the host side are not implemented.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-06 15:21                               ` Russell King - ARM Linux admin
@ 2021-01-06 15:23                                 ` Russell King - ARM Linux admin
  2021-01-06 15:27                                   ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-06 15:23 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Thomas Schreiber, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wed, Jan 06, 2021 at 03:21:38PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Jan 06, 2021 at 03:55:32PM +0100, Pali Rohár wrote:
> > On my tested CarlitoxxPro module is:
> > 
> >         Option values                             : 0x00 0x1c
> >         Option                                    : RX_LOS implemented, inverted
> >         Option                                    : TX_FAULT implemented
> >         Option                                    : TX_DISABLE implemented
> > 
> > When cable is disconnected then in EEPROM at position 0x16e is value
> > 0x82. If I call 'ip link set eth1 up' then value changes to 0x02, module
> > itself has a link and I can connect to its internal telnet/webserver to
> > configure it.
> 
> Bit 7 reflects the TX_DISABLE pin state. Bit 1 reflects the RX_LOS pin
> state. It isn't specified whether the inverted/non-inverted state is
> reflected in bit 1 or not - the definition just says that bit 1 is
> "Digital state of the RX_LOS Output Pin."
> 
> > I also tested UBNT module and result is:
> > 
> >         Option values                             : 0x00 0x06
> >         Option                                    : RX_LOS implemented
> >         Option                                    : RX_LOS implemented, inverted
> > 
> > Which means that those bits are not implemented.
> > 
> > Anyway I check position 0x16e and value on its value is randomly either
> > 0x79 or 0xff independently of the state of the GPON module.
> > 
> > So it is really not implemented on UBNT.
> 
> There are enhanced options at offset 93 which tell you which of the
> offset 110 signals are implemented.

That's the ID EEPROM (A0) offset 93 for the Diagnostic address (A2)
offset 110.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-06 15:23                                 ` Russell King - ARM Linux admin
@ 2021-01-06 15:27                                   ` Russell King - ARM Linux admin
  2021-01-06 15:35                                     ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-06 15:27 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, Thomas Schreiber, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wed, Jan 06, 2021 at 03:23:38PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Jan 06, 2021 at 03:21:38PM +0000, Russell King - ARM Linux admin wrote:
> > On Wed, Jan 06, 2021 at 03:55:32PM +0100, Pali Rohár wrote:
> > > On my tested CarlitoxxPro module is:
> > > 
> > >         Option values                             : 0x00 0x1c
> > >         Option                                    : RX_LOS implemented, inverted
> > >         Option                                    : TX_FAULT implemented
> > >         Option                                    : TX_DISABLE implemented
> > > 
> > > When cable is disconnected then in EEPROM at position 0x16e is value
> > > 0x82. If I call 'ip link set eth1 up' then value changes to 0x02, module
> > > itself has a link and I can connect to its internal telnet/webserver to
> > > configure it.
> > 
> > Bit 7 reflects the TX_DISABLE pin state. Bit 1 reflects the RX_LOS pin
> > state. It isn't specified whether the inverted/non-inverted state is
> > reflected in bit 1 or not - the definition just says that bit 1 is
> > "Digital state of the RX_LOS Output Pin."
> > 
> > > I also tested UBNT module and result is:
> > > 
> > >         Option values                             : 0x00 0x06
> > >         Option                                    : RX_LOS implemented
> > >         Option                                    : RX_LOS implemented, inverted
> > > 
> > > Which means that those bits are not implemented.
> > > 
> > > Anyway I check position 0x16e and value on its value is randomly either
> > > 0x79 or 0xff independently of the state of the GPON module.
> > > 
> > > So it is really not implemented on UBNT.
> > 
> > There are enhanced options at offset 93 which tell you which of the
> > offset 110 signals are implemented.
> 
> That's the ID EEPROM (A0) offset 93 for the Diagnostic address (A2)
> offset 110.

Looking at the EEPROM dumps you've sent me... the VSOL V2801F has
0xe0 at offset 93, meaning TX_DISABLE and TX_FAULT soft signals
(which is basically offset 110) are implemented, RX_LOS is not. No
soft signals are implemented on the Ubiquiti module.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-06 15:27                                   ` Russell King - ARM Linux admin
@ 2021-01-06 15:35                                     ` Pali Rohár
  0 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-06 15:35 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Thomas Schreiber, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, Marek Behún, netdev, linux-kernel

On Wednesday 06 January 2021 15:27:07 Russell King - ARM Linux admin wrote:
> On Wed, Jan 06, 2021 at 03:23:38PM +0000, Russell King - ARM Linux admin wrote:
> > On Wed, Jan 06, 2021 at 03:21:38PM +0000, Russell King - ARM Linux admin wrote:
> > > On Wed, Jan 06, 2021 at 03:55:32PM +0100, Pali Rohár wrote:
> > > > On my tested CarlitoxxPro module is:
> > > > 
> > > >         Option values                             : 0x00 0x1c
> > > >         Option                                    : RX_LOS implemented, inverted
> > > >         Option                                    : TX_FAULT implemented
> > > >         Option                                    : TX_DISABLE implemented
> > > > 
> > > > When cable is disconnected then in EEPROM at position 0x16e is value
> > > > 0x82. If I call 'ip link set eth1 up' then value changes to 0x02, module
> > > > itself has a link and I can connect to its internal telnet/webserver to
> > > > configure it.
> > > 
> > > Bit 7 reflects the TX_DISABLE pin state. Bit 1 reflects the RX_LOS pin
> > > state. It isn't specified whether the inverted/non-inverted state is
> > > reflected in bit 1 or not - the definition just says that bit 1 is
> > > "Digital state of the RX_LOS Output Pin."
> > > 
> > > > I also tested UBNT module and result is:
> > > > 
> > > >         Option values                             : 0x00 0x06
> > > >         Option                                    : RX_LOS implemented
> > > >         Option                                    : RX_LOS implemented, inverted
> > > > 
> > > > Which means that those bits are not implemented.
> > > > 
> > > > Anyway I check position 0x16e and value on its value is randomly either
> > > > 0x79 or 0xff independently of the state of the GPON module.
> > > > 
> > > > So it is really not implemented on UBNT.
> > > 
> > > There are enhanced options at offset 93 which tell you which of the
> > > offset 110 signals are implemented.
> > 
> > That's the ID EEPROM (A0) offset 93 for the Diagnostic address (A2)
> > offset 110.
> 
> Looking at the EEPROM dumps you've sent me... the VSOL V2801F has
> 0xe0 at offset 93, meaning TX_DISABLE and TX_FAULT soft signals
> (which is basically offset 110) are implemented, RX_LOS is not. No
> soft signals are implemented on the Ubiquiti module.

UBNT has at EEPROM offset 0x5E value 0x80.

CarlitoxxPRO has at this offset value 0xE0.

So both SFPs claims that support alarm/warning flags and CarlitoxxPRO
claims that support TX_DISABLE and TX_FAULT.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
                   ` (3 preceding siblings ...)
  2020-12-30 15:47 ` [PATCH 4/4] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
@ 2021-01-06 15:37 ` Pali Rohár
  2021-01-06 15:37   ` [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
                     ` (2 more replies)
  2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
  2021-01-25 15:02 ` [PATCH v4 " Pali Rohár
  6 siblings, 3 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-06 15:37 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

This is second patch series which adds workaround for broken GPON SFP
modules based on Realtek RTL8672/RTL9601C chips with broken EEPROM
emulator.

PATCH 2/4 was dropped and replaced by specific UBNT quirk in modified
PATCH v2 3/3.

hwmon interface was for these SFP modules completely disabled as EEPROM
is totally broken and does not support reading 16bit values automically.

Pali Rohár (2):
  net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant

Russell King (1):
  net: sfp: assume that LOS is not implemented if both LOS normal and
    inverted is set

 drivers/net/phy/sfp-bus.c |  15 ++++
 drivers/net/phy/sfp.c     | 145 +++++++++++++++++++++++++-------------
 2 files changed, 110 insertions(+), 50 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 78+ messages in thread

* [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-06 15:37 ` [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
@ 2021-01-06 15:37   ` Pali Rohár
  2021-01-07  2:02     ` Andrew Lunn
  2021-01-07 17:19     ` Andrew Lunn
  2021-01-06 15:37   ` [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
  2021-01-06 15:37   ` [PATCH v2 3/3] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
  2 siblings, 2 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-06 15:37 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

Workaround for GPON SFP modules based on VSOL V2801F brand was added in
commit 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490
v2.0 workaround"). But it works only for ids explicitly added to the list.
As there are more rebraded VSOL V2801F modules and OEM vendors are putting
into vendor name random strings we cannot build workaround based on ids.

Moreover issue which that commit tried to workaround is generic not only to
VSOL based modules, but rather to all GPON modules based on Realtek RTL8672
and RTL9601C chips.

They can be found for example in following GPON modules:
* V-SOL V2801F
* C-Data FD511GX-RM0
* OPTON GP801R
* BAUDCOM BD-1234-SFM
* CPGOS03-0490 v2.0
* Ubiquiti U-Fiber Instant
* EXOT EGS1

Those Realtek chips have broken EEPROM emulator which for N-byte read
operation returns just one byte of EEPROM data followed by N-1 zeros.

So introduce a new function sfp_id_needs_byte_io() which detects SFP
modules with these Realtek chips which have broken EEPROM emulator based on
N-1 zeros and switch to 1 byte EEPROM reading operation which workaround
this issue.

Function sfp_i2c_read() now always use single byte reading when it is
required and when function sfp_hwmon_probe() detects single byte access
then it disable registration of hwmon device. As in this case we cannot
reliable and atomically read 2 bytes as it is required for retrieving some
values from diagnostic area.

These Realtek chips are broken in a way that violate SFP standards for
diagnostic interface. Kernel in this case cannot do anything, only skipping
registration of hwmon interface.

This patch fixes reading of EEPROM content from SFP modules based on
Realtek RTL8672 and RTL9601C chips. Diagnostic interface of EEPROM stay
broken and cannot be fixed.

Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* Add explanation why also for second address is used one byte read op
* Skip hwmon registration when eeprom does not support atomic 16bit read op
---
 drivers/net/phy/sfp.c | 92 +++++++++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 34 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 91d74c1a920a..c0a891cdcd73 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -336,19 +336,11 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
 			size_t len)
 {
 	struct i2c_msg msgs[2];
-	size_t block_size;
+	u8 bus_addr = a2 ? 0x51 : 0x50;
+	size_t block_size = sfp->i2c_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;
@@ -1282,6 +1274,20 @@ static void sfp_hwmon_probe(struct work_struct *work)
 	struct sfp *sfp = container_of(work, struct sfp, hwmon_probe.work);
 	int err, i;
 
+	/* hwmon interface needs to access 16bit registers in atomic way to
+	 * guarantee coherency of the diagnostic monitoring data. If it is not
+	 * possible to guarantee coherency because EEPROM is broken in such way
+	 * that does not support atomic 16bit read operation then we have to
+	 * skip registration of hwmon device.
+	 */
+	if (sfp->i2c_block_size < 2) {
+		dev_info(sfp->dev, "skipping hwmon device registration "
+				   "due to broken EEPROM\n");
+		dev_info(sfp->dev, "diagnostic EEPROM area cannot be read "
+				   "atomically to guarantee data coherency\n");
+		return;
+	}
+
 	err = sfp_read(sfp, true, 0, &sfp->diag, sizeof(sfp->diag));
 	if (err < 0) {
 		if (sfp->hwmon_tries--) {
@@ -1642,26 +1648,30 @@ 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.
+/* GPON modules based on Realtek RTL8672 and RTL9601C chips (e.g. V-SOL
+ * V2801F, CarlitoxxPro CPGOS03-0490, Ubiquiti U-Fiber Instant, ...) do
+ * not support multibyte reads from the EEPROM. Each multi-byte read
+ * operation returns just one byte of EEPROM followed by zeros. There is
+ * no way to identify which modules are using Realtek RTL8672 and RTL9601C
+ * chips. Moreover every OEM of V-SOL V2801F module puts its own vendor
+ * name and vendor id into EEPROM, so there is even no way to detect if
+ * module is V-SOL V2801F. Therefore check for those zeros in the read
+ * data and then based on check switch to reading EEPROM to one byte
+ * at a time.
  */
-static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
+static bool sfp_id_needs_byte_io(struct sfp *sfp, void *buf, size_t len)
 {
-	if (!memcmp(base->vendor_name, "VSOL            ", 16))
-		return 1;
-	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
-	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
-		return 1;
+	size_t i, block_size = sfp->i2c_block_size;
 
-	/* Some modules can't cope with long reads */
-	return 16;
-}
+	/* Already using byte IO */
+	if (block_size == 1)
+		return false;
 
-static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base *base)
-{
-	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
+	for (i = 1; i < len; i += block_size) {
+		if (memchr_inv(buf + i, '\0', block_size - 1))
+			return false;
+	}
+	return true;
 }
 
 static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)
@@ -1705,11 +1715,7 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	u8 check;
 	int ret;
 
-	/* 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;
+	sfp->i2c_block_size = 16;
 
 	ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base));
 	if (ret < 0) {
@@ -1723,6 +1729,27 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 		return -EAGAIN;
 	}
 
+	if (sfp_id_needs_byte_io(sfp, &id.base, sizeof(id.base))) {
+		dev_info(sfp->dev,
+			 "Detected broken RTL8672/RTL9601C emulated EEPROM\n");
+		dev_info(sfp->dev,
+			 "Switching to reading EEPROM to one byte at a time\n");
+		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.base)) {
+			dev_err(sfp->dev, "EEPROM short read: %d\n", ret);
+			return -EAGAIN;
+		}
+	}
+
 	/* Cotsworks do not seem to update the checksums when they
 	 * do the final programming with the final module part number,
 	 * serial number and date code.
@@ -1757,9 +1784,6 @@ 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)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2021-01-06 15:37 ` [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
  2021-01-06 15:37   ` [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
@ 2021-01-06 15:37   ` Pali Rohár
  2021-01-07 16:54     ` Andrew Lunn
  2021-01-06 15:37   ` [PATCH v2 3/3] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
  2 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-06 15:37 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

From: Russell King <rmk+kernel@armlinux.org.uk>

Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.

Such combination of bits is meaningless so assume that LOS signal is not
implemented.

This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* Fix author/signed-off-by lines
---
 drivers/net/phy/sfp.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index c0a891cdcd73..15fb8f7dfe5b 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1488,15 +1488,19 @@ static void sfp_sm_link_down(struct sfp *sfp)
 
 static void sfp_sm_link_check_los(struct sfp *sfp)
 {
-	unsigned int los = sfp->state & SFP_F_LOS;
+	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
+	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
+	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
+	bool los = false;
 
 	/* If neither SFP_OPTIONS_LOS_INVERTED nor SFP_OPTIONS_LOS_NORMAL
-	 * are set, we assume that no LOS signal is available.
+	 * are set, we assume that no LOS signal is available. If both are
+	 * set, we assume LOS is not implemented (and is meaningless.)
 	 */
-	if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED))
-		los ^= SFP_F_LOS;
-	else if (!(sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL)))
-		los = 0;
+	if (los_options == los_inverted)
+		los = !(sfp->state & SFP_F_LOS);
+	else if (los_options == los_normal)
+		los = !!(sfp->state & SFP_F_LOS);
 
 	if (los)
 		sfp_sm_next(sfp, SFP_S_WAIT_LOS, 0);
@@ -1506,18 +1510,22 @@ static void sfp_sm_link_check_los(struct sfp *sfp)
 
 static bool sfp_los_event_active(struct sfp *sfp, unsigned int event)
 {
-	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
-		event == SFP_E_LOS_LOW) ||
-	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
-		event == SFP_E_LOS_HIGH);
+	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
+	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
+	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
+
+	return (los_options == los_inverted && event == SFP_E_LOS_LOW) ||
+	       (los_options == los_normal && event == SFP_E_LOS_HIGH);
 }
 
 static bool sfp_los_event_inactive(struct sfp *sfp, unsigned int event)
 {
-	return (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_INVERTED) &&
-		event == SFP_E_LOS_HIGH) ||
-	       (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_LOS_NORMAL) &&
-		event == SFP_E_LOS_LOW);
+	const __be16 los_inverted = cpu_to_be16(SFP_OPTIONS_LOS_INVERTED);
+	const __be16 los_normal = cpu_to_be16(SFP_OPTIONS_LOS_NORMAL);
+	__be16 los_options = sfp->id.ext.options & (los_inverted | los_normal);
+
+	return (los_options == los_inverted && event == SFP_E_LOS_HIGH) ||
+	       (los_options == los_normal && event == SFP_E_LOS_LOW);
 }
 
 static void sfp_sm_fault(struct sfp *sfp, unsigned int next_state, bool warn)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* [PATCH v2 3/3] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
  2021-01-06 15:37 ` [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
  2021-01-06 15:37   ` [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
  2021-01-06 15:37   ` [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
@ 2021-01-06 15:37   ` Pali Rohár
  2021-01-07 16:51     ` Andrew Lunn
  2 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-06 15:37 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

SFP GPON module Ubiquiti U-Fiber Instant has in its EEPROM stored nonsense
information. It claims that support all transceiver types including 10G
Ethernet which is not truth. So clear all claimed modes and set only one
mode which module supports: 1000baseX_Full.

Also this module have set SFF phys_id in its EEPROM. Kernel SFP subsustem
currently does not allow to use SFP modules detected as SFF. Therefore add
and exception for this module so it can be detected as supported.

This change finally allows to detect and use SFP GPON module Ubiquiti
U-Fiber Instant on Linux system.

EEPROM content of this SFP module is (where XX is serial number):

00: 02 04 0b ff ff ff ff ff ff ff ff 03 0c 00 14 c8    ???........??.??
10: 00 00 00 00 55 42 4e 54 20 20 20 20 20 20 20 20    ....UBNT
20: 20 20 20 20 00 18 e8 29 55 46 2d 49 4e 53 54 41        .??)UF-INSTA
30: 4e 54 20 20 20 20 20 20 34 20 20 20 05 1e 00 36    NT      4   ??.6
40: 00 06 00 00 55 42 4e 54 XX XX XX XX XX XX XX XX    .?..UBNTXXXXXXXX
50: 20 20 20 20 31 34 30 31 32 33 20 20 60 80 02 41        140123  `??A

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* add this module also into sfp_module_supported() function
---
 drivers/net/phy/sfp-bus.c | 15 +++++++++++++++
 drivers/net/phy/sfp.c     | 17 +++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 20b91f5dfc6e..4cf874fb5c5b 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -44,6 +44,17 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
 	phylink_set(modes, 2500baseX_Full);
 }
 
+static void sfp_quirk_ubnt_uf_instant(const struct sfp_eeprom_id *id,
+				      unsigned long *modes)
+{
+	/* Ubiquiti U-Fiber Instant module claims that support all transceiver
+	 * types including 10G Ethernet which is not truth. So clear all claimed
+	 * modes and set only one mode which module supports: 1000baseX_Full.
+	 */
+	phylink_zero(modes);
+	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 +74,10 @@ static const struct sfp_quirk sfp_quirks[] = {
 		.vendor = "HUAWEI",
 		.part = "MA5671A",
 		.modes = sfp_quirk_2500basex,
+	}, {
+		.vendor = "UBNT",
+		.part = "UF-INSTANT",
+		.modes = sfp_quirk_ubnt_uf_instant,
 	},
 };
 
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 15fb8f7dfe5b..c3a0dcc737fd 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -273,8 +273,21 @@ static const struct sff_data sff_data = {
 
 static bool sfp_module_supported(const struct sfp_eeprom_id *id)
 {
-	return id->base.phys_id == SFF8024_ID_SFP &&
-	       id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP;
+	if (id->base.phys_id == SFF8024_ID_SFP &&
+	    id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP)
+		return true;
+
+	/* SFP GPON module Ubiquiti U-Fiber Instant has in its EEPROM stored
+	 * phys id SFF instead of SFP. Therefore mark this module explicitly
+	 * as supported based on vendor name and pn match.
+	 */
+	if (id->base.phys_id == SFF8024_ID_SFF_8472 &&
+	    id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP &&
+	    !memcmp(id->base.vendor_name, "UBNT            ", 16) &&
+	    !memcmp(id->base.vendor_pn, "UF-INSTANT      ", 16))
+		return true;
+
+	return false;
 }
 
 static const struct sff_data sfp_data = {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-06 15:37   ` [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
@ 2021-01-07  2:02     ` Andrew Lunn
  2021-01-07  9:08       ` Pali Rohár
  2021-01-07 17:19     ` Andrew Lunn
  1 sibling, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2021-01-07  2:02 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

> +	/* hwmon interface needs to access 16bit registers in atomic way to
> +	 * guarantee coherency of the diagnostic monitoring data. If it is not
> +	 * possible to guarantee coherency because EEPROM is broken in such way
> +	 * that does not support atomic 16bit read operation then we have to
> +	 * skip registration of hwmon device.
> +	 */
> +	if (sfp->i2c_block_size < 2) {
> +		dev_info(sfp->dev, "skipping hwmon device registration "
> +				   "due to broken EEPROM\n");
> +		dev_info(sfp->dev, "diagnostic EEPROM area cannot be read "
> +				   "atomically to guarantee data coherency\n");
> +		return;
> +	}

This solves hwmon. But we still return the broken data to ethtool -m.
I wonder if we should prevent that?

  Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-07  2:02     ` Andrew Lunn
@ 2021-01-07  9:08       ` Pali Rohár
  0 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-07  9:08 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Russell King - ARM Linux admin, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Thursday 07 January 2021 03:02:36 Andrew Lunn wrote:
> > +	/* hwmon interface needs to access 16bit registers in atomic way to
> > +	 * guarantee coherency of the diagnostic monitoring data. If it is not
> > +	 * possible to guarantee coherency because EEPROM is broken in such way
> > +	 * that does not support atomic 16bit read operation then we have to
> > +	 * skip registration of hwmon device.
> > +	 */
> > +	if (sfp->i2c_block_size < 2) {
> > +		dev_info(sfp->dev, "skipping hwmon device registration "
> > +				   "due to broken EEPROM\n");
> > +		dev_info(sfp->dev, "diagnostic EEPROM area cannot be read "
> > +				   "atomically to guarantee data coherency\n");
> > +		return;
> > +	}
> 
> This solves hwmon. But we still return the broken data to ethtool -m.
> I wonder if we should prevent that?

Looks like that it is not too simple for now.

And because we already export these data for these broken chips in
current mainline kernel, I would propose to postpone fix for ethtool and
let it for future patches. This patch series does not change (nor make
it worse) behavior.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 3/3] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
  2021-01-06 15:37   ` [PATCH v2 3/3] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
@ 2021-01-07 16:51     ` Andrew Lunn
  0 siblings, 0 replies; 78+ messages in thread
From: Andrew Lunn @ 2021-01-07 16:51 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Wed, Jan 06, 2021 at 04:37:49PM +0100, Pali Rohár wrote:
> SFP GPON module Ubiquiti U-Fiber Instant has in its EEPROM stored nonsense
> information. It claims that support all transceiver types including 10G
> Ethernet which is not truth. So clear all claimed modes and set only one
> mode which module supports: 1000baseX_Full.
> 
> Also this module have set SFF phys_id in its EEPROM. Kernel SFP subsustem
> currently does not allow to use SFP modules detected as SFF. Therefore add
> and exception for this module so it can be detected as supported.
> 
> This change finally allows to detect and use SFP GPON module Ubiquiti
> U-Fiber Instant on Linux system.
> 
> EEPROM content of this SFP module is (where XX is serial number):
> 
> 00: 02 04 0b ff ff ff ff ff ff ff ff 03 0c 00 14 c8    ???........??.??
> 10: 00 00 00 00 55 42 4e 54 20 20 20 20 20 20 20 20    ....UBNT
> 20: 20 20 20 20 00 18 e8 29 55 46 2d 49 4e 53 54 41        .??)UF-INSTA
> 30: 4e 54 20 20 20 20 20 20 34 20 20 20 05 1e 00 36    NT      4   ??.6
> 40: 00 06 00 00 55 42 4e 54 XX XX XX XX XX XX XX XX    .?..UBNTXXXXXXXX
> 50: 20 20 20 20 31 34 30 31 32 33 20 20 60 80 02 41        140123  `??A
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

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

    Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2021-01-06 15:37   ` [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
@ 2021-01-07 16:54     ` Andrew Lunn
  2021-01-09 15:46       ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2021-01-07 16:54 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote:
> From: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> 
> Such combination of bits is meaningless so assume that LOS signal is not
> implemented.
> 
> This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Pali Rohár <pali@kernel.org>

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

    Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-06 15:37   ` [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
  2021-01-07  2:02     ` Andrew Lunn
@ 2021-01-07 17:19     ` Andrew Lunn
  2021-01-07 17:40       ` Russell King - ARM Linux admin
                         ` (2 more replies)
  1 sibling, 3 replies; 78+ messages in thread
From: Andrew Lunn @ 2021-01-07 17:19 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

> +	if (sfp->i2c_block_size < 2) {
> +		dev_info(sfp->dev, "skipping hwmon device registration "
> +				   "due to broken EEPROM\n");
> +		dev_info(sfp->dev, "diagnostic EEPROM area cannot be read "
> +				   "atomically to guarantee data coherency\n");

Strings like this are the exception to the 80 character rule. People
grep for them, and when they are split, they are harder to find.

> -static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
> +static bool sfp_id_needs_byte_io(struct sfp *sfp, void *buf, size_t len)
>  {
> -	if (!memcmp(base->vendor_name, "VSOL            ", 16))
> -		return 1;
> -	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
> -	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
> -		return 1;
> +	size_t i, block_size = sfp->i2c_block_size;
>  
> -	/* Some modules can't cope with long reads */
> -	return 16;
> -}
> +	/* Already using byte IO */
> +	if (block_size == 1)
> +		return false;

This seems counter intuitive. We don't need byte IO because we are
doing btye IO? Can we return True here?

>  
> -static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base *base)
> -{
> -	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
> +	for (i = 1; i < len; i += block_size) {
> +		if (memchr_inv(buf + i, '\0', block_size - 1))
> +			return false;
> +	}

Is the loop needed?

I also wonder if on the last iteration of the loop you go passed the
end of buf? Don't you need a min(block_size -1, len - i) or
similar?

> -	/* 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;
> +	sfp->i2c_block_size = 16;

Did we loose the comment:

/* Some modules (Nokia 3FE46541AA) lock up if byte 0x51 is read as a
 * single read. Switch back to reading 16 byte blocks ...

That explains why 16 is used. Given how broken stuff is and the number
of workaround we need, we should try to document as much as we cam, so
we don't break stuff when adding more workarounds.

     Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-07 17:19     ` Andrew Lunn
@ 2021-01-07 17:40       ` Russell King - ARM Linux admin
  2021-01-07 19:18         ` Pali Rohár
  2021-01-07 19:17       ` Pali Rohár
  2021-01-07 19:45       ` Russell King - ARM Linux admin
  2 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-07 17:40 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Pali Rohár, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Thu, Jan 07, 2021 at 06:19:23PM +0100, Andrew Lunn wrote:
> Did we loose the comment:
> 
> /* Some modules (Nokia 3FE46541AA) lock up if byte 0x51 is read as a
>  * single read. Switch back to reading 16 byte blocks ...
> 
> That explains why 16 is used. Given how broken stuff is and the number
> of workaround we need, we should try to document as much as we cam, so
> we don't break stuff when adding more workarounds.

It is _not_ why 16 is used at all.

We used to read the whole lot in one go. However, some modules could
not cope with a full read - also some Linux I2C drivers struggled with
it.

So, we reduced it down to 16 bytes. See commit 28e74a7cfd64 ("net: sfp:
read eeprom in maximum 16 byte increments"). That had nothing to do
with the 3FE46541AA, which came along later. It has been discovered
that 3FE46541AA reacts badly to a single byte read to address 0x51 -
it locks the I2C bus. Hence why we can't just go to single byte reads
for every module.

So, the comment needs to be kept to explain why we are unable to go
to single byte reads for all modules.  The choice of 16 remains
relatively arbitary.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-07 17:19     ` Andrew Lunn
  2021-01-07 17:40       ` Russell King - ARM Linux admin
@ 2021-01-07 19:17       ` Pali Rohár
  2021-01-07 19:45       ` Russell King - ARM Linux admin
  2 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-07 19:17 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Russell King - ARM Linux admin, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Thursday 07 January 2021 18:19:23 Andrew Lunn wrote:
> > +	if (sfp->i2c_block_size < 2) {
> > +		dev_info(sfp->dev, "skipping hwmon device registration "
> > +				   "due to broken EEPROM\n");
> > +		dev_info(sfp->dev, "diagnostic EEPROM area cannot be read "
> > +				   "atomically to guarantee data coherency\n");
> 
> Strings like this are the exception to the 80 character rule. People
> grep for them, and when they are split, they are harder to find.

Ok. I will fix it.

> > -static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
> > +static bool sfp_id_needs_byte_io(struct sfp *sfp, void *buf, size_t len)
> >  {
> > -	if (!memcmp(base->vendor_name, "VSOL            ", 16))
> > -		return 1;
> > -	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
> > -	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
> > -		return 1;
> > +	size_t i, block_size = sfp->i2c_block_size;
> >  
> > -	/* Some modules can't cope with long reads */
> > -	return 16;
> > -}
> > +	/* Already using byte IO */
> > +	if (block_size == 1)
> > +		return false;
> 
> This seems counter intuitive. We don't need byte IO because we are
> doing btye IO? Can we return True here?

I do not know this part was written by Russel.

Currently function is used in a way if sfp subsystem should switch to
byte IO. So if we are already using byte IO we are not going to do
switch and therefore false is returning.

At least this is how I understood why 'return false' is there.

> >  
> > -static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base *base)
> > -{
> > -	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
> > +	for (i = 1; i < len; i += block_size) {
> > +		if (memchr_inv(buf + i, '\0', block_size - 1))
> > +			return false;
> > +	}
> 
> Is the loop needed?

Originally I wanted to use just four memcmp() calls but Russel told me
that code should be generic (in case in future initial block size would
be changed, which is a good argument) and come up with this code with
for-loop.

So I think loop is needed.

> I also wonder if on the last iteration of the loop you go passed the
> end of buf? Don't you need a min(block_size -1, len - i) or
> similar?

You are right, if code is generic this needs to be fixed to prevent
reading reading undefined memory. I will replace it by proposed min(...)
call.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-07 17:40       ` Russell King - ARM Linux admin
@ 2021-01-07 19:18         ` Pali Rohár
  0 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-07 19:18 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, Thomas Schreiber,
	Heiner Kallweit, Marek Behún, netdev, linux-kernel

On Thursday 07 January 2021 17:40:06 Russell King - ARM Linux admin wrote:
> On Thu, Jan 07, 2021 at 06:19:23PM +0100, Andrew Lunn wrote:
> > Did we loose the comment:
> > 
> > /* Some modules (Nokia 3FE46541AA) lock up if byte 0x51 is read as a
> >  * single read. Switch back to reading 16 byte blocks ...
> > 
> > That explains why 16 is used. Given how broken stuff is and the number
> > of workaround we need, we should try to document as much as we cam, so
> > we don't break stuff when adding more workarounds.
> 
> It is _not_ why 16 is used at all.
> 
> We used to read the whole lot in one go. However, some modules could
> not cope with a full read - also some Linux I2C drivers struggled with
> it.
> 
> So, we reduced it down to 16 bytes. See commit 28e74a7cfd64 ("net: sfp:
> read eeprom in maximum 16 byte increments"). That had nothing to do
> with the 3FE46541AA, which came along later. It has been discovered
> that 3FE46541AA reacts badly to a single byte read to address 0x51 -
> it locks the I2C bus. Hence why we can't just go to single byte reads
> for every module.
> 
> So, the comment needs to be kept to explain why we are unable to go
> to single byte reads for all modules.  The choice of 16 remains
> relatively arbitary.

Do you have an idea where to put a comment?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-07 17:19     ` Andrew Lunn
  2021-01-07 17:40       ` Russell King - ARM Linux admin
  2021-01-07 19:17       ` Pali Rohár
@ 2021-01-07 19:45       ` Russell King - ARM Linux admin
  2021-01-07 20:21         ` Marek Behún
  2 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-07 19:45 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Pali Rohár, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Thu, Jan 07, 2021 at 06:19:23PM +0100, Andrew Lunn wrote:
> > -static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
> > +static bool sfp_id_needs_byte_io(struct sfp *sfp, void *buf, size_t len)
> >  {
> > -	if (!memcmp(base->vendor_name, "VSOL            ", 16))
> > -		return 1;
> > -	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
> > -	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
> > -		return 1;
> > +	size_t i, block_size = sfp->i2c_block_size;
> >  
> > -	/* Some modules can't cope with long reads */
> > -	return 16;
> > -}
> > +	/* Already using byte IO */
> > +	if (block_size == 1)
> > +		return false;
> 
> This seems counter intuitive. We don't need byte IO because we are
> doing btye IO? Can we return True here?

It is counter-intuitive, but as this is indicating whether we need to
switch to byte IO, if we're already doing byte IO, then we don't need
to switch.

> > -static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base *base)
> > -{
> > -	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
> > +	for (i = 1; i < len; i += block_size) {
> > +		if (memchr_inv(buf + i, '\0', block_size - 1))
> > +			return false;
> > +	}
> 
> Is the loop needed?

I think you're not reading the code very well. It checks for bytes at
offset 1..blocksize-1, blocksize+1..2*blocksize-1, etc are zero. It
does _not_ check that byte 0 or the byte at N*blocksize is zero - these
bytes are skipped. In other words, the first byte of each transfer can
be any value. The other bytes of the _entire_ ID must be zero.

> I also wonder if on the last iteration of the loop you go passed the
> end of buf? Don't you need a min(block_size -1, len - i) or
> similar?

The ID is 64 bytes long, and is fixed. block_size could be a non-power
of two, but that is highly unlikely. block_size will never be larger
than 16 either.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-07 19:45       ` Russell King - ARM Linux admin
@ 2021-01-07 20:21         ` Marek Behún
  2021-01-08  0:49           ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Marek Behún @ 2021-01-07 20:21 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Pali Rohár, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, netdev, linux-kernel

On Thu, 7 Jan 2021 19:45:49 +0000
Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:

> I think you're not reading the code very well. It checks for bytes at
> offset 1..blocksize-1, blocksize+1..2*blocksize-1, etc are zero. It
> does _not_ check that byte 0 or the byte at N*blocksize is zero - these
> bytes are skipped. In other words, the first byte of each transfer can
> be any value. The other bytes of the _entire_ ID must be zero.

Wouldn't it be better, instead of checking if 1..blocksize-1 are zero,
to check whether reading byte by byte returns the same as reading 16
bytes whole?

Marek

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-07 20:21         ` Marek Behún
@ 2021-01-08  0:49           ` Pali Rohár
  0 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-08  0:49 UTC (permalink / raw)
  To: Marek Behún
  Cc: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski, Thomas Schreiber, Heiner Kallweit, netdev,
	linux-kernel

On Thursday 07 January 2021 21:21:16 Marek Behún wrote:
> On Thu, 7 Jan 2021 19:45:49 +0000
> Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> 
> > I think you're not reading the code very well. It checks for bytes at
> > offset 1..blocksize-1, blocksize+1..2*blocksize-1, etc are zero. It
> > does _not_ check that byte 0 or the byte at N*blocksize is zero - these
> > bytes are skipped. In other words, the first byte of each transfer can
> > be any value. The other bytes of the _entire_ ID must be zero.
> 
> Wouldn't it be better, instead of checking if 1..blocksize-1 are zero,
> to check whether reading byte by byte returns the same as reading 16
> bytes whole?

It would means to read EEPROM two times unconditionally for every SFP.
With current solution we read EEPROM two times only for these buggy
RTL-based SFP modules. For all other SFPs EEPROM content is read only
one time. I like current solution because we do not change the way how
are other (non-broken) SFPs detected. It is better to not touch things
which are not broken.

And as we know that these zeros are expected behavior on these broken
RTL-based SFPs I think such test is fine.

Moreover there are Nokia SFPs which do not like one byte read and locks
i2c bus. Yes, it happens only for EEPROM content on second address
(therefore ID part for this test is not affected) but who knows how
broken would be any other SFPs in future.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2021-01-07 16:54     ` Andrew Lunn
@ 2021-01-09 15:46       ` Russell King - ARM Linux admin
  2021-01-09 15:54         ` Andrew Lunn
  2021-01-09 19:14         ` Pali Rohár
  0 siblings, 2 replies; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-09 15:46 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Pali Rohár, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote:
> On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote:
> > From: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > 
> > Such combination of bits is meaningless so assume that LOS signal is not
> > implemented.
> > 
> > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

I'd like to send this patch irrespective of discussion on the other
patches - I already have it committed in my repository with a different
description, but the patch content is the same.

Are you happy if I transfer Andrew's r-b tag, and convert yours to an
acked-by before I send it?

I'd also like to add a patch that allows 2.5G if no other modes are
found, but the bitrate specified by the module allows 2.5G speed - much
like we do for 1G speeds.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2021-01-09 15:46       ` Russell King - ARM Linux admin
@ 2021-01-09 15:54         ` Andrew Lunn
  2021-01-09 16:27           ` Russell King - ARM Linux admin
  2021-01-09 19:14         ` Pali Rohár
  1 sibling, 1 reply; 78+ messages in thread
From: Andrew Lunn @ 2021-01-09 15:54 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Pali Rohár, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Sat, Jan 09, 2021 at 03:46:01PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote:
> > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote:
> > > From: Russell King <rmk+kernel@armlinux.org.uk>
> > > 
> > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > > 
> > > Such combination of bits is meaningless so assume that LOS signal is not
> > > implemented.
> > > 
> > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> I'd like to send this patch irrespective of discussion on the other
> patches - I already have it committed in my repository with a different
> description, but the patch content is the same.
> 
> Are you happy if I transfer Andrew's r-b tag

Hi Russell

If it is the same contest, no problem. I can always NACK it later...

   Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2021-01-09 15:54         ` Andrew Lunn
@ 2021-01-09 16:27           ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-09 16:27 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Pali Rohár, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Sat, Jan 09, 2021 at 04:54:22PM +0100, Andrew Lunn wrote:
> On Sat, Jan 09, 2021 at 03:46:01PM +0000, Russell King - ARM Linux admin wrote:
> > On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote:
> > > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote:
> > > > From: Russell King <rmk+kernel@armlinux.org.uk>
> > > > 
> > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > > > 
> > > > Such combination of bits is meaningless so assume that LOS signal is not
> > > > implemented.
> > > > 
> > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > > > 
> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > 
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > 
> > I'd like to send this patch irrespective of discussion on the other
> > patches - I already have it committed in my repository with a different
> > description, but the patch content is the same.
> > 
> > Are you happy if I transfer Andrew's r-b tag
> 
> Hi Russell
> 
> If it is the same contest, no problem. I can always NACK it later...

The commit message is different:

   net: sfp: cope with SFPs that set both LOS normal and LOS inverted

   The SFP MSA defines two option bits in byte 65 to indicate how the
   Rx_LOS signal on SFP pin 8 behaves:

   bit 2 - Loss of Signal implemented, signal inverted from standard
           definition in SFP MSA (often called "Signal Detect").
   bit 1 - Loss of Signal implemented, signal as defined in SFP MSA
           (often called "Rx_LOS").

   Clearly, setting both bits results in a meaningless situation: it would
   mean that LOS is implemented in both the normal sense (1 = signal loss)
   and inverted sense (0 = signal loss).

   Unfortunately, there are modules out there which set both bits, which
   will be initially interpret as "inverted" sense, and then, if the LOS
   signal changes state, we will toggle between LINK_UP and WAIT_LOS
   states.

   Change our LOS handling to give well defined behaviour: only interpret
   these bits as meaningful if exactly one is set, otherwise treat it as
   if LOS is not implemented.

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

As I say, the actual patch is the same.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2021-01-09 15:46       ` Russell King - ARM Linux admin
  2021-01-09 15:54         ` Andrew Lunn
@ 2021-01-09 19:14         ` Pali Rohár
  2021-01-09 23:19           ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-09 19:14 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, Thomas Schreiber,
	Heiner Kallweit, Marek Behún, netdev, linux-kernel

On Saturday 09 January 2021 15:46:01 Russell King - ARM Linux admin wrote:
> On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote:
> > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote:
> > > From: Russell King <rmk+kernel@armlinux.org.uk>
> > > 
> > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > > 
> > > Such combination of bits is meaningless so assume that LOS signal is not
> > > implemented.
> > > 
> > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> I'd like to send this patch irrespective of discussion on the other
> patches - I already have it committed in my repository with a different
> description, but the patch content is the same.
> 
> Are you happy if I transfer Andrew's r-b tag, and convert yours to an
> acked-by before I send it?
> 
> I'd also like to add a patch that allows 2.5G if no other modes are
> found, but the bitrate specified by the module allows 2.5G speed - much
> like we do for 1G speeds.

Russell, should I send a new version of patch series without this patch?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2021-01-09 19:14         ` Pali Rohár
@ 2021-01-09 23:19           ` Russell King - ARM Linux admin
  2021-01-09 23:50             ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-09 23:19 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, Thomas Schreiber,
	Heiner Kallweit, Marek Behún, netdev, linux-kernel

On Sat, Jan 09, 2021 at 08:14:47PM +0100, Pali Rohár wrote:
> On Saturday 09 January 2021 15:46:01 Russell King - ARM Linux admin wrote:
> > On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote:
> > > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote:
> > > > From: Russell King <rmk+kernel@armlinux.org.uk>
> > > > 
> > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > > > 
> > > > Such combination of bits is meaningless so assume that LOS signal is not
> > > > implemented.
> > > > 
> > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > > > 
> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > 
> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > 
> > I'd like to send this patch irrespective of discussion on the other
> > patches - I already have it committed in my repository with a different
> > description, but the patch content is the same.
> > 
> > Are you happy if I transfer Andrew's r-b tag, and convert yours to an
> > acked-by before I send it?
> > 
> > I'd also like to add a patch that allows 2.5G if no other modes are
> > found, but the bitrate specified by the module allows 2.5G speed - much
> > like we do for 1G speeds.
> 
> Russell, should I send a new version of patch series without this patch?

I think there's some work to be done for patch 1, so I was thinking of
sending this with another SFP patch. It's really a bug fix since the
existing code doesn't behave very well if both bits are set - it will
toggle state on every RX_LOS event received irrespective of the RX_LOS
state.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set
  2021-01-09 23:19           ` Russell King - ARM Linux admin
@ 2021-01-09 23:50             ` Pali Rohár
  0 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-09 23:50 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, Thomas Schreiber,
	Heiner Kallweit, Marek Behún, netdev, linux-kernel

On Saturday 09 January 2021 23:19:54 Russell King - ARM Linux admin wrote:
> On Sat, Jan 09, 2021 at 08:14:47PM +0100, Pali Rohár wrote:
> > On Saturday 09 January 2021 15:46:01 Russell King - ARM Linux admin wrote:
> > > On Thu, Jan 07, 2021 at 05:54:28PM +0100, Andrew Lunn wrote:
> > > > On Wed, Jan 06, 2021 at 04:37:48PM +0100, Pali Rohár wrote:
> > > > > From: Russell King <rmk+kernel@armlinux.org.uk>
> > > > > 
> > > > > Some GPON SFP modules (e.g. Ubiquiti U-Fiber Instant) have set both
> > > > > SFP_OPTIONS_LOS_INVERTED and SFP_OPTIONS_LOS_NORMAL bits in their EEPROM.
> > > > > 
> > > > > Such combination of bits is meaningless so assume that LOS signal is not
> > > > > implemented.
> > > > > 
> > > > > This patch fixes link carrier for GPON SFP module Ubiquiti U-Fiber Instant.
> > > > > 
> > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > 
> > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > > 
> > > I'd like to send this patch irrespective of discussion on the other
> > > patches - I already have it committed in my repository with a different
> > > description, but the patch content is the same.
> > > 
> > > Are you happy if I transfer Andrew's r-b tag, and convert yours to an
> > > acked-by before I send it?
> > > 
> > > I'd also like to add a patch that allows 2.5G if no other modes are
> > > found, but the bitrate specified by the module allows 2.5G speed - much
> > > like we do for 1G speeds.
> > 
> > Russell, should I send a new version of patch series without this patch?
> 
> I think there's some work to be done for patch 1, so I was thinking of
> sending this with another SFP patch. It's really a bug fix since the
> existing code doesn't behave very well if both bits are set - it will
> toggle state on every RX_LOS event received irrespective of the RX_LOS
> state.

Ok! So I will fix what is needed for patch 1, send it with patch 3 as
next version and let patch 2 to you.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
                   ` (4 preceding siblings ...)
  2021-01-06 15:37 ` [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
@ 2021-01-11 11:39 ` Pali Rohár
  2021-01-11 11:39   ` [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
                     ` (3 more replies)
  2021-01-25 15:02 ` [PATCH v4 " Pali Rohár
  6 siblings, 4 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-11 11:39 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

This is a third version of patches which add workarounds for
RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.

Russel's PATCH v2 2/3 was dropped from this patch series as
it is being handled separately.

Pali Rohár (2):
  net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant

 drivers/net/phy/sfp-bus.c |  15 +++++
 drivers/net/phy/sfp.c     | 117 ++++++++++++++++++++++++++------------
 2 files changed, 97 insertions(+), 35 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 78+ messages in thread

* [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
@ 2021-01-11 11:39   ` Pali Rohár
  2021-01-11 15:28     ` Marek Behún
  2021-01-11 11:39   ` [PATCH v3 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-11 11:39 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

Workaround for GPON SFP modules based on VSOL V2801F brand was added in
commit 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490
v2.0 workaround"). But it works only for ids explicitly added to the list.
As there are more rebraded VSOL V2801F modules and OEM vendors are putting
into vendor name random strings we cannot build workaround based on ids.

Moreover issue which that commit tried to workaround is generic not only to
VSOL based modules, but rather to all GPON modules based on Realtek RTL8672
and RTL9601C chips.

They can be found for example in following GPON modules:
* V-SOL V2801F
* C-Data FD511GX-RM0
* OPTON GP801R
* BAUDCOM BD-1234-SFM
* CPGOS03-0490 v2.0
* Ubiquiti U-Fiber Instant
* EXOT EGS1

Those Realtek chips have broken EEPROM emulator which for N-byte read
operation returns just one byte of EEPROM data followed by N-1 zeros.

So introduce a new function sfp_id_needs_byte_io() which detects SFP
modules with these Realtek chips which have broken EEPROM emulator based on
N-1 zeros and switch to 1 byte EEPROM reading operation which workaround
this issue.

Function sfp_i2c_read() now always use single byte reading when it is
required and when function sfp_hwmon_probe() detects single byte access
then it disable registration of hwmon device. As in this case we cannot
reliable and atomically read 2 bytes as it is required for retrieving some
values from diagnostic area.

These Realtek chips are broken in a way that violate SFP standards for
diagnostic interface. Kernel in this case cannot do anything, only skipping
registration of hwmon interface.

This patch fixes reading of EEPROM content from SFP modules based on
Realtek RTL8672 and RTL9601C chips. Diagnostic interface of EEPROM stay
broken and cannot be fixed.

Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* Add explanation why also for second address is used one byte read op
* Skip hwmon registration when eeprom does not support atomic 16bit read op

Changes in v3:
* Do not break longer info messages
* Do not read memory after the end of buffer in sfp_id_needs_byte_io()
* Add comments for default i2c_block_size and Nokia 3FE46541AA module
---
 drivers/net/phy/sfp.c | 100 ++++++++++++++++++++++++++++--------------
 1 file changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 91d74c1a920a..f2b5e467a800 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -336,19 +336,11 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
 			size_t len)
 {
 	struct i2c_msg msgs[2];
-	size_t block_size;
+	u8 bus_addr = a2 ? 0x51 : 0x50;
+	size_t block_size = sfp->i2c_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;
@@ -1282,6 +1274,20 @@ static void sfp_hwmon_probe(struct work_struct *work)
 	struct sfp *sfp = container_of(work, struct sfp, hwmon_probe.work);
 	int err, i;
 
+	/* hwmon interface needs to access 16bit registers in atomic way to
+	 * guarantee coherency of the diagnostic monitoring data. If it is not
+	 * possible to guarantee coherency because EEPROM is broken in such way
+	 * that does not support atomic 16bit read operation then we have to
+	 * skip registration of hwmon device.
+	 */
+	if (sfp->i2c_block_size < 2) {
+		dev_info(sfp->dev,
+			 "skipping hwmon device registration due to broken EEPROM\n");
+		dev_info(sfp->dev,
+			 "diagnostic EEPROM area cannot be read atomically to guarantee data coherency\n");
+		return;
+	}
+
 	err = sfp_read(sfp, true, 0, &sfp->diag, sizeof(sfp->diag));
 	if (err < 0) {
 		if (sfp->hwmon_tries--) {
@@ -1642,26 +1648,30 @@ 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.
+/* GPON modules based on Realtek RTL8672 and RTL9601C chips (e.g. V-SOL
+ * V2801F, CarlitoxxPro CPGOS03-0490, Ubiquiti U-Fiber Instant, ...) do
+ * not support multibyte reads from the EEPROM. Each multi-byte read
+ * operation returns just one byte of EEPROM followed by zeros. There is
+ * no way to identify which modules are using Realtek RTL8672 and RTL9601C
+ * chips. Moreover every OEM of V-SOL V2801F module puts its own vendor
+ * name and vendor id into EEPROM, so there is even no way to detect if
+ * module is V-SOL V2801F. Therefore check for those zeros in the read
+ * data and then based on check switch to reading EEPROM to one byte
+ * at a time.
  */
-static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
+static bool sfp_id_needs_byte_io(struct sfp *sfp, void *buf, size_t len)
 {
-	if (!memcmp(base->vendor_name, "VSOL            ", 16))
-		return 1;
-	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
-	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
-		return 1;
+	size_t i, block_size = sfp->i2c_block_size;
 
-	/* Some modules can't cope with long reads */
-	return 16;
-}
+	/* Already using byte IO */
+	if (block_size == 1)
+		return false;
 
-static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base *base)
-{
-	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
+	for (i = 1; i < len; i += block_size) {
+		if (memchr_inv(buf + i, '\0', min(block_size - 1, len - i)))
+			return false;
+	}
+	return true;
 }
 
 static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)
@@ -1705,11 +1715,11 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	u8 check;
 	int ret;
 
-	/* 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.
+	/* Some SFP modules and also some Linux I2C drivers do not like reads
+	 * longer than 16 bytes, so read the EEPROM in chunks of 16 bytes at
+	 * a time.
 	 */
-	sfp->i2c_block_size = 1;
+	sfp->i2c_block_size = 16;
 
 	ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base));
 	if (ret < 0) {
@@ -1723,6 +1733,33 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 		return -EAGAIN;
 	}
 
+	/* Some SFP modules (e.g. Nokia 3FE46541AA) lock up if read from
+	 * address 0x51 is just one byte at a time. Also SFF-8472 requires
+	 * that EEPROM supports atomic 16bit read operation for diagnostic
+	 * fields, so do not switch to one byte reading at a time unless it
+	 * is really required and we have no other option.
+	 */
+	if (sfp_id_needs_byte_io(sfp, &id.base, sizeof(id.base))) {
+		dev_info(sfp->dev,
+			 "Detected broken RTL8672/RTL9601C emulated EEPROM\n");
+		dev_info(sfp->dev,
+			 "Switching to reading EEPROM to one byte at a time\n");
+		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.base)) {
+			dev_err(sfp->dev, "EEPROM short read: %d\n", ret);
+			return -EAGAIN;
+		}
+	}
+
 	/* Cotsworks do not seem to update the checksums when they
 	 * do the final programming with the final module part number,
 	 * serial number and date code.
@@ -1757,9 +1794,6 @@ 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)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* [PATCH v3 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
  2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
  2021-01-11 11:39   ` [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
@ 2021-01-11 11:39   ` Pali Rohár
  2021-01-11 15:32     ` Marek Behún
  2021-01-12 13:33   ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
  2021-01-18  9:34   ` Pali Rohár
  3 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-11 11:39 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

SFP GPON module Ubiquiti U-Fiber Instant has in its EEPROM stored nonsense
information. It claims that support all transceiver types including 10G
Ethernet which is not truth. So clear all claimed modes and set only one
mode which module supports: 1000baseX_Full.

Also this module have set SFF phys_id in its EEPROM. Kernel SFP subsustem
currently does not allow to use SFP modules detected as SFF. Therefore add
and exception for this module so it can be detected as supported.

This change finally allows to detect and use SFP GPON module Ubiquiti
U-Fiber Instant on Linux system.

EEPROM content of this SFP module is (where XX is serial number):

00: 02 04 0b ff ff ff ff ff ff ff ff 03 0c 00 14 c8    ???........??.??
10: 00 00 00 00 55 42 4e 54 20 20 20 20 20 20 20 20    ....UBNT
20: 20 20 20 20 00 18 e8 29 55 46 2d 49 4e 53 54 41        .??)UF-INSTA
30: 4e 54 20 20 20 20 20 20 34 20 20 20 05 1e 00 36    NT      4   ??.6
40: 00 06 00 00 55 42 4e 54 XX XX XX XX XX XX XX XX    .?..UBNTXXXXXXXX
50: 20 20 20 20 31 34 30 31 32 33 20 20 60 80 02 41        140123  `??A

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* add this module also into sfp_module_supported() function

Changes in v3:
* no change
---
 drivers/net/phy/sfp-bus.c | 15 +++++++++++++++
 drivers/net/phy/sfp.c     | 17 +++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 20b91f5dfc6e..4cf874fb5c5b 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -44,6 +44,17 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
 	phylink_set(modes, 2500baseX_Full);
 }
 
+static void sfp_quirk_ubnt_uf_instant(const struct sfp_eeprom_id *id,
+				      unsigned long *modes)
+{
+	/* Ubiquiti U-Fiber Instant module claims that support all transceiver
+	 * types including 10G Ethernet which is not truth. So clear all claimed
+	 * modes and set only one mode which module supports: 1000baseX_Full.
+	 */
+	phylink_zero(modes);
+	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 +74,10 @@ static const struct sfp_quirk sfp_quirks[] = {
 		.vendor = "HUAWEI",
 		.part = "MA5671A",
 		.modes = sfp_quirk_2500basex,
+	}, {
+		.vendor = "UBNT",
+		.part = "UF-INSTANT",
+		.modes = sfp_quirk_ubnt_uf_instant,
 	},
 };
 
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index f2b5e467a800..7a680b5177f5 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -273,8 +273,21 @@ static const struct sff_data sff_data = {
 
 static bool sfp_module_supported(const struct sfp_eeprom_id *id)
 {
-	return id->base.phys_id == SFF8024_ID_SFP &&
-	       id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP;
+	if (id->base.phys_id == SFF8024_ID_SFP &&
+	    id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP)
+		return true;
+
+	/* SFP GPON module Ubiquiti U-Fiber Instant has in its EEPROM stored
+	 * phys id SFF instead of SFP. Therefore mark this module explicitly
+	 * as supported based on vendor name and pn match.
+	 */
+	if (id->base.phys_id == SFF8024_ID_SFF_8472 &&
+	    id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP &&
+	    !memcmp(id->base.vendor_name, "UBNT            ", 16) &&
+	    !memcmp(id->base.vendor_pn, "UF-INSTANT      ", 16))
+		return true;
+
+	return false;
 }
 
 static const struct sff_data sfp_data = {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-11 11:39   ` [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
@ 2021-01-11 15:28     ` Marek Behún
  0 siblings, 0 replies; 78+ messages in thread
From: Marek Behún @ 2021-01-11 15:28 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski, Thomas Schreiber, Heiner Kallweit, netdev,
	linux-kernel

Hi Pali,
I have rewritten the commit message a little:

The workaround for VSOL V2801F brand based GPON SFP modules added in
commit 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490
v2.0 workaround") works only for IDs added explicitly to the list.
Since there are rebranded modules where OEM vendors put different
strings into the vendor name field, we cannot base workaround on IDs
only.

Moreover the issue which the above mentioned commit tried to work
around is generic not only to VSOL based modules, but rather to all
GPON modules based on Realtek RTL8672 and RTL9601C chips.
 
These include at least the following GPON modules:
* V-SOL V2801F
* C-Data FD511GX-RM0
* OPTON GP801R
* BAUDCOM BD-1234-SFM
* CPGOS03-0490 v2.0
* Ubiquiti U-Fiber Instant
* EXOT EGS1

These Realtek chips have broken EEPROM emulator which for N-byte read
operation returns just the first byte of EEPROM data, followed by N-1 zeros.

Introduce a new function, sfp_id_needs_byte_io(), which detects SFP
modules with broken EEPROM emulator based on N-1 zeros and switch to 1
byte EEPROM reading operation.

Function sfp_i2c_read() now always uses single byte reading when it is
required and when function sfp_hwmon_probe() detects single byte access,
it disables registration of hwmon device, because in this case we
cannot reliably and atomically read 2 bytes as is required byt the
standard for retrieving values from diagnostic area.

(These Realtek chips are broken in a way that violates SFP standards for
 diagnostic interface. Kernel in this case simply cannot do anything
 less of skipping registration of the hwmon interface.)

This patch fixes reading of EEPROM content from SFP modules based on
Realtek RTL8672 and RTL9601C chips. Diagnostic interface of EEPROM stays
broken and cannot be fixed.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
  2021-01-11 11:39   ` [PATCH v3 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
@ 2021-01-11 15:32     ` Marek Behún
  0 siblings, 0 replies; 78+ messages in thread
From: Marek Behún @ 2021-01-11 15:32 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski, Thomas Schreiber, Heiner Kallweit, netdev,
	linux-kernel

On Mon, 11 Jan 2021 12:39:09 +0100
Pali Rohár <pali@kernel.org> wrote:

> SFP GPON module Ubiquiti U-Fiber Instant has in its EEPROM stored nonsense
> information. It claims that support all transceiver types including 10G
> Ethernet which is not truth. So clear all claimed modes and set only one
> mode which module supports: 1000baseX_Full.

The Ubiquiti U-Fiber Instant SFP GPON module has nonsensical
information stored in int EEPROM. It claims to support all transceiver
types including 10G Ethernet. Clear all claimed modes and set only
1000baseX_Full, which is the only one supported.

> Also this module have set SFF phys_id in its EEPROM. Kernel SFP subsustem
> currently does not allow to use SFP modules detected as SFF. Therefore add
> and exception for this module so it can be detected as supported.

This module has also phys_id set to SFF, and the SFP subsystem
currently does not allow to use SFP modules detected as SFFs. Add
exception for this module so it can be detected as supported.

> This change finally allows to detect and use SFP GPON module Ubiquiti
> U-Fiber Instant on Linux system.
>
> Original EEPROM content is as follows (where XX is serial number):
> 
> 00: 02 04 0b ff ff ff ff ff ff ff ff 03 0c 00 14 c8    ???........??.??
> 10: 00 00 00 00 55 42 4e 54 20 20 20 20 20 20 20 20    ....UBNT
> 20: 20 20 20 20 00 18 e8 29 55 46 2d 49 4e 53 54 41        .??)UF-INSTA
> 30: 4e 54 20 20 20 20 20 20 34 20 20 20 05 1e 00 36    NT      4   ??.6
> 40: 00 06 00 00 55 42 4e 54 XX XX XX XX XX XX XX XX    .?..UBNTXXXXXXXX
> 50: 20 20 20 20 31 34 30 31 32 33 20 20 60 80 02 41        140123  `??A

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
  2021-01-11 11:39   ` [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
  2021-01-11 11:39   ` [PATCH v3 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
@ 2021-01-12 13:33   ` Pali Rohár
  2021-01-18  9:34   ` Pali Rohár
  3 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-12 13:33 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Monday 11 January 2021 12:39:07 Pali Rohár wrote:
> This is a third version of patches which add workarounds for
> RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.
> 
> Russel's PATCH v2 2/3 was dropped from this patch series as
> it is being handled separately.
> 
> Pali Rohár (2):
>   net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
>   net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
> 
>  drivers/net/phy/sfp-bus.c |  15 +++++
>  drivers/net/phy/sfp.c     | 117 ++++++++++++++++++++++++++------------
>  2 files changed, 97 insertions(+), 35 deletions(-)

I'm fine with Marek's commit message changes.

Russell, Andrew, anything else is needed for these two patches?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
                     ` (2 preceding siblings ...)
  2021-01-12 13:33   ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
@ 2021-01-18  9:34   ` Pali Rohár
  2021-01-25 14:09     ` Pali Rohár
  3 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-18  9:34 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Monday 11 January 2021 12:39:07 Pali Rohár wrote:
> This is a third version of patches which add workarounds for
> RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.
> 
> Russel's PATCH v2 2/3 was dropped from this patch series as
> it is being handled separately.

Andrew and Russel, are you fine with this third iteration of patches?
Or are there still some issues which needs to be fixed?

> Pali Rohár (2):
>   net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
>   net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
> 
>  drivers/net/phy/sfp-bus.c |  15 +++++
>  drivers/net/phy/sfp.c     | 117 ++++++++++++++++++++++++++------------
>  2 files changed, 97 insertions(+), 35 deletions(-)
> 
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-18  9:34   ` Pali Rohár
@ 2021-01-25 14:09     ` Pali Rohár
  2021-01-25 14:16       ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-25 14:09 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

On Monday 18 January 2021 10:34:35 Pali Rohár wrote:
> On Monday 11 January 2021 12:39:07 Pali Rohár wrote:
> > This is a third version of patches which add workarounds for
> > RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.
> > 
> > Russel's PATCH v2 2/3 was dropped from this patch series as
> > it is being handled separately.
> 
> Andrew and Russel, are you fine with this third iteration of patches?
> Or are there still some issues which needs to be fixed?

PING!

> > Pali Rohár (2):
> >   net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
> >   net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
> > 
> >  drivers/net/phy/sfp-bus.c |  15 +++++
> >  drivers/net/phy/sfp.c     | 117 ++++++++++++++++++++++++++------------
> >  2 files changed, 97 insertions(+), 35 deletions(-)
> > 
> > -- 
> > 2.20.1
> > 

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-25 14:09     ` Pali Rohár
@ 2021-01-25 14:16       ` Russell King - ARM Linux admin
  2021-01-25 14:23         ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-25 14:16 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, Thomas Schreiber,
	Heiner Kallweit, Marek Behún, netdev, linux-kernel

On Mon, Jan 25, 2021 at 03:09:57PM +0100, Pali Rohár wrote:
> On Monday 18 January 2021 10:34:35 Pali Rohár wrote:
> > On Monday 11 January 2021 12:39:07 Pali Rohár wrote:
> > > This is a third version of patches which add workarounds for
> > > RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.
> > > 
> > > Russel's PATCH v2 2/3 was dropped from this patch series as
> > > it is being handled separately.
> > 
> > Andrew and Russel, are you fine with this third iteration of patches?
> > Or are there still some issues which needs to be fixed?
> 
> PING!

What about the commit message suggestions from Marek?

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-25 14:16       ` Russell King - ARM Linux admin
@ 2021-01-25 14:23         ` Pali Rohár
  2021-01-25 14:42           ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-25 14:23 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, Thomas Schreiber,
	Heiner Kallweit, Marek Behún, netdev, linux-kernel

On Monday 25 January 2021 14:16:44 Russell King - ARM Linux admin wrote:
> On Mon, Jan 25, 2021 at 03:09:57PM +0100, Pali Rohár wrote:
> > On Monday 18 January 2021 10:34:35 Pali Rohár wrote:
> > > On Monday 11 January 2021 12:39:07 Pali Rohár wrote:
> > > > This is a third version of patches which add workarounds for
> > > > RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.
> > > > 
> > > > Russel's PATCH v2 2/3 was dropped from this patch series as
> > > > it is being handled separately.
> > > 
> > > Andrew and Russel, are you fine with this third iteration of patches?
> > > Or are there still some issues which needs to be fixed?
> > 
> > PING!
> 
> What about the commit message suggestions from Marek?

I have already wrote that I'm fine with those suggestions.

It is the only thing to handle? If yes, should I send a new patch series
with fixed commit messages?

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-25 14:23         ` Pali Rohár
@ 2021-01-25 14:42           ` Russell King - ARM Linux admin
  2021-01-25 14:47             ` Pali Rohár
  0 siblings, 1 reply; 78+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-25 14:42 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, Thomas Schreiber,
	Heiner Kallweit, Marek Behún, netdev, linux-kernel

On Mon, Jan 25, 2021 at 03:23:01PM +0100, Pali Rohár wrote:
> On Monday 25 January 2021 14:16:44 Russell King - ARM Linux admin wrote:
> > On Mon, Jan 25, 2021 at 03:09:57PM +0100, Pali Rohár wrote:
> > > On Monday 18 January 2021 10:34:35 Pali Rohár wrote:
> > > > On Monday 11 January 2021 12:39:07 Pali Rohár wrote:
> > > > > This is a third version of patches which add workarounds for
> > > > > RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.
> > > > > 
> > > > > Russel's PATCH v2 2/3 was dropped from this patch series as
> > > > > it is being handled separately.
> > > > 
> > > > Andrew and Russel, are you fine with this third iteration of patches?
> > > > Or are there still some issues which needs to be fixed?
> > > 
> > > PING!
> > 
> > What about the commit message suggestions from Marek?
> 
> I have already wrote that I'm fine with those suggestions.
> 
> It is the only thing to handle? If yes, should I send a new patch series
> with fixed commit messages?

Yes, because that's the way the netdev list works - patches sent to
netdev go into patchwork, when they get reviewed and acks etc,
patchwork updates itself. Jakub or David can then see what the status
is and apply them to the net or net-next trees as appropriate.

The "finished" patches need to be posted for this process to start.

Thanks.

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

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-25 14:42           ` Russell King - ARM Linux admin
@ 2021-01-25 14:47             ` Pali Rohár
  2021-01-25 15:41               ` Andrew Lunn
  0 siblings, 1 reply; 78+ messages in thread
From: Pali Rohár @ 2021-01-25 14:47 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, Thomas Schreiber,
	Heiner Kallweit, Marek Behún, netdev, linux-kernel

On Monday 25 January 2021 14:42:21 Russell King - ARM Linux admin wrote:
> On Mon, Jan 25, 2021 at 03:23:01PM +0100, Pali Rohár wrote:
> > On Monday 25 January 2021 14:16:44 Russell King - ARM Linux admin wrote:
> > > On Mon, Jan 25, 2021 at 03:09:57PM +0100, Pali Rohár wrote:
> > > > On Monday 18 January 2021 10:34:35 Pali Rohár wrote:
> > > > > On Monday 11 January 2021 12:39:07 Pali Rohár wrote:
> > > > > > This is a third version of patches which add workarounds for
> > > > > > RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.
> > > > > > 
> > > > > > Russel's PATCH v2 2/3 was dropped from this patch series as
> > > > > > it is being handled separately.
> > > > > 
> > > > > Andrew and Russel, are you fine with this third iteration of patches?
> > > > > Or are there still some issues which needs to be fixed?
> > > > 
> > > > PING!
> > > 
> > > What about the commit message suggestions from Marek?
> > 
> > I have already wrote that I'm fine with those suggestions.
> > 
> > It is the only thing to handle? If yes, should I send a new patch series
> > with fixed commit messages?
> 
> Yes, because that's the way the netdev list works - patches sent to
> netdev go into patchwork, when they get reviewed and acks etc,
> patchwork updates itself. Jakub or David can then see what the status
> is and apply them to the net or net-next trees as appropriate.

Ok! If this is the only remaining issue, I will update commit messages
and send a new patch series. I was just waiting for a response if
somebody else has other comments or if somebody write that is fine with
it.

> The "finished" patches need to be posted for this process to start.
> 
> Thanks.

^ permalink raw reply	[flat|nested] 78+ messages in thread

* [PATCH v4 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
                   ` (5 preceding siblings ...)
  2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
@ 2021-01-25 15:02 ` Pali Rohár
  2021-01-25 15:02   ` [PATCH v4 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
                     ` (2 more replies)
  6 siblings, 3 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-25 15:02 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

This is fourth version of patches which add workarounds for
RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.

The only change since third version is modification of commit messages.

Pali Rohár (2):
  net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant

 drivers/net/phy/sfp-bus.c |  15 +++++
 drivers/net/phy/sfp.c     | 117 ++++++++++++++++++++++++++------------
 2 files changed, 97 insertions(+), 35 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 78+ messages in thread

* [PATCH v4 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
  2021-01-25 15:02 ` [PATCH v4 " Pali Rohár
@ 2021-01-25 15:02   ` Pali Rohár
  2021-01-25 15:02   ` [PATCH v4 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
  2021-01-28 21:50   ` [PATCH v4 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber patchwork-bot+netdevbpf
  2 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-25 15:02 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

The workaround for VSOL V2801F brand based GPON SFP modules added in commit
0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0
workaround") works only for IDs added explicitly to the list. Since there
are rebranded modules where OEM vendors put different strings into the
vendor name field, we cannot base workaround on IDs only.

Moreover the issue which the above mentioned commit tried to work around is
generic not only to VSOL based modules, but rather to all GPON modules
based on Realtek RTL8672 and RTL9601C chips.

These include at least the following GPON modules:
* V-SOL V2801F
* C-Data FD511GX-RM0
* OPTON GP801R
* BAUDCOM BD-1234-SFM
* CPGOS03-0490 v2.0
* Ubiquiti U-Fiber Instant
* EXOT EGS1

These Realtek chips have broken EEPROM emulator which for N-byte read
operation returns just the first byte of EEPROM data, followed by N-1
zeros.

Introduce a new function, sfp_id_needs_byte_io(), which detects SFP modules
with broken EEPROM emulator based on N-1 zeros and switch to 1 byte EEPROM
reading operation.

Function sfp_i2c_read() now always uses single byte reading when it is
required and when function sfp_hwmon_probe() detects single byte access,
it disables registration of hwmon device, because in this case we cannot
reliably and atomically read 2 bytes as is required by the standard for
retrieving values from diagnostic area.

(These Realtek chips are broken in a way that violates SFP standards for
diagnostic interface. Kernel in this case simply cannot do anything less
of skipping registration of the hwmon interface.)

This patch fixes reading of EEPROM content from SFP modules based on
Realtek RTL8672 and RTL9601C chips. Diagnostic interface of EEPROM stays
broken and cannot be fixed.

Fixes: 0d035bed2a4a ("net: sfp: VSOL V2801F / CarlitoxxPro CPGOS03-0490 v2.0 workaround")
Co-developed-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v4:
* Rewritten the commit message by Marek's suggestion

Changes in v3:
* Do not break longer info messages
* Do not read memory after the end of buffer in sfp_id_needs_byte_io()
* Add comments for default i2c_block_size and Nokia 3FE46541AA module

Changes in v2:
* Add explanation why also for second address is used one byte read op
* Skip hwmon registration when eeprom does not support atomic 16bit read op
---
 drivers/net/phy/sfp.c | 100 ++++++++++++++++++++++++++++--------------
 1 file changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 91d74c1a920a..f2b5e467a800 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -336,19 +336,11 @@ static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
 			size_t len)
 {
 	struct i2c_msg msgs[2];
-	size_t block_size;
+	u8 bus_addr = a2 ? 0x51 : 0x50;
+	size_t block_size = sfp->i2c_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;
@@ -1282,6 +1274,20 @@ static void sfp_hwmon_probe(struct work_struct *work)
 	struct sfp *sfp = container_of(work, struct sfp, hwmon_probe.work);
 	int err, i;
 
+	/* hwmon interface needs to access 16bit registers in atomic way to
+	 * guarantee coherency of the diagnostic monitoring data. If it is not
+	 * possible to guarantee coherency because EEPROM is broken in such way
+	 * that does not support atomic 16bit read operation then we have to
+	 * skip registration of hwmon device.
+	 */
+	if (sfp->i2c_block_size < 2) {
+		dev_info(sfp->dev,
+			 "skipping hwmon device registration due to broken EEPROM\n");
+		dev_info(sfp->dev,
+			 "diagnostic EEPROM area cannot be read atomically to guarantee data coherency\n");
+		return;
+	}
+
 	err = sfp_read(sfp, true, 0, &sfp->diag, sizeof(sfp->diag));
 	if (err < 0) {
 		if (sfp->hwmon_tries--) {
@@ -1642,26 +1648,30 @@ 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.
+/* GPON modules based on Realtek RTL8672 and RTL9601C chips (e.g. V-SOL
+ * V2801F, CarlitoxxPro CPGOS03-0490, Ubiquiti U-Fiber Instant, ...) do
+ * not support multibyte reads from the EEPROM. Each multi-byte read
+ * operation returns just one byte of EEPROM followed by zeros. There is
+ * no way to identify which modules are using Realtek RTL8672 and RTL9601C
+ * chips. Moreover every OEM of V-SOL V2801F module puts its own vendor
+ * name and vendor id into EEPROM, so there is even no way to detect if
+ * module is V-SOL V2801F. Therefore check for those zeros in the read
+ * data and then based on check switch to reading EEPROM to one byte
+ * at a time.
  */
-static int sfp_quirk_i2c_block_size(const struct sfp_eeprom_base *base)
+static bool sfp_id_needs_byte_io(struct sfp *sfp, void *buf, size_t len)
 {
-	if (!memcmp(base->vendor_name, "VSOL            ", 16))
-		return 1;
-	if (!memcmp(base->vendor_name, "OEM             ", 16) &&
-	    !memcmp(base->vendor_pn,   "V2801F          ", 16))
-		return 1;
+	size_t i, block_size = sfp->i2c_block_size;
 
-	/* Some modules can't cope with long reads */
-	return 16;
-}
+	/* Already using byte IO */
+	if (block_size == 1)
+		return false;
 
-static void sfp_quirks_base(struct sfp *sfp, const struct sfp_eeprom_base *base)
-{
-	sfp->i2c_block_size = sfp_quirk_i2c_block_size(base);
+	for (i = 1; i < len; i += block_size) {
+		if (memchr_inv(buf + i, '\0', min(block_size - 1, len - i)))
+			return false;
+	}
+	return true;
 }
 
 static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)
@@ -1705,11 +1715,11 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 	u8 check;
 	int ret;
 
-	/* 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.
+	/* Some SFP modules and also some Linux I2C drivers do not like reads
+	 * longer than 16 bytes, so read the EEPROM in chunks of 16 bytes at
+	 * a time.
 	 */
-	sfp->i2c_block_size = 1;
+	sfp->i2c_block_size = 16;
 
 	ret = sfp_read(sfp, false, 0, &id.base, sizeof(id.base));
 	if (ret < 0) {
@@ -1723,6 +1733,33 @@ static int sfp_sm_mod_probe(struct sfp *sfp, bool report)
 		return -EAGAIN;
 	}
 
+	/* Some SFP modules (e.g. Nokia 3FE46541AA) lock up if read from
+	 * address 0x51 is just one byte at a time. Also SFF-8472 requires
+	 * that EEPROM supports atomic 16bit read operation for diagnostic
+	 * fields, so do not switch to one byte reading at a time unless it
+	 * is really required and we have no other option.
+	 */
+	if (sfp_id_needs_byte_io(sfp, &id.base, sizeof(id.base))) {
+		dev_info(sfp->dev,
+			 "Detected broken RTL8672/RTL9601C emulated EEPROM\n");
+		dev_info(sfp->dev,
+			 "Switching to reading EEPROM to one byte at a time\n");
+		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.base)) {
+			dev_err(sfp->dev, "EEPROM short read: %d\n", ret);
+			return -EAGAIN;
+		}
+	}
+
 	/* Cotsworks do not seem to update the checksums when they
 	 * do the final programming with the final module part number,
 	 * serial number and date code.
@@ -1757,9 +1794,6 @@ 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)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* [PATCH v4 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
  2021-01-25 15:02 ` [PATCH v4 " Pali Rohár
  2021-01-25 15:02   ` [PATCH v4 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
@ 2021-01-25 15:02   ` Pali Rohár
  2021-01-28 21:50   ` [PATCH v4 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber patchwork-bot+netdevbpf
  2 siblings, 0 replies; 78+ messages in thread
From: Pali Rohár @ 2021-01-25 15:02 UTC (permalink / raw)
  To: Russell King - ARM Linux admin, Andrew Lunn, David S. Miller,
	Jakub Kicinski
  Cc: Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

The Ubiquiti U-Fiber Instant SFP GPON module has nonsensical information
stored in its EEPROM. It claims to support all transceiver types including
10G Ethernet. Clear all claimed modes and set only 1000baseX_Full, which is
the only one supported.

This module has also phys_id set to SFF, and the SFP subsystem currently
does not allow to use SFP modules detected as SFFs. Add exception for this
module so it can be detected as supported.

This change finally allows to detect and use SFP GPON module Ubiquiti
U-Fiber Instant on Linux system.

EEPROM content of this SFP module is (where XX is serial number):

00: 02 04 0b ff ff ff ff ff ff ff ff 03 0c 00 14 c8    ???........??.??
10: 00 00 00 00 55 42 4e 54 20 20 20 20 20 20 20 20    ....UBNT
20: 20 20 20 20 00 18 e8 29 55 46 2d 49 4e 53 54 41        .??)UF-INSTA
30: 4e 54 20 20 20 20 20 20 34 20 20 20 05 1e 00 36    NT      4   ??.6
40: 00 06 00 00 55 42 4e 54 XX XX XX XX XX XX XX XX    .?..UBNTXXXXXXXX
50: 20 20 20 20 31 34 30 31 32 33 20 20 60 80 02 41        140123  `??A

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v4:
* Rewritten the commit message by Marek's suggestion

Changes in v3:
* no change

Changes in v2:
* add this module also into sfp_module_supported() function
---
 drivers/net/phy/sfp-bus.c | 15 +++++++++++++++
 drivers/net/phy/sfp.c     | 17 +++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 20b91f5dfc6e..4cf874fb5c5b 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -44,6 +44,17 @@ static void sfp_quirk_2500basex(const struct sfp_eeprom_id *id,
 	phylink_set(modes, 2500baseX_Full);
 }
 
+static void sfp_quirk_ubnt_uf_instant(const struct sfp_eeprom_id *id,
+				      unsigned long *modes)
+{
+	/* Ubiquiti U-Fiber Instant module claims that support all transceiver
+	 * types including 10G Ethernet which is not truth. So clear all claimed
+	 * modes and set only one mode which module supports: 1000baseX_Full.
+	 */
+	phylink_zero(modes);
+	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 +74,10 @@ static const struct sfp_quirk sfp_quirks[] = {
 		.vendor = "HUAWEI",
 		.part = "MA5671A",
 		.modes = sfp_quirk_2500basex,
+	}, {
+		.vendor = "UBNT",
+		.part = "UF-INSTANT",
+		.modes = sfp_quirk_ubnt_uf_instant,
 	},
 };
 
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index f2b5e467a800..7a680b5177f5 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -273,8 +273,21 @@ static const struct sff_data sff_data = {
 
 static bool sfp_module_supported(const struct sfp_eeprom_id *id)
 {
-	return id->base.phys_id == SFF8024_ID_SFP &&
-	       id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP;
+	if (id->base.phys_id == SFF8024_ID_SFP &&
+	    id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP)
+		return true;
+
+	/* SFP GPON module Ubiquiti U-Fiber Instant has in its EEPROM stored
+	 * phys id SFF instead of SFP. Therefore mark this module explicitly
+	 * as supported based on vendor name and pn match.
+	 */
+	if (id->base.phys_id == SFF8024_ID_SFF_8472 &&
+	    id->base.phys_ext_id == SFP_PHYS_EXT_ID_SFP &&
+	    !memcmp(id->base.vendor_name, "UBNT            ", 16) &&
+	    !memcmp(id->base.vendor_pn, "UF-INSTANT      ", 16))
+		return true;
+
+	return false;
 }
 
 static const struct sff_data sfp_data = {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 78+ messages in thread

* Re: [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-25 14:47             ` Pali Rohár
@ 2021-01-25 15:41               ` Andrew Lunn
  0 siblings, 0 replies; 78+ messages in thread
From: Andrew Lunn @ 2021-01-25 15:41 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Russell King - ARM Linux admin, David S. Miller, Jakub Kicinski,
	Thomas Schreiber, Heiner Kallweit, Marek Behún, netdev,
	linux-kernel

> Ok! If this is the only remaining issue, I will update commit messages
> and send a new patch series. I was just waiting for a response if
> somebody else has other comments or if somebody write that is fine with
> it.

Hi Pali

As a general rule of thumb, with netdev, wait 3 days and then send the
next version. If the change request is minor, everybody seems to be in
agreement, you can wait just one day. Please don't send new versions
faster than that.

       Andrew

^ permalink raw reply	[flat|nested] 78+ messages in thread

* Re: [PATCH v4 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber
  2021-01-25 15:02 ` [PATCH v4 " Pali Rohár
  2021-01-25 15:02   ` [PATCH v4 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
  2021-01-25 15:02   ` [PATCH v4 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
@ 2021-01-28 21:50   ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 78+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-01-28 21:50 UTC (permalink / raw)
  To: =?utf-8?b?UGFsaSBSb2jDoXIgPHBhbGlAa2VybmVsLm9yZz4=?=
  Cc: linux, andrew, davem, kuba, tschreibe, hkallweit1, kabel, netdev,
	linux-kernel

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Mon, 25 Jan 2021 16:02:26 +0100 you wrote:
> This is fourth version of patches which add workarounds for
> RTL8672/RTL9601C EEPROMs and Ubiquiti U-Fiber Instant SFP.
> 
> The only change since third version is modification of commit messages.
> 
> Pali Rohár (2):
>   net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
>   net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
> 
> [...]

Here is the summary with links:
  - [v4,1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips
    https://git.kernel.org/netdev/net-next/c/426c6cbc409c
  - [v4,2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant
    https://git.kernel.org/netdev/net-next/c/f0b4f8476732

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 78+ messages in thread

end of thread, other threads:[~2021-01-28 21:52 UTC | newest]

Thread overview: 78+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-30 15:47 [PATCH 0/4] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2020-12-30 15:47 ` [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2020-12-30 16:10   ` Russell King - ARM Linux admin
2020-12-30 16:56     ` Pali Rohár
2020-12-30 17:05       ` Russell King - ARM Linux admin
2020-12-30 17:13         ` Andrew Lunn
2020-12-30 17:43           ` Pali Rohár
2020-12-30 19:09             ` Russell King - ARM Linux admin
2020-12-30 19:49               ` Andrew Lunn
2020-12-31 12:14               ` Pali Rohár
2020-12-31 15:09                 ` Andrew Lunn
2020-12-31 15:40                   ` Pali Rohár
2020-12-31 15:30                 ` Andrew Lunn
2020-12-31 17:00                   ` Pali Rohár
2020-12-31 17:13                     ` Andrew Lunn
2021-01-02  1:49                       ` Pali Rohár
2021-01-03  2:25                         ` Thomas Schreiber
2021-01-03  2:41                           ` Pali Rohár
2021-01-06 14:55                             ` Pali Rohár
2021-01-06 15:21                               ` Russell King - ARM Linux admin
2021-01-06 15:23                                 ` Russell King - ARM Linux admin
2021-01-06 15:27                                   ` Russell King - ARM Linux admin
2021-01-06 15:35                                     ` Pali Rohár
2020-12-30 19:30             ` Andrew Lunn
2020-12-30 19:39               ` Russell King - ARM Linux admin
2020-12-30 17:31         ` Pali Rohár
2020-12-30 19:18           ` Russell King - ARM Linux admin
2020-12-30 15:47 ` [PATCH 2/4] net: sfp: allow to use also SFP modules which are detected as SFF Pali Rohár
2020-12-30 16:11   ` Russell King - ARM Linux admin
2020-12-30 17:06     ` Pali Rohár
2020-12-30 17:27       ` Marek Behún
2020-12-30 19:12         ` Russell King - ARM Linux admin
2020-12-31 13:52           ` Pali Rohár
2020-12-30 15:47 ` [PATCH 3/4] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
2020-12-30 16:13   ` Russell King - ARM Linux admin
2020-12-30 16:57     ` Pali Rohár
2020-12-30 17:06       ` Russell King - ARM Linux admin
2020-12-30 17:17         ` Andrew Lunn
2020-12-30 17:32           ` Pali Rohár
2020-12-30 15:47 ` [PATCH 4/4] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-06 15:37 ` [PATCH v2 0/3] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-06 15:37   ` [PATCH v2 1/3] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-07  2:02     ` Andrew Lunn
2021-01-07  9:08       ` Pali Rohár
2021-01-07 17:19     ` Andrew Lunn
2021-01-07 17:40       ` Russell King - ARM Linux admin
2021-01-07 19:18         ` Pali Rohár
2021-01-07 19:17       ` Pali Rohár
2021-01-07 19:45       ` Russell King - ARM Linux admin
2021-01-07 20:21         ` Marek Behún
2021-01-08  0:49           ` Pali Rohár
2021-01-06 15:37   ` [PATCH v2 2/3] net: sfp: assume that LOS is not implemented if both LOS normal and inverted is set Pali Rohár
2021-01-07 16:54     ` Andrew Lunn
2021-01-09 15:46       ` Russell King - ARM Linux admin
2021-01-09 15:54         ` Andrew Lunn
2021-01-09 16:27           ` Russell King - ARM Linux admin
2021-01-09 19:14         ` Pali Rohár
2021-01-09 23:19           ` Russell King - ARM Linux admin
2021-01-09 23:50             ` Pali Rohár
2021-01-06 15:37   ` [PATCH v2 3/3] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-07 16:51     ` Andrew Lunn
2021-01-11 11:39 ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-11 11:39   ` [PATCH v3 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-11 15:28     ` Marek Behún
2021-01-11 11:39   ` [PATCH v3 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-11 15:32     ` Marek Behún
2021-01-12 13:33   ` [PATCH v3 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber Pali Rohár
2021-01-18  9:34   ` Pali Rohár
2021-01-25 14:09     ` Pali Rohár
2021-01-25 14:16       ` Russell King - ARM Linux admin
2021-01-25 14:23         ` Pali Rohár
2021-01-25 14:42           ` Russell King - ARM Linux admin
2021-01-25 14:47             ` Pali Rohár
2021-01-25 15:41               ` Andrew Lunn
2021-01-25 15:02 ` [PATCH v4 " Pali Rohár
2021-01-25 15:02   ` [PATCH v4 1/2] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips Pali Rohár
2021-01-25 15:02   ` [PATCH v4 2/2] net: sfp: add mode quirk for GPON module Ubiquiti U-Fiber Instant Pali Rohár
2021-01-28 21:50   ` [PATCH v4 0/2] net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).