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

[Andrew Lunn <andrew@lunn.ch>]

> > > + * struct ethtool_eeprom_data - EEPROM dump from specified page
> > > + * @offset: Offset within the specified EEPROM page to begin read, in bytes.
> > > + * @length: Number of bytes to read. On successful return, number of bytes
> > > + *	actually 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. Zero indicates a driver should choose
> > > + *	by itself.
> > I don't particularly like the idea of the driver deciding what to
> > read. User space should really be passing 0x50 or 0x51 for the normal
> > case. And we need to make it clear what these addresses mean, since
> > they are often referred to as 0xA0 and 0xA2, due to addresses being
> > shifted one bit left and a r/w bit added.
> 
> I was thinking what address should I send from userspace by default?
> Should I send 0x50 or 0xA0 and at some point to do the shift?
> mlx5 uses 0x50 and 0x51, for example.

I would use the same meaning of i2c address as the i2c subsystem. So
addresses in the range of 0 to 0x7f. And please add a comment about
this here.

> > I also don't think the in place length should be modified. It would be
> > better to follow the use semantics of returning a negative value on
> > error, or a positive value for the length actually
> > read. ethtool_eeprom_data can then be passed as a const.
> But how would userspace know how much actually was read?

You are going to put the data into a netlink attribute. That attribute
has a length. So userspace gets to know that way.

> Should I fail the command if only part of a data requested was read?

That is not the usual POSIX semantics. I would return whatever is
available.

> > > @@ -1865,6 +1888,8 @@ static inline int ethtool_validate_duplex(__u8 duplex)
> > >   #define ETH_MODULE_SFF_8636_MAX_LEN     640
> > >   #define ETH_MODULE_SFF_8436_MAX_LEN     640
> > > +#define ETH_MODULE_EEPROM_MAX_LEN	640
> > I'm surprised such a high value is allowed. 128 seems more
> > appropriate, given the size of 1/2 pages.
> 
> This is done for backwards compatibility mechanism (last patch in the
> series)
> to work properly. I wanted to limit the length to 128 (or 256, maybe), but
> currently ethtool supports dumps of 640 bytes at most.
> I guess I can add another value like ETH_MODULE_EEPROM_PAGE_MAX_LEN 256 and
> if
> new ndo is available or page number is specified, use it for check.
> If ndo is not implemented, use 640 instead.

I need to look at the last patch. But for backwards compatibility with
the old IOCTL call, we might want the glue code to read each 1/2 page
individually, and combine them. That way we keep the new API uniform,
reads only 1/2 pages.

      Andrew