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

quoted

quoted

+ * 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.

quoted

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.

quoted

quoted

@@ -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