Message-ID: <CAL_jBfSNsveJSgtZ+5wPEOM+wQZPbTysj20jgttkgSf+E-0Pbg@mail.gmail.com>
Date:   Sun, 28 Jun 2020 13:27:21 +0100
From:   Adrian Pop <popadrian1996@...il.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, jiri@...lanox.com,
        vadimp@...lanox.com, mlxsw@...lanox.com,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 1/2] mlxsw: core: Add ethtool support for QSFP-DD transceivers

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 <idosch@...sch.org> wrote:
>
> On Sat, Jun 27, 2020 at 09:42:10PM +0100, Adrian Pop wrote:
> > >
> > > 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.
> >
> > > 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

Powered by blists - more mailing lists