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
