Patchwork [RFC] : e1000: allow VLAN devices to use TSO and CSUM offload

login
register
mail settings
Submitter Patrick McHardy
Date Oct. 10, 2008, 2:39 p.m.
Message ID <48EF690F.5070105@trash.net>
Download mbox | patch
Permalink /patch/3801/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Patrick McHardy - Oct. 10, 2008, 2:39 p.m.
Patrick McHardy wrote:
> This patch changes e1000 to set vlan_features so TSO and CSUM
> offload can be used by VLAN devices, similar as with the other
> Intel drivers.
> 
> Only RFC because I don't know whether there is buggy hardware
> that doesn't support this properly, but feel free to apply in
> case there isn't.

I never received a response to this. Any comments?
commit 9648c6ef22e4f6e14ed5b5d91d9809779bb520c7
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed Jul 16 11:45:09 2008 +0200

    e1000: allow VLAN devices to use TSO and CSUM offload
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>
Jesse Brandeburg - Oct. 10, 2008, 5:48 p.m.
Patrick McHardy wrote:
> Patrick McHardy wrote:
>> This patch changes e1000 to set vlan_features so TSO and CSUM
>> offload can be used by VLAN devices, similar as with the other Intel
>> drivers. 
>> 
>> Only RFC because I don't know whether there is buggy hardware
>> that doesn't support this properly, but feel free to apply in
>> case there isn't.
> 
> I never received a response to this. Any comments?

Sorry to delay the reply, the only one that stands out to me as a
possible problem with the devices that e1000 still supports is the
NETIF_F_TSO6.

I know that some of our older parts like 82544 don't support TSO6.

As I read: these should work:
82546GB/EB, 82545GM/EM, 82541(PI/GI/EI), 82541ER, 82547GI/EI, and
82540EP/EM

These parts should all work (at least as advertised in the software
reference manual available at
http://superb-east.dl.sourceforge.net/sourceforge/e1000/OpenSDM_8254x-37
.pdf)

with the exception of 82544 GC/EI

So as it stands your patch will not work on some hardware.  Also, would
we need to hook the ethtool methods such that if a user disables TSO it
will disable vlan_features(TSO) as well?

Jesse



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Brandeburg - Oct. 10, 2008, 5:50 p.m.
Brandeburg, Jesse wrote:
> Patrick McHardy wrote:
>> Patrick McHardy wrote:
>>> This patch changes e1000 to set vlan_features so TSO and CSUM
>>> offload can be used by VLAN devices, similar as with the other
>>> Intel drivers. 
>>> 
>>> Only RFC because I don't know whether there is buggy hardware
>>> that doesn't support this properly, but feel free to apply in
>>> case there isn't.
>> 
>> I never received a response to this. Any comments?
> 
> Sorry to delay the reply, the only one that stands out to me as a
> possible problem with the devices that e1000 still supports is the
> NETIF_F_TSO6.  
> 
> I know that some of our older parts like 82544 don't support TSO6.
> 
> As I read: these should work:
> 82546GB/EB, 82545GM/EM, 82541(PI/GI/EI), 82541ER, 82547GI/EI, and
> 82540EP/EM 
> 
> These parts should all work (at least as advertised in the software
> reference manual available at
>
http://superb-east.dl.sourceforge.net/sourceforge/e1000/OpenSDM_8254x-37
.pdf)
> 
> with the exception of 82544 GC/EI
> 
> So as it stands your patch will not work on some hardware.  Also,
> would we need to hook the ethtool methods such that if a user
> disables TSO it will disable vlan_features(TSO) as well?  

Oh, and the 82542 and 82543 based parts don't do this either, they
aren't covered by the manual above.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Oct. 10, 2008, 5:59 p.m.
On Fri, 2008-10-10 at 10:48 -0700, Brandeburg, Jesse wrote:
> Patrick McHardy wrote:
> > Patrick McHardy wrote:
> >> This patch changes e1000 to set vlan_features so TSO and CSUM
> >> offload can be used by VLAN devices, similar as with the other Intel
> >> drivers. 
> >> 
> >> Only RFC because I don't know whether there is buggy hardware
> >> that doesn't support this properly, but feel free to apply in
> >> case there isn't.
> > 
> > I never received a response to this. Any comments?
> 
> Sorry to delay the reply, the only one that stands out to me as a
> possible problem with the devices that e1000 still supports is the
> NETIF_F_TSO6.

vlan_features is a mask for features.  So long as all parts that do
support TSO-IPv6 also support it for VLAN-tagged packets, it should be
safe to set NETIF_F_TSO6 in vlan_features unconditionally.

Ben.
David Miller - Oct. 10, 2008, 7:09 p.m.
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 10 Oct 2008 18:59:36 +0100

> On Fri, 2008-10-10 at 10:48 -0700, Brandeburg, Jesse wrote:
> > Patrick McHardy wrote:
> > > Patrick McHardy wrote:
> > >> This patch changes e1000 to set vlan_features so TSO and CSUM
> > >> offload can be used by VLAN devices, similar as with the other Intel
> > >> drivers. 
> > >> 
> > >> Only RFC because I don't know whether there is buggy hardware
> > >> that doesn't support this properly, but feel free to apply in
> > >> case there isn't.
> > > 
> > > I never received a response to this. Any comments?
> > 
> > Sorry to delay the reply, the only one that stands out to me as a
> > possible problem with the devices that e1000 still supports is the
> > NETIF_F_TSO6.
> 
> vlan_features is a mask for features.  So long as all parts that do
> support TSO-IPv6 also support it for VLAN-tagged packets, it should be
> safe to set NETIF_F_TSO6 in vlan_features unconditionally.

Right, it's an additional mask which gets and'd with the normal
device feature bits.

Jesse, any objections, given that?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Brandeburg - Oct. 10, 2008, 10:01 p.m.
David Miller wrote:
>> vlan_features is a mask for features.  So long as all parts that do
>> support TSO-IPv6 also support it for VLAN-tagged packets, it should
>> be safe to set NETIF_F_TSO6 in vlan_features unconditionally.
> 
> Right, it's an additional mask which gets and'd with the normal
> device feature bits.
> 
> Jesse, any objections, given that?

No, once that is explained I think it gets rid of any objections.  We
are going to test the patch now, so would you mind if you waited to take
it until jkirsher pushes it to you?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 10, 2008, 10:11 p.m.
From: "Brandeburg, Jesse" <jesse.brandeburg@intel.com>
Date: Fri, 10 Oct 2008 15:01:09 -0700

> David Miller wrote:
> >> vlan_features is a mask for features.  So long as all parts that do
> >> support TSO-IPv6 also support it for VLAN-tagged packets, it should
> >> be safe to set NETIF_F_TSO6 in vlan_features unconditionally.
> > 
> > Right, it's an additional mask which gets and'd with the normal
> > device feature bits.
> > 
> > Jesse, any objections, given that?
> 
> No, once that is explained I think it gets rid of any objections.  We
> are going to test the patch now, so would you mind if you waited to take
> it until jkirsher pushes it to you?

Sure, no problem.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Patrick McHardy - Oct. 11, 2008, 2:35 p.m.
Brandeburg, Jesse wrote:
>> These parts should all work (at least as advertised in the software
>> reference manual available at
>>
> http://superb-east.dl.sourceforge.net/sourceforge/e1000/OpenSDM_8254x-37
> .pdf)
>> with the exception of 82544 GC/EI
>>
>> So as it stands your patch will not work on some hardware.  Also,
>> would we need to hook the ethtool methods such that if a user
>> disables TSO it will disable vlan_features(TSO) as well?  
> 
> Oh, and the 82542 and 82543 based parts don't do this either, they
> aren't covered by the manual above.

Thanks for the pointer (and for pushing the patch already).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index f8df8bd..b6b3135 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -1011,6 +1011,11 @@  e1000_probe(struct pci_dev *pdev,
 
 	netdev->features |= NETIF_F_LLTX;
 
+	netdev->vlan_features |= NETIF_F_TSO;
+	netdev->vlan_features |= NETIF_F_TSO6;
+	netdev->vlan_features |= NETIF_F_HW_CSUM;
+	netdev->vlan_features |= NETIF_F_SG;
+
 	adapter->en_mng_pt = e1000_enable_mng_pass_thru(&adapter->hw);
 
 	/* initialize eeprom parameters */