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

From: Moshe Shemesh <moshe@nvidia.com>
To: Don Bollinger <don@thebollingers.org>, 'Andrew Lunn' <andrew@lunn.ch>
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:14:43 +0200	[thread overview]
Message-ID: <811a3af6-84f7-6f44-68c3-eba8be06e3f4@nvidia.com> (raw)
In-Reply-To: <008901d7200c$8a59db40$9f0d91c0$@thebollingers.org>

On 3/23/2021 7:47 PM, Don Bollinger wrote:
>>>> 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.
>>> I like the clean approach, but...   How do you request low memory?
>> Duh!
>>
>> I got my conditions wrong. Too focused on 1/2 pages to think that two of
>> them makes one page!
>>
>> Lets try again:
>>
>> offset < 256
>> 0 < len < 128
> Actually 0 < len <= 128.  Length of 128 is not only legal, but very common.
> "Read the whole 1/2 page block".
Ack.
>> if (offset < 128)
>>     offset + len < 128
> Again, offset + len <= 128
>
>> else
>>     offset + len < 256
> offset + len <= 256
Ack.
>> Does that look better?
>>
>> Reading bytes from the lower 1/2 of page 0 should give the same data as
>> reading data from the lower 1/2 of page 42. So we can allow that, but
> don't
>> be too surprised when an SFP gets it wrong and gives you rubbish. I would
> The spec is clear that the lower half is the same for all pages.  If the SFP
> gives you rubbish you should throw the device in the rubbish.
>
>> suggest ethtool(1) never actually does read from the lower 1/2 of any page
>> other than 0.
> I agree, despite my previous comment.  While the spec is clear that should
> work, I believe virtually all such instances are bugs not yet discovered.

Agreed, so we will accept offset < 128 only on page zero.

> And, note that the legacy API provides no way to access lower memory from
> any page but 0.  There's just no syntax for it.  Not that we care about
> legacy :-).
>
>> And i agree about documentation. I would suggest a comment in
>> ethtool_netlink.h, and the RST documentation.

Ack, will comment there on limiting new KAPI only to half pages reading.

We may address reading cross pages by user space (implementing multiple 
calls to KAPI to get such data).

>>                   Andrew
> Don
>
>