Received-SPF: pass (google.com: domain of linux-kernel+bounces-107386-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Sunil Kovvuri Goutham <sgoutham@marvell.com>
To: Simon Horman <horms@kernel.org>, 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" <intel-wired-lan@lists.osuosl.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
Thread-Topic: [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
Thread-Index: AQHaeef1gzyIIVpicky8bDRYKHLBGg==
Date: Tue, 19 Mar 2024 10:26:52 +0000
Message-ID: 
 <BY3PR18MB4737A4F337BE3417177090B7C62C2@BY3PR18MB4737.namprd18.prod.outlook.com>
References: <20240313090719.33627-2-e.velu@criteo.com>
 <20240318174503.GE1623@kernel.org>
In-Reply-To: <20240318174503.GE1623@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?xH0QlOBkPQEwLL54yC9vKKd6iTy5J935t9Tq0oEUY5bH2dZEhHqG2C8HCqXx?=
 =?us-ascii?Q?eeZCSpDBnS8YosRPu3a8c1kMbOs84qVNRA+6KHyPsSFphpXmmrjKd/fA6ie8?=
 =?us-ascii?Q?l/GSa5PPQXbAa9cYyfVB2dnqicDS9FT+vFz2lnLRXMhZMIUNBj4CzFEqNsGq?=
 =?us-ascii?Q?MYJYenfh0e6hg6ZyhyAnSGughgLXt7Fh+01NFwG+RXszKoNWf4KeeWhy00WC?=
 =?us-ascii?Q?dVRnH82V3APNLsHg3vAQhETrlAgzVan+RsUO1p6nfHK3X6KUq+WnuE8iKq7N?=
 =?us-ascii?Q?j0WqF3IZtMztzF2OlPTjh2m+qG+ZU7q2aDp/vjsdCsQ2Ye5ly/TOEgS4Qpko?=
 =?us-ascii?Q?YM+i6hGBYfMlckRjSXep1X2E2oXanntOLbYn59vd/yqGykt39QLRu+MQJIPi?=
 =?us-ascii?Q?RpoHT/W7ABNRjfGvd5yctF3eO9bTw3iFhAo6JQG8kB4+r0ZHb1eUhpszHWpY?=
 =?us-ascii?Q?IzKJBY/wK3IvoS6ov4DgM8aVw7OZtufkWMiyfXIP/VmYPlDnIXxcOZY4csDP?=
 =?us-ascii?Q?xXUttN/wPrFLD7Gtwpv4h/g0w9UgVbpqyC76JVlwL32jIvnYNJqq6Ax4A/d0?=
 =?us-ascii?Q?Mz/LwKiAwbknKTSXoDgdsY9M7oLVfX9WB5I6Wpb7TwiVhMw6RfVq8Gy0BQ2c?=
 =?us-ascii?Q?0NPJFM89raKofvJAM/zkptdtODcPeAlzTs1jN4GgsQTTUxHnOUMQVtQTMOp/?=
 =?us-ascii?Q?O45aamR89nsoQ+dEcYi3yhpSg3uKuLjcdaBPIaXqBTzcCQw0jmpVlo4z0Koe?=
 =?us-ascii?Q?PFZT4drzEBA6Mdnmcav32lRRxmDOQFQriqWVnjAMHqUp9rMeOo6ReFYEgr8Y?=
 =?us-ascii?Q?NHw7lJ1Yuqx+YDLobN91HggmZ3l1/rcb91tLvf/LfV6Rt5lhXtJ6nrc15ANk?=
 =?us-ascii?Q?Jnz8NiZ9Aa480xyFidJuGSEdvHdu6j3XbngolgnDpmTcmheI5H2gaIiAmuwq?=
 =?us-ascii?Q?xrmi7FymIKXqaAi//r5tutiWR9Bgv4e0qxdecD1G0xI7PteLKn5KRXH80OmJ?=
 =?us-ascii?Q?sPY2g3FJQLwG9HO2fBR2eaxX2jMePszACblSxX5nWZPuRdJDIDJG5Rz7hyT/?=
 =?us-ascii?Q?0+LDEQB1561oKPRpqkrGidkdOJU7bPMNCAk+/s00oQ5mRUzTc2s5YZL0v6zi?=
 =?us-ascii?Q?BL4pXI5eNDXQMq4fCXbsFUAXHqTe217zqSg7VTHyL1bJaf3XZp7qXj8GQ/pq?=
 =?us-ascii?Q?zKp8zwP+DPi/n3N62D/8xivqTjEu0KoVpa1mLr93kS0/7QdgDmHdQLxGYFnE?=
 =?us-ascii?Q?5EDOgwf2+66Agc1tj6WoPLQX2OU0GlkweVjJv1ZzPq4dN//n2mWs+hROnw+X?=
 =?us-ascii?Q?+T87afboh9HhjpeB0u8WeHqIaqh/+P2tr3PajxHkLTQokRD5RH8VVNSy3sW3?=
 =?us-ascii?Q?hO+uGQ8GExDYzUA5CX+18fWIMohKajXzfKz959rp1Kt3en1RwSEDT6DI7Pej?=
 =?us-ascii?Q?fSsZhjr9+fkcmLz/XKrruK0KVd1H1+AUN6aXejWACVo2cEkkgais7neUzaJj?=
 =?us-ascii?Q?KqO7FLHYZao7hm54TJdKxMxpk9ZFuPCfc8XF3nEiemEaG9ZVecjDN1ZG9ivQ?=
 =?us-ascii?Q?aPiz8gMTmOgLxfunXXZO6SjhxYfvK91fUt4GneNb?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BY3PR18MB4737.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 874b98c2-63bc-4594-86ba-08dc47ff1858
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Mar 2024 10:26:52.9979
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ieru2tTwZ9ud5yZYWlLEaNqGDbN9/FpYg3xbF4yc3BAMALYcHod1jDPsQKpz09vhS1XmyYa16Ov9E4s/gHWAjw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR18MB3507
X-Proofpoint-GUID: 4gxe1L0KIdtHndiIgy8xGNl2HofydWea
X-Proofpoint-ORIG-GUID: 4gxe1L0KIdtHndiIgy8xGNl2HofydWea
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-03-18_12,2024-03-18_03,2023-05-22_02


> -----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
>=20
> 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 =3D
> 1536.
> > See
> > https://urldefense.proofpoint.com/v2/url?u=3Dhttps-
> 3A__github.com_ipxe_i
> >
> pxe_commit_6871a7de705&d=3DDwIBAg&c=3DnKjWec2b6R0mOyPaz7xtfQ&r=3Dq
> 3VKxXQKibo
> >
> Rw_F01ggTzHuhwawxR1P9_tMCN2FODU4&m=3DJ_D0216agwnkMc8GuVR13
> qCCDBQWZmqULkA
> >
> mTpcwoDk9e1Yw0Z28r7RoYVuLyMex&s=3Ds9O0JKM9qId17Qw5d2dH7c3wLJT
> Bz-frLsKFbz
> > jliE0&e=3D
> >
> > 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.
> >
>=20
> Hi Erwan, all,
>=20
> As a fix, I think this patch warrants a fixes tag.
> Perhaps this one is appropriate?
>=20
> Fixes: 41c445ff0f48 ("i40e: main driver core")
>=20
> > 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 =3D netdev_priv(netdev);
> >  	struct i40e_vsi *vsi =3D np->vsi;
> >  	struct i40e_pf *pf =3D vsi->back;
> > -	int frame_size;
> > +	int frame_size, mfs, max_mtu;
> >
> >  	frame_size =3D i40e_max_vsi_frame_size(vsi, vsi->xdp_prog);
> >  	if (new_mtu > frame_size - I40E_PACKET_HDR_PAD) {
>=20
> I am fine with this patch, so please take what follows as a suggestion fo=
r
> improvement, possibly as a follow-up. Not as a hard requirement from my
> side.
>=20
> The part of this function between the two hunks of this patch is:
>=20
> 		netdev_err(netdev, "Error changing mtu to %d, Max is
> %d\n",
> 			   new_mtu, frame_size - I40E_PACKET_HDR_PAD);
>=20
> My reading is that with this patch two different limits are checked wrt
> maximum MTU size:
>=20
> 1. A VSI level limit, which relates to RX buffer size 2. A PHY level limi=
t that
> relates to the MFS
>=20
> That seems fine to me. But the log message for 1 (above) does not seem
> particularly informative wrt which limit has been exceeded.
>=20
> > @@ -2955,6 +2955,14 @@ static int i40e_change_mtu(struct net_device
> *netdev, int new_mtu)
> >  		return -EINVAL;
> >  	}
> >
> > +	mfs =3D pf->hw.phy.link_info.max_frame_size;
> > +	max_mtu =3D 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 generi=
c 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 =3D resp->ext_info;
        hw_link_info->loopback =3D resp->loopback & I40E_AQ_LOOPBACK_MASK;
        hw_link_info->max_frame_size =3D le16_to_cpu(resp->max_frame_size);
+       netdev->max_mtu =3D hw_link_info->max_frame_size - I40E_PACKET_HDR_=
PAD;
        hw_link_info->pacing =3D resp->config & I40E_AQ_CONFIG_PACING_MASK;

So that stack will take care checking max MTU.

Thanks,
Sunil.