Re: [RFC PATCH V2 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data

[Andrew Lunn <andrew@lunn.ch>]

> > + * @length: Number of bytes to read.
> > + * @page: Page number to read from.
> > + * @bank: Page bank number to read from, if applicable by EEPROM spec.
> > + * @i2c_address: I2C address of a page. Value less than 0x7f expected.
> > Most
> > + *	EEPROMs use 0x50 or 0x51.
> 
> The standards are all very clear

Our experience so far is that manufactures of SFP modules like to
ignore the standard. And none of the standards seem to cover copper
modules, which have additional registers at some other page.
Admittedly, they cannot be mapped as pages, you need some proprietary
protocol to map MDIO onto I2C. But i would not be surprised to find
some SFP that maps the FLASH of the microcontroller onto an address,
which we might be able to read out using this API.

So i suggested we keep it generic, allowing access to these
proprietary registers at other addresses. And if there is nothing
there, you probably get a 1/2 page of 0xff.

> I suggest that 0xA0 and 0xA2 also be silently accepted, and
> translated to 0x50 and 0x51 respectively.

No, i don't like having two different values mean the same thing.  It
just leads to confusion. And userspace is going to be confused when it
asks for 0xA0 but the reply says it is for 0x50.

The Linux I2C subsystem does not magically map 8bit addresses in 7bit
addresses. We should follow what the Linux I2C subsystem does.

> > +
> > +	request->offset =
> > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_OFFSET]);
> > +	request->length =
> > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_LENGTH]);
> > +	if (request->length > ETH_MODULE_EEPROM_MAX_LEN)
> > +		return -EINVAL;	
> 
> This is really problematic as there are MANY different max values, within
> the specs

I agree. We should only be returning one 1/2 page as a maximum. So it
should be limited to 128 bytes. And offset+length should not go beyond
the end of a 1/2 page.

> > +	if (tb[ETHTOOL_A_EEPROM_DATA_PAGE])
> > +		request->page =
> > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_PAGE]);
> > +	if (tb[ETHTOOL_A_EEPROM_DATA_BANK])
> > +		request->bank =
> > nla_get_u32(tb[ETHTOOL_A_EEPROM_DATA_BANK]);
> 
> Other checks:
> 
> Page and bank have to be between 0 and 255 (inclusive), they
> go into an 8 bit register in the eeprom.

Yes, a u8 would be a better type here.

     Andrew