| Message ID | 20241204052932.112446-2-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add more feautues for ENETC v4 - round 1 | expand |
On Wed, Dec 04, 2024 at 01:29:28PM +0800, Wei Fang wrote: > ENETC rev 4.1 supports TCP and UDP checksum offload for receive, the bit > 108 of the Rx BD will be set if the TCP/UDP checksum is correct. Since > this capability is not defined in register, the rx_csum bit is added to > struct enetc_drvdata to indicate whether the device supports Rx checksum > offload. > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > --- > v2: no changes > v3: no changes > v4: no changes > v5: no changes > v6: no changes > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 14 ++++++++++---- > drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++ > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 ++ > .../net/ethernet/freescale/enetc/enetc_pf_common.c | 3 +++ > 4 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 35634c516e26..3137b6ee62d3 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -1011,10 +1011,15 @@ static void enetc_get_offloads(struct enetc_bdr *rx_ring, > > /* TODO: hashing */ > if (rx_ring->ndev->features & NETIF_F_RXCSUM) { > - u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > - > - skb->csum = csum_unfold((__force __sum16)~htons(inet_csum)); > - skb->ip_summed = CHECKSUM_COMPLETE; > + if (priv->active_offloads & ENETC_F_RXCSUM && > + le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_L4_CSUM_OK) { > + skb->ip_summed = CHECKSUM_UNNECESSARY; > + } else { > + u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > + > + skb->csum = csum_unfold((__force __sum16)~htons(inet_csum)); > + skb->ip_summed = CHECKSUM_COMPLETE; > + } > } Hi Wei, I am wondering about the relationship between the above and hardware support for CHECKSUM_COMPLETE. Prior to this patch CHECKSUM_COMPLETE was always used, which seems desirable. But with this patch, CHECKSUM_UNNECESSARY is conditionally used. If those cases don't work with CHECKSUM_COMPLETE then is this a bug-fix? Or, alternatively, if those cases do work with CHECKSUM_COMPLETE, then I'm unsure why this change is necessary or desirable. It's my understanding that from the Kernel's perspective CHECKSUM_COMPLETE is preferable to CHECKSUM_UNNECESSARY. ...
> -----Original Message----- > From: Simon Horman <horms@kernel.org> > Sent: 2024年12月6日 17:23 > To: Wei Fang <wei.fang@nxp.com> > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > offload for i.MX95 ENETC > > On Wed, Dec 04, 2024 at 01:29:28PM +0800, Wei Fang wrote: > > ENETC rev 4.1 supports TCP and UDP checksum offload for receive, the bit > > 108 of the Rx BD will be set if the TCP/UDP checksum is correct. Since > > this capability is not defined in register, the rx_csum bit is added to > > struct enetc_drvdata to indicate whether the device supports Rx checksum > > offload. > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > --- > > v2: no changes > > v3: no changes > > v4: no changes > > v5: no changes > > v6: no changes > > --- > > drivers/net/ethernet/freescale/enetc/enetc.c | 14 ++++++++++---- > > drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++ > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 ++ > > .../net/ethernet/freescale/enetc/enetc_pf_common.c | 3 +++ > > 4 files changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > b/drivers/net/ethernet/freescale/enetc/enetc.c > > index 35634c516e26..3137b6ee62d3 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > @@ -1011,10 +1011,15 @@ static void enetc_get_offloads(struct enetc_bdr > *rx_ring, > > > > /* TODO: hashing */ > > if (rx_ring->ndev->features & NETIF_F_RXCSUM) { > > - u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > - > > - skb->csum = csum_unfold((__force __sum16)~htons(inet_csum)); > > - skb->ip_summed = CHECKSUM_COMPLETE; > > + if (priv->active_offloads & ENETC_F_RXCSUM && > > + le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_L4_CSUM_OK) > { > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > + } else { > > + u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > + > > + skb->csum = csum_unfold((__force __sum16)~htons(inet_csum)); > > + skb->ip_summed = CHECKSUM_COMPLETE; > > + } > > } > > Hi Wei, > > I am wondering about the relationship between the above and > hardware support for CHECKSUM_COMPLETE. > > Prior to this patch CHECKSUM_COMPLETE was always used, which seems > desirable. But with this patch, CHECKSUM_UNNECESSARY is conditionally used. > > If those cases don't work with CHECKSUM_COMPLETE then is this a bug-fix? > > Or, alternatively, if those cases do work with CHECKSUM_COMPLETE, then > I'm unsure why this change is necessary or desirable. It's my understanding > that from the Kernel's perspective CHECKSUM_COMPLETE is preferable to > CHECKSUM_UNNECESSARY. > > ... Rx checksum offload is a new feature of ENETC v4. We would like to exploit this capability of the hardware to save CPU cycles in calculating and verifying checksum.
On Fri, Dec 06, 2024 at 10:33:15AM +0000, Wei Fang wrote: > > -----Original Message----- > > From: Simon Horman <horms@kernel.org> > > Sent: 2024年12月6日 17:23 > > To: Wei Fang <wei.fang@nxp.com> > > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > > offload for i.MX95 ENETC > > > > On Wed, Dec 04, 2024 at 01:29:28PM +0800, Wei Fang wrote: > > > ENETC rev 4.1 supports TCP and UDP checksum offload for receive, the bit > > > 108 of the Rx BD will be set if the TCP/UDP checksum is correct. Since > > > this capability is not defined in register, the rx_csum bit is added to > > > struct enetc_drvdata to indicate whether the device supports Rx checksum > > > offload. > > > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > > --- > > > v2: no changes > > > v3: no changes > > > v4: no changes > > > v5: no changes > > > v6: no changes > > > --- > > > drivers/net/ethernet/freescale/enetc/enetc.c | 14 ++++++++++---- > > > drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++ > > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 ++ > > > .../net/ethernet/freescale/enetc/enetc_pf_common.c | 3 +++ > > > 4 files changed, 17 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > > b/drivers/net/ethernet/freescale/enetc/enetc.c > > > index 35634c516e26..3137b6ee62d3 100644 > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > > @@ -1011,10 +1011,15 @@ static void enetc_get_offloads(struct enetc_bdr > > *rx_ring, > > > > > > /* TODO: hashing */ > > > if (rx_ring->ndev->features & NETIF_F_RXCSUM) { > > > - u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > - > > > - skb->csum = csum_unfold((__force __sum16)~htons(inet_csum)); > > > - skb->ip_summed = CHECKSUM_COMPLETE; > > > + if (priv->active_offloads & ENETC_F_RXCSUM && > > > + le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_L4_CSUM_OK) > > { > > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > > + } else { > > > + u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > + > > > + skb->csum = csum_unfold((__force __sum16)~htons(inet_csum)); > > > + skb->ip_summed = CHECKSUM_COMPLETE; > > > + } > > > } > > > > Hi Wei, > > > > I am wondering about the relationship between the above and > > hardware support for CHECKSUM_COMPLETE. > > > > Prior to this patch CHECKSUM_COMPLETE was always used, which seems > > desirable. But with this patch, CHECKSUM_UNNECESSARY is conditionally used. > > > > If those cases don't work with CHECKSUM_COMPLETE then is this a bug-fix? > > > > Or, alternatively, if those cases do work with CHECKSUM_COMPLETE, then > > I'm unsure why this change is necessary or desirable. It's my understanding > > that from the Kernel's perspective CHECKSUM_COMPLETE is preferable to > > CHECKSUM_UNNECESSARY. > > > > ... > > Rx checksum offload is a new feature of ENETC v4. We would like to exploit this > capability of the hardware to save CPU cycles in calculating and verifying checksum. > Understood, but CHECKSUM_UNNECESSARY is usually the preferred option as it is more flexible, e.g. allowing low-cost calculation of inner checksums in the presence of encapsulation.
> -----Original Message----- > From: Simon Horman <horms@kernel.org> > Sent: 2024年12月6日 20:31 > To: Wei Fang <wei.fang@nxp.com> > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > offload for i.MX95 ENETC > > On Fri, Dec 06, 2024 at 10:33:15AM +0000, Wei Fang wrote: > > > -----Original Message----- > > > From: Simon Horman <horms@kernel.org> > > > Sent: 2024年12月6日 17:23 > > > To: Wei Fang <wei.fang@nxp.com> > > > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > > > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > > > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev > > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > > > offload for i.MX95 ENETC > > > > > > On Wed, Dec 04, 2024 at 01:29:28PM +0800, Wei Fang wrote: > > > > ENETC rev 4.1 supports TCP and UDP checksum offload for receive, the bit > > > > 108 of the Rx BD will be set if the TCP/UDP checksum is correct. Since > > > > this capability is not defined in register, the rx_csum bit is added to > > > > struct enetc_drvdata to indicate whether the device supports Rx > checksum > > > > offload. > > > > > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > > > --- > > > > v2: no changes > > > > v3: no changes > > > > v4: no changes > > > > v5: no changes > > > > v6: no changes > > > > --- > > > > drivers/net/ethernet/freescale/enetc/enetc.c | 14 > ++++++++++---- > > > > drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++ > > > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 ++ > > > > .../net/ethernet/freescale/enetc/enetc_pf_common.c | 3 +++ > > > > 4 files changed, 17 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > > > b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > index 35634c516e26..3137b6ee62d3 100644 > > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > @@ -1011,10 +1011,15 @@ static void enetc_get_offloads(struct > enetc_bdr > > > *rx_ring, > > > > > > > > /* TODO: hashing */ > > > > if (rx_ring->ndev->features & NETIF_F_RXCSUM) { > > > > - u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > > - > > > > - skb->csum = csum_unfold((__force > __sum16)~htons(inet_csum)); > > > > - skb->ip_summed = CHECKSUM_COMPLETE; > > > > + if (priv->active_offloads & ENETC_F_RXCSUM && > > > > + le16_to_cpu(rxbd->r.flags) & > ENETC_RXBD_FLAG_L4_CSUM_OK) > > > { > > > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > > > + } else { > > > > + u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > > + > > > > + skb->csum = csum_unfold((__force > __sum16)~htons(inet_csum)); > > > > + skb->ip_summed = CHECKSUM_COMPLETE; > > > > + } > > > > } > > > > > > Hi Wei, > > > > > > I am wondering about the relationship between the above and > > > hardware support for CHECKSUM_COMPLETE. > > > > > > Prior to this patch CHECKSUM_COMPLETE was always used, which seems > > > desirable. But with this patch, CHECKSUM_UNNECESSARY is conditionally > used. > > > > > > If those cases don't work with CHECKSUM_COMPLETE then is this a > bug-fix? > > > > > > Or, alternatively, if those cases do work with CHECKSUM_COMPLETE, then > > > I'm unsure why this change is necessary or desirable. It's my understanding > > > that from the Kernel's perspective CHECKSUM_COMPLETE is preferable to > > > CHECKSUM_UNNECESSARY. > > > > > > ... > > > > Rx checksum offload is a new feature of ENETC v4. We would like to exploit > this > > capability of the hardware to save CPU cycles in calculating and verifying > checksum. > > > > Understood, but CHECKSUM_UNNECESSARY is usually the preferred option as > it > is more flexible, e.g. allowing low-cost calculation of inner checksums > in the presence of encapsulation. I think you mean 'CHECKSUM_COMPLETE' is the preferred option. But there is no strong reason against using CHECKSUM_UNNECESSARY. So I hope to keep this patch.
On Fri, Dec 06, 2024 at 12:45:02PM +0000, Wei Fang wrote: > > -----Original Message----- > > From: Simon Horman <horms@kernel.org> > > Sent: 2024年12月6日 20:31 > > To: Wei Fang <wei.fang@nxp.com> > > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > > offload for i.MX95 ENETC > > > > On Fri, Dec 06, 2024 at 10:33:15AM +0000, Wei Fang wrote: > > > > -----Original Message----- > > > > From: Simon Horman <horms@kernel.org> > > > > Sent: 2024年12月6日 17:23 > > > > To: Wei Fang <wei.fang@nxp.com> > > > > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > > > > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > > > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > > > > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev > > > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > > > > offload for i.MX95 ENETC > > > > > > > > On Wed, Dec 04, 2024 at 01:29:28PM +0800, Wei Fang wrote: > > > > > ENETC rev 4.1 supports TCP and UDP checksum offload for receive, the bit > > > > > 108 of the Rx BD will be set if the TCP/UDP checksum is correct. Since > > > > > this capability is not defined in register, the rx_csum bit is added to > > > > > struct enetc_drvdata to indicate whether the device supports Rx > > checksum > > > > > offload. > > > > > > > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > > > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > > > > --- > > > > > v2: no changes > > > > > v3: no changes > > > > > v4: no changes > > > > > v5: no changes > > > > > v6: no changes > > > > > --- > > > > > drivers/net/ethernet/freescale/enetc/enetc.c | 14 > > ++++++++++---- > > > > > drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++ > > > > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 ++ > > > > > .../net/ethernet/freescale/enetc/enetc_pf_common.c | 3 +++ > > > > > 4 files changed, 17 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > > > > b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > index 35634c516e26..3137b6ee62d3 100644 > > > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > @@ -1011,10 +1011,15 @@ static void enetc_get_offloads(struct > > enetc_bdr > > > > *rx_ring, > > > > > > > > > > /* TODO: hashing */ > > > > > if (rx_ring->ndev->features & NETIF_F_RXCSUM) { > > > > > - u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > > > - > > > > > - skb->csum = csum_unfold((__force > > __sum16)~htons(inet_csum)); > > > > > - skb->ip_summed = CHECKSUM_COMPLETE; > > > > > + if (priv->active_offloads & ENETC_F_RXCSUM && > > > > > + le16_to_cpu(rxbd->r.flags) & > > ENETC_RXBD_FLAG_L4_CSUM_OK) > > > > { > > > > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > > > > + } else { > > > > > + u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > > > + > > > > > + skb->csum = csum_unfold((__force > > __sum16)~htons(inet_csum)); > > > > > + skb->ip_summed = CHECKSUM_COMPLETE; > > > > > + } > > > > > } > > > > > > > > Hi Wei, > > > > > > > > I am wondering about the relationship between the above and > > > > hardware support for CHECKSUM_COMPLETE. > > > > > > > > Prior to this patch CHECKSUM_COMPLETE was always used, which seems > > > > desirable. But with this patch, CHECKSUM_UNNECESSARY is conditionally > > used. > > > > > > > > If those cases don't work with CHECKSUM_COMPLETE then is this a > > bug-fix? > > > > > > > > Or, alternatively, if those cases do work with CHECKSUM_COMPLETE, then > > > > I'm unsure why this change is necessary or desirable. It's my understanding > > > > that from the Kernel's perspective CHECKSUM_COMPLETE is preferable to > > > > CHECKSUM_UNNECESSARY. > > > > > > > > ... > > > > > > Rx checksum offload is a new feature of ENETC v4. We would like to exploit > > this > > > capability of the hardware to save CPU cycles in calculating and verifying > > checksum. > > > > > > > Understood, but CHECKSUM_UNNECESSARY is usually the preferred option as > > it > > is more flexible, e.g. allowing low-cost calculation of inner checksums > > in the presence of encapsulation. > > I think you mean 'CHECKSUM_COMPLETE' is the preferred option. But there is no > strong reason against using CHECKSUM_UNNECESSARY. So I hope to keep this patch. I was also under the impression that CHECKSUM_COMPLETE is more desirable than CHECKSUM_UNNECESSARY. Maybe Tom can help. Tom: If a device can report both CHECKSUM_UNNECESSARY and CHECKSUM_COMPLETE, is there any advantage in reporting CHECKSUM_UNNECESSARY? The only advantage I can think of is that when the kernel pulls headers (IPv6 for example) it wouldn't need to compute their checksum in order to adjust skb->csum, but I am not sure how critical that is. I am asking because I am interested in knowing what is the recommendation for future devices: Implement both or only CHECKSUM_COMPLETE? Original patch is here [1] and I did read your paper [2] and David's presentation [3]. Thanks [1] https://lore.kernel.org/netdev/20241204052932.112446-1-wei.fang@nxp.com/T/#mf89bb4c6c72e8dd4a697551cbc9485217366d013 [2] https://people.netfilter.org/pablo/netdev0.1/papers/UDP-Encapsulation-in-Linux.pdf [3] https://www.netdevconf.info/1.1/proceedings/slides/miller-hardware-checksumming.pdf
> -----Original Message----- > From: Ido Schimmel <idosch@idosch.org> > Sent: 2024年12月8日 23:47 > To: Wei Fang <wei.fang@nxp.com>; tom@herbertland.com > Cc: Simon Horman <horms@kernel.org>; Claudiu Manoil > <claudiu.manoil@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>; Clark > Wang <xiaoning.wang@nxp.com>; andrew+netdev@lunn.ch; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; Frank Li <frank.li@nxp.com>; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; imx@lists.linux.dev > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > offload for i.MX95 ENETC > > On Fri, Dec 06, 2024 at 12:45:02PM +0000, Wei Fang wrote: > > > -----Original Message----- > > > From: Simon Horman <horms@kernel.org> > > > Sent: 2024年12月6日 20:31 > > > To: Wei Fang <wei.fang@nxp.com> > > > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > > > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > > > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev > > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > > > offload for i.MX95 ENETC > > > > > > On Fri, Dec 06, 2024 at 10:33:15AM +0000, Wei Fang wrote: > > > > > -----Original Message----- > > > > > From: Simon Horman <horms@kernel.org> > > > > > Sent: 2024年12月6日 17:23 > > > > > To: Wei Fang <wei.fang@nxp.com> > > > > > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > > > > > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > > > > andrew+netdev@lunn.ch; davem@davemloft.net; > edumazet@google.com; > > > > > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > > > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > imx@lists.linux.dev > > > > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx > checksum > > > > > offload for i.MX95 ENETC > > > > > > > > > > On Wed, Dec 04, 2024 at 01:29:28PM +0800, Wei Fang wrote: > > > > > > ENETC rev 4.1 supports TCP and UDP checksum offload for receive, the > bit > > > > > > 108 of the Rx BD will be set if the TCP/UDP checksum is correct. Since > > > > > > this capability is not defined in register, the rx_csum bit is added to > > > > > > struct enetc_drvdata to indicate whether the device supports Rx > > > checksum > > > > > > offload. > > > > > > > > > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > > > > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > > > > > --- > > > > > > v2: no changes > > > > > > v3: no changes > > > > > > v4: no changes > > > > > > v5: no changes > > > > > > v6: no changes > > > > > > --- > > > > > > drivers/net/ethernet/freescale/enetc/enetc.c | 14 > > > ++++++++++---- > > > > > > drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++ > > > > > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 ++ > > > > > > .../net/ethernet/freescale/enetc/enetc_pf_common.c | 3 +++ > > > > > > 4 files changed, 17 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > > index 35634c516e26..3137b6ee62d3 100644 > > > > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > > @@ -1011,10 +1011,15 @@ static void enetc_get_offloads(struct > > > enetc_bdr > > > > > *rx_ring, > > > > > > > > > > > > /* TODO: hashing */ > > > > > > if (rx_ring->ndev->features & NETIF_F_RXCSUM) { > > > > > > - u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > > > > - > > > > > > - skb->csum = csum_unfold((__force > > > __sum16)~htons(inet_csum)); > > > > > > - skb->ip_summed = CHECKSUM_COMPLETE; > > > > > > + if (priv->active_offloads & ENETC_F_RXCSUM && > > > > > > + le16_to_cpu(rxbd->r.flags) & > > > ENETC_RXBD_FLAG_L4_CSUM_OK) > > > > > { > > > > > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > > > > > + } else { > > > > > > + u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > > > > + > > > > > > + skb->csum = csum_unfold((__force > > > __sum16)~htons(inet_csum)); > > > > > > + skb->ip_summed = CHECKSUM_COMPLETE; > > > > > > + } > > > > > > } > > > > > > > > > > Hi Wei, > > > > > > > > > > I am wondering about the relationship between the above and > > > > > hardware support for CHECKSUM_COMPLETE. > > > > > > > > > > Prior to this patch CHECKSUM_COMPLETE was always used, which seems > > > > > desirable. But with this patch, CHECKSUM_UNNECESSARY is conditionally > > > used. > > > > > > > > > > If those cases don't work with CHECKSUM_COMPLETE then is this a > > > bug-fix? > > > > > > > > > > Or, alternatively, if those cases do work with CHECKSUM_COMPLETE, > then > > > > > I'm unsure why this change is necessary or desirable. It's my > understanding > > > > > that from the Kernel's perspective CHECKSUM_COMPLETE is preferable > to > > > > > CHECKSUM_UNNECESSARY. > > > > > > > > > > ... > > > > > > > > Rx checksum offload is a new feature of ENETC v4. We would like to exploit > > > this > > > > capability of the hardware to save CPU cycles in calculating and verifying > > > checksum. > > > > > > > > > > Understood, but CHECKSUM_UNNECESSARY is usually the preferred option > as > > > it > > > is more flexible, e.g. allowing low-cost calculation of inner checksums > > > in the presence of encapsulation. > > > > I think you mean 'CHECKSUM_COMPLETE' is the preferred option. But there is > no > > strong reason against using CHECKSUM_UNNECESSARY. So I hope to keep this > patch. > > I was also under the impression that CHECKSUM_COMPLETE is more desirable > than CHECKSUM_UNNECESSARY. Maybe Tom can help. From the kernel doc [1] it should be necessary to use CHECKSUM_COMPLETE in enetc driver, because ENETCv4 only supports UDP/TCP checksum offload. So I will drop this patch from the patch set. thanks. [1] https://docs.kernel.org/networking/skbuff.html#:~:text=Even%20if%20device%20supports%20only%20some%20protocols%2C%20but%20is%20able%20to%20produce%20skb%2D%3Ecsum%2C%20it%20MUST%20use%20CHECKSUM_COMPLETE%2C%20not%20CHECKSUM_UNNECESSARY.
On Tue, Dec 10, 2024 at 07:49:18AM +0000, Wei Fang wrote: > > -----Original Message----- > > From: Ido Schimmel <idosch@idosch.org> > > Sent: 2024年12月8日 23:47 > > To: Wei Fang <wei.fang@nxp.com>; tom@herbertland.com > > Cc: Simon Horman <horms@kernel.org>; Claudiu Manoil > > <claudiu.manoil@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>; Clark > > Wang <xiaoning.wang@nxp.com>; andrew+netdev@lunn.ch; > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > pabeni@redhat.com; Frank Li <frank.li@nxp.com>; netdev@vger.kernel.org; > > linux-kernel@vger.kernel.org; imx@lists.linux.dev > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > > offload for i.MX95 ENETC > > > > On Fri, Dec 06, 2024 at 12:45:02PM +0000, Wei Fang wrote: > > > > -----Original Message----- > > > > From: Simon Horman <horms@kernel.org> > > > > Sent: 2024年12月6日 20:31 > > > > To: Wei Fang <wei.fang@nxp.com> > > > > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > > > > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > > > andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; > > > > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev > > > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx checksum > > > > offload for i.MX95 ENETC > > > > > > > > On Fri, Dec 06, 2024 at 10:33:15AM +0000, Wei Fang wrote: > > > > > > -----Original Message----- > > > > > > From: Simon Horman <horms@kernel.org> > > > > > > Sent: 2024年12月6日 17:23 > > > > > > To: Wei Fang <wei.fang@nxp.com> > > > > > > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > > > > > > <vladimir.oltean@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > > > > > > andrew+netdev@lunn.ch; davem@davemloft.net; > > edumazet@google.com; > > > > > > kuba@kernel.org; pabeni@redhat.com; Frank Li <frank.li@nxp.com>; > > > > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > > imx@lists.linux.dev > > > > > > Subject: Re: [PATCH v6 RESEND net-next 1/5] net: enetc: add Rx > > checksum > > > > > > offload for i.MX95 ENETC > > > > > > > > > > > > On Wed, Dec 04, 2024 at 01:29:28PM +0800, Wei Fang wrote: > > > > > > > ENETC rev 4.1 supports TCP and UDP checksum offload for receive, the > > bit > > > > > > > 108 of the Rx BD will be set if the TCP/UDP checksum is correct. Since > > > > > > > this capability is not defined in register, the rx_csum bit is added to > > > > > > > struct enetc_drvdata to indicate whether the device supports Rx > > > > checksum > > > > > > > offload. > > > > > > > > > > > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > > > > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > > > > > Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com> > > > > > > > --- > > > > > > > v2: no changes > > > > > > > v3: no changes > > > > > > > v4: no changes > > > > > > > v5: no changes > > > > > > > v6: no changes > > > > > > > --- > > > > > > > drivers/net/ethernet/freescale/enetc/enetc.c | 14 > > > > ++++++++++---- > > > > > > > drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++ > > > > > > > drivers/net/ethernet/freescale/enetc/enetc_hw.h | 2 ++ > > > > > > > .../net/ethernet/freescale/enetc/enetc_pf_common.c | 3 +++ > > > > > > > 4 files changed, 17 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > > b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > > > index 35634c516e26..3137b6ee62d3 100644 > > > > > > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > > > > > > @@ -1011,10 +1011,15 @@ static void enetc_get_offloads(struct > > > > enetc_bdr > > > > > > *rx_ring, > > > > > > > > > > > > > > /* TODO: hashing */ > > > > > > > if (rx_ring->ndev->features & NETIF_F_RXCSUM) { > > > > > > > - u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > > > > > - > > > > > > > - skb->csum = csum_unfold((__force > > > > __sum16)~htons(inet_csum)); > > > > > > > - skb->ip_summed = CHECKSUM_COMPLETE; > > > > > > > + if (priv->active_offloads & ENETC_F_RXCSUM && > > > > > > > + le16_to_cpu(rxbd->r.flags) & > > > > ENETC_RXBD_FLAG_L4_CSUM_OK) > > > > > > { > > > > > > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > > > > > > + } else { > > > > > > > + u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); > > > > > > > + > > > > > > > + skb->csum = csum_unfold((__force > > > > __sum16)~htons(inet_csum)); > > > > > > > + skb->ip_summed = CHECKSUM_COMPLETE; > > > > > > > + } > > > > > > > } > > > > > > > > > > > > Hi Wei, > > > > > > > > > > > > I am wondering about the relationship between the above and > > > > > > hardware support for CHECKSUM_COMPLETE. > > > > > > > > > > > > Prior to this patch CHECKSUM_COMPLETE was always used, which seems > > > > > > desirable. But with this patch, CHECKSUM_UNNECESSARY is conditionally > > > > used. > > > > > > > > > > > > If those cases don't work with CHECKSUM_COMPLETE then is this a > > > > bug-fix? > > > > > > > > > > > > Or, alternatively, if those cases do work with CHECKSUM_COMPLETE, > > then > > > > > > I'm unsure why this change is necessary or desirable. It's my > > understanding > > > > > > that from the Kernel's perspective CHECKSUM_COMPLETE is preferable > > to > > > > > > CHECKSUM_UNNECESSARY. > > > > > > > > > > > > ... > > > > > > > > > > Rx checksum offload is a new feature of ENETC v4. We would like to exploit > > > > this > > > > > capability of the hardware to save CPU cycles in calculating and verifying > > > > checksum. > > > > > > > > > > > > > Understood, but CHECKSUM_UNNECESSARY is usually the preferred option > > as > > > > it > > > > is more flexible, e.g. allowing low-cost calculation of inner checksums > > > > in the presence of encapsulation. > > > > > > I think you mean 'CHECKSUM_COMPLETE' is the preferred option. But there is > > no > > > strong reason against using CHECKSUM_UNNECESSARY. So I hope to keep this > > patch. > > > > I was also under the impression that CHECKSUM_COMPLETE is more desirable > > than CHECKSUM_UNNECESSARY. Maybe Tom can help. > > From the kernel doc [1] it should be necessary to use CHECKSUM_COMPLETE in > enetc driver, because ENETCv4 only supports UDP/TCP checksum offload. So I will > drop this patch from the patch set. thanks. Thanks. > > [1] https://docs.kernel.org/networking/skbuff.html#:~:text=Even%20if%20device%20supports%20only%20some%20protocols%2C%20but%20is%20able%20to%20produce%20skb%2D%3Ecsum%2C%20it%20MUST%20use%20CHECKSUM_COMPLETE%2C%20not%20CHECKSUM_UNNECESSARY.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 35634c516e26..3137b6ee62d3 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -1011,10 +1011,15 @@ static void enetc_get_offloads(struct enetc_bdr *rx_ring, /* TODO: hashing */ if (rx_ring->ndev->features & NETIF_F_RXCSUM) { - u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); - - skb->csum = csum_unfold((__force __sum16)~htons(inet_csum)); - skb->ip_summed = CHECKSUM_COMPLETE; + if (priv->active_offloads & ENETC_F_RXCSUM && + le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_L4_CSUM_OK) { + skb->ip_summed = CHECKSUM_UNNECESSARY; + } else { + u16 inet_csum = le16_to_cpu(rxbd->r.inet_csum); + + skb->csum = csum_unfold((__force __sum16)~htons(inet_csum)); + skb->ip_summed = CHECKSUM_COMPLETE; + } } if (le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_VLAN) { @@ -3281,6 +3286,7 @@ static const struct enetc_drvdata enetc_pf_data = { static const struct enetc_drvdata enetc4_pf_data = { .sysclk_freq = ENETC_CLK_333M, .pmac_offset = ENETC4_PMAC_OFFSET, + .rx_csum = 1, .eth_ops = &enetc4_pf_ethtool_ops, }; diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h index 72fa03dbc2dd..5b65f79e05be 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.h +++ b/drivers/net/ethernet/freescale/enetc/enetc.h @@ -234,6 +234,7 @@ enum enetc_errata { struct enetc_drvdata { u32 pmac_offset; /* Only valid for PSI which supports 802.1Qbu */ + u8 rx_csum:1; u64 sysclk_freq; const struct ethtool_ops *eth_ops; }; @@ -341,6 +342,7 @@ enum enetc_active_offloads { ENETC_F_QBV = BIT(9), ENETC_F_QCI = BIT(10), ENETC_F_QBU = BIT(11), + ENETC_F_RXCSUM = BIT(12), }; enum enetc_flags_bit { diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h index 7c3285584f8a..4b8fd1879005 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h +++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h @@ -645,6 +645,8 @@ union enetc_rx_bd { #define ENETC_RXBD_LSTATUS(flags) ((flags) << 16) #define ENETC_RXBD_FLAG_VLAN BIT(9) #define ENETC_RXBD_FLAG_TSTMP BIT(10) +/* UDP and TCP checksum offload, for ENETC 4.1 and later */ +#define ENETC_RXBD_FLAG_L4_CSUM_OK BIT(12) #define ENETC_RXBD_FLAG_TPID GENMASK(1, 0) #define ENETC_MAC_ADDR_FILT_CNT 8 /* # of supported entries per port */ diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c index 0eecfc833164..91e79582a541 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c @@ -119,6 +119,9 @@ void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev, ndev->priv_flags |= IFF_UNICAST_FLT; + if (si->drvdata->rx_csum) + priv->active_offloads |= ENETC_F_RXCSUM; + /* TODO: currently, i.MX95 ENETC driver does not support advanced features */ if (!is_enetc_rev1(si)) { ndev->hw_features &= ~(NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_LOOPBACK);