Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20230925-ixp4xx-eth-mtu-v2-1-393caab75cb0@linaro.org> <b109470b-99d1-441d-0648-7b8e4a8c86fd@intel.com>
In-Reply-To: <b109470b-99d1-441d-0648-7b8e4a8c86fd@intel.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Tue, 26 Sep 2023 08:44:20 +0200
Message-ID: <CACRpkdbcMCXg8H8SsuhPSvvtdqD1reNxpCiFv5eD=gPYKUyr9A@mail.gmail.com>
Subject: Re: [PATCH net-next v2] net: ixp4xx_eth: Support changing the MTU
To:     Jacob Keller <jacob.e.keller@intel.com>
Cc:     Krzysztof Halasa <khalasa@piap.pl>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Dan Williams <dan.j.williams@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Sep 26, 2023 at 12:29=E2=80=AFAM Jacob Keller <jacob.e.keller@intel=
.com> wrote:

> > +/* MRU is said to be 14320 in a code dump, the SW manual says that
> > + * MRU/MTU is 16320 and includes VLAN and ethernet headers.
> > + * See "IXP400 Software Programmer's Guide" section 10.3.2, page 161.
> > + *
> > + * FIXME: we have chosen the safe default (14320) but if you can test
> > + * jumboframes, experiment with 16320 and see what happens!
> > + */
>
> Ok, so you're choosing a conservative upper limit that is known to work
> while leaving the higher 16320 value for later if/when someone cares?

Mostly if someone can test it. But maybe I can have authoritative
information from Intel that the statement in the Software Programmers
Guide is correct? ;)

> > +static int ixp4xx_do_change_mtu(struct net_device *dev, int new_mtu)
> > +{
> > +     struct port *port =3D netdev_priv(dev);
> > +     struct npe *npe =3D port->npe;
> > +     struct msg msg;
> > +     /* adjust for ethernet headers */
> > +     int framesize =3D new_mtu + VLAN_ETH_HLEN;
> > +     /* max rx/tx 64 byte chunks */
> > +     int chunks =3D DIV_ROUND_UP(framesize, 64);
> > +
>
> netdev coding style wants all of the declarations in "reverse christmas
> tree" ordering. Assign to the local variables after the block if
> necessary. Something like:
>
>         struct port *port =3D netdev_priv(dev);
>         struct npe *npe =3D port->npe;
>         int framesize, chunks;
>         struct msg msg;
>
>         /* adjust for ethernet headers */
>         framesize =3D new_mtu + VLAN_ETH_HLEN;
>         /* max rx/tx 64 byte chunks */
>         chunks =3D DIV_ROUND_UP(framesize, 64);

Right, I fix!

> > +     memset(&msg, 0, sizeof(msg));
>
> You could also use "struct msg msg =3D {}" instead of memset here.

OK

> > +     msg.cmd =3D NPE_SETMAXFRAMELENGTHS;
> > +     msg.eth_id =3D port->id;
> > +
> > +     /* Firmware wants to know buffer size in 64 byte chunks */
> > +     msg.byte2 =3D chunks << 8;
> > +     msg.byte3 =3D chunks << 8;
>
> I am not sure I follow the "<< 8" here.

I actually only have this vendor code, but clearly <<8 is not
"multiply by 256" in this case, rather that the number of 64 byte
chunks is in the second byte.

The software manual just describes a "OS abstraction layer"
used by both VXworks and Linux, and since that wasn't acceptable
in the Linux driver, someone has ripped out the code to
talk directly to the NPE firmware, and that is what we are seeing.
If you have the source code to the abstraction layer
"ixEthAcc" or the source code to the NPE microcode, I think the
answer is in there... (I have neither, maybe you can check internally,
hehe. Dan Williams used to work with this hardware!)

> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks!

> > base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
> > change-id: 20230923-ixp4xx-eth-mtu-c041d7efe932
>
> Curious what this change-id thing represents I've never seen it before..
> I know base-commit is used by git. Would be interested in an explanation
> if you happen to know! :D

It's metadata generated by b4 which is the tool we use for kernel mailing
list handling:
https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-=
1

I think this change ID cross-references mails I send with my
git branch, so it's easy to collect review tags etc.

Yours,
Linus Walleij