| Message ID | 20240823030242.3083528-1-jiawenwu@trustnetic.com |
|---|---|
| Headers | show |
| Series | Add I2C bus lock for Wangxun | expand |
Hi Hi On 8/23/24 6:02 AM, Jiawen Wu wrote: > Sometimes the driver can not get the SFP information because the I2C bus > is accessed by the firmware at the same time. So we need to add the lock > on the I2C bus access. The hardware semaphores perform this lock. > > Jiawen Wu (3): > net: txgbe: add IO address in I2C platform device data > i2c: designware: add device private data passing to lock functions > i2c: designware: support hardware lock for Wangxun 10Gb NIC > I was wondering the Fixes tag use in the series. Obviously patchset is not fixing a regression so question is what happens when issue occurs? I don't think e.g. failing I2C transfer with an error code yet qualifies backporting into Linux stable?
On Fri, Aug 23, 2024 at 11:02:39AM +0800, Jiawen Wu wrote: > Sometimes the driver can not get the SFP information because the I2C bus > is accessed by the firmware at the same time. Please could you explain this some more. What firmware? There some registers which are clear on read. They don't work when you have multiple entities reading them. Andrew
On Mon, Aug 26, 2024 9:33 AM, Andrew Lunn wrote: > On Fri, Aug 23, 2024 at 11:02:39AM +0800, Jiawen Wu wrote: > > Sometimes the driver can not get the SFP information because the I2C bus > > is accessed by the firmware at the same time. > > Please could you explain this some more. What firmware? It's the firmware of our ethernet devices. > There some registers which are clear on read. They don't work when you > have multiple entities reading them. I'm not trying to multiple access the I2C registers, but these registers cannot be accessed by other interfaces in the process of reading complete information each time. So there is a semaphore needed that locks up the entire read process.
On Mon, Aug 26, 2024 at 10:04:42AM +0800, Jiawen Wu wrote: > On Mon, Aug 26, 2024 9:33 AM, Andrew Lunn wrote: > > On Fri, Aug 23, 2024 at 11:02:39AM +0800, Jiawen Wu wrote: > > > Sometimes the driver can not get the SFP information because the I2C bus > > > is accessed by the firmware at the same time. > > > > Please could you explain this some more. What firmware? > > It's the firmware of our ethernet devices. > > > There some registers which are clear on read. They don't work when you > > have multiple entities reading them. > > I'm not trying to multiple access the I2C registers, but these registers cannot > be accessed by other interfaces in the process of reading complete information > each time. So there is a semaphore needed that locks up the entire read process. More details please. Linux assume it is driving the hardware. Your firmware cannot be touching any registers which will clear on read. QSFP states that registers 3-31 of page 0 are all clear on read, for example. The firmware should also not be setting any registers, otherwise you can confuse Linux which assumes registers it set stay set, because it is controlling the hardware. Your firmware also needs to handle that Linux can change the page. If the firmware changes the page, it must restore it back to whatever page Linux selected, etc. The fact you are submitting this for net suggests you have seen real issues. Please describe what those issues are. Andrew
On Mon, Aug 26, 2024 10:34 AM, Andrew Lunn wrote: > On Mon, Aug 26, 2024 at 10:04:42AM +0800, Jiawen Wu wrote: > > On Mon, Aug 26, 2024 9:33 AM, Andrew Lunn wrote: > > > On Fri, Aug 23, 2024 at 11:02:39AM +0800, Jiawen Wu wrote: > > > > Sometimes the driver can not get the SFP information because the I2C bus > > > > is accessed by the firmware at the same time. > > > > > > Please could you explain this some more. What firmware? > > > > It's the firmware of our ethernet devices. > > > > > There some registers which are clear on read. They don't work when you > > > have multiple entities reading them. > > > > I'm not trying to multiple access the I2C registers, but these registers cannot > > be accessed by other interfaces in the process of reading complete information > > each time. So there is a semaphore needed that locks up the entire read process. > > More details please. > > Linux assume it is driving the hardware. Your firmware cannot be > touching any registers which will clear on read. QSFP states that > registers 3-31 of page 0 are all clear on read, for example. The > firmware should also not be setting any registers, otherwise you can > confuse Linux which assumes registers it set stay set, because it is > controlling the hardware. > > Your firmware also needs to handle that Linux can change the page. If > the firmware changes the page, it must restore it back to whatever > page Linux selected, etc. > > The fact you are submitting this for net suggests you have seen real > issues. Please describe what those issues are. The error log shows: [257681.367827] sfp sfp.1025: Host maximum power 1.0W [257681.370813] txgbe 0000:04:00.1: 31.504 Gb/s available PCIe bandwidth, limited by 8.0 GT/s PCIe x4 link at 0000:02:02.0 (capable of 63.008 Gb/s with 8.0 GT/s PCIe x8 link) [257681.373364] txgbe 0000:04:00.1 enp4s0f1: renamed from eth0 [257681.434719] txgbe 0000:04:00.1 enp4s0f1: configuring for inband/10gbase-r link mode [257681.676747] sfp sfp.1025: EEPROM base structure checksum failure: 0x63 != 0x1f [257681.676755] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00 ............g... [257681.676757] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 64 ....FiberStore d [257681.676759] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53 ...!SFP-10GS [257681.676760] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f R-85 A .R.. [257681.676762] sfp EE: 00000040: 00 81 cd 5b df 25 0a bd 40 f6 c6 ce 47 8e ff ff ...[.%..@...G... [257681.676763] sfp EE: 00000050: 10 d8 24 33 44 8e ff ff 10 41 b0 9a ff ff ff ff ..$3D....A...... It looks like some fields are read incorrectly. For comparison, I printed the SFP info when it loaded correctly: [260908.194533] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00 ............g... [260908.194536] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 20 ....FiberStore [260908.194538] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53 ...!SFP-10GS [260908.194540] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f R-85 A .R.. [260908.194541] sfp EE: 00000040: 40 63 bd df 40 8e ff ff 10 41 b0 9a ff ff ff ff @c..@....A...... [260908.194543] sfp EE: 00000050: 10 58 5b 29 41 8e ff ff 10 41 b0 9a ff ff ff ff .X[)A....A...... [260908.198205] sfp sfp.1025: module FiberStore SFP-10GSR-85 rev A sn G1804125607 dc 180605 Since the read mechanism of I2C is to write the offset and read command first, and then read the target address. I think it's possible that the different offsets be written at the same time, from Linux and firmware.
On Fri, Aug 23, 2024 7:05 PM, Jarkko Nikula wrote: > Hi > Hi > > On 8/23/24 6:02 AM, Jiawen Wu wrote: > > Sometimes the driver can not get the SFP information because the I2C bus > > is accessed by the firmware at the same time. So we need to add the lock > > on the I2C bus access. The hardware semaphores perform this lock. > > > > Jiawen Wu (3): > > net: txgbe: add IO address in I2C platform device data > > i2c: designware: add device private data passing to lock functions > > i2c: designware: support hardware lock for Wangxun 10Gb NIC > > > I was wondering the Fixes tag use in the series. Obviously patchset is > not fixing a regression so question is what happens when issue occurs? > > I don't think e.g. failing I2C transfer with an error code yet qualifies > backporting into Linux stable? Ah, I think this was a leftover bug from when I added Wangxun NIC support in I2C designware, so I added a fix tag.
> > More details please. > > > > Linux assume it is driving the hardware. Your firmware cannot be > > touching any registers which will clear on read. QSFP states that > > registers 3-31 of page 0 are all clear on read, for example. The > > firmware should also not be setting any registers, otherwise you can > > confuse Linux which assumes registers it set stay set, because it is > > controlling the hardware. > > > > Your firmware also needs to handle that Linux can change the page. If > > the firmware changes the page, it must restore it back to whatever > > page Linux selected, etc. > > > > The fact you are submitting this for net suggests you have seen real > > issues. Please describe what those issues are. > > The error log shows: > > [257681.367827] sfp sfp.1025: Host maximum power 1.0W > [257681.370813] txgbe 0000:04:00.1: 31.504 Gb/s available PCIe bandwidth, limited by 8.0 GT/s PCIe x4 link at 0000:02:02.0 (capable > of 63.008 Gb/s with 8.0 GT/s PCIe x8 link) > [257681.373364] txgbe 0000:04:00.1 enp4s0f1: renamed from eth0 > [257681.434719] txgbe 0000:04:00.1 enp4s0f1: configuring for inband/10gbase-r link mode > [257681.676747] sfp sfp.1025: EEPROM base structure checksum failure: 0x63 != 0x1f > [257681.676755] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00 ............g... > [257681.676757] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 64 ....FiberStore d > [257681.676759] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53 ...!SFP-10GS > [257681.676760] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f R-85 A .R.. > [257681.676762] sfp EE: 00000040: 00 81 cd 5b df 25 0a bd 40 f6 c6 ce 47 8e ff ff ...[.%..@...G... > [257681.676763] sfp EE: 00000050: 10 d8 24 33 44 8e ff ff 10 41 b0 9a ff ff ff ff ..$3D....A...... > > It looks like some fields are read incorrectly. For comparison, I printed the > SFP info when it loaded correctly: > > [260908.194533] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00 ............g... > [260908.194536] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 20 ....FiberStore > [260908.194538] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53 ...!SFP-10GS > [260908.194540] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f R-85 A .R.. > [260908.194541] sfp EE: 00000040: 40 63 bd df 40 8e ff ff 10 41 b0 9a ff ff ff ff @c..@....A...... > [260908.194543] sfp EE: 00000050: 10 58 5b 29 41 8e ff ff 10 41 b0 9a ff ff ff ff .X[)A....A...... > [260908.198205] sfp sfp.1025: module FiberStore SFP-10GSR-85 rev A sn G1804125607 dc 180605 > > Since the read mechanism of I2C is to write the offset and read command > first, and then read the target address. I think it's possible that the different > offsets be written at the same time, from Linux and firmware. O.K, that is bad. The SFP is totally unreliable... You however have still not answered my question. What is the firmware accessing? How does it handle pages? The hack you have put in place is per i2c transaction. But accessing pages is likely to be multiple transactions. One to change the page, followed by a few reads/writes in the new page, then maybe followed by a transactions to return to page 0. I think your best solution is to simply take the mutex and never release it. Block your firmware from accessing the SFP. Andrew
On Tue, Aug 27, 2024 8:19 PM, Andrew Lunn wrote: > > > More details please. > > > > > > Linux assume it is driving the hardware. Your firmware cannot be > > > touching any registers which will clear on read. QSFP states that > > > registers 3-31 of page 0 are all clear on read, for example. The > > > firmware should also not be setting any registers, otherwise you can > > > confuse Linux which assumes registers it set stay set, because it is > > > controlling the hardware. > > > > > > Your firmware also needs to handle that Linux can change the page. If > > > the firmware changes the page, it must restore it back to whatever > > > page Linux selected, etc. > > > > > > The fact you are submitting this for net suggests you have seen real > > > issues. Please describe what those issues are. > > > > The error log shows: > > > > [257681.367827] sfp sfp.1025: Host maximum power 1.0W > > [257681.370813] txgbe 0000:04:00.1: 31.504 Gb/s available PCIe bandwidth, limited by 8.0 GT/s PCIe x4 link at 0000:02:02.0 (capable > > of 63.008 Gb/s with 8.0 GT/s PCIe x8 link) > > [257681.373364] txgbe 0000:04:00.1 enp4s0f1: renamed from eth0 > > [257681.434719] txgbe 0000:04:00.1 enp4s0f1: configuring for inband/10gbase-r link mode > > [257681.676747] sfp sfp.1025: EEPROM base structure checksum failure: 0x63 != 0x1f > > [257681.676755] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00 ............g... > > [257681.676757] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 64 ....FiberStore d > > [257681.676759] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53 ...!SFP-10GS > > [257681.676760] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f R-85 A .R.. > > [257681.676762] sfp EE: 00000040: 00 81 cd 5b df 25 0a bd 40 f6 c6 ce 47 8e ff ff ...[.%..@...G... > > [257681.676763] sfp EE: 00000050: 10 d8 24 33 44 8e ff ff 10 41 b0 9a ff ff ff ff ..$3D....A...... > > > > It looks like some fields are read incorrectly. For comparison, I printed the > > SFP info when it loaded correctly: > > > > [260908.194533] sfp EE: 00000000: 03 04 07 10 00 00 01 00 00 00 00 06 67 02 00 00 ............g... > > [260908.194536] sfp EE: 00000010: 1e 0f 00 00 46 69 62 65 72 53 74 6f 72 65 20 20 ....FiberStore > > [260908.194538] sfp EE: 00000020: 20 20 20 20 00 00 1b 21 53 46 50 2d 31 30 47 53 ...!SFP-10GS > > [260908.194540] sfp EE: 00000030: 52 2d 38 35 20 20 20 20 41 20 20 20 03 52 00 1f R-85 A .R.. > > [260908.194541] sfp EE: 00000040: 40 63 bd df 40 8e ff ff 10 41 b0 9a ff ff ff ff @c..@....A...... > > [260908.194543] sfp EE: 00000050: 10 58 5b 29 41 8e ff ff 10 41 b0 9a ff ff ff ff .X[)A....A...... > > [260908.198205] sfp sfp.1025: module FiberStore SFP-10GSR-85 rev A sn G1804125607 dc 180605 > > > > Since the read mechanism of I2C is to write the offset and read command > > first, and then read the target address. I think it's possible that the different > > offsets be written at the same time, from Linux and firmware. > > O.K, that is bad. The SFP is totally unreliable... > > You however have still not answered my question. What is the firmware > accessing? How does it handle pages? > > The hack you have put in place is per i2c transaction. But accessing > pages is likely to be multiple transactions. One to change the page, > followed by a few reads/writes in the new page, then maybe followed by > a transactions to return to page 0. Do you mean the bus address A0 or A2? Firmware accesses I2C just like driver, but it only change the page once per full transaction, during a possession of the semaphore. What you fear seems unlikely to happen. > I think your best solution is to simply take the mutex and never > release it. Block your firmware from accessing the SFP. Firmware accesses the SFP in order to provide information to the BMC. So it cannot simply be blocked.
> > O.K, that is bad. The SFP is totally unreliable... > > > > You however have still not answered my question. What is the firmware > > accessing? How does it handle pages? > > > > The hack you have put in place is per i2c transaction. But accessing > > pages is likely to be multiple transactions. One to change the page, > > followed by a few reads/writes in the new page, then maybe followed by > > a transactions to return to page 0. > > Do you mean the bus address A0 or A2? Firmware accesses I2C just like driver, > but it only change the page once per full transaction, during a possession of > the semaphore. What you fear seems unlikely to happen. What sort of SFP is this? QSFP byte 127 selects the page for addresses 128-255. Paged 0 and 3 are mandatory, pages 1 and 2 are optional. SFP+ also uses byte 127 in the same way: 10.3 Optional Page Select Byte [Address A2h, Byte 127] In order to provide memory space for DWDM and CDR control functions and for other potential extensions, multiple Pages can be defined for the upper half of the A2h address space. At startup the value of byte 127 defaults to 00h, which points to the User EEPROM. This ensures backward compatibility for transceivers that do not implement the optional Page structure. When a Page value is written to byte 127, subsequent reads and writes to bytes 128-255 are made to the relevant Page. This specification defines functions in Pages 00h-02h. Pages 03-7Fh are reserved for future use. Writing the value of a non-supported Page shall not be accepted by the transceiver. The Page Select byte shall revert to 0 and read / write operations shall be to the unpaged A2h memory map. ethtool allows you to access more than page 0. ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off] [hex on|off] [offset N] [length N] [page N] [bank N] [i2c N] > > I think your best solution is to simply take the mutex and never > > release it. Block your firmware from accessing the SFP. > > Firmware accesses the SFP in order to provide information to the BMC. > So it cannot simply be blocked. Then you have a design problem. And i don't think locking the I2C bus per transaction is sufficient. Andrew
On Thu, Aug 29, 2024 11:28 PM, Andrew Lunn wrote: > > > O.K, that is bad. The SFP is totally unreliable... > > > > > > You however have still not answered my question. What is the firmware > > > accessing? How does it handle pages? > > > > > > The hack you have put in place is per i2c transaction. But accessing > > > pages is likely to be multiple transactions. One to change the page, > > > followed by a few reads/writes in the new page, then maybe followed by > > > a transactions to return to page 0. > > > > Do you mean the bus address A0 or A2? Firmware accesses I2C just like driver, > > but it only change the page once per full transaction, during a possession of > > the semaphore. What you fear seems unlikely to happen. > > What sort of SFP is this? QSFP byte 127 selects the page for addresses > 128-255. Paged 0 and 3 are mandatory, pages 1 and 2 are optional. > > SFP+ also uses byte 127 in the same way: > > 10.3 Optional Page Select Byte [Address A2h, Byte 127] > > In order to provide memory space for DWDM and CDR control functions > and for other potential extensions, multiple Pages can be defined for > the upper half of the A2h address space. At startup the value of byte > 127 defaults to 00h, which points to the User EEPROM. This ensures > backward compatibility for transceivers that do not implement the > optional Page structure. When a Page value is written to byte 127, > subsequent reads and writes to bytes 128-255 are made to the relevant > Page. > > This specification defines functions in Pages 00h-02h. Pages 03-7Fh > are reserved for future use. Writing the value of a non-supported Page > shall not be accepted by the transceiver. The Page Select byte shall > revert to 0 and read / write operations shall be to the unpaged A2h > memory map. > > ethtool allows you to access more than page 0. > > ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off] > [hex on|off] [offset N] [length N] [page N] [bank N] [i2c N] > > > > I think your best solution is to simply take the mutex and never > > > release it. Block your firmware from accessing the SFP. > > > > Firmware accesses the SFP in order to provide information to the BMC. > > So it cannot simply be blocked. > > Then you have a design problem. And i don't think locking the I2C bus > per transaction is sufficient. SFP+ is used on our device. But I don't quite understand why this lock is not sufficient. The entire transaction is locked, include setting the bus address and selecting pages, and all subsequent reads and writes on this page. Also, firmware uses this lock (hardware semaphore) in the same way. Neither driver nor firmware switches pages whiling the other is reading / writing ?
> SFP+ is used on our device. > > But I don't quite understand why this lock is not sufficient. The entire > transaction is locked, include setting the bus address and selecting pages, > and all subsequent reads and writes on this page. Also, firmware uses this > lock (hardware semaphore) in the same way. Neither driver nor firmware > switches pages whiling the other is reading / writing ? The standard say you cannot read past a 1/2 page boundary. So you do one i2c transaction to write to byte 127. You then do transactions to write/read in the range 128-255. And then you do a transaction to write to byte 127 to set the page back, maybe. You would need to hold the lock over all these transactions. The i2c bus driver is too low for this, it would need to be done in the SFP layer. The firmware would also need to do the same, hold the lock over all the transactions, not just one. And i said 'maybe'. It could leave the page select byte on something other than page 0. Linux is driving the hardware... It might know it is going to use the same page sometime later. So the firmware will need to take the lock, read byte 127, change it to whatever it needs, do its read/writes, and then restore the page value at 127, and then release the lock.. Also, you have to think about the quirks. Cut/paste from sfp.c: /* SFP_EEPROM_BLOCK_SIZE is the size of data chunk to read the EEPROM * at a time. Some SFP modules and also some Linux I2C drivers do not like * reads longer than 16 bytes. */ #define SFP_EEPROM_BLOCK_SIZE 16 /* 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. */ /* 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. */ How easy it is to update your firmware each time we add a quirk? There is no point Linux working around all the broken SFPs if your firmware then breaks it by not doing word reads when it should, reading 128 bytes not 16, not falling back to byte reads and disabling your equivalent of HWMON for the sensors. Andrew