Re: [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS

[Sunil Kovvuri Goutham <sgoutham@marvell.com>]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Monday, March 18, 2024 11:15 PM
> To: Erwan Velu <erwanaliasr1@gmail.com>
> Cc: Erwan Velu <e.velu@criteo.com>; Jesse Brandeburg
> <jesse.brandeburg@intel.com>; Tony Nguyen
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>;
> Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH v4 iwl-net] i40e: Prevent setting MTU if
> greater than MFS
> 
> On Wed, Mar 13, 2024 at 10:07:16AM +0100, Erwan Velu wrote:
> > Commit 6871a7de705 ("[intelxl] Use admin queue to set port MAC
> address
> > and maximum frame size") from iPXE project set the MFS to 0x600 =
> 1536.
> > See
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_ipxe_i
> >
> pxe_commit_6871a7de705&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=q
> 3VKxXQKibo
> >
> Rw_F01ggTzHuhwawxR1P9_tMCN2FODU4&m=J_D0216agwnkMc8GuVR13
> qCCDBQWZmqULkA
> >
> mTpcwoDk9e1Yw0Z28r7RoYVuLyMex&s=s9O0JKM9qId17Qw5d2dH7c3wLJT
> Bz-frLsKFbz
> > jliE0&e=
> >
> > 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
> >
> > If the MTU size is increased, the driver accepts it but large packets
> > will not be processed by the firmware generating tx_errors. The issue
> > is pretty silent for users. i.e doing TCP in such context will
> > generates lots of retransmissions until the proper window size (below
> 1500) will be used.
> >
> > To fix this case, it would have been ideal to increase the MFS, via
> > i40e_aqc_opc_set_mac_config, incoming patch will take care of it.
> >
> > At least, 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.
> >
> > A typical trace looks like:
> > [  377.548696] i40e 0000:5d:00.0 eno5: Error changing mtu to 9000, Max
> is 1500. MFS is too small.
> >
> 
> Hi Erwan, all,
> 
> As a fix, I think this patch warrants a fixes tag.
> Perhaps this one is appropriate?
> 
> Fixes: 41c445ff0f48 ("i40e: main driver core")
> 
> > Signed-off-by: Erwan Velu <e.velu@criteo.com>
> > ---
> >  drivers/net/ethernet/intel/i40e/i40e_main.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index f86578857e8a..85ecf2f3de18 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -2946,7 +2946,7 @@ static int i40e_change_mtu(struct net_device
> *netdev, int new_mtu)
> >  	struct i40e_netdev_priv *np = netdev_priv(netdev);
> >  	struct i40e_vsi *vsi = np->vsi;
> >  	struct i40e_pf *pf = vsi->back;
> > -	int frame_size;
> > +	int frame_size, mfs, max_mtu;
> >
> >  	frame_size = i40e_max_vsi_frame_size(vsi, vsi->xdp_prog);
> >  	if (new_mtu > frame_size - I40E_PACKET_HDR_PAD) {
> 
> I am fine with this patch, so please take what follows as a suggestion for
> improvement, possibly as a follow-up. Not as a hard requirement from my
> side.
> 
> The part of this function between the two hunks of this patch is:
> 
> 		netdev_err(netdev, "Error changing mtu to %d, Max is
> %d\n",
> 			   new_mtu, frame_size - I40E_PACKET_HDR_PAD);
> 
> My reading is that with this patch two different limits are checked wrt
> maximum MTU size:
> 
> 1. A VSI level limit, which relates to RX buffer size 2. A PHY level limit that
> relates to the MFS
> 
> That seems fine to me. But the log message for 1 (above) does not seem
> particularly informative wrt which limit has been exceeded.
> 
> > @@ -2955,6 +2955,14 @@ static int i40e_change_mtu(struct net_device
> *netdev, int new_mtu)
> >  		return -EINVAL;
> >  	}
> >
> > +	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);
> > +		return -EINVAL;
> > +	}
> > +

Aren't these driver specific checks and messages deprecated in favor generic one ?
Is it not possible to do below

@@ -1607,6 +1607,7 @@ int i40e_aq_get_link_info(struct i40e_hw *hw,
        hw_link_info->ext_info = resp->ext_info;
        hw_link_info->loopback = resp->loopback & I40E_AQ_LOOPBACK_MASK;
        hw_link_info->max_frame_size = le16_to_cpu(resp->max_frame_size);
+       netdev->max_mtu = hw_link_info->max_frame_size - I40E_PACKET_HDR_PAD;
        hw_link_info->pacing = resp->config & I40E_AQ_CONFIG_PACING_MASK;

So that stack will take care checking max MTU.

Thanks,
Sunil.