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
