Hi!

Regarding multiple banks, I think that we can have a much more
creative way of dealing with them (in the future). At page 76, we have
"In particular, support of the Lower Memory and of Page 00h is
required for all modules, including passive copper cables. These pages
are therefore always implemented. Additional support for Pages 01h,
02h and bank 0 of Pages 10h and 11h is required for all paged memory
modules."

My old patch clearly supports only the 1st (and mandatory) bank. So
the memory layout is 00h, 01h, 02h, 10h, 11h. If we will extend
ethtool to work for multiple banks, we might have something like 00h,
01h, 02h, (10h, 11h | bank 0), (10h, 11h | bank 1) etc. So we can
still check bits 1-0 of byte 142 from page 01h to determine how many
banks we have and we can still follow the "we can trim, but only to
the right" rule. Of course, this is only an idea, at the moment I
don't think we can even test something like that.

I'm sure that I can work something out to deal with sometimes having
page 03h and sometimes not, but first I think we need to decide if we
always add it or not. As I mentioned in my previous email, I think
Andrew can argue for its presence better than me. Based on that, I can
re-submit my old patch for ethtool.

Adrian

On Sun, 28 Jun 2020 at 12:56, Ido Schimmel [off-list ref] wrote:

On Sat, Jun 27, 2020 at 09:42:10PM +0100, Adrian Pop wrote:

quoted

quoted

Hi Adrian, Andrew,

Not sure I understand... You want the kernel to always pass page 03h to
user space (potentially zeroed)? Page 03h is not mandatory according to
the standard and page 01h contains information if page 03h is present or

Hi Ido!

Andrew was thinking of having 03h after 02h (potentially zeroed) just
for the purpose of having a similar layout for QSFP-DD the same way we
do for QSFP. But as you said, it is not mandatory according to the
standard and I also don't know the entire codebase for ethtool and
where it might be actually needed. I think Andrew can argue for its
presence better than me.

quoted

not. So user space has the information it needs to determine if after
page 02h we have page 03h or page 10h. Why always pass page 03h then?

If we decide to add 03h but only sometimes, I think we will add an
extra layer of complexity. Sometimes after 02h we would have 03h and
sometimes 10h. In qsfp-dd.h (following the convention from qsfp.h) in
my patch there are a lot of different constants defined with respect
to the offset of the parent page in the memory layout and "dynamic
offsets" don't sound very good, at least for me. So even if there's a
way of checking in the user space which page is after 02h, a more
stable memory layout works better on the long run.

Adrian,

Thanks for the detailed response. I don't think the kernel should pass
fake pages only to make it easier for user space to parse the
information. What you are describing is basic dissection and it's done
all the time by wireshark / tcpdump.

Anyway, even we pass a fake page 03h, page 11h can still be at a
variable offset. See table 8-28 [1], bits 1-0 at offset 142 in page 01h
determine the size of pages 10h and 11h:

0 - each page is 128 bytes in size
1 - each page is 256 bytes in size
2 - each page is 512 bytes in size

So a completely stable layout (unless I missed something) will entail
the kernel sending 1664 bytes to user space each time. This looks
unnecessarily rigid to me. The people who wrote the standard obviously
took into account the fact that the page layout needs to be discoverable
from the data and I think we should embrace it and only pass valid
information to user space.

Regardless, can Andrew and you let us know if you have a problems with
current patch set which only exposes pages 00h-02h? I see it's marked as
"Changes Requested", so I will need to re-submit.

Thanks

[1] http://www.qsfp-dd.com/wp-content/uploads/2019/05/QSFP-DD-CMIS-rev4p0.pdf