Re: [net-next 08/14] ixgbe: Add support for generic Tx checksums

[Jeff Kirsher <jeffrey.t.kirsher@intel.com>]

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

On Mon, 2016-04-04 at 15:54 -0700, Alexander Duyck wrote:
> On Mon, Apr 4, 2016 at 3:32 PM, Samudrala, Sridhar
> <sridhar.samudrala@intel.com> wrote:
> > On 4/4/2016 3:14 PM, Jeff Kirsher wrote:
> >>
> >> From: Alexander Duyck <aduyck@mirantis.com>
> >>
> >> This patch adds support for generic Tx checksums to the ixgbe
> driver.  It
> >> turns out this is actually pretty easy after going over the
> datasheet as
> >> we
> >> were doing a number of steps we didn't need to.
> >>
> >> In order to perform a Tx checksum for an L4 header we need to fill
> in the
> >> following fields in the Tx descriptor:
> >>    MACLEN (maximum of 127), retrieved from:
> >>                 skb_network_offset()
> >>    IPLEN  (maximum of 511), retrieved from:
> >>                 skb_checksum_start_offset() - skb_network_offset()
> >>    TUCMD.L4T indicates offset and if checksum or crc32c, based on:
> >>                 skb->csum_offset
> >>
> >> The added advantage to doing this is that we can support inner
> checksum
> >> offloads for tunnels and MPLS while still being able to
> transparently
> >> insert VLAN tags.
> >>
> >> I also took the opportunity to clean-up many of the feature flag
> >> configuration bits to make them a bit more consistent between
> drivers.
> >>
> >> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> >> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> >> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >> ---
> >>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 164
> >> +++++++++-----------------
> >>   1 file changed, 59 insertions(+), 105 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >> index bce5737..8d248c8 100644
> >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> >> @@ -7202,103 +7202,61 @@ static int ixgbe_tso(struct ixgbe_ring
> *tx_ring,
> >>         return 1;
> >>   }
> >>
> >
> >
> > <snip>
> >
> >
> >
> >> @@ -9190,41 +9148,37 @@ skip_sriov:
> >>     #endif
> >>         netdev->features = NETIF_F_SG |
> >> -                          NETIF_F_IP_CSUM |
> >> -                          NETIF_F_IPV6_CSUM |
> >> -                          NETIF_F_HW_VLAN_CTAG_TX |
> >> -                          NETIF_F_HW_VLAN_CTAG_RX |
> >>                            NETIF_F_TSO |
> >>                            NETIF_F_TSO6 |
> >>                            NETIF_F_RXHASH |
> >> -                          NETIF_F_RXCSUM;
> >> -
> >> -       netdev->hw_features = netdev->features |
> NETIF_F_HW_L2FW_DOFFLOAD;
> >> +                          NETIF_F_RXCSUM |
> >> +                          NETIF_F_HW_CSUM |
> >> +                          NETIF_F_SCTP_CRC |
> >> +                          NETIF_F_HW_VLAN_CTAG_TX |
> >> +                          NETIF_F_HW_VLAN_CTAG_RX;
> >>   -     switch (adapter->hw.mac.type) {
> >> -       case ixgbe_mac_82599EB:
> >> -       case ixgbe_mac_X540:
> >> -       case ixgbe_mac_X550:
> >> -       case ixgbe_mac_X550EM_x:
> >> +       if (hw->mac.type >= ixgbe_mac_82599EB)
> >>                 netdev->features |= NETIF_F_SCTP_CRC;
> >> -               netdev->hw_features |= NETIF_F_SCTP_CRC |
> >> -                                      NETIF_F_NTUPLE |
> >> -                                      NETIF_F_HW_TC;
> >> -               break;
> >> -       default:
> >> -               break;
> >> -       }
> >>   -     netdev->hw_features |= NETIF_F_RXALL;
> >> +       /* copy netdev features into list of user selectable
> features */
> >> +       netdev->hw_features |= netdev->features;
> >> +       netdev->hw_features |= NETIF_F_RXALL |
> >> +                              NETIF_F_HW_L2FW_DOFFLOAD;
> >> +
> >> +       if (hw->mac.type >= ixgbe_mac_82599EB)
> >> +               netdev->hw_features |= NETIF_F_NTUPLE;
> >
> >
> > looks like the cleanup missed moving NETIF_F_HW_TC flag here that
> enables
> > cls_u32 offloads via TC.
> 
> Back when I submitted this patch the NETIF_F_HW_TC flag didn't exist.
> I'm pretty sure this is probably a case of a patch getting mangled
> after sitting in the tree for so long.
> 
> In addition I just noticed there is a bug in the setting of the
> SCTP_CRC flag as well.  It is set twice when we should only be
> setting
> it in the if statement.  It is just a quick 2 line fix that is
> needed.
> I can either submit a follow-up or I can respin the original patch
> real quick so that it applies on top of the current ixgbe queue.

It was because Sridhar needed his patch applied before yours as a fix,
that was the issue.  Just send me the two line fix you have for the
patch and I will re-spin the patch with Sridhar's needed fix as well.

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