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