Message-ID: <6a9bbcb0-c0c4-92fe-f3c1-581408d1e7da@nvidia.com>
Date:   Fri, 27 Nov 2020 17:12:37 +0200
From:   Moshe Shemesh <moshe@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Jakub Kicinski <kuba@...nel.org>,
        Moshe Shemesh <moshe@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Adrian Pop <pop.adrian61@...il.com>,
        Michal Kubecek <mkubecek@...e.cz>, <netdev@...r.kernel.org>,
        "Vladyslav Tarasiuk" <vladyslavt@...dia.com>,
        Maxim Mikityanskiy <maximmi@...dia.com>
Subject: Re: [PATCH net-next v2 0/2] Add support for DSFP transceiver type


On 11/26/2020 5:21 PM, Andrew Lunn wrote:
>>> If i was implementing the ethtool side of it, i would probably do some
>>> sort of caching system. We know page 0 should always exist, so
>>> pre-load that into the cache. Try the netlink API first. If that
>>> fails, use the ioctl interface. If the ioctl is used, put everything
>>> returned into the cache.
>> I am not sure what you mean by cache here. Don't you want to read page 0
>> once you got the ethtool command to read from the module ? If not, then at
>> what stage ?
> At the beginning, you try the netlink API and ask for pager 0, bytes
> 0-127. If you get a page, put it into the cache. If not, use the ioctl
> interface, which could return one page, or multiple pages. Put them
> all into the cache.


OK, but if the caching system is checking one time netlink and one time 
ioctl, it means this cache should be in user space, or did you mean to 
have this cache in kernel ?

>>>    The decoder can then start decoding, see what
>>> bits are set indicating other pages should be available. Ask for them
>>> from the cache. The netlink API can go fetch them and load them into
>>> the cache. If they cannot be loaded return ENODEV, and the decoder has
>>> to skip what it wanted to decode.
>> So the decoder should read page 0 and check according to page 0 and
>> specification which pages should be present, right ?
> Yes. It ask the cache, give me a pointer to page 0, bytes 0-127. It
> then decodes that, looking at the enumeration data to indicate what
> other pages should be available. Maybe it decides, page 0, bytes
> 128-255 should exist, so it asks the cache for a pointer to that. If
> using netlink, it would ask the kernel for that data, put it into the
> cache, and return a pointer. If using ioctl, it already knows if it
> has that data, so it just returns a pointer, so says sorry, not
> available.
>
>> What about the global offset that we currently got when user doesn't specify
>> a page, do you mean that this global offset goes through the optional and
>> non optional pages that exist and skip the ones that are missing according
>> to the specific EEPROM ?
> ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off] [hex on|off] [offset N] [length N]
>
> So you mean [offset N] [length N].


Yes, that's the current options and we can either try coding new 
implementation for that or just call the current ioctl implementation. 
The new code can be triggered once options [bank N] and [Page N] are used.

>
> That is going to be hard, but the API is broken for complex SFPs with
> optional pages. And it is not well defined exactly what offset means.
> You can keep backwards compatibility by identifying the SFP from page
> 0, and then reading the pages in the order the ioctl would do. Let
> user space handle it, for those SFPs which the kernel already
> supports. For SFPs which the kernel does not support, i would just
> return not supported. You can do the same for raw. However, for new
> SFPs, for raw you can run the decoder but output to /dev/null. That
> loads into the cache all the pages which the decoder knows about. You
> can then dump the cache. You probably need a new format, to give an
> indication of what each page actually is.
OK, if I got it right on current API [offset N] [length N] just call 
ioctl current implementation, while using the option [raw on] will call 
new implementation for new SFPs (CMIS 4). Also using [bank N] and [page 
N] will call new implementation for new SFPs.
> Maybe you want to add new options [page N] [ bank N] to allow
> arbitrary queries to be made?
Exactly what I meant, I actually thought of letting the user ask for the 
page he wants, he should know what he needs.
> Again, you can answer these from the
> cache, so the old ioctl interface could work if asked for pages which
> the old API had.
Yes, for the simple EEPROM types that have 1 or 4 pages, ioctl read 
should be enough to get the data.
>
>      Andrew

Powered by blists - more mailing lists