[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Sasha Neftin]

commit f38b782dccab ("igc: Add support for TSO")
Add option to setting transmit command (TUCMD) of the context
descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.

Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
 drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 6468253ec4f5..b70651c42098 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -529,6 +529,7 @@
 #define IGC_VLAPQF_QUEUE_MASK	0x03
 
 #define IGC_ADVTXD_MACLEN_SHIFT		9  /* Adv ctxt desc mac len shift */
+#define IGC_ADVTXD_TUCMD_L4T_UDP	0x00000000  /* L4 Packet TYPE of UDP */
 #define IGC_ADVTXD_TUCMD_IPV4		0x00000400  /* IP Packet Type:1=IPv4 */
 #define IGC_ADVTXD_TUCMD_L4T_TCP	0x00000800  /* L4 Packet Type of TCP */
 #define IGC_ADVTXD_TUCMD_L4T_SCTP	0x00001000 /* L4 packet TYPE of SCTP */
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index dec6f5a4f03f..660b14abce1d 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1231,7 +1231,8 @@ static int igc_tso(struct igc_ring *tx_ring,
 	l4.hdr = skb_checksum_start(skb);
 
 	/* ADV DTYP TUCMD MKRLOC/ISCSIHEDLEN */
-	type_tucmd = IGC_ADVTXD_TUCMD_L4T_TCP;
+	type_tucmd = (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) ?
+		      IGC_ADVTXD_TUCMD_L4T_UDP : IGC_ADVTXD_TUCMD_L4T_TCP;
 
 	/* initialize outer IP header fields */
 	if (ip.v4->version == 4) {
-- 
2.11.0

[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Jesse Brandeburg]

On Wed, 5 Feb 2020 14:31:15 +0200 Sasha wrote:
> commit f38b782dccab ("igc: Add support for TSO")

is that a Fixes tag?

> Add option to setting transmit command (TUCMD) of the context
> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.

You said what you did but not why.

[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Neftin, Sasha]

On 2/5/2020 21:03, Jesse Brandeburg wrote:
> On Wed, 5 Feb 2020 14:31:15 +0200 Sasha wrote:
>> commit f38b782dccab ("igc: Add support for TSO")
> 
> is that a Fixes tag?
> 
no, this is previous commit for support TSO
>> Add option to setting transmit command (TUCMD) of the context
>> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
> 
> You said what you did but not why.
> 
I would align the igc driver with the same approach as igb and ixgbe 
drivers. I referred to their code.

[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Sasha Neftin
> Sent: Wednesday, February 5, 2020 4:31 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
> support for TSO
> 
> commit f38b782dccab ("igc: Add support for TSO")
> Add option to setting transmit command (TUCMD) of the context
> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
> 
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
>  drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
Aside from possibly including the responses to Jesse's questions on this...
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Alexander Duyck]

On Wed, Feb 12, 2020 at 2:39 PM Brown, Aaron F <aaron.f.brown@intel.com> wrote:
>
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Sasha Neftin
> > Sent: Wednesday, February 5, 2020 4:31 AM
> > To: intel-wired-lan at lists.osuosl.org
> > Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
> > support for TSO
> >
> > commit f38b782dccab ("igc: Add support for TSO")
> > Add option to setting transmit command (TUCMD) of the context
> > descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
> >
> > Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
> >  drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> Aside from possibly including the responses to Jesse's questions on this...
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>

I'm not sure the patch makes any sense. Does the driver support UDP
GSO? I don't see the feature flag (NETIF_F_GSO_UDP_L4) anywhere that
enables it.

If it doesn't enable it then it doesn't make much sense to update the
code to add this one bit until it does since you can't actually test
it as the stack will not ask you to segment UDP frames.

[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Neftin, Sasha]

On 2/13/2020 00:47, Alexander Duyck wrote:
> On Wed, Feb 12, 2020 at 2:39 PM Brown, Aaron F <aaron.f.brown@intel.com> wrote:
>>
>>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>>> Sasha Neftin
>>> Sent: Wednesday, February 5, 2020 4:31 AM
>>> To: intel-wired-lan at lists.osuosl.org
>>> Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
>>> support for TSO
>>>
>>> commit f38b782dccab ("igc: Add support for TSO")
>>> Add option to setting transmit command (TUCMD) of the context
>>> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
>>>
>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
>>>   drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>> Aside from possibly including the responses to Jesse's questions on this...
>> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> 
> I'm not sure the patch makes any sense. Does the driver support UDP
> GSO? I don't see the feature flag (NETIF_F_GSO_UDP_L4) anywhere that
> enables it.
> 
Yes, it is supported. The MAC is similar to igb from segmentation 
perspectives. I missed add it. Please, let me finish some testing and I 
will add NETIF_F_GSO_UDP_L4 flag to _features_check and _probe methods. 
I will process another patch with complements.
> If it doesn't enable it then it doesn't make much sense to update the
> code to add this one bit until it does since you can't actually test
> it as the stack will not ask you to segment UDP frames.
> 

[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Alexander Duyck]

On Thu, Feb 13, 2020 at 12:02 AM Neftin, Sasha <sasha.neftin@intel.com> wrote:
>
> On 2/13/2020 00:47, Alexander Duyck wrote:
> > On Wed, Feb 12, 2020 at 2:39 PM Brown, Aaron F <aaron.f.brown@intel.com> wrote:
> >>
> >>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> >>> Sasha Neftin
> >>> Sent: Wednesday, February 5, 2020 4:31 AM
> >>> To: intel-wired-lan at lists.osuosl.org
> >>> Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
> >>> support for TSO
> >>>
> >>> commit f38b782dccab ("igc: Add support for TSO")
> >>> Add option to setting transmit command (TUCMD) of the context
> >>> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
> >>>
> >>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> >>> ---
> >>>   drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
> >>>   drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
> >>>   2 files changed, 3 insertions(+), 1 deletion(-)
> >>>
> >> Aside from possibly including the responses to Jesse's questions on this...
> >> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> >
> > I'm not sure the patch makes any sense. Does the driver support UDP
> > GSO? I don't see the feature flag (NETIF_F_GSO_UDP_L4) anywhere that
> > enables it.
> >
> Yes, it is supported. The MAC is similar to igb from segmentation
> perspectives. I missed add it. Please, let me finish some testing and I
> will add NETIF_F_GSO_UDP_L4 flag to _features_check and _probe methods.

Thats fine. I just wouldn't submit this patch as it is. It should be
adding the NETIF_F_GSO_UDP_L4 flag so you can actually test the code
and verify the hardware is handling it correctly.

[Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add support for TSO

[Neftin, Sasha]

On 2/13/2020 18:59, Alexander Duyck wrote:
> On Thu, Feb 13, 2020 at 12:02 AM Neftin, Sasha <sasha.neftin@intel.com> wrote:
>>
>> On 2/13/2020 00:47, Alexander Duyck wrote:
>>> On Wed, Feb 12, 2020 at 2:39 PM Brown, Aaron F <aaron.f.brown@intel.com> wrote:
>>>>
>>>>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>>>>> Sasha Neftin
>>>>> Sent: Wednesday, February 5, 2020 4:31 AM
>>>>> To: intel-wired-lan at lists.osuosl.org
>>>>> Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
>>>>> support for TSO
>>>>>
>>>>> commit f38b782dccab ("igc: Add support for TSO")
>>>>> Add option to setting transmit command (TUCMD) of the context
>>>>> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
>>>>>
>>>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>>>> ---
>>>>>    drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
>>>>>    drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
>>>>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>> Aside from possibly including the responses to Jesse's questions on this...
>>>> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>>>
>>> I'm not sure the patch makes any sense. Does the driver support UDP
>>> GSO? I don't see the feature flag (NETIF_F_GSO_UDP_L4) anywhere that
>>> enables it.
>>>
>> Yes, it is supported. The MAC is similar to igb from segmentation
>> perspectives. I missed add it. Please, let me finish some testing and I
>> will add NETIF_F_GSO_UDP_L4 flag to _features_check and _probe methods.
> 
> Thats fine. I just wouldn't submit this patch as it is. It should be
> adding the NETIF_F_GSO_UDP_L4 flag so you can actually test the code
> and verify the hardware is handling it correctly.
> 
Thanks Alex. I will address fix in v2.