Re: [RFC PATCH net-next 1/9] ethtool: Add ethtool operation to write to a transceiver module EEPROM

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Jan 22, 2024 at 10:45:22AM +0200, Danielle Ratson wrote:
>  /**
> - * struct ethtool_module_eeprom - EEPROM dump from specified page
> - * @offset: Offset within the specified EEPROM page to begin read, in bytes.
> - * @length: Number of bytes to read.
> - * @page: Page number to read from.
> - * @bank: Page bank number to read from, if applicable by EEPROM spec.
> + * struct ethtool_module_eeprom - plug-in module EEPROM read / write parameters
> + * @offset: Offset within the specified page, in bytes.
> + * @length: Number of bytes to read / write.
> + * @page: Page number.
> + * @bank: Bank number, if supported by EEPROM spec.

I suppose I should have reviewed the addition of this (I can't recall
whether I got the original or not.)

If one looks at SFF-8472, then the first 128 bytes of the EEPROM at
0x50 (0xA0 on the wire) are not paged. Whereas bytes 128..255 are the
paged bytes. Therefore, "offset within the specified page" can sensibly
be interpreted as referring to the EEPROM at 0x50, at an offset of
128 + offset.

Meanwhile, the actual implementation doesn't do that - the offset is
the offset from the beginning of the EEPROM, and offsets >= 128 access
the paged area.

What this means is that the parameter description here is basically
wrong, both before and after your change.

This really ought to be fixed so that we describe things correctly
rather than misleading people who read documentation. Otherwise, it's
a recipe for broken implementations... and it's also completely
pointless documenting it if the documentation is wrong.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!