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

From: Moshe Shemesh <moshe@nvidia.com>
To: Andrew Lunn <andrew@lunn.ch>, Don Bollinger <don@thebollingers.org>
Cc: "'David S. Miller'" <davem@davemloft.net>,
	'Jakub Kicinski' <kuba@kernel.org>,
	'Adrian Pop' <pop.adrian61@gmail.com>,
	'Michal Kubecek' <mkubecek@suse.cz>, <netdev@vger.kernel.org>,
	'Vladyslav Tarasiuk' <vladyslavt@nvidia.com>
Subject: Re: [RFC PATCH V4 net-next 1/5] ethtool: Allow network drivers to dump arbitrary EEPROM data
Date: Wed, 24 Mar 2021 12:03:32 +0200	[thread overview]
Message-ID: <3e78a1cd-e63d-142a-3b78-511238e48bef@nvidia.com> (raw)
In-Reply-To: <YFk13y19yMC0rr04@lunn.ch>

On 3/23/2021 2:27 AM, Andrew Lunn wrote:
>
>>> +#define ETH_MODULE_EEPROM_PAGE_LEN 256
>> Sorry to keep raising issues, but I think you want to make this constant
>> 128.
> Yes, i also think the KAPI should be limited to returning a maximum of
> a 1/2 page per call.
OK.
>>> +#define MODULE_EEPROM_MAX_OFFSET (257 *
>>> ETH_MODULE_EEPROM_PAGE_LEN)
>> The device actually has 257 addressable chunks of 128 bytes each.  With
>> ETH_MODULE_EEPROM_PAGE_LEN set to 256, your constant is 2X too big.
>>
>> Note also, SFP devices (but not QSFP or CMIS) actually have another 256
>> bytes available at 0x50, in addition to the full 257*128 at 0x51.  So SFP is
>> actually 259*128 or (256 + 257 * 128).
>>
>> Devices that don't support pages have much lower limits (256 bytes for
>> QSFP/CMIS and 512 for SFP).  Some SFP only support 256 bytes.  Most devices
>> will return nonsense for pages above 3.  So, this check is really only an
>> absolute limit.  The SFP driver that takes this request will probably check
>> against a more refined MAX length (eg modinfo->eeprom_len).
>>
>> I suggest setting this constant to 259 * (ETH_MODULE_EEPROM_PAGE_LEN / 2).
>> Let the driver refine it from there.
> I don't even see a need for this. The offset should be within one 1/2
> page, of one bank. So offset >= 0 and <= 127. Length is also > 0 and
> <- 127. And offset+length is <= 127.
>
> For the moment, please forget about backwards compatibility with the
> IOCTL interface. Lets get a new clean KAPI and a new clean internal
> API between the ethtool core and the drivers. Once we have that agreed
> on, we can work on the various compatibility shims we need to work
> between old and new APIs in various places.

OK, so following the comments here, I will ignore backward compatibility 
of having global offset and length.

That means I assume in this KAPI I am asked to get data from specific 
page. Should I also require user space to send page number to this KAPI 
(I mean make page number mandatory) ?

>        Andrew