| Message ID | 20211006201308.2492890-2-ioana.ciornei@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: enetc: add support for software TSO | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 48 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Wed, 6 Oct 2021 23:13:07 +0300 Ioana Ciornei wrote: > This is just a preparation patch for software TSO in the enetc driver. > Unfortunately, ENETC does not support Tx checksum offload which would > normally render TSO, even software, impossible. > > Declare NETIF_F_IP_CSUM as part of the feature set and do it at driver > level using skb_csum_hwoffload_help() so that we can move forward and > also add support for TSO in the next patch. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> Did you choose NETIF_F_IP_CSUM intentionally? It'll only support IPv4, and since you always fall back to SW I'd think NETIF_F_HW_CSUM makes more sense. > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 3cbfa8b4e265..a92bfd660f22 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -319,7 +319,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, > { > struct enetc_ndev_priv *priv = netdev_priv(ndev); > struct enetc_bdr *tx_ring; > - int count; > + int count, err; > > /* Queue one-step Sync packet if already locked */ > if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) { > @@ -342,6 +342,12 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, > return NETDEV_TX_BUSY; > } > > + if (skb->ip_summed == CHECKSUM_PARTIAL) { > + err = skb_csum_hwoffload_help(skb, 0); > + if (err) > + goto drop_packet_err; > + } Any reason no to call skb_checksum_help() directly?
On Wed, Oct 06, 2021 at 05:24:18PM -0700, Jakub Kicinski wrote: > On Wed, 6 Oct 2021 23:13:07 +0300 Ioana Ciornei wrote: > > This is just a preparation patch for software TSO in the enetc driver. > > Unfortunately, ENETC does not support Tx checksum offload which would > > normally render TSO, even software, impossible. > > > > Declare NETIF_F_IP_CSUM as part of the feature set and do it at driver > > level using skb_csum_hwoffload_help() so that we can move forward and > > also add support for TSO in the next patch. > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > Did you choose NETIF_F_IP_CSUM intentionally? > It'll only support IPv4, and since you always fall back to SW > I'd think NETIF_F_HW_CSUM makes more sense. Somewhat intentionally, yes. If I would use NETIF_F_HW_CSUM, as I understand it, the GSO path, added in the next patch, would have to compute the checksum not only for IPv6 but also for any other protocols other than UDP and TCP (which currently it supports). I just didn't look into that at the moment. > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > > index 3cbfa8b4e265..a92bfd660f22 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > @@ -319,7 +319,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, > > { > > struct enetc_ndev_priv *priv = netdev_priv(ndev); > > struct enetc_bdr *tx_ring; > > - int count; > > + int count, err; > > > > /* Queue one-step Sync packet if already locked */ > > if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) { > > @@ -342,6 +342,12 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, > > return NETDEV_TX_BUSY; > > } > > > > + if (skb->ip_summed == CHECKSUM_PARTIAL) { > > + err = skb_csum_hwoffload_help(skb, 0); > > + if (err) > > + goto drop_packet_err; > > + } > > Any reason no to call skb_checksum_help() directly? No, no reason. Will change. Ioana
On Thu, Oct 07, 2021 at 09:47:20AM +0300, Ioana Ciornei wrote: > On Wed, Oct 06, 2021 at 05:24:18PM -0700, Jakub Kicinski wrote: > > On Wed, 6 Oct 2021 23:13:07 +0300 Ioana Ciornei wrote: > > > This is just a preparation patch for software TSO in the enetc driver. > > > Unfortunately, ENETC does not support Tx checksum offload which would > > > normally render TSO, even software, impossible. > > > > > > Declare NETIF_F_IP_CSUM as part of the feature set and do it at driver > > > level using skb_csum_hwoffload_help() so that we can move forward and > > > also add support for TSO in the next patch. > > > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > > > Did you choose NETIF_F_IP_CSUM intentionally? > > It'll only support IPv4, and since you always fall back to SW > > I'd think NETIF_F_HW_CSUM makes more sense. > > Somewhat intentionally, yes. > > If I would use NETIF_F_HW_CSUM, as I understand it, the GSO path, added > in the next patch, would have to compute the checksum not only for IPv6 > but also for any other protocols other than UDP and TCP (which currently > it supports). > I just didn't look into that at the moment. > Now that I think of it, you have a point with declaring NETIF_F_HW_CSUM. In the non-GSO case, skb_checksum_help() will be able to handle anything that we throw at it. On the GSO case, only skbs with TCP over IPv4 or IPv6 (depending on what we declare in the features - TSO/TSO6) will be received - which the csum code can handle. Anyhow, I'll change this to use NETIF_F_HW_CSUM instead of NETIF_F_IP_CSUM and I'll also look into IPv6 for the TSO part. Thanks, Ioana
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 3cbfa8b4e265..a92bfd660f22 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -319,7 +319,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, { struct enetc_ndev_priv *priv = netdev_priv(ndev); struct enetc_bdr *tx_ring; - int count; + int count, err; /* Queue one-step Sync packet if already locked */ if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) { @@ -342,6 +342,12 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, return NETDEV_TX_BUSY; } + if (skb->ip_summed == CHECKSUM_PARTIAL) { + err = skb_csum_hwoffload_help(skb, 0); + if (err) + goto drop_packet_err; + } + enetc_lock_mdio(); count = enetc_map_tx_buffs(tx_ring, skb); enetc_unlock_mdio(); diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c index 628a74ba630c..7ac276f8ee4f 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c @@ -759,10 +759,12 @@ static void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, ndev->hw_features = NETIF_F_SG | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | - NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK; + NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK | + NETIF_F_IP_CSUM; ndev->features = NETIF_F_HIGHDMA | NETIF_F_SG | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_TX | - NETIF_F_HW_VLAN_CTAG_RX; + NETIF_F_HW_VLAN_CTAG_RX | + NETIF_F_IP_CSUM; if (si->num_rss) ndev->hw_features |= NETIF_F_RXHASH; diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c index 0704f6bf12fd..2166a436f818 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c @@ -122,10 +122,12 @@ static void enetc_vf_netdev_setup(struct enetc_si *si, struct net_device *ndev, ndev->hw_features = NETIF_F_SG | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_TX | - NETIF_F_HW_VLAN_CTAG_RX; + NETIF_F_HW_VLAN_CTAG_RX | + NETIF_F_IP_CSUM; ndev->features = NETIF_F_HIGHDMA | NETIF_F_SG | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_CTAG_TX | - NETIF_F_HW_VLAN_CTAG_RX; + NETIF_F_HW_VLAN_CTAG_RX | + NETIF_F_IP_CSUM; if (si->num_rss) ndev->hw_features |= NETIF_F_RXHASH;
This is just a preparation patch for software TSO in the enetc driver. Unfortunately, ENETC does not support Tx checksum offload which would normally render TSO, even software, impossible. Declare NETIF_F_IP_CSUM as part of the feature set and do it at driver level using skb_csum_hwoffload_help() so that we can move forward and also add support for TSO in the next patch. Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- drivers/net/ethernet/freescale/enetc/enetc.c | 8 +++++++- drivers/net/ethernet/freescale/enetc/enetc_pf.c | 6 ++++-- drivers/net/ethernet/freescale/enetc/enetc_vf.c | 6 ++++-- 3 files changed, 15 insertions(+), 5 deletions(-)