Re: [PATCH 1/4] net: sfp: add workaround for Realtek RTL8672 and RTL9601C chips

[Pali Rohár]

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.