Re: [PATCH net-next v2 0/2] Add support for DSFP transceiver type

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Jan 04, 2021 at 05:24:11PM +0200, Vladyslav Tarasiuk wrote:
> 
> On 30-Dec-20 17:36, Andrew Lunn wrote:
> > On Wed, Dec 30, 2020 at 03:55:02PM +0200, Vladyslav Tarasiuk wrote:
> > > On 29-Dec-20 18:25, Andrew Lunn wrote:
> > > > > Hi Andrew,
> > > > > 
> > > > > Following this conversation, I wrote some pseudocode checking if I'm on
> > > > > right path here.
> > > > > Please review:
> > > > > 
> > > > > struct eeprom_page {
> > > > >           u8 page_number;
> > > > >           u8 bank_number;
> > > > >           u16 offset;
> > > > >           u16 data_length;
> > > > >           u8 *data;
> > > > > }
> > > > I'm wondering about offset and data_length, in this context. I would
> > > > expect you always ask the kernel for the full page, not part of
> > > > it. Even when user space asks for just part of a page. That keeps you
> > > > cache management simpler.
> > > As far as I know, there may be bytes, which may change on read.
> > > For example, clear on read values in CMIS 4.0.
> > Ah, i did not know there were such bits. I will go read the spec. But
> > it should not really matter. If the SFP driver is interested in these
> > bits, it will have to intercept the read and act on the values.
> 
> But in case user requests a few bytes from a page with clear-on-read
> values, reading full page will clear all such bytesfrom user perspective
> even if they were not requested. Driver may intercept the read, but for
> user it will look like those bytes were not set.

Yes, O.K. Reading individual words does make sense.

> Without command line argument user will not be able to request a single
> A2h page, for example. He will see it only in some kind of general dump -
> with human-readable decoder usage or multiple page dump.
> 
> And same goes forpages on other i2c addresses. How to know what to dump,
> if user does not provide i2c address and there is no way to know what to
> request from proprietary SFPs?

So we should look at this from the perspective of use cases. Currently
we have:

ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off] [hex on|off] [offset N] [length N]

If you use it without any of [raw on|off] [hex on|off] [offset N]
[length N] it decodes what it finds. As soon as you pass any of these
options, the decoding is disabled and is just dumps values, either raw
or hex.

I would say, i2c address as a parameter can be added, but again,
passing it disables decoding, is just dumps raw or hex.

When not passed, and decoding is enabled, the decoder should decide if
A2 is available based on what is finds in page 0, and ask for it.

We also need to clearly define what offset and length means in this
context. It has to be within a specific page if page, bank or i2c
address has been passed, unlike what it currently means which is
offset into the current blob returned by the kernel.

I also took a look at CMIS. It has interesting semantics for address
wrap around when doing multiple byte reads. A read which reaches 127
wraps around to 0. A read which reached 255 wraps around to 128. So
for the kernel API, we probably do not want to allow offset/length to
cause a wrap around. You can only read within the low 128 bytes, or
the upper 128 bytes.

   Andrew