netdev - Re: [PATCH net 0/3] Add I2C bus lock for Wangxun

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7485fc03-79c3-4797-ab9a-9026b09aac7f@lunn.ch>
Date: Tue, 3 Sep 2024 14:45:27 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: andi.shyti@...nel.org, jarkko.nikula@...ux.intel.com,
	andriy.shevchenko@...ux.intel.com, mika.westerberg@...ux.intel.com,
	jsd@...ihalf.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, rmk+kernel@...linux.org.uk,
	linux-i2c@...r.kernel.org, netdev@...r.kernel.org,
	mengyuanlou@...-swift.com, duanqiangwen@...-swift.com
Subject: Re: [PATCH net 0/3] Add I2C bus lock for Wangxun

> 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