e1000e max frame calculation

e1000e max frame calculation

[Michael Cronenworth]

Hello,

I have attempted to discuss this with upstream directly[1], but they will not 
respond to me. I will now attempt here.

The e1000e driver uses a different maximum frame calculation after a commit[2] in 
kernel 3.15. This caused everyone's setups that were using 9000 for MTU to break 
after they rebooted on their kernel update. The addition of the VLAN bytes force 
users to use 8996.

Other drivers appear to be using the "old" maximum frame calculation and can 
continue to use 9000.

Can a standard be decided upon that all drivers use or is it acceptable that one 
driver "does its own thing?"

Thanks,
Michael


[1] https://sourceforge.net/p/e1000/bugs/459/
[2] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/intel/e1000e/netdev.c?id=c751a3d58cf2dae89ec941a259025b0175d67b0c

Re: e1000e max frame calculation

[Alexander Duyck]

On 04/06/2015 09:38 AM, Michael Cronenworth wrote:
> Hello,
>
> I have attempted to discuss this with upstream directly[1], but they 
> will not respond to me. I will now attempt here.
>
> The e1000e driver uses a different maximum frame calculation after a 
> commit[2] in kernel 3.15. This caused everyone's setups that were 
> using 9000 for MTU to break after they rebooted on their kernel 
> update. The addition of the VLAN bytes force users to use 8996.
>
> Other drivers appear to be using the "old" maximum frame calculation 
> and can continue to use 9000.
>
> Can a standard be decided upon that all drivers use or is it 
> acceptable that one driver "does its own thing?"
>
> Thanks,
> Michael

What do you mean by "does its own thing"?  The problem here (I'm 
assuming, please correct me if I am wrong) is that the e1000e hardware 
is configured with a maximum frame size of 9018 based on the 
max_hw_frame_size value reported in the e1000_info struct for your 
part.  As a result you are limited to 8996 since it is forced to 
guarantee there is enough room for 1 VLAN header (4 bytes), the Ethernet 
header (14 bytes), and a CRC (4 bytes).  What you would need to 
determine is the reason why the hardware is limited to 9018, and not 
something like 9216 (9K) which is the more common value since it is 
rounded of to the nearest 1K.

Every piece of hardware will do things differently.  Within just the 
e1000e driver there are some parts that don't support jumbo frames, and 
some that only support 4K jumbo frames.  What would need to be 
determined is if 9018 is the actual limit on the hardware due to some 
limitation in the part, or if it was just an arbitrary value that was 
added by a developer so that it would come out at 9000 for a limit when 
changing the MTU.

- Alex

[Intel-wired-lan] e1000e max frame calculation

[Alexander Duyck]

On 04/06/2015 09:38 AM, Michael Cronenworth wrote:
> Hello,
>
> I have attempted to discuss this with upstream directly[1], but they 
> will not respond to me. I will now attempt here.
>
> The e1000e driver uses a different maximum frame calculation after a 
> commit[2] in kernel 3.15. This caused everyone's setups that were 
> using 9000 for MTU to break after they rebooted on their kernel 
> update. The addition of the VLAN bytes force users to use 8996.
>
> Other drivers appear to be using the "old" maximum frame calculation 
> and can continue to use 9000.
>
> Can a standard be decided upon that all drivers use or is it 
> acceptable that one driver "does its own thing?"
>
> Thanks,
> Michael

What do you mean by "does its own thing"?  The problem here (I'm 
assuming, please correct me if I am wrong) is that the e1000e hardware 
is configured with a maximum frame size of 9018 based on the 
max_hw_frame_size value reported in the e1000_info struct for your 
part.  As a result you are limited to 8996 since it is forced to 
guarantee there is enough room for 1 VLAN header (4 bytes), the Ethernet 
header (14 bytes), and a CRC (4 bytes).  What you would need to 
determine is the reason why the hardware is limited to 9018, and not 
something like 9216 (9K) which is the more common value since it is 
rounded of to the nearest 1K.

Every piece of hardware will do things differently.  Within just the 
e1000e driver there are some parts that don't support jumbo frames, and 
some that only support 4K jumbo frames.  What would need to be 
determined is if 9018 is the actual limit on the hardware due to some 
limitation in the part, or if it was just an arbitrary value that was 
added by a developer so that it would come out at 9000 for a limit when 
changing the MTU.

- Alex

Re: e1000e max frame calculation

[Michael Cronenworth]

Preface: I am not here to start a discussion about header sizes or hardware limits. 
This is solely about the calculation used to limit a user when they pass a MTU to a 
driver.

On 04/06/2015 11:38 AM, Michael Cronenworth wrote:
> Other drivers appear to be using the "old" maximum frame calculation and can
> continue to use 9000.
>
> Can a standard be decided upon that all drivers use or is it acceptable that one
> driver "does its own thing?"

To be specific here are some examples of maximum MTU calculation and their tests:

sky2: user_mtu > ETH_JUMBO_MTU

r8169: user_mtu < ETH_ZLEN || user_mtu > (ETH_DATA_LEN||x*1024 - ETH_HLEN - 2)

igb:   user_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN > MAX_JUMBO_FRAME_SIZE

e1000e <3.15:  user_mtu + ETH_HLEN + ETH_FCS_LEN > adapter->max_hw_frame_size;

e1000e >=3.15: user_mtu + VLAN_HLEN + ETH_HLEN + ETH_FCS_LEN >
                   adapter->max_hw_frame_size;

Let's say the hardware has a limit of 4096 bytes. The user wishes to set an MTU of 
4078 bytes.

sky2: false, mtu is set

r8169: false, mtu is set

igb: true, mtu is not set

e1000e <3.15: false, mtu is set

e1000e <3.15: true, mtu is not set

So some drivers work with the 4078 MTU and some do not. They all had the same 
hardware limit, but their driver's software calculated limit was different. Am I crazy?

Thanks,
Michael

Re: e1000e max frame calculation

[Alexander Duyck]

On 04/06/2015 02:45 PM, Michael Cronenworth wrote:
> Preface: I am not here to start a discussion about header sizes or 
> hardware limits. This is solely about the calculation used to limit a 
> user when they pass a MTU to a driver.
>
> On 04/06/2015 11:38 AM, Michael Cronenworth wrote:
>> Other drivers appear to be using the "old" maximum frame calculation 
>> and can
>> continue to use 9000.
>>
>> Can a standard be decided upon that all drivers use or is it 
>> acceptable that one
>> driver "does its own thing?"
>
> To be specific here are some examples of maximum MTU calculation and 
> their tests:
>
> sky2: user_mtu > ETH_JUMBO_MTU
>
> r8169: user_mtu < ETH_ZLEN || user_mtu > (ETH_DATA_LEN||x*1024 - 
> ETH_HLEN - 2)
>
> igb:   user_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN > 
> MAX_JUMBO_FRAME_SIZE
>
> e1000e <3.15:  user_mtu + ETH_HLEN + ETH_FCS_LEN > 
> adapter->max_hw_frame_size;
>
> e1000e >=3.15: user_mtu + VLAN_HLEN + ETH_HLEN + ETH_FCS_LEN >
>                   adapter->max_hw_frame_size;
>
> Let's say the hardware has a limit of 4096 bytes. The user wishes to 
> set an MTU of 4078 bytes.
>
> sky2: false, mtu is set
>
> r8169: false, mtu is set
>
> igb: true, mtu is not set
>
> e1000e <3.15: false, mtu is set
>
> e1000e <3.15: true, mtu is not set
>
> So some drivers work with the 4078 MTU and some do not. They all had 
> the same hardware limit, but their driver's software calculated limit 
> was different. Am I crazy?

That isn't really a fair comparison and gets back to my original response.

If the hardware limit was 4096 bytes many of those static defines in the 
other drivers would likely be very different.  For example in the case 
if igb I think the hardware limit is somewhere around 9.5K, but the fact 
is 9K(9216, not 9000) is used as there isn't much demand for jumbo frame 
sizes over 9000 and things can be kept pretty simple since you can drop 
a bunch of VLAN headers in with all that wiggle room left over.  Another 
thing to check would be how many of these parts support the option of 
toggling VLAN tag stripping and RXFCS stripping.  That is the reason why 
these sizes were changed as these impact the resultant buffer size.

The e1000e pch parts seem very limited in terms of their jumbo frames 
support.  It looks like the first pch part only supported 4K, and in the 
case of the later generations I only see support for a 9018 jumbo frame 
listed in the datasheet.  I suspect this is why that is set as the limit 
in the driver.  The question at this point is if 9018 is the limit, why 
is it the limit and is there any wiggle room to it.

- Alex

Re: e1000e max frame calculation

[Hisashi T Fujinaka]

On Mon, 6 Apr 2015, Alexander Duyck wrote:

> The e1000e pch parts seem very limited in terms of their jumbo frames 
> support.  It looks like the first pch part only supported 4K, and in the case 
> of the later generations I only see support for a 9018 jumbo frame listed in 
> the datasheet.  I suspect this is why that is set as the limit in the driver. 
> The question at this point is if 9018 is the limit, why is it the limit and 
> is there any wiggle room to it.

It's a limit that was told to us by the HW group for the 82579.

Re: e1000e max frame calculation

[Alexander Duyck]

On 04/06/2015 08:47 PM, Hisashi T Fujinaka wrote:
> On Mon, 6 Apr 2015, Alexander Duyck wrote:
>
>> The e1000e pch parts seem very limited in terms of their jumbo frames
>> support.  It looks like the first pch part only supported 4K, and in
>> the case of the later generations I only see support for a 9018 jumbo
>> frame listed in the datasheet.  I suspect this is why that is set as
>> the limit in the driver. The question at this point is if 9018 is the
>> limit, why is it the limit and is there any wiggle room to it.
>
> It's a limit that was told to us by the HW group for the 82579.

The question I have is if the limit of 9018 includes the VLAN header or
not.  The reason why I ask is that prior to 3.15 you could configure an
MTU of 9000, and then still enable VLANs which would push the upper size
to a theoretical limit of 9022.

For example, what is the upper limit on the Windows driver for this
part?  Does it not support a jumbo frame size of 9014 (Excluding the CRC
and VLAN header)?  If so we could probably increase the value to 9022 in
these drivers, not suffer any ill effects, and resolve the issue that
Michael has been having after the change to include the VLAN_HLEN on the
max frame size calculation.

- Alex

Re: e1000e max frame calculation

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Alexander Duyck wrote:

> On 04/06/2015 08:47 PM, Hisashi T Fujinaka wrote:
>> On Mon, 6 Apr 2015, Alexander Duyck wrote:
>>
>>> The e1000e pch parts seem very limited in terms of their jumbo frames
>>> support.  It looks like the first pch part only supported 4K, and in
>>> the case of the later generations I only see support for a 9018 jumbo
>>> frame listed in the datasheet.  I suspect this is why that is set as
>>> the limit in the driver. The question at this point is if 9018 is the
>>> limit, why is it the limit and is there any wiggle room to it.
>>
>> It's a limit that was told to us by the HW group for the 82579.
>
> The question I have is if the limit of 9018 includes the VLAN header or
> not.  The reason why I ask is that prior to 3.15 you could configure an
> MTU of 9000, and then still enable VLANs which would push the upper size
> to a theoretical limit of 9022.
>
> For example, what is the upper limit on the Windows driver for this
> part?  Does it not support a jumbo frame size of 9014 (Excluding the CRC
> and VLAN header)?  If so we could probably increase the value to 9022 in
> these drivers, not suffer any ill effects, and resolve the issue that
> Michael has been having after the change to include the VLAN_HLEN on the
> max frame size calculation.

I've been searching the documentation for the max MTU. This is something
coming from a completely different division and I haven't seen it
written down anywhere. In fact, I can't find much documentation for
client networking parts at all. It doesn't mean that it doesn't exist,
it just means that I'm very unfamiliar with the documentation and
processes from the Intel client groups.

I'll keep trying to track it down, but David Ertman (the old e1000e
driver maintainer) was very clear that he was told by the hardware
engineers not to exceed 9018.

[PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

When the VLAN_HLEN was added to the calculation for the maximum frame size
there seems to have been a number of issues added to the driver.

The first issue is that in some cases the maximum frame size for a device
never really reached the actual maximum frame size as the VLAN header
length was not included the calculation for that value.  As a result some
parts only supported a maximum frame size of either 1496 in the case of
parts that didn't support jumbo frames, and 8996 in the case of the parts
that do.

The second issue is the fact that there were several checks that weren't
updated so as a result setting an MTU of 1500 was treated as enabling jumbo
frames as the calculated value was 1522 instead of 1518.  I have addressed
those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where appropriate.

The final issue was the fact that lowering the MTU below 1500 would cause
the driver to allocate 2K buffers for the rings.  This is an old issue that
was fixed several years ago in igb/ixgbe and I am addressing now by just
replacing == with a <= so that we always just round up to 1522 for anything
that isn't a jumbo frame.

Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when changing interface MTU")
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---

I have only build tested this though I am 99% sure the fixes here are
correct.  This patch should fix issues on 82573 and ich8 w/ setting an MTU
of 1500, and for the PCH series w/ setting an MTU of 9000.

I assume I can get away with bumping the max_hw_frame_size for the PCH
parts from 9018 to 9022 based on the fact that the Windows INF for the parts
lists supporting either 1514, 4088, and 9014 all of which exclude the 8
bytes for CRC and VLAN header.

 drivers/net/ethernet/intel/e1000e/82571.c   |    2 +-
 drivers/net/ethernet/intel/e1000e/ich8lan.c |   10 +++++-----
 drivers/net/ethernet/intel/e1000e/netdev.c  |   18 ++++++++----------
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/82571.c b/drivers/net/ethernet/intel/e1000e/82571.c
index dc79ed85030b..32e77755a9c6 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c
@@ -2010,7 +2010,7 @@ const struct e1000_info e1000_82573_info = {
 	.flags2			= FLAG2_DISABLE_ASPM_L1
 				  | FLAG2_DISABLE_ASPM_L0S,
 	.pba			= 20,
-	.max_hw_frame_size	= ETH_FRAME_LEN + ETH_FCS_LEN,
+	.max_hw_frame_size	= VLAN_ETH_FRAME_LEN + ETH_FCS_LEN,
 	.get_variants		= e1000_get_variants_82571,
 	.mac_ops		= &e82571_mac_ops,
 	.phy_ops		= &e82_phy_ops_m88,
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 9d81c0317433..e2498dbf3c3b 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -1563,7 +1563,7 @@ static s32 e1000_get_variants_ich8lan(struct e1000_adapter *adapter)
 	    ((adapter->hw.mac.type >= e1000_pch2lan) &&
 	     (!(er32(CTRL_EXT) & E1000_CTRL_EXT_LSECCK)))) {
 		adapter->flags &= ~FLAG_HAS_JUMBO_FRAMES;
-		adapter->max_hw_frame_size = ETH_FRAME_LEN + ETH_FCS_LEN;
+		adapter->max_hw_frame_size = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 
 		hw->mac.ops.blink_led = NULL;
 	}
@@ -5681,7 +5681,7 @@ const struct e1000_info e1000_ich8_info = {
 				  | FLAG_HAS_FLASH
 				  | FLAG_APME_IN_WUC,
 	.pba			= 8,
-	.max_hw_frame_size	= ETH_FRAME_LEN + ETH_FCS_LEN,
+	.max_hw_frame_size	= VLAN_ETH_FRAME_LEN + ETH_FCS_LEN,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5754,7 +5754,7 @@ const struct e1000_info e1000_pch2_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5774,7 +5774,7 @@ const struct e1000_info e1000_pch_lpt_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5794,7 +5794,7 @@ const struct e1000_info e1000_pch_spt_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 74ec185a697f..f77db9304060 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3807,7 +3807,7 @@ void e1000e_reset(struct e1000_adapter *adapter)
 	/* reset Packet Buffer Allocation to default */
 	ew32(PBA, pba);
 
-	if (adapter->max_frame_size > ETH_FRAME_LEN + ETH_FCS_LEN) {
+	if (adapter->max_frame_size > (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)) {
 		/* To maintain wire speed transmits, the Tx FIFO should be
 		 * large enough to accommodate two full transmit packets,
 		 * rounded up to the next 1KB and expressed in KB.  Likewise,
@@ -4196,9 +4196,9 @@ static int e1000_sw_init(struct e1000_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 
-	adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN;
+	adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 	adapter->rx_ps_bsize0 = 128;
-	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+	adapter->max_frame_size = netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 	adapter->tx_ring_count = E1000_DEFAULT_TXD;
 	adapter->rx_ring_count = E1000_DEFAULT_RXD;
@@ -5781,17 +5781,17 @@ struct rtnl_link_stats64 *e1000e_get_stats64(struct net_device *netdev,
 static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
-	int max_frame = new_mtu + VLAN_HLEN + ETH_HLEN + ETH_FCS_LEN;
+	int max_frame = new_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 
 	/* Jumbo frame support */
-	if ((max_frame > ETH_FRAME_LEN + ETH_FCS_LEN) &&
+	if ((max_frame > (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)) &&
 	    !(adapter->flags & FLAG_HAS_JUMBO_FRAMES)) {
 		e_err("Jumbo Frames not supported.\n");
 		return -EINVAL;
 	}
 
 	/* Supported frame sizes */
-	if ((new_mtu < ETH_ZLEN + ETH_FCS_LEN + VLAN_HLEN) ||
+	if ((new_mtu < (VLAN_ETH_ZLEN + ETH_FCS_LEN)) ||
 	    (max_frame > adapter->max_hw_frame_size)) {
 		e_err("Unsupported MTU setting\n");
 		return -EINVAL;
@@ -5831,10 +5831,8 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 		adapter->rx_buffer_len = 4096;
 
 	/* adjust allocation if LPE protects us, and we aren't using SBP */
-	if ((max_frame == ETH_FRAME_LEN + ETH_FCS_LEN) ||
-	    (max_frame == ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN))
-		adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN
-		    + ETH_FCS_LEN;
+	if (max_frame <= (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN))
+		adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 
 	if (netif_running(netdev))
 		e1000e_up(adapter);

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

When the VLAN_HLEN was added to the calculation for the maximum frame size
there seems to have been a number of issues added to the driver.

The first issue is that in some cases the maximum frame size for a device
never really reached the actual maximum frame size as the VLAN header
length was not included the calculation for that value.  As a result some
parts only supported a maximum frame size of either 1496 in the case of
parts that didn't support jumbo frames, and 8996 in the case of the parts
that do.

The second issue is the fact that there were several checks that weren't
updated so as a result setting an MTU of 1500 was treated as enabling jumbo
frames as the calculated value was 1522 instead of 1518.  I have addressed
those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where appropriate.

The final issue was the fact that lowering the MTU below 1500 would cause
the driver to allocate 2K buffers for the rings.  This is an old issue that
was fixed several years ago in igb/ixgbe and I am addressing now by just
replacing == with a <= so that we always just round up to 1522 for anything
that isn't a jumbo frame.

Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when changing interface MTU")
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---

I have only build tested this though I am 99% sure the fixes here are
correct.  This patch should fix issues on 82573 and ich8 w/ setting an MTU
of 1500, and for the PCH series w/ setting an MTU of 9000.

I assume I can get away with bumping the max_hw_frame_size for the PCH
parts from 9018 to 9022 based on the fact that the Windows INF for the parts
lists supporting either 1514, 4088, and 9014 all of which exclude the 8
bytes for CRC and VLAN header.

 drivers/net/ethernet/intel/e1000e/82571.c   |    2 +-
 drivers/net/ethernet/intel/e1000e/ich8lan.c |   10 +++++-----
 drivers/net/ethernet/intel/e1000e/netdev.c  |   18 ++++++++----------
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/82571.c b/drivers/net/ethernet/intel/e1000e/82571.c
index dc79ed85030b..32e77755a9c6 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c
@@ -2010,7 +2010,7 @@ const struct e1000_info e1000_82573_info = {
 	.flags2			= FLAG2_DISABLE_ASPM_L1
 				  | FLAG2_DISABLE_ASPM_L0S,
 	.pba			= 20,
-	.max_hw_frame_size	= ETH_FRAME_LEN + ETH_FCS_LEN,
+	.max_hw_frame_size	= VLAN_ETH_FRAME_LEN + ETH_FCS_LEN,
 	.get_variants		= e1000_get_variants_82571,
 	.mac_ops		= &e82571_mac_ops,
 	.phy_ops		= &e82_phy_ops_m88,
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 9d81c0317433..e2498dbf3c3b 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -1563,7 +1563,7 @@ static s32 e1000_get_variants_ich8lan(struct e1000_adapter *adapter)
 	    ((adapter->hw.mac.type >= e1000_pch2lan) &&
 	     (!(er32(CTRL_EXT) & E1000_CTRL_EXT_LSECCK)))) {
 		adapter->flags &= ~FLAG_HAS_JUMBO_FRAMES;
-		adapter->max_hw_frame_size = ETH_FRAME_LEN + ETH_FCS_LEN;
+		adapter->max_hw_frame_size = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 
 		hw->mac.ops.blink_led = NULL;
 	}
@@ -5681,7 +5681,7 @@ const struct e1000_info e1000_ich8_info = {
 				  | FLAG_HAS_FLASH
 				  | FLAG_APME_IN_WUC,
 	.pba			= 8,
-	.max_hw_frame_size	= ETH_FRAME_LEN + ETH_FCS_LEN,
+	.max_hw_frame_size	= VLAN_ETH_FRAME_LEN + ETH_FCS_LEN,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5754,7 +5754,7 @@ const struct e1000_info e1000_pch2_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5774,7 +5774,7 @@ const struct e1000_info e1000_pch_lpt_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
@@ -5794,7 +5794,7 @@ const struct e1000_info e1000_pch_spt_info = {
 	.flags2			= FLAG2_HAS_PHY_STATS
 				  | FLAG2_HAS_EEE,
 	.pba			= 26,
-	.max_hw_frame_size	= 9018,
+	.max_hw_frame_size	= 9022,
 	.get_variants		= e1000_get_variants_ich8lan,
 	.mac_ops		= &ich8_mac_ops,
 	.phy_ops		= &ich8_phy_ops,
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 74ec185a697f..f77db9304060 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -3807,7 +3807,7 @@ void e1000e_reset(struct e1000_adapter *adapter)
 	/* reset Packet Buffer Allocation to default */
 	ew32(PBA, pba);
 
-	if (adapter->max_frame_size > ETH_FRAME_LEN + ETH_FCS_LEN) {
+	if (adapter->max_frame_size > (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)) {
 		/* To maintain wire speed transmits, the Tx FIFO should be
 		 * large enough to accommodate two full transmit packets,
 		 * rounded up to the next 1KB and expressed in KB.  Likewise,
@@ -4196,9 +4196,9 @@ static int e1000_sw_init(struct e1000_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 
-	adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN;
+	adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 	adapter->rx_ps_bsize0 = 128;
-	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
+	adapter->max_frame_size = netdev->mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 	adapter->tx_ring_count = E1000_DEFAULT_TXD;
 	adapter->rx_ring_count = E1000_DEFAULT_RXD;
@@ -5781,17 +5781,17 @@ struct rtnl_link_stats64 *e1000e_get_stats64(struct net_device *netdev,
 static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
-	int max_frame = new_mtu + VLAN_HLEN + ETH_HLEN + ETH_FCS_LEN;
+	int max_frame = new_mtu + VLAN_ETH_HLEN + ETH_FCS_LEN;
 
 	/* Jumbo frame support */
-	if ((max_frame > ETH_FRAME_LEN + ETH_FCS_LEN) &&
+	if ((max_frame > (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN)) &&
 	    !(adapter->flags & FLAG_HAS_JUMBO_FRAMES)) {
 		e_err("Jumbo Frames not supported.\n");
 		return -EINVAL;
 	}
 
 	/* Supported frame sizes */
-	if ((new_mtu < ETH_ZLEN + ETH_FCS_LEN + VLAN_HLEN) ||
+	if ((new_mtu < (VLAN_ETH_ZLEN + ETH_FCS_LEN)) ||
 	    (max_frame > adapter->max_hw_frame_size)) {
 		e_err("Unsupported MTU setting\n");
 		return -EINVAL;
@@ -5831,10 +5831,8 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 		adapter->rx_buffer_len = 4096;
 
 	/* adjust allocation if LPE protects us, and we aren't using SBP */
-	if ((max_frame == ETH_FRAME_LEN + ETH_FCS_LEN) ||
-	    (max_frame == ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN))
-		adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN
-		    + ETH_FCS_LEN;
+	if (max_frame <= (VLAN_ETH_FRAME_LEN + ETH_FCS_LEN))
+		adapter->rx_buffer_len = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
 
 	if (netif_running(netdev))
 		e1000e_up(adapter);

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Alexander Duyck wrote:

> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when changing interface MTU")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>
> I have only build tested this though I am 99% sure the fixes here are
> correct.  This patch should fix issues on 82573 and ich8 w/ setting an MTU
> of 1500, and for the PCH series w/ setting an MTU of 9000.

Since the original fix was something submitted by Red Hat, can you check
that you're not re-breaking whatever it was that Red Hat thought they
were fixing?

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Alexander Duyck wrote:

> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when changing interface MTU")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>
> I have only build tested this though I am 99% sure the fixes here are
> correct.  This patch should fix issues on 82573 and ich8 w/ setting an MTU
> of 1500, and for the PCH series w/ setting an MTU of 9000.

Since the original fix was something submitted by Red Hat, can you check
that you're not re-breaking whatever it was that Red Hat thought they
were fixing?

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

On 04/08/2015 02:15 PM, Hisashi T Fujinaka wrote:
> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>
>> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
>> changing interface MTU")
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>> ---
>>
>> I have only build tested this though I am 99% sure the fixes here are
>> correct.  This patch should fix issues on 82573 and ich8 w/ setting
>> an MTU
>> of 1500, and for the PCH series w/ setting an MTU of 9000.
>
> Since the original fix was something submitted by Red Hat, can you check
> that you're not re-breaking whatever it was that Red Hat thought they
> were fixing?

The original issue is referenced in the patch that this fixes.  The
problem was that the VLAN header wasn't being considered when computing
the Rx buffer size, so you could change the MTU to 1504 and the if
statement at the end of e1000_change_mtu was still using a 1522 Rx
buffer size and max frame even though we had technically just configured
things for 1526.

The updated logic is correctly taking the VLAN header into account so if
you bump the MTU 1504 it will switch over to jumbo frames mode w/ 2K
buffers.

The bit I am fixing is that there were several spots including the
backend value for max_hw_frame_size that didn't take VLAN header length
into account.  There were cases where 1500 MTU was being treated as a
jumbo frame, or we were coming up 4 bytes shy as in the pch2, ich8, and
82573 e1000_info structures.

- Alex

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

On 04/08/2015 02:15 PM, Hisashi T Fujinaka wrote:
> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>
>> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
>> changing interface MTU")
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>> ---
>>
>> I have only build tested this though I am 99% sure the fixes here are
>> correct.  This patch should fix issues on 82573 and ich8 w/ setting
>> an MTU
>> of 1500, and for the PCH series w/ setting an MTU of 9000.
>
> Since the original fix was something submitted by Red Hat, can you check
> that you're not re-breaking whatever it was that Red Hat thought they
> were fixing?

The original issue is referenced in the patch that this fixes.  The
problem was that the VLAN header wasn't being considered when computing
the Rx buffer size, so you could change the MTU to 1504 and the if
statement at the end of e1000_change_mtu was still using a 1522 Rx
buffer size and max frame even though we had technically just configured
things for 1526.

The updated logic is correctly taking the VLAN header into account so if
you bump the MTU 1504 it will switch over to jumbo frames mode w/ 2K
buffers.

The bit I am fixing is that there were several spots including the
backend value for max_hw_frame_size that didn't take VLAN header length
into account.  There were cases where 1500 MTU was being treated as a
jumbo frame, or we were coming up 4 bytes shy as in the pch2, ich8, and
82573 e1000_info structures.

- Alex

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Alexander Duyck wrote:

> On 04/08/2015 02:15 PM, Hisashi T Fujinaka wrote:
>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>
>>> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
>>> changing interface MTU")
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>> ---
>>>
>>> I have only build tested this though I am 99% sure the fixes here are
>>> correct.  This patch should fix issues on 82573 and ich8 w/ setting
>>> an MTU
>>> of 1500, and for the PCH series w/ setting an MTU of 9000.
>>
>> Since the original fix was something submitted by Red Hat, can you check
>> that you're not re-breaking whatever it was that Red Hat thought they
>> were fixing?
>
> The original issue is referenced in the patch that this fixes.  The
> problem was that the VLAN header wasn't being considered when computing
> the Rx buffer size, so you could change the MTU to 1504 and the if
> statement at the end of e1000_change_mtu was still using a 1522 Rx
> buffer size and max frame even though we had technically just configured
> things for 1526.
>
> The updated logic is correctly taking the VLAN header into account so if
> you bump the MTU 1504 it will switch over to jumbo frames mode w/ 2K
> buffers.
>
> The bit I am fixing is that there were several spots including the
> backend value for max_hw_frame_size that didn't take VLAN header length
> into account.  There were cases where 1500 MTU was being treated as a
> jumbo frame, or we were coming up 4 bytes shy as in the pch2, ich8, and
> 82573 e1000_info structures.

The max_hw_frame_size should still be limited to 9018.

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Alexander Duyck wrote:

> On 04/08/2015 02:15 PM, Hisashi T Fujinaka wrote:
>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>
>>> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
>>> changing interface MTU")
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>> ---
>>>
>>> I have only build tested this though I am 99% sure the fixes here are
>>> correct.  This patch should fix issues on 82573 and ich8 w/ setting
>>> an MTU
>>> of 1500, and for the PCH series w/ setting an MTU of 9000.
>>
>> Since the original fix was something submitted by Red Hat, can you check
>> that you're not re-breaking whatever it was that Red Hat thought they
>> were fixing?
>
> The original issue is referenced in the patch that this fixes.  The
> problem was that the VLAN header wasn't being considered when computing
> the Rx buffer size, so you could change the MTU to 1504 and the if
> statement at the end of e1000_change_mtu was still using a 1522 Rx
> buffer size and max frame even though we had technically just configured
> things for 1526.
>
> The updated logic is correctly taking the VLAN header into account so if
> you bump the MTU 1504 it will switch over to jumbo frames mode w/ 2K
> buffers.
>
> The bit I am fixing is that there were several spots including the
> backend value for max_hw_frame_size that didn't take VLAN header length
> into account.  There were cases where 1500 MTU was being treated as a
> jumbo frame, or we were coming up 4 bytes shy as in the pch2, ich8, and
> 82573 e1000_info structures.

The max_hw_frame_size should still be limited to 9018.

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

On 04/08/2015 04:05 PM, Hisashi T Fujinaka wrote:
> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>
>> On 04/08/2015 02:15 PM, Hisashi T Fujinaka wrote:
>>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>>
>>>> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
>>>> changing interface MTU")
>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>>> ---
>>>>
>>>> I have only build tested this though I am 99% sure the fixes here are
>>>> correct.  This patch should fix issues on 82573 and ich8 w/ setting
>>>> an MTU
>>>> of 1500, and for the PCH series w/ setting an MTU of 9000.
>>>
>>> Since the original fix was something submitted by Red Hat, can you
>>> check
>>> that you're not re-breaking whatever it was that Red Hat thought they
>>> were fixing?
>>
>> The original issue is referenced in the patch that this fixes.  The
>> problem was that the VLAN header wasn't being considered when computing
>> the Rx buffer size, so you could change the MTU to 1504 and the if
>> statement at the end of e1000_change_mtu was still using a 1522 Rx
>> buffer size and max frame even though we had technically just configured
>> things for 1526.
>>
>> The updated logic is correctly taking the VLAN header into account so if
>> you bump the MTU 1504 it will switch over to jumbo frames mode w/ 2K
>> buffers.
>>
>> The bit I am fixing is that there were several spots including the
>> backend value for max_hw_frame_size that didn't take VLAN header length
>> into account.  There were cases where 1500 MTU was being treated as a
>> jumbo frame, or we were coming up 4 bytes shy as in the pch2, ich8, and
>> 82573 e1000_info structures.
>
> The max_hw_frame_size should still be limited to 9018.

It is but it isn't.  If you look in e1000_change_mtu you will find the
node about "Jumbo frame workaround on 82579 and newer requires CRC be
stripped".  With that being the case I'm wondering if the 9018 doesn't
include CRC but instead includes VLAN header.  So as a result the actual
hardware is processing frames that are 9022 in size, but the buffer only
ever receives at most 9018 since the CRC is stripped.

I suspect that is why the Windows driver has had no issues reporting
support for a size of 9014 (excluding VLAN and CRC) on these parts and
hasn't had any issues.

- Alex

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

On 04/08/2015 04:05 PM, Hisashi T Fujinaka wrote:
> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>
>> On 04/08/2015 02:15 PM, Hisashi T Fujinaka wrote:
>>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>>
>>>> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
>>>> changing interface MTU")
>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>>> ---
>>>>
>>>> I have only build tested this though I am 99% sure the fixes here are
>>>> correct.  This patch should fix issues on 82573 and ich8 w/ setting
>>>> an MTU
>>>> of 1500, and for the PCH series w/ setting an MTU of 9000.
>>>
>>> Since the original fix was something submitted by Red Hat, can you
>>> check
>>> that you're not re-breaking whatever it was that Red Hat thought they
>>> were fixing?
>>
>> The original issue is referenced in the patch that this fixes.  The
>> problem was that the VLAN header wasn't being considered when computing
>> the Rx buffer size, so you could change the MTU to 1504 and the if
>> statement at the end of e1000_change_mtu was still using a 1522 Rx
>> buffer size and max frame even though we had technically just configured
>> things for 1526.
>>
>> The updated logic is correctly taking the VLAN header into account so if
>> you bump the MTU 1504 it will switch over to jumbo frames mode w/ 2K
>> buffers.
>>
>> The bit I am fixing is that there were several spots including the
>> backend value for max_hw_frame_size that didn't take VLAN header length
>> into account.  There were cases where 1500 MTU was being treated as a
>> jumbo frame, or we were coming up 4 bytes shy as in the pch2, ich8, and
>> 82573 e1000_info structures.
>
> The max_hw_frame_size should still be limited to 9018.

It is but it isn't.  If you look in e1000_change_mtu you will find the
node about "Jumbo frame workaround on 82579 and newer requires CRC be
stripped".  With that being the case I'm wondering if the 9018 doesn't
include CRC but instead includes VLAN header.  So as a result the actual
hardware is processing frames that are 9022 in size, but the buffer only
ever receives at most 9018 since the CRC is stripped.

I suspect that is why the Windows driver has had no issues reporting
support for a size of 9014 (excluding VLAN and CRC) on these parts and
hasn't had any issues.

- Alex

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2362 bytes --]

On Wed, 2015-04-08 at 14:02 -0700, Alexander Duyck wrote:
> When the VLAN_HLEN was added to the calculation for the maximum frame
> size
> there seems to have been a number of issues added to the driver.
> 
> The first issue is that in some cases the maximum frame size for a
> device
> never really reached the actual maximum frame size as the VLAN header
> length was not included the calculation for that value.  As a result
> some
> parts only supported a maximum frame size of either 1496 in the case
> of
> parts that didn't support jumbo frames, and 8996 in the case of the
> parts
> that do.
> 
> The second issue is the fact that there were several checks that
> weren't
> updated so as a result setting an MTU of 1500 was treated as enabling
> jumbo
> frames as the calculated value was 1522 instead of 1518.  I have
> addressed
> those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where
> appropriate.
> 
> The final issue was the fact that lowering the MTU below 1500 would
> cause
> the driver to allocate 2K buffers for the rings.  This is an old issue
> that
> was fixed several years ago in igb/ixgbe and I am addressing now by
> just
> replacing == with a <= so that we always just round up to 1522 for
> anything
> that isn't a jumbo frame.
> 
> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
> changing interface MTU")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
> 
> I have only build tested this though I am 99% sure the fixes here are
> correct.  This patch should fix issues on 82573 and ich8 w/ setting an
> MTU
> of 1500, and for the PCH series w/ setting an MTU of 9000.
> 
> I assume I can get away with bumping the max_hw_frame_size for the PCH
> parts from 9018 to 9022 based on the fact that the Windows INF for the
> parts
> lists supporting either 1514, 4088, and 9014 all of which exclude the
> 8
> bytes for CRC and VLAN header.
> 
>  drivers/net/ethernet/intel/e1000e/82571.c   |    2 +-
>  drivers/net/ethernet/intel/e1000e/ich8lan.c |   10 +++++-----
>  drivers/net/ethernet/intel/e1000e/netdev.c  |   18 ++++++++----------
>  3 files changed, 14 insertions(+), 16 deletions(-)

I have applied your patch to my queue, thanks Alex.
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Jeff Kirsher]

On Wed, 2015-04-08 at 14:02 -0700, Alexander Duyck wrote:
> When the VLAN_HLEN was added to the calculation for the maximum frame
> size
> there seems to have been a number of issues added to the driver.
> 
> The first issue is that in some cases the maximum frame size for a
> device
> never really reached the actual maximum frame size as the VLAN header
> length was not included the calculation for that value.  As a result
> some
> parts only supported a maximum frame size of either 1496 in the case
> of
> parts that didn't support jumbo frames, and 8996 in the case of the
> parts
> that do.
> 
> The second issue is the fact that there were several checks that
> weren't
> updated so as a result setting an MTU of 1500 was treated as enabling
> jumbo
> frames as the calculated value was 1522 instead of 1518.  I have
> addressed
> those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where
> appropriate.
> 
> The final issue was the fact that lowering the MTU below 1500 would
> cause
> the driver to allocate 2K buffers for the rings.  This is an old issue
> that
> was fixed several years ago in igb/ixgbe and I am addressing now by
> just
> replacing == with a <= so that we always just round up to 1522 for
> anything
> that isn't a jumbo frame.
> 
> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
> changing interface MTU")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
> 
> I have only build tested this though I am 99% sure the fixes here are
> correct.  This patch should fix issues on 82573 and ich8 w/ setting an
> MTU
> of 1500, and for the PCH series w/ setting an MTU of 9000.
> 
> I assume I can get away with bumping the max_hw_frame_size for the PCH
> parts from 9018 to 9022 based on the fact that the Windows INF for the
> parts
> lists supporting either 1514, 4088, and 9014 all of which exclude the
> 8
> bytes for CRC and VLAN header.
> 
>  drivers/net/ethernet/intel/e1000e/82571.c   |    2 +-
>  drivers/net/ethernet/intel/e1000e/ich8lan.c |   10 +++++-----
>  drivers/net/ethernet/intel/e1000e/netdev.c  |   18 ++++++++----------
>  3 files changed, 14 insertions(+), 16 deletions(-)

I have applied your patch to my queue, thanks Alex.
-- 
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git
dev-queue
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150408/7e6195a5/attachment.asc>

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Alexander Duyck wrote:

> On 04/08/2015 04:05 PM, Hisashi T Fujinaka wrote:
>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>
>>> On 04/08/2015 02:15 PM, Hisashi T Fujinaka wrote:
>>>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>>>
>>>>> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
>>>>> changing interface MTU")
>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>>>> ---
>>>>>
>>>>> I have only build tested this though I am 99% sure the fixes here are
>>>>> correct.  This patch should fix issues on 82573 and ich8 w/ setting
>>>>> an MTU
>>>>> of 1500, and for the PCH series w/ setting an MTU of 9000.
>>>>
>>>> Since the original fix was something submitted by Red Hat, can you
>>>> check
>>>> that you're not re-breaking whatever it was that Red Hat thought they
>>>> were fixing?
>>>
>>> The original issue is referenced in the patch that this fixes.  The
>>> problem was that the VLAN header wasn't being considered when computing
>>> the Rx buffer size, so you could change the MTU to 1504 and the if
>>> statement at the end of e1000_change_mtu was still using a 1522 Rx
>>> buffer size and max frame even though we had technically just configured
>>> things for 1526.
>>>
>>> The updated logic is correctly taking the VLAN header into account so if
>>> you bump the MTU 1504 it will switch over to jumbo frames mode w/ 2K
>>> buffers.
>>>
>>> The bit I am fixing is that there were several spots including the
>>> backend value for max_hw_frame_size that didn't take VLAN header length
>>> into account.  There were cases where 1500 MTU was being treated as a
>>> jumbo frame, or we were coming up 4 bytes shy as in the pch2, ich8, and
>>> 82573 e1000_info structures.
>>
>> The max_hw_frame_size should still be limited to 9018.
>
> It is but it isn't.  If you look in e1000_change_mtu you will find the
> node about "Jumbo frame workaround on 82579 and newer requires CRC be
> stripped".  With that being the case I'm wondering if the 9018 doesn't
> include CRC but instead includes VLAN header.  So as a result the actual
> hardware is processing frames that are 9022 in size, but the buffer only
> ever receives at most 9018 since the CRC is stripped.
>
> I suspect that is why the Windows driver has had no issues reporting
> support for a size of 9014 (excluding VLAN and CRC) on these parts and
> hasn't had any issues.

I can only tell you what was told to me by Dave Ertman, which is that
there is a hard hardware limit of 9018 bytes. I wouldn't know if we do
have Windows issues because it's a completely different division and
there's no reason for any of those issues to be routed to us.

In fact, the problem with different max MTU across the drivers in Linux
has only ever been reported by one user.

I'm still looking for the HW documentation and would like Jeff to hold
off until we find it.

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Alexander Duyck wrote:

> On 04/08/2015 04:05 PM, Hisashi T Fujinaka wrote:
>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>
>>> On 04/08/2015 02:15 PM, Hisashi T Fujinaka wrote:
>>>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>>>
>>>>> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when
>>>>> changing interface MTU")
>>>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>>>> ---
>>>>>
>>>>> I have only build tested this though I am 99% sure the fixes here are
>>>>> correct.  This patch should fix issues on 82573 and ich8 w/ setting
>>>>> an MTU
>>>>> of 1500, and for the PCH series w/ setting an MTU of 9000.
>>>>
>>>> Since the original fix was something submitted by Red Hat, can you
>>>> check
>>>> that you're not re-breaking whatever it was that Red Hat thought they
>>>> were fixing?
>>>
>>> The original issue is referenced in the patch that this fixes.  The
>>> problem was that the VLAN header wasn't being considered when computing
>>> the Rx buffer size, so you could change the MTU to 1504 and the if
>>> statement at the end of e1000_change_mtu was still using a 1522 Rx
>>> buffer size and max frame even though we had technically just configured
>>> things for 1526.
>>>
>>> The updated logic is correctly taking the VLAN header into account so if
>>> you bump the MTU 1504 it will switch over to jumbo frames mode w/ 2K
>>> buffers.
>>>
>>> The bit I am fixing is that there were several spots including the
>>> backend value for max_hw_frame_size that didn't take VLAN header length
>>> into account.  There were cases where 1500 MTU was being treated as a
>>> jumbo frame, or we were coming up 4 bytes shy as in the pch2, ich8, and
>>> 82573 e1000_info structures.
>>
>> The max_hw_frame_size should still be limited to 9018.
>
> It is but it isn't.  If you look in e1000_change_mtu you will find the
> node about "Jumbo frame workaround on 82579 and newer requires CRC be
> stripped".  With that being the case I'm wondering if the 9018 doesn't
> include CRC but instead includes VLAN header.  So as a result the actual
> hardware is processing frames that are 9022 in size, but the buffer only
> ever receives at most 9018 since the CRC is stripped.
>
> I suspect that is why the Windows driver has had no issues reporting
> support for a size of 9014 (excluding VLAN and CRC) on these parts and
> hasn't had any issues.

I can only tell you what was told to me by Dave Ertman, which is that
there is a hard hardware limit of 9018 bytes. I wouldn't know if we do
have Windows issues because it's a completely different division and
there's no reason for any of those issues to be routed to us.

In fact, the problem with different max MTU across the drivers in Linux
has only ever been reported by one user.

I'm still looking for the HW documentation and would like Jeff to hold
off until we find it.

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Hisashi T Fujinaka wrote:

> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>> 
>> It is but it isn't.  If you look in e1000_change_mtu you will find the
>> node about "Jumbo frame workaround on 82579 and newer requires CRC be
>> stripped".  With that being the case I'm wondering if the 9018 doesn't
>> include CRC but instead includes VLAN header.  So as a result the actual
>> hardware is processing frames that are 9022 in size, but the buffer only
>> ever receives at most 9018 since the CRC is stripped.
>> 
>> I suspect that is why the Windows driver has had no issues reporting
>> support for a size of 9014 (excluding VLAN and CRC) on these parts and
>> hasn't had any issues.
>
> I can only tell you what was told to me by Dave Ertman, which is that
> there is a hard hardware limit of 9018 bytes. I wouldn't know if we do
> have Windows issues because it's a completely different division and
> there's no reason for any of those issues to be routed to us.
>
> In fact, the problem with different max MTU across the drivers in Linux
> has only ever been reported by one user.
>
> I'm still looking for the HW documentation and would like Jeff to hold
> off until we find it.

OK. So the data sheet states:

LPE controls whether long packet reception is permitted. Hardware
discards long packets if LPE is 0. A long packet is one longer than 1522
bytes. If LPE is 1, the maximum packet size that the device can receive
is 9018 bytes.

So if you're pre-stripping the CRC, then it will be less than 9018.

I guess I'd like to hear what Dave thinks.

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Hisashi T Fujinaka]

On Wed, 8 Apr 2015, Hisashi T Fujinaka wrote:

> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>> 
>> It is but it isn't.  If you look in e1000_change_mtu you will find the
>> node about "Jumbo frame workaround on 82579 and newer requires CRC be
>> stripped".  With that being the case I'm wondering if the 9018 doesn't
>> include CRC but instead includes VLAN header.  So as a result the actual
>> hardware is processing frames that are 9022 in size, but the buffer only
>> ever receives at most 9018 since the CRC is stripped.
>> 
>> I suspect that is why the Windows driver has had no issues reporting
>> support for a size of 9014 (excluding VLAN and CRC) on these parts and
>> hasn't had any issues.
>
> I can only tell you what was told to me by Dave Ertman, which is that
> there is a hard hardware limit of 9018 bytes. I wouldn't know if we do
> have Windows issues because it's a completely different division and
> there's no reason for any of those issues to be routed to us.
>
> In fact, the problem with different max MTU across the drivers in Linux
> has only ever been reported by one user.
>
> I'm still looking for the HW documentation and would like Jeff to hold
> off until we find it.

OK. So the data sheet states:

LPE controls whether long packet reception is permitted. Hardware
discards long packets if LPE is 0. A long packet is one longer than 1522
bytes. If LPE is 1, the maximum packet size that the device can receive
is 9018 bytes.

So if you're pre-stripping the CRC, then it will be less than 9018.

I guess I'd like to hear what Dave thinks.

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

On 04/08/2015 05:26 PM, Hisashi T Fujinaka wrote:
> On Wed, 8 Apr 2015, Hisashi T Fujinaka wrote:
>
>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>>
>>> It is but it isn't.  If you look in e1000_change_mtu you will find the
>>> node about "Jumbo frame workaround on 82579 and newer requires CRC be
>>> stripped".  With that being the case I'm wondering if the 9018 doesn't
>>> include CRC but instead includes VLAN header.  So as a result the
>>> actual
>>> hardware is processing frames that are 9022 in size, but the buffer
>>> only
>>> ever receives at most 9018 since the CRC is stripped.
>>>
>>> I suspect that is why the Windows driver has had no issues reporting
>>> support for a size of 9014 (excluding VLAN and CRC) on these parts and
>>> hasn't had any issues.
>>
>> I can only tell you what was told to me by Dave Ertman, which is that
>> there is a hard hardware limit of 9018 bytes. I wouldn't know if we do
>> have Windows issues because it's a completely different division and
>> there's no reason for any of those issues to be routed to us.
>>
>> In fact, the problem with different max MTU across the drivers in Linux
>> has only ever been reported by one user.
>>
>> I'm still looking for the HW documentation and would like Jeff to hold
>> off until we find it.
>
> OK. So the data sheet states:
>
> LPE controls whether long packet reception is permitted. Hardware
> discards long packets if LPE is 0. A long packet is one longer than 1522
> bytes. If LPE is 1, the maximum packet size that the device can receive
> is 9018 bytes.

Yeah, that is mostly clear except it doesn't indicate if that includes
or excludes the CRC and/or VLAN header.  All the documentation I can
find seems to indicate it is likely skipping one or the other since it
recommends setting any switches to support 9022 in order to account for
both.

> So if you're pre-stripping the CRC, then it will be less than 9018.

Right so the argument is probably moot since you cannot enable jumbo
frames unless CRC stripping is enabled.  Ugh, but it looks like nothing
prevents disabling CRC stripping once jumbo frames has been enabled. 
I'll patch that shortly.

> I guess I'd like to hear what Dave thinks.

The problem is this pre-dates when Dave Ertman started working on
e1000e.  All of this went into effect back during the introduction of
82579 and i217/i218 and the two patches from Bruce that reduced the size
down to 9018 didn't specify where the number came from.  I suspect it is
the same data sheet we have been looking at which is a bit vague about
what is included in that size.

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

On 04/08/2015 05:26 PM, Hisashi T Fujinaka wrote:
> On Wed, 8 Apr 2015, Hisashi T Fujinaka wrote:
>
>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>>
>>> It is but it isn't.  If you look in e1000_change_mtu you will find the
>>> node about "Jumbo frame workaround on 82579 and newer requires CRC be
>>> stripped".  With that being the case I'm wondering if the 9018 doesn't
>>> include CRC but instead includes VLAN header.  So as a result the
>>> actual
>>> hardware is processing frames that are 9022 in size, but the buffer
>>> only
>>> ever receives at most 9018 since the CRC is stripped.
>>>
>>> I suspect that is why the Windows driver has had no issues reporting
>>> support for a size of 9014 (excluding VLAN and CRC) on these parts and
>>> hasn't had any issues.
>>
>> I can only tell you what was told to me by Dave Ertman, which is that
>> there is a hard hardware limit of 9018 bytes. I wouldn't know if we do
>> have Windows issues because it's a completely different division and
>> there's no reason for any of those issues to be routed to us.
>>
>> In fact, the problem with different max MTU across the drivers in Linux
>> has only ever been reported by one user.
>>
>> I'm still looking for the HW documentation and would like Jeff to hold
>> off until we find it.
>
> OK. So the data sheet states:
>
> LPE controls whether long packet reception is permitted. Hardware
> discards long packets if LPE is 0. A long packet is one longer than 1522
> bytes. If LPE is 1, the maximum packet size that the device can receive
> is 9018 bytes.

Yeah, that is mostly clear except it doesn't indicate if that includes
or excludes the CRC and/or VLAN header.  All the documentation I can
find seems to indicate it is likely skipping one or the other since it
recommends setting any switches to support 9022 in order to account for
both.

> So if you're pre-stripping the CRC, then it will be less than 9018.

Right so the argument is probably moot since you cannot enable jumbo
frames unless CRC stripping is enabled.  Ugh, but it looks like nothing
prevents disabling CRC stripping once jumbo frames has been enabled. 
I'll patch that shortly.

> I guess I'd like to hear what Dave thinks.

The problem is this pre-dates when Dave Ertman started working on
e1000e.  All of this went into effect back during the introduction of
82579 and i217/i218 and the two patches from Bruce that reduced the size
down to 9018 didn't specify where the number came from.  I suspect it is
the same data sheet we have been looking at which is a bit vague about
what is included in that size.

Re: [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Michael Cronenworth]

On 04/08/2015 04:02 PM, Alexander Duyck wrote:
> When the VLAN_HLEN was added to the calculation for the maximum frame size
> there seems to have been a number of issues added to the driver.
>
> The first issue is that in some cases the maximum frame size for a device
> never really reached the actual maximum frame size as the VLAN header
> length was not included the calculation for that value.  As a result some
> parts only supported a maximum frame size of either 1496 in the case of
> parts that didn't support jumbo frames, and 8996 in the case of the parts
> that do.
>
> The second issue is the fact that there were several checks that weren't
> updated so as a result setting an MTU of 1500 was treated as enabling jumbo
> frames as the calculated value was 1522 instead of 1518.  I have addressed
> those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where appropriate.
>
> The final issue was the fact that lowering the MTU below 1500 would cause
> the driver to allocate 2K buffers for the rings.  This is an old issue that
> was fixed several years ago in igb/ixgbe and I am addressing now by just
> replacing == with a <= so that we always just round up to 1522 for anything
> that isn't a jumbo frame.

Alex,

Thanks taking the time to work on a patch.

I have tested this patch in 4.0 on i218-v hardware and it is working. I see 9000 in 
tcpdump (tso/gso off) and my switch blocks packets if I set the max frame size to 
9017 down from 9018.

I also played with a VLAN real quick, and did not encounter any problems, but I 
don't normally use VLANs so it may need further testing.

Thanks,
Michael

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Michael Cronenworth]

On 04/08/2015 04:02 PM, Alexander Duyck wrote:
> When the VLAN_HLEN was added to the calculation for the maximum frame size
> there seems to have been a number of issues added to the driver.
>
> The first issue is that in some cases the maximum frame size for a device
> never really reached the actual maximum frame size as the VLAN header
> length was not included the calculation for that value.  As a result some
> parts only supported a maximum frame size of either 1496 in the case of
> parts that didn't support jumbo frames, and 8996 in the case of the parts
> that do.
>
> The second issue is the fact that there were several checks that weren't
> updated so as a result setting an MTU of 1500 was treated as enabling jumbo
> frames as the calculated value was 1522 instead of 1518.  I have addressed
> those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where appropriate.
>
> The final issue was the fact that lowering the MTU below 1500 would cause
> the driver to allocate 2K buffers for the rings.  This is an old issue that
> was fixed several years ago in igb/ixgbe and I am addressing now by just
> replacing == with a <= so that we always just round up to 1522 for anything
> that isn't a jumbo frame.

Alex,

Thanks taking the time to work on a patch.

I have tested this patch in 4.0 on i218-v hardware and it is working. I see 9000 in 
tcpdump (tso/gso off) and my switch blocks packets if I set the max frame size to 
9017 down from 9018.

I also played with a VLAN real quick, and did not encounter any problems, but I 
don't normally use VLANs so it may need further testing.

Thanks,
Michael

RE: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Templeman, Chaim]

We will look into this with the HW architect and get back to you on what the HW limitations actually are on this issue.

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Alexander Duyck
Sent: Thursday, April 09, 2015 04:19
To: Hisashi T Fujinaka
Cc: netdev@vger.kernel.org; mike@cchtml.com; intel-wired-lan@lists.osuosl.org; Ertman, David M
Subject: Re: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

On 04/08/2015 05:26 PM, Hisashi T Fujinaka wrote:
> On Wed, 8 Apr 2015, Hisashi T Fujinaka wrote:
>
>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>>
>>> It is but it isn't.  If you look in e1000_change_mtu you will find the
>>> node about "Jumbo frame workaround on 82579 and newer requires CRC be
>>> stripped".  With that being the case I'm wondering if the 9018 doesn't
>>> include CRC but instead includes VLAN header.  So as a result the
>>> actual
>>> hardware is processing frames that are 9022 in size, but the buffer
>>> only
>>> ever receives at most 9018 since the CRC is stripped.
>>>
>>> I suspect that is why the Windows driver has had no issues reporting
>>> support for a size of 9014 (excluding VLAN and CRC) on these parts and
>>> hasn't had any issues.
>>
>> I can only tell you what was told to me by Dave Ertman, which is that
>> there is a hard hardware limit of 9018 bytes. I wouldn't know if we do
>> have Windows issues because it's a completely different division and
>> there's no reason for any of those issues to be routed to us.
>>
>> In fact, the problem with different max MTU across the drivers in Linux
>> has only ever been reported by one user.
>>
>> I'm still looking for the HW documentation and would like Jeff to hold
>> off until we find it.
>
> OK. So the data sheet states:
>
> LPE controls whether long packet reception is permitted. Hardware
> discards long packets if LPE is 0. A long packet is one longer than 1522
> bytes. If LPE is 1, the maximum packet size that the device can receive
> is 9018 bytes.

Yeah, that is mostly clear except it doesn't indicate if that includes
or excludes the CRC and/or VLAN header.  All the documentation I can
find seems to indicate it is likely skipping one or the other since it
recommends setting any switches to support 9022 in order to account for
both.

> So if you're pre-stripping the CRC, then it will be less than 9018.

Right so the argument is probably moot since you cannot enable jumbo
frames unless CRC stripping is enabled.  Ugh, but it looks like nothing
prevents disabling CRC stripping once jumbo frames has been enabled. 
I'll patch that shortly.

> I guess I'd like to hear what Dave thinks.

The problem is this pre-dates when Dave Ertman started working on
e1000e.  All of this went into effect back during the introduction of
82579 and i217/i218 and the two patches from Bruce that reduced the size
down to 9018 didn't specify where the number came from.  I suspect it is
the same data sheet we have been looking at which is a bit vague about
what is included in that size.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Templeman, Chaim]

We will look into this with the HW architect and get back to you on what the HW limitations actually are on this issue.

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Alexander Duyck
Sent: Thursday, April 09, 2015 04:19
To: Hisashi T Fujinaka
Cc: netdev at vger.kernel.org; mike at cchtml.com; intel-wired-lan at lists.osuosl.org; Ertman, David M
Subject: Re: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

On 04/08/2015 05:26 PM, Hisashi T Fujinaka wrote:
> On Wed, 8 Apr 2015, Hisashi T Fujinaka wrote:
>
>> On Wed, 8 Apr 2015, Alexander Duyck wrote:
>>>
>>> It is but it isn't.  If you look in e1000_change_mtu you will find the
>>> node about "Jumbo frame workaround on 82579 and newer requires CRC be
>>> stripped".  With that being the case I'm wondering if the 9018 doesn't
>>> include CRC but instead includes VLAN header.  So as a result the
>>> actual
>>> hardware is processing frames that are 9022 in size, but the buffer
>>> only
>>> ever receives at most 9018 since the CRC is stripped.
>>>
>>> I suspect that is why the Windows driver has had no issues reporting
>>> support for a size of 9014 (excluding VLAN and CRC) on these parts and
>>> hasn't had any issues.
>>
>> I can only tell you what was told to me by Dave Ertman, which is that
>> there is a hard hardware limit of 9018 bytes. I wouldn't know if we do
>> have Windows issues because it's a completely different division and
>> there's no reason for any of those issues to be routed to us.
>>
>> In fact, the problem with different max MTU across the drivers in Linux
>> has only ever been reported by one user.
>>
>> I'm still looking for the HW documentation and would like Jeff to hold
>> off until we find it.
>
> OK. So the data sheet states:
>
> LPE controls whether long packet reception is permitted. Hardware
> discards long packets if LPE is 0. A long packet is one longer than 1522
> bytes. If LPE is 1, the maximum packet size that the device can receive
> is 9018 bytes.

Yeah, that is mostly clear except it doesn't indicate if that includes
or excludes the CRC and/or VLAN header.  All the documentation I can
find seems to indicate it is likely skipping one or the other since it
recommends setting any switches to support 9022 in order to account for
both.

> So if you're pre-stripping the CRC, then it will be less than 9018.

Right so the argument is probably moot since you cannot enable jumbo
frames unless CRC stripping is enabled.  Ugh, but it looks like nothing
prevents disabling CRC stripping once jumbo frames has been enabled. 
I'll patch that shortly.

> I guess I'd like to hear what Dave thinks.

The problem is this pre-dates when Dave Ertman started working on
e1000e.  All of this went into effect back during the introduction of
82579 and i217/i218 and the two patches from Bruce that reduced the size
down to 9018 didn't specify where the number came from.  I suspect it is
the same data sheet we have been looking at which is a bit vague about
what is included in that size.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

On 04/08/2015 11:17 PM, Templeman, Chaim wrote:
> We will look into this with the HW architect and get back to you on what the HW limitations actually are on this issue.

Chaim, thanks for looking into this.  The key piece that would be useful
to know is if the 9018 should really be 9022 if you account for both
VLAN header and CRC.  Based on the documentation for the Windows driver
that seems to be the case as they support an MTU of 9014 excluding the 8
bytes for CRC and VLAN header.

- Alex

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Alexander Duyck]

On 04/08/2015 11:17 PM, Templeman, Chaim wrote:
> We will look into this with the HW architect and get back to you on what the HW limitations actually are on this issue.

Chaim, thanks for looking into this.  The key piece that would be useful
to know is if the 9018 should really be 9022 if you account for both
VLAN header and CRC.  Based on the documentation for the Windows driver
that seems to be the case as they support an MTU of 9014 excluding the 8
bytes for CRC and VLAN header.

- Alex

RE: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Templeman, Chaim]

I checked with the HW guys, and they confirmed that a total packet size of 9022 is acceptable.
The original size of 9018 was defined as:
Frame size (9018) = DST (6) + SRC (6) + VLAN (4) + TYPE (2) + MTU (x) + CRC (4).
If an additional 4 bytes is required, that is ok.

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck@gmail.com] 
Sent: Thursday, April 09, 2015 18:51
To: Templeman, Chaim; Hisashi T Fujinaka
Cc: netdev@vger.kernel.org; mike@cchtml.com; intel-wired-lan@lists.osuosl.org; Ertman, David M; Lubetkin, YanirX
Subject: Re: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

On 04/08/2015 11:17 PM, Templeman, Chaim wrote:
> We will look into this with the HW architect and get back to you on what the HW limitations actually are on this issue.

Chaim, thanks for looking into this.  The key piece that would be useful
to know is if the 9018 should really be 9022 if you account for both
VLAN header and CRC.  Based on the documentation for the Windows driver
that seems to be the case as they support an MTU of 9014 excluding the 8
bytes for CRC and VLAN header.

- Alex
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Templeman, Chaim]

I checked with the HW guys, and they confirmed that a total packet size of 9022 is acceptable.
The original size of 9018 was defined as:
Frame size (9018) = DST (6) + SRC (6) + VLAN (4) + TYPE (2) + MTU (x) + CRC (4).
If an additional 4 bytes is required, that is ok.

-----Original Message-----
From: Alexander Duyck [mailto:alexander.duyck at gmail.com] 
Sent: Thursday, April 09, 2015 18:51
To: Templeman, Chaim; Hisashi T Fujinaka
Cc: netdev at vger.kernel.org; mike at cchtml.com; intel-wired-lan at lists.osuosl.org; Ertman, David M; Lubetkin, YanirX
Subject: Re: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

On 04/08/2015 11:17 PM, Templeman, Chaim wrote:
> We will look into this with the HW architect and get back to you on what the HW limitations actually are on this issue.

Chaim, thanks for looking into this.  The key piece that would be useful
to know is if the 9018 should really be 9022 if you account for both
VLAN header and CRC.  Based on the documentation for the Windows driver
that seems to be the case as they support an MTU of 9014 excluding the 8
bytes for CRC and VLAN header.

- Alex
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Alexander Duyck
> Sent: Wednesday, April 08, 2015 2:03 PM
> To: intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T
> Cc: netdev@vger.kernel.org; mike@cchtml.com; htodd@twofifty.com
> Subject: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN
> as a part of max frame size
> 
> When the VLAN_HLEN was added to the calculation for the maximum frame size
> there seems to have been a number of issues added to the driver.
> 
> The first issue is that in some cases the maximum frame size for a device
> never really reached the actual maximum frame size as the VLAN header
> length was not included the calculation for that value.  As a result some
> parts only supported a maximum frame size of either 1496 in the case of
> parts that didn't support jumbo frames, and 8996 in the case of the parts
> that do.
> 
> The second issue is the fact that there were several checks that weren't
> updated so as a result setting an MTU of 1500 was treated as enabling
> jumbo
> frames as the calculated value was 1522 instead of 1518.  I have addressed
> those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where
> appropriate.
> 
> The final issue was the fact that lowering the MTU below 1500 would cause
> the driver to allocate 2K buffers for the rings.  This is an old issue
> that
> was fixed several years ago in igb/ixgbe and I am addressing now by just
> replacing == with a <= so that we always just round up to 1522 for
> anything
> that isn't a jumbo frame.
> 
> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when changing
> interface MTU")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
> 
> I have only build tested this though I am 99% sure the fixes here are
> correct.  This patch should fix issues on 82573 and ich8 w/ setting an MTU
> of 1500, and for the PCH series w/ setting an MTU of 9000.
> 
> I assume I can get away with bumping the max_hw_frame_size for the PCH
> parts from 9018 to 9022 based on the fact that the Windows INF for the
> parts
> lists supporting either 1514, 4088, and 9014 all of which exclude the 8
> bytes for CRC and VLAN header.
> 
>  drivers/net/ethernet/intel/e1000e/82571.c   |    2 +-
>  drivers/net/ethernet/intel/e1000e/ich8lan.c |   10 +++++-----
>  drivers/net/ethernet/intel/e1000e/netdev.c  |   18 ++++++++----------
>  3 files changed, 14 insertions(+), 16 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN as a part of max frame size

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Alexander Duyck
> Sent: Wednesday, April 08, 2015 2:03 PM
> To: intel-wired-lan at lists.osuosl.org; Kirsher, Jeffrey T
> Cc: netdev at vger.kernel.org; mike at cchtml.com; htodd at twofifty.com
> Subject: [Intel-wired-lan] [PATCH] e1000e: Cleanup handling of VLAN_HLEN
> as a part of max frame size
> 
> When the VLAN_HLEN was added to the calculation for the maximum frame size
> there seems to have been a number of issues added to the driver.
> 
> The first issue is that in some cases the maximum frame size for a device
> never really reached the actual maximum frame size as the VLAN header
> length was not included the calculation for that value.  As a result some
> parts only supported a maximum frame size of either 1496 in the case of
> parts that didn't support jumbo frames, and 8996 in the case of the parts
> that do.
> 
> The second issue is the fact that there were several checks that weren't
> updated so as a result setting an MTU of 1500 was treated as enabling
> jumbo
> frames as the calculated value was 1522 instead of 1518.  I have addressed
> those by replacing ETH_FRAME_LEN with VLAN_ETH_FRAME_LEN where
> appropriate.
> 
> The final issue was the fact that lowering the MTU below 1500 would cause
> the driver to allocate 2K buffers for the rings.  This is an old issue
> that
> was fixed several years ago in igb/ixgbe and I am addressing now by just
> replacing == with a <= so that we always just round up to 1522 for
> anything
> that isn't a jumbo frame.
> 
> Fixes: c751a3d58cf2d ("e1000e: Correctly include VLAN_HLEN when changing
> interface MTU")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
> 
> I have only build tested this though I am 99% sure the fixes here are
> correct.  This patch should fix issues on 82573 and ich8 w/ setting an MTU
> of 1500, and for the PCH series w/ setting an MTU of 9000.
> 
> I assume I can get away with bumping the max_hw_frame_size for the PCH
> parts from 9018 to 9022 based on the fact that the Windows INF for the
> parts
> lists supporting either 1514, 4088, and 9014 all of which exclude the 8
> bytes for CRC and VLAN header.
> 
>  drivers/net/ethernet/intel/e1000e/82571.c   |    2 +-
>  drivers/net/ethernet/intel/e1000e/ich8lan.c |   10 +++++-----
>  drivers/net/ethernet/intel/e1000e/netdev.c  |   18 ++++++++----------
>  3 files changed, 14 insertions(+), 16 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>