linux-kernel - Re: [PATCH net-next v2] net: ixp4xx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5ee77d8e-c5f1-cf53-268c-dc74fd3e37b6@intel.com>
Date:   Tue, 26 Sep 2023 14:42:11 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     Krzysztof Halasa <khalasa@...p.pl>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH net-next v2] net: ixp4xx_eth: Support changing the MTU



On 9/25/2023 11:44 PM, Linus Walleij wrote:
> On Tue, Sep 26, 2023 at 12:29 AM Jacob Keller <jacob.e.keller@...el.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? ;)
> 

Unfortunately I'm one of the folks who worked on this driver/hardware,
so I can't comment.

>>> +static int ixp4xx_do_change_mtu(struct net_device *dev, int new_mtu)
>>> +{
>>> +     struct port *port = netdev_priv(dev);
>>> +     struct npe *npe = port->npe;
>>> +     struct msg msg;
>>> +     /* adjust for ethernet headers */
>>> +     int framesize = new_mtu + VLAN_ETH_HLEN;
>>> +     /* max rx/tx 64 byte chunks */
>>> +     int chunks = 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 = netdev_priv(dev);
>>         struct npe *npe = port->npe;
>>         int framesize, chunks;
>>         struct msg msg;
>>
>>         /* adjust for ethernet headers */
>>         framesize = new_mtu + VLAN_ETH_HLEN;
>>         /* max rx/tx 64 byte chunks */
>>         chunks = DIV_ROUND_UP(framesize, 64);
> 
> Right, I fix!
> 
>>> +     memset(&msg, 0, sizeof(msg));
>>
>> You could also use "struct msg msg = {}" instead of memset here.
> 
> OK
> 
>>> +     msg.cmd = NPE_SETMAXFRAMELENGTHS;
>>> +     msg.eth_id = port->id;
>>> +
>>> +     /* Firmware wants to know buffer size in 64 byte chunks */
>>> +     msg.byte2 = chunks << 8;
>>> +     msg.byte3 = 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!)
> 

Yea. We could use FIELD_PREP and some definitions, but I think its ok as-is.

>> Reviewed-by: Jacob Keller <jacob.e.keller@...el.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.
> 

Neat! thanks for explaining :) b4 seems quite excellent :D

> Yours,
> Linus Walleij