| Message ID | 20210105165749.16920-1-rohitm@chelsio.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] cxgb4: advertise NETIF_F_HW_CSUM | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: dm@chelsio.com rajur@chelsio.com mirq-linux@rere.qmqm.pl |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 43 this patch: 43 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 35 this patch: 35 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Jan 5, 2021 at 9:01 AM Rohit Maheshwari <rohitm@chelsio.com> wrote: > > advertise NETIF_F_HW_CSUM instead of protocol specific values of > NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM. This change is added long > back in other drivers. This issue is seen recently when TLS offload > made it mandatory to enable NETIF_F_HW_CSUM. > > Fixes: 2ed28baa7076 ("net: cxgb4{,vf}: convert to hw_features") > Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com> > --- > drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > index 7fd264a6d085..f99f43570d41 100644 > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > @@ -6831,14 +6831,13 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > netdev->irq = pdev->irq; > > netdev->hw_features = NETIF_F_SG | TSO_FLAGS | > - NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | > + NETIF_F_HW_CSUM | > NETIF_F_RXCSUM | NETIF_F_RXHASH | NETIF_F_GRO | > NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | > NETIF_F_HW_TC | NETIF_F_NTUPLE; > > if (chip_ver > CHELSIO_T5) { > - netdev->hw_enc_features |= NETIF_F_IP_CSUM | > - NETIF_F_IPV6_CSUM | > + netdev->hw_enc_features |= NETIF_F_HW_CSUM | > NETIF_F_RXCSUM | > NETIF_F_GSO_UDP_TUNNEL | > NETIF_F_GSO_UDP_TUNNEL_CSUM | If you are going to enable the feature you should fully enable the feature. My concern is the "nocsum:" label in hwcsum(). If you are going to say you support the feature you should at least look at dealing with the exception case and process a software checksum via skb_checksum_help() rather than just not bothering and "hope a bad packet is detected". - Alex
On Tue, 5 Jan 2021 22:27:49 +0530 Rohit Maheshwari wrote: > advertise NETIF_F_HW_CSUM instead of protocol specific values of > NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM. This change is added long > back in other drivers. This issue is seen recently when TLS offload > made it mandatory to enable NETIF_F_HW_CSUM. > > Fixes: 2ed28baa7076 ("net: cxgb4{,vf}: convert to hw_features") > Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com> Ugh, commit ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM is disabled") is buggy, it should probably use NETIF_F_CSUM_MASK. Can you fix that instead?
On 1/5/2021 10:29 PM, Jakub Kicinski wrote: > On Tue, 5 Jan 2021 22:27:49 +0530 Rohit Maheshwari wrote: >> advertise NETIF_F_HW_CSUM instead of protocol specific values of >> NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM. This change is added long >> back in other drivers. This issue is seen recently when TLS offload >> made it mandatory to enable NETIF_F_HW_CSUM. >> >> Fixes: 2ed28baa7076 ("net: cxgb4{,vf}: convert to hw_features") >> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com> > > Ugh, commit ae0b04b238e2 ("net: Disable NETIF_F_HW_TLS_TX when HW_CSUM > is disabled") is buggy, it should probably use NETIF_F_CSUM_MASK. > Can you fix that instead? > The HW_TLS_TX feature is for both IPv4/6. We do not want to allow NETIF_F_HW_TLS_TX if only one of (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM) is set (of course NETIF_F_HW_CSUM is not set in this case). Hence, using NETIF_F_CSUM_MASK would not be a strong enough condition. I think we have two options here: Either (1) request device drivers to move to NETIF_F_HW_CSUM if they want to have HW_TLS_TX (as it is today), or (2) use the following condition: ((features & NETIF_F_IP_CSUM) && (features & NETIF_F_IPV6_CSUM)) || (features & NETIF_F_HW_CSUM). Thanks, Tariq
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c index 7fd264a6d085..f99f43570d41 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c @@ -6831,14 +6831,13 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent) netdev->irq = pdev->irq; netdev->hw_features = NETIF_F_SG | TSO_FLAGS | - NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | + NETIF_F_HW_CSUM | NETIF_F_RXCSUM | NETIF_F_RXHASH | NETIF_F_GRO | NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_TC | NETIF_F_NTUPLE; if (chip_ver > CHELSIO_T5) { - netdev->hw_enc_features |= NETIF_F_IP_CSUM | - NETIF_F_IPV6_CSUM | + netdev->hw_enc_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM | NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM |
advertise NETIF_F_HW_CSUM instead of protocol specific values of NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM. This change is added long back in other drivers. This issue is seen recently when TLS offload made it mandatory to enable NETIF_F_HW_CSUM. Fixes: 2ed28baa7076 ("net: cxgb4{,vf}: convert to hw_features") Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com> --- drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)