* [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
* 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 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 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 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 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 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 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 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 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: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
* 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 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 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
* [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
* 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 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 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 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 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
* [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
* 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 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 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 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 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
* [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
* [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
* 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 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: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 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: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
* [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
* 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 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 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 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
* [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
* 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
* [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 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
* 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
* [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 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).