[PATCH] igc: enable HW VLAN insertion/stripping by default

[PATCH] igc: enable HW VLAN insertion/stripping by default

[Rui Salvaterra]

This is enabled by default in other Intel drivers I've checked (e1000, e1000e,
iavf, igb and ice). Fixes an out-of-the-box performance issue when running
OpenWrt on typical mini-PCs with igc-supported Ethernet controllers and 802.1Q
VLAN configurations, as ethtool isn't part of the default packages and sane
defaults are expected.

In my specific case, with an Intel N100-based machine with four I226-V Ethernet
controllers, my upload performance increased from under 30 Mb/s to the expected
~1 Gb/s.

Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
---

This patch cost me two afternoons of network debugging, last weekend. Is there
any plausible reason why VLAN acceleration wasn't enabled by default for this
driver, specifically?

 drivers/net/ethernet/intel/igc/igc_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 84307bb7313e..6fef763239bc 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7049,6 +7049,9 @@ static int igc_probe(struct pci_dev *pdev,
 	netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
 			       NETDEV_XDP_ACT_XSK_ZEROCOPY;
 
+	/* enable hardware VLAN insertion/stripping by default */
+	netdev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
+
 	/* MTU range: 68 - 9216 */
 	netdev->min_mtu = ETH_MIN_MTU;
 	netdev->max_mtu = MAX_STD_JUMBO_FRAME_SIZE;
-- 
2.48.1

Re: [PATCH] igc: enable HW VLAN insertion/stripping by default

[Simon Horman]

On Fri, Mar 07, 2025 at 11:02:39AM +0000, Rui Salvaterra wrote:
> This is enabled by default in other Intel drivers I've checked (e1000, e1000e,
> iavf, igb and ice). Fixes an out-of-the-box performance issue when running
> OpenWrt on typical mini-PCs with igc-supported Ethernet controllers and 802.1Q
> VLAN configurations, as ethtool isn't part of the default packages and sane
> defaults are expected.
> 
> In my specific case, with an Intel N100-based machine with four I226-V Ethernet
> controllers, my upload performance increased from under 30 Mb/s to the expected
> ~1 Gb/s.
> 
> Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
> ---
> 
> This patch cost me two afternoons of network debugging, last weekend. Is there
> any plausible reason why VLAN acceleration wasn't enabled by default for this
> driver, specifically?

Having looked over this I am also curious to know the answer to that question.
This does seem to be the default for other Intel drivers (at least).

Re: [PATCH] igc: enable HW VLAN insertion/stripping by default

[Rui Salvaterra]

Hi, Simon,

On Tue, 11 Mar 2025 at 13:52, Simon Horman <horms@kernel.org> wrote:
>
> Having looked over this I am also curious to know the answer to that question.
> This does seem to be the default for other Intel drivers (at least).

Well, r8169 also enables it, and RealTek controllers are used everywhere. :)

Kind regards,
Rui Salvaterra

Re: [PATCH] igc: enable HW VLAN insertion/stripping by default

[Tony Nguyen]

On 3/11/2025 6:59 AM, Rui Salvaterra wrote:
> Hi, Simon,
> 
> On Tue, 11 Mar 2025 at 13:52, Simon Horman <horms@kernel.org> wrote:
>>
>> Having looked over this I am also curious to know the answer to that question.
>> This does seem to be the default for other Intel drivers (at least).

Unfortunately, I'm unable check with the original author or those 
involved at the time. From asking around it sounds like there may have 
been some initial issues when implementing this; my theory is this was 
off by default so that it would minimize the affects if additional 
issues were discovered.

I see that you missed Intel Wired LAN (intel-wired-lan@lists.osuosl.org) 
on the patch. Could you resend with the list included? Also, if you 
could make it 'PATCH iwl-next' to target the Intel -next tree as that 
seems the appropriate tree.

Thanks,
Tony

> Well, r8169 also enables it, and RealTek controllers are used everywhere. :)
> 
> Kind regards,
> Rui Salvaterra

Re: [PATCH] igc: enable HW VLAN insertion/stripping by default

[Rui Salvaterra]

Hi, Tony,

On Wed, 12 Mar 2025 at 21:43, Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>
> Unfortunately, I'm unable check with the original author or those
> involved at the time. From asking around it sounds like there may have
> been some initial issues when implementing this; my theory is this was
> off by default so that it would minimize the affects if additional
> issues were discovered.

I thought about that possibility, but in the end it didn't seem
logical to me. If the feature was WIP and/or broken in some way, why
would the user be allowed to enable it via ethtool, ever since it was
first implemented, in commit 8d7449630e3450bc0546dc0cb692fbb57d1852c0
(almost four years ago)?

> I see that you missed Intel Wired LAN (intel-wired-lan@lists.osuosl.org)
> on the patch. Could you resend with the list included? Also, if you
> could make it 'PATCH iwl-next' to target the Intel -next tree as that
> seems the appropriate tree.

Oh. That list is marked as moderated, I (wrongly, for sure) assumed
there would be restrictions when mailing to it. I'll resend correctly
soon.

Kind regards,
Rui Salvaterra

Re: [PATCH] igc: enable HW VLAN insertion/stripping by default

[Tony Nguyen]

On 3/13/2025 2:23 AM, Rui Salvaterra wrote:
> Hi, Tony,
> 
> On Wed, 12 Mar 2025 at 21:43, Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
>>
>> Unfortunately, I'm unable check with the original author or those
>> involved at the time. From asking around it sounds like there may have
>> been some initial issues when implementing this; my theory is this was
>> off by default so that it would minimize the affects if additional
>> issues were discovered.
> 
> I thought about that possibility, but in the end it didn't seem
> logical to me. If the feature was WIP and/or broken in some way, why
> would the user be allowed to enable it via ethtool, ever since it was
> first implemented, in commit 8d7449630e3450bc0546dc0cb692fbb57d1852c0
> (almost four years ago)?

It would allow users to opt-in and use the feature; if an adverse 
problem was later found, it could be easily mitigated by not turning the 
feature on. If it were on by default, and a problem found, there would a 
larger impact. As you mentioned, since the feature has now been in 
existence for many years, it's probably safe to have on by default.

>> I see that you missed Intel Wired LAN (intel-wired-lan@lists.osuosl.org)
>> on the patch. Could you resend with the list included? Also, if you
>> could make it 'PATCH iwl-next' to target the Intel -next tree as that
>> seems the appropriate tree.
> 
> Oh. That list is marked as moderated, I (wrongly, for sure) assumed
> there would be restrictions when mailing to it. I'll resend correctly
> soon.

Thank you.

- Tony