diff mbox series

[v3] i40e: Prevent setting MTU if greater than MFS

Message ID 20240312094259.770554-1-e.velu@criteo.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v3] i40e: Prevent setting MTU if greater than MFS | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 940 this patch: 940
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 956 this patch: 956
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 956 this patch: 956
netdev/checkpatch warning WARNING: From:/Signed-off-by: email address mismatch: 'From: Erwan Velu <erwanaliasr1@gmail.com>' != 'Signed-off-by: Erwan Velu <e.velu@criteo.com>' WARNING: Unknown commit id '6871a7de705b6f6a4046f0d19da9bcd689c3bc8e', maybe rebased or not pulled?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Erwan Velu March 12, 2024, 9:42 a.m. UTC
Commit 6871a7de705b6f6a4046f0d19da9bcd689c3bc8e from iPXE project is
setting the MFS to 0x600 = 1536.

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 accept 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, 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.

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(-)

Comments

Paul Menzel March 12, 2024, 9:48 a.m. UTC | #1
Dear Erwan,


Thank you very umch for your patch.

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.

> 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?

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

accept*s*

> 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, 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.

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

> 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 89a3401d20ab..225b2fd0449e 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -2950,7 +2950,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) {
> @@ -2959,6 +2959,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);

The other log messages capitalize MTU.

> +		return -EINVAL;
> +	}
> +
>   	netdev_dbg(netdev, "changing MTU from %d to %d\n",
>   		   netdev->mtu, new_mtu);
>   	netdev->mtu = new_mtu;


The rest looks reasonable.


Kind regards,

Paul
Paolo Abeni March 12, 2024, 10:01 a.m. UTC | #2
Hi,

On Tue, 2024-03-12 at 10:42 +0100, Erwan Velu wrote:
> Commit 6871a7de705b6f6a4046f0d19da9bcd689c3bc8e from iPXE project is

Please use the checkpatch-friendly commit reference: <12char hash>
("<title>")

> setting the MFS to 0x600 = 1536.
> 
> 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 accept 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, 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.

This looks like a legit fix that should target the 'net' tree, @Tony:
do you agree? 

If so, Erwan, please include a suitable fixes tag in the next revision.
Please include into the subj prefix a suitable target tree. I think
this should go first via the intel tree for testing, so 'iwl-net'
should fit.

In any case please respect the 24h grace period when posting on netdev:

https://elixir.bootlin.com/linux/latest/source/Documentation/process/maintainer-netdev.rst#L399

Cheers,

Paolo
Erwan Velu March 12, 2024, 10:18 a.m. UTC | #3
> 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.
Erwan Velu March 12, 2024, 10:29 a.m. UTC | #4
> On Tue, 2024-03-12 at 10:42 +0100, Erwan Velu wrote:
> > Commit 6871a7de705b6f6a4046f0d19da9bcd689c3bc8e from iPXE project is
>
> Please use the checkpatch-friendly commit reference: <12char hash>
> ("<title>")
Done.

> This looks like a legit fix that should target the 'net' tree, @Tony:
> do you agree?
> If so, Erwan, please include a suitable fixes tag in the next revision.
> Please include into the subj prefix a suitable target tree. I think
> this should go first via the intel tree for testing, so 'iwl-net'
> should fit.
Oh I didn't knew that part, thx. I'll wait @Tony to see what target I
should use.

> In any case please respect the 24h grace period when posting on netdev:
Yeah sorry for the v3 ... I shouldn't have done that way, that fast.
Tony Nguyen March 12, 2024, 10:07 p.m. UTC | #5
On 3/12/2024 3:29 AM, Erwan Velu wrote:
>> On Tue, 2024-03-12 at 10:42 +0100, Erwan Velu wrote:
> 
>> This looks like a legit fix that should target the 'net' tree, @Tony:
>> do you agree?
>> If so, Erwan, please include a suitable fixes tag in the next revision.
>> Please include into the subj prefix a suitable target tree. I think
>> this should go first via the intel tree for testing, so 'iwl-net'
>> should fit.
> Oh I didn't knew that part, thx. I'll wait @Tony to see what target I
> should use.

I agree with both parts, so 'iwl-net' would be best.

Thanks,
Tony

> 
>> In any case please respect the 24h grace period when posting on netdev:
> Yeah sorry for the v3 ... I shouldn't have done that way, that fast.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 89a3401d20ab..225b2fd0449e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -2950,7 +2950,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) {
@@ -2959,6 +2959,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;
+	}
+
 	netdev_dbg(netdev, "changing MTU from %d to %d\n",
 		   netdev->mtu, new_mtu);
 	netdev->mtu = new_mtu;