| Message ID | 20210106153749.6748-2-pali@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sfp: add support for GPON RTL8672/RTL9601C and Ubiquiti U-Fiber | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: quoted string split across lines |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> + /* 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
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.
> + 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
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.
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.
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?
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.
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
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.
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)
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(-)