Received-SPF: pass (google.com: domain of linux-kernel+bounces-100100-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240312094259.770554-1-e.velu@criteo.com> <fd37860e-306f-42bf-9a1d-e4d98ddf338c@molgen.mpg.de>
In-Reply-To: <fd37860e-306f-42bf-9a1d-e4d98ddf338c@molgen.mpg.de>
From: Erwan Velu <erwanaliasr1@gmail.com>
Date: Tue, 12 Mar 2024 11:18:17 +0100
Message-ID: <CAL2JzuzjRTHzKPVU9+zfFeEgfDQPeRDUuaUjsqgDfgQ865en8Q@mail.gmail.com>
Subject: Re: [Intel-wired-lan] [PATCH v3] i40e: Prevent setting MTU if greater
 than MFS
To: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Erwan Velu <e.velu@criteo.com>, linux-kernel@vger.kernel.org, 
	Eric Dumazet <edumazet@google.com>, netdev@vger.kernel.org, 
	Tony Nguyen <anthony.l.nguyen@intel.com>, intel-wired-lan@lists.osuosl.org, 
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	"David S. Miller" <davem@davemloft.net>
Content-Type: text/plain; charset="UTF-8"

> Am 12.03.24 um 10:42 schrieb Erwan Velu:
> > Commit 6871a7de705b6f6a4046f0d19da9bcd689c3bc8e from iPXE project is
> > setting the MFS to 0x600 = 1536.
>
> Please add a link, as most people do not have the iPXE archive checked
> out. Maybe also add the commit message summary.
I will, thanks.

> > At boot time the i40e driver complains about it with
> > the following message but continues.
> >
> >       MFS for port 1 has been set below the default: 600
> Hmm, but 1536 > 600. So the log message is incorrect?

As mentioned earlier in the commit message, the 600 is 0x600 = 1536.
I can offer a patch to report it in decimal or add an explicit 0x prefix.

> > If the MTU size is increased, the driver accept it but large packets will not
> accept*s*
Fixed.


[...]
> > At least, this commit prevents setting up an MTU greater than the current MFS.
> > It will avoid being in the position of having an MTU set to 9000 on the
> > netdev with a firmware refusing packets larger than 1536.
> Maybe add the new log message.
Done.

> One last formal nit: Please use a line length limit of 75 characters per
> line.
Done.

> > +     mfs = pf->hw.phy.link_info.max_frame_size;
> > +     max_mtu = mfs - I40E_PACKET_HDR_PAD;
> > +     if (new_mtu > max_mtu) {
> > +             netdev_err(netdev, "Error changing mtu to %d, Max is %d. MFS is too small.\n",
> > +                        new_mtu, max_mtu);
>
> The other log messages capitalize MTU.
Yeah but the exact previous one was in the same case. Shall I bump all
of them to upper or lower cast ?


> The rest looks reasonable.
Thx for the review.