[Intel-wired-lan] [PATCH] i40e: Prevent setting MTU if greater than MFS
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, but I didn't found a reliable way to do 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.
A typical trace looks like the following :
[ 377.548696] i40e :5d:00.0 eno5: Error changing mtu to 9000 which is
greater than the current mfs: 1536
Signed-off-by: Erwan Velu
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 54eb55464e31..14fc70d854d3 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;
frame_size = i40e_max_vsi_frame_size(vsi, vsi->xdp_prog);
if (new_mtu > frame_size - I40E_PACKET_HDR_PAD) {
@@ -2959,6 +2959,13 @@ static int i40e_change_mtu(struct net_device *netdev,
int new_mtu)
return -EINVAL;
}
+ mfs = pf->hw.phy.link_info.max_frame_size;
+ if (new_mtu > mfs) {
+ netdev_err(netdev, "Error changing mtu to %d which is greater
than the current mfs: %d\n",
+ new_mtu, mfs);
+ return -EINVAL;
+ }
+
netdev_dbg(netdev, "changing MTU from %d to %d\n",
netdev->mtu, new_mtu);
netdev->mtu = new_mtu;
--
2.43.2
Re: [Intel-wired-lan] [PATCH] i40e: Prevent setting MTU if greater than MFS
Le lun. 4 mars 2024 à 23:10, Tony Nguyen a écrit : > > Signed-off-by: Erwan Velu > > The Author and Sign-off needs to be fixed; they don't match. > > WARNING: From:/Signed-off-by: email address mismatch: 'From: Erwan Velu > ' != 'Signed-off-by: Erwan Velu ' Yeah, I have a complicated email setup between my personal and professional emails. I'll see how I can fix that. I was also wondering if I shouldn't subtract I40E_PACKET_HDR_PAD from the mfs to be more accurate, can you confirm this ? If one can have a look at what is the exact procedure to fix the MFS size when too small, that would be lovely/ideal in addition to my patch.
[Intel-wired-lan] [PATCH v2] i40e: Prevent setting MTU if greater than MFS
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, but I didn't found a reliable way to do 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.
A typical trace looks like the following :
[ 377.548696] i40e :5d:00.0 eno5: Error changing mtu to 9000 which is
greater than the current mfs: 1536
Signed-off-by: Erwan Velu
---
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);
+ return -EINVAL;
+ }
+
netdev_dbg(netdev, "changing MTU from %d to %d\n",
netdev->mtu, new_mtu);
netdev->mtu = new_mtu;
--
2.44.0
[Intel-wired-lan] [PATCH v3] i40e: Prevent setting MTU if greater than MFS
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
---
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);
+ return -EINVAL;
+ }
+
netdev_dbg(netdev, "changing MTU from %d to %d\n",
netdev->mtu, new_mtu);
netdev->mtu = new_mtu;
--
2.44.0
Re: [Intel-wired-lan] [PATCH v3] i40e: Prevent setting MTU if greater than MFS
> 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.
Re: [Intel-wired-lan] [PATCH v3] i40e: Prevent setting MTU if greater than MFS
> 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>
> ("")
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.
[Intel-wired-lan] [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
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://github.com/ipxe/ipxe/commit/6871a7de705
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 :5d:00.0 eno5: Error changing mtu to 9000, Max is 1500.
MFS is too small.
Signed-off-by: Erwan Velu
---
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) {
@@ -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;
+ }
+
netdev_dbg(netdev, "changing MTU from %d to %d\n",
netdev->mtu, new_mtu);
netdev->mtu = new_mtu;
--
2.44.0
Re: [Intel-wired-lan] [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
Le 14/03/2024 à 17:10, Brett Creeley a écrit : [...] If this is how the max_mtu is determined, does it make sense to set this before registering the netdev, i.e. netdev->max_mtu in i40e_config_netdev()? The absolute max is properly set but I think that's only true if we ensure the value of the MFS. So if with another patch to set the MFS to the right value when asking a bigger MTU, having this value makes sense this is the absolute max for this device. Erwan,
Re: [Intel-wired-lan] [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
Le 14/03/2024 à 18:55, Brett Creeley a écrit : [...] AFAIK there is no API for a user to change the max_mtu, so the only way the device's MFS would need to change is if it's done during initialization time, which should be done before netdev registration anyway. Sorry Brett, I was probably unclear and please note that I'm not a network developer, just a user that faced a bug. My initial though was to check the mfs size in i40e_change_mtu() and if mfs is too small, then let's increase it. Maybe just resetting it at init time to the largest value (which seems to be the default fw behavior) is a best approach. I'd love to ear from Intel dev that knows this driver/cards/fw better on what's the best approach here. Erwan,
Re: [Intel-wired-lan] [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
Le 14/03/2024 à 21:31, Tony Nguyen a écrit : [..] Setting the mfs size to max values during init and reset would better; this is what the ice driver does. However, this would take implementing new AdminQ calls. IMO this patch is ok to prevent the issue being reported and allow for ease of backport. That was my first intention, ensure that no one else get stuck in the same situation. It would be nice to backport it to all stable releases once merged. Erwan,
Re: [Intel-wired-lan] [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
Le 18/03/2024 à 18:45, Simon Horman a écrit :
[...]
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")
Simon
Isn't that a bit too generic ?
[..]
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.
I got some comments around this.
I wanted to keep my patch being focused on the mfs issue, but I can
offer a patch to get a similar output for this. What WRT stands for ?
I wanted also to make another patch for this :
dev_warn(&pdev->dev, "MFS for port %x has been set below the default:
%x\n",pf->hw.port, val);
The MFS reported as hex without a "0x" prefix is very misleading, I can
offer a patch for this too.
Erwan,
Re: [Intel-wired-lan] [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
Le 19/03/2024 à 13:20, Simon Horman a écrit : [...] FWIIW, I think handling these questions in follow-up patches is fine. I wonder if the previous patch must be merged first, so I can reference it in the commit message, or if I should shoot it now. Erwan,
[Intel-wired-lan] [PATCH iwl-net] i40e: Report MFS in decimal base instead of hex
If the MFS is set below the default (0x2600), a warning message is reported like the following : MFS for port 1 has been set below the default: 600 This message is a bit confusing as the number shown here (600) is in fact an hexa number: 0x600 = 1536 Without any explicit "0x" prefix, this message is read like the MFS is set to 600 bytes. MFS, as per MTUs, are usually expressed in decimal base. This commit reports both current and default MFS values in decimal so it's less confusing for end-users. A typical warning message looks like the following : MFS for port 1 (1536) has been set below the default (9728) Signed-off-by: Erwan Velu --- drivers/net/ethernet/intel/i40e/i40e_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 85ecf2f3de18..9a142562db38 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -16106,8 +16106,8 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent) val = FIELD_GET(I40E_PRTGL_SAH_MFS_MASK, rd32(&pf->hw, I40E_PRTGL_SAH)); if (val < MAX_FRAME_SIZE_DEFAULT) - dev_warn(&pdev->dev, "MFS for port %x has been set below the default: %x\n", -pf->hw.port, val); + dev_warn(&pdev->dev, "MFS for port %x (%d) has been set below the default (%d)\n", +pf->hw.port, val, MAX_FRAME_SIZE_DEFAULT); /* Add a filter to drop all Flow control frames from any VSI from being * transmitted. By doing so we stop a malicious VF from sending out -- 2.44.0
