* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 [not found] <20190911194453.2595021-1-vijaykhemka@fb.com> @ 2019-09-17 19:34 ` Vijay Khemka 2019-09-24 17:26 ` Vijay Khemka 2019-10-10 19:20 ` Vijay Khemka 1 sibling, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-09-17 19:34 UTC (permalink / raw) To: David S. Miller, Florian Fainelli, YueHaibing, Andrew Lunn, Kate Stewart, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel, linux-aspeed, joel Cc: openbmc @ lists . ozlabs . org, Sai Dasari Please review below patch and provide your valuable feedback. Regards -Vijay On 9/11/19, 1:05 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote: HW checksum generation is not working for AST2500, specially with IPV6 over NCSI. All TCP packets with IPv6 get dropped. By disabling this it works perfectly fine with IPV6. As it works for IPV4 so enabled hw checksum back for IPV4. Verified with IPV6 enabled and can do ssh. Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> --- Changes since v1: Enabled IPV4 hw checksum generation as it works for IPV4. drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index 030fed65393e..0255a28d2958 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct platform_device *pdev) /* AST2400 doesn't have working HW checksum generation */ if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) netdev->hw_features &= ~NETIF_F_HW_CSUM; + + /* AST2500 doesn't have working HW checksum generation for IPV6 + * but it works for IPV4, so disabling hw checksum and enabling + * it for only IPV4. + */ + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) { + netdev->hw_features &= ~NETIF_F_HW_CSUM; + netdev->hw_features |= NETIF_F_IP_CSUM; + } + if (np && of_get_property(np, "no-hw-checksum", NULL)) - netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM); + netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM + | NETIF_F_IP_CSUM); netdev->features |= netdev->hw_features; /* register network device */ -- 2.17.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-09-17 19:34 ` [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 Vijay Khemka @ 2019-09-24 17:26 ` Vijay Khemka 0 siblings, 0 replies; 14+ messages in thread From: Vijay Khemka @ 2019-09-24 17:26 UTC (permalink / raw) To: David S. Miller, Florian Fainelli, YueHaibing, Andrew Lunn, Kate Stewart, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel, linux-aspeed, joel Cc: openbmc @ lists . ozlabs . org, Sai Dasari Florian/Joel, Can you please look into below patch and let me know who can apply this. Regards -Vijay On 9/17/19, 12:34 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote: Please review below patch and provide your valuable feedback. Regards -Vijay On 9/11/19, 1:05 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote: HW checksum generation is not working for AST2500, specially with IPV6 over NCSI. All TCP packets with IPv6 get dropped. By disabling this it works perfectly fine with IPV6. As it works for IPV4 so enabled hw checksum back for IPV4. Verified with IPV6 enabled and can do ssh. Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> --- Changes since v1: Enabled IPV4 hw checksum generation as it works for IPV4. drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index 030fed65393e..0255a28d2958 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct platform_device *pdev) /* AST2400 doesn't have working HW checksum generation */ if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) netdev->hw_features &= ~NETIF_F_HW_CSUM; + + /* AST2500 doesn't have working HW checksum generation for IPV6 + * but it works for IPV4, so disabling hw checksum and enabling + * it for only IPV4. + */ + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) { + netdev->hw_features &= ~NETIF_F_HW_CSUM; + netdev->hw_features |= NETIF_F_IP_CSUM; + } + if (np && of_get_property(np, "no-hw-checksum", NULL)) - netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM); + netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM + | NETIF_F_IP_CSUM); netdev->features |= netdev->hw_features; /* register network device */ -- 2.17.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 [not found] <20190911194453.2595021-1-vijaykhemka@fb.com> 2019-09-17 19:34 ` [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 Vijay Khemka @ 2019-10-10 19:20 ` Vijay Khemka 2019-10-11 1:16 ` Jakub Kicinski 1 sibling, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-10-10 19:20 UTC (permalink / raw) To: David S. Miller, Florian Fainelli, YueHaibing, Andrew Lunn, Kate Stewart, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel, Benjamin Herrenschmidt Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari Resending this patch again. On 9/11/19, 1:05 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote: HW checksum generation is not working for AST2500, specially with IPV6 over NCSI. All TCP packets with IPv6 get dropped. By disabling this it works perfectly fine with IPV6. As it works for IPV4 so enabled hw checksum back for IPV4. Verified with IPV6 enabled and can do ssh. Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> --- Changes since v1: Enabled IPV4 hw checksum generation as it works for IPV4. drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index 030fed65393e..0255a28d2958 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct platform_device *pdev) /* AST2400 doesn't have working HW checksum generation */ if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) netdev->hw_features &= ~NETIF_F_HW_CSUM; + + /* AST2500 doesn't have working HW checksum generation for IPV6 + * but it works for IPV4, so disabling hw checksum and enabling + * it for only IPV4. + */ + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) { + netdev->hw_features &= ~NETIF_F_HW_CSUM; + netdev->hw_features |= NETIF_F_IP_CSUM; + } + if (np && of_get_property(np, "no-hw-checksum", NULL)) - netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM); + netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM + | NETIF_F_IP_CSUM); netdev->features |= netdev->hw_features; /* register network device */ -- 2.17.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-10 19:20 ` Vijay Khemka @ 2019-10-11 1:16 ` Jakub Kicinski 0 siblings, 0 replies; 14+ messages in thread From: Jakub Kicinski @ 2019-10-11 1:16 UTC (permalink / raw) To: Vijay Khemka Cc: David S. Miller, Florian Fainelli, YueHaibing, Andrew Lunn, Kate Stewart, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel, Benjamin Herrenschmidt, openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On Thu, 10 Oct 2019 19:20:47 +0000, Vijay Khemka wrote: > Resending this patch again. Perhaps I'm missing context but what's the intention here? In case this is resubmitting the patch for inclusion in the upstream kernel you need to send it out properly with git send-email or such.. > On 9/11/19, 1:05 PM, "Vijay Khemka" <vijaykhemka@fb.com> wrote: > > HW checksum generation is not working for AST2500, specially with IPV6 > over NCSI. All TCP packets with IPv6 get dropped. By disabling this > it works perfectly fine with IPV6. As it works for IPV4 so enabled > hw checksum back for IPV4. > > Verified with IPV6 enabled and can do ssh. > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20191011213027.2110008-1-vijaykhemka@fb.com>]
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 [not found] <20191011213027.2110008-1-vijaykhemka@fb.com> @ 2019-10-17 1:28 ` Benjamin Herrenschmidt 2019-10-17 22:01 ` Vijay Khemka 0 siblings, 1 reply; 14+ messages in thread From: Benjamin Herrenschmidt @ 2019-10-17 1:28 UTC (permalink / raw) To: Vijay Khemka, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, sdasari On Fri, 2019-10-11 at 14:30 -0700, Vijay Khemka wrote: > HW checksum generation is not working for AST2500, specially with > IPV6 > over NCSI. All TCP packets with IPv6 get dropped. By disabling this > it works perfectly fine with IPV6. As it works for IPV4 so enabled > hw checksum back for IPV4. > > Verified with IPV6 enabled and can do ssh. So while this probably works, I don't think this is the right approach, at least according to the comments in skbuff.h The driver should have handled unsupported csum via SW fallback already in ftgmac100_prep_tx_csum() Can you check why this didn't work for you ? Cheers, Ben. > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > --- > Changes since v1: > Enabled IPV4 hw checksum generation as it works for IPV4. > > drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > b/drivers/net/ethernet/faraday/ftgmac100.c > index 030fed65393e..0255a28d2958 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct > platform_device *pdev) > /* AST2400 doesn't have working HW checksum generation */ > if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) > netdev->hw_features &= ~NETIF_F_HW_CSUM; > + > + /* AST2500 doesn't have working HW checksum generation for IPV6 > + * but it works for IPV4, so disabling hw checksum and enabling > + * it for only IPV4. > + */ > + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) > { > + netdev->hw_features &= ~NETIF_F_HW_CSUM; > + netdev->hw_features |= NETIF_F_IP_CSUM; > + } > + > if (np && of_get_property(np, "no-hw-checksum", NULL)) > - netdev->hw_features &= ~(NETIF_F_HW_CSUM | > NETIF_F_RXCSUM); > + netdev->hw_features &= ~(NETIF_F_HW_CSUM | > NETIF_F_RXCSUM > + | NETIF_F_IP_CSUM); > netdev->features |= netdev->hw_features; > > /* register network device */ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-17 1:28 ` Benjamin Herrenschmidt @ 2019-10-17 22:01 ` Vijay Khemka 2019-10-17 23:14 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-10-17 22:01 UTC (permalink / raw) To: Benjamin Herrenschmidt, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On 10/16/19, 6:29 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote: On Fri, 2019-10-11 at 14:30 -0700, Vijay Khemka wrote: > HW checksum generation is not working for AST2500, specially with > IPV6 > over NCSI. All TCP packets with IPv6 get dropped. By disabling this > it works perfectly fine with IPV6. As it works for IPV4 so enabled > hw checksum back for IPV4. > > Verified with IPV6 enabled and can do ssh. So while this probably works, I don't think this is the right approach, at least according to the comments in skbuff.h This is not a matter of unsupported csum, it is broken hw csum. That's why we disable hw checksum. My guess is once we disable Hw checksum, it will use sw checksum. So I am just disabling hw Checksum. The driver should have handled unsupported csum via SW fallback already in ftgmac100_prep_tx_csum() Can you check why this didn't work for you ? Cheers, Ben. > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > --- > Changes since v1: > Enabled IPV4 hw checksum generation as it works for IPV4. > > drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > b/drivers/net/ethernet/faraday/ftgmac100.c > index 030fed65393e..0255a28d2958 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct > platform_device *pdev) > /* AST2400 doesn't have working HW checksum generation */ > if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) > netdev->hw_features &= ~NETIF_F_HW_CSUM; > + > + /* AST2500 doesn't have working HW checksum generation for IPV6 > + * but it works for IPV4, so disabling hw checksum and enabling > + * it for only IPV4. > + */ > + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) > { > + netdev->hw_features &= ~NETIF_F_HW_CSUM; > + netdev->hw_features |= NETIF_F_IP_CSUM; > + } > + > if (np && of_get_property(np, "no-hw-checksum", NULL)) > - netdev->hw_features &= ~(NETIF_F_HW_CSUM | > NETIF_F_RXCSUM); > + netdev->hw_features &= ~(NETIF_F_HW_CSUM | > NETIF_F_RXCSUM > + | NETIF_F_IP_CSUM); > netdev->features |= netdev->hw_features; > > /* register network device */ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-17 22:01 ` Vijay Khemka @ 2019-10-17 23:14 ` Benjamin Herrenschmidt 2019-10-17 23:34 ` Benjamin Herrenschmidt 2019-10-18 0:06 ` Vijay Khemka 0 siblings, 2 replies; 14+ messages in thread From: Benjamin Herrenschmidt @ 2019-10-17 23:14 UTC (permalink / raw) To: Vijay Khemka, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On Thu, 2019-10-17 at 22:01 +0000, Vijay Khemka wrote: > > On 10/16/19, 6:29 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote: > > On Fri, 2019-10-11 at 14:30 -0700, Vijay Khemka wrote: > > HW checksum generation is not working for AST2500, specially with > > IPV6 > > over NCSI. All TCP packets with IPv6 get dropped. By disabling this > > it works perfectly fine with IPV6. As it works for IPV4 so enabled > > hw checksum back for IPV4. > > > > Verified with IPV6 enabled and can do ssh. > > So while this probably works, I don't think this is the right > approach, at least according to the comments in skbuff.h > > This is not a matter of unsupported csum, it is broken hw csum. > That's why we disable hw checksum. My guess is once we disable > Hw checksum, it will use sw checksum. So I am just disabling hw > Checksum. I don't understand what you are saying. You reported a problem with IPV6 checksums generation. The HW doesn't support it. What's "not a matter of unsupported csum" ? Your patch uses a *deprecated* bit to tell the network stack to only do HW checksum generation on IPV4. This bit is deprecated for a reason, again, see skbuff.h. The right approach, *which the driver already does*, is to tell the stack that we support HW checksuming using NETIF_F_HW_CSUM, and then, in the transmit handler, to call skb_checksum_help() to have the SW calculate the checksum if it's not a supported type. This is exactly what ftgmac100_prep_tx_csum() does. It only enables HW checksum generation on supported types and uses skb_checksum_help() otherwise, supported types being protocol ETH_P_IP and IP protocol being raw IP, TCP and UDP. So this *should* have fallen back to SW for IPV6. So either something in my code there is making an incorrect assumption, or something is broken in skb_checksum_help() for IPV6 (which I somewhat doubt) or something else I can't think of, but setting a *deprecated* flag is definitely not the right answer, neither is completely disabling HW checksumming. So can you investigate what's going on a bit more closely please ? I can try myself, though I have very little experience with IPV6 and probably won't have time before next week. Cheers, Ben. > The driver should have handled unsupported csum via SW fallback > already in ftgmac100_prep_tx_csum() > > Can you check why this didn't work for you ? > > Cheers, > Ben. > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > --- > > Changes since v1: > > Enabled IPV4 hw checksum generation as it works for IPV4. > > > > drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > > b/drivers/net/ethernet/faraday/ftgmac100.c > > index 030fed65393e..0255a28d2958 100644 > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct > > platform_device *pdev) > > /* AST2400 doesn't have working HW checksum generation */ > > if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) > > netdev->hw_features &= ~NETIF_F_HW_CSUM; > > + > > + /* AST2500 doesn't have working HW checksum generation for IPV6 > > + * but it works for IPV4, so disabling hw checksum and enabling > > + * it for only IPV4. > > + */ > > + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) > > { > > + netdev->hw_features &= ~NETIF_F_HW_CSUM; > > + netdev->hw_features |= NETIF_F_IP_CSUM; > > + } > > + > > if (np && of_get_property(np, "no-hw-checksum", NULL)) > > - netdev->hw_features &= ~(NETIF_F_HW_CSUM | > > NETIF_F_RXCSUM); > > + netdev->hw_features &= ~(NETIF_F_HW_CSUM | > > NETIF_F_RXCSUM > > + | NETIF_F_IP_CSUM); > > netdev->features |= netdev->hw_features; > > > > /* register network device */ > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-17 23:14 ` Benjamin Herrenschmidt @ 2019-10-17 23:34 ` Benjamin Herrenschmidt 2019-10-18 0:06 ` Vijay Khemka 1 sibling, 0 replies; 14+ messages in thread From: Benjamin Herrenschmidt @ 2019-10-17 23:34 UTC (permalink / raw) To: Vijay Khemka, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On Fri, 2019-10-18 at 10:14 +1100, Benjamin Herrenschmidt wrote: > > I don't understand what you are saying. You reported a problem with > IPV6 checksums generation. The HW doesn't support it. What's "not a > matter of unsupported csum" ? > > Your patch uses a *deprecated* bit to tell the network stack to only do > HW checksum generation on IPV4. > > This bit is deprecated for a reason, again, see skbuff.h. The right > approach, *which the driver already does*, is to tell the stack that we > support HW checksuming using NETIF_F_HW_CSUM, and then, in the transmit > handler, to call skb_checksum_help() to have the SW calculate the > checksum if it's not a supported type. > > This is exactly what ftgmac100_prep_tx_csum() does. It only enables HW > checksum generation on supported types and uses skb_checksum_help() > otherwise, supported types being protocol ETH_P_IP and IP protocol > being raw IP, TCP and UDP. > > So this *should* have fallen back to SW for IPV6. So either something > in my code there is making an incorrect assumption, or something is > broken in skb_checksum_help() for IPV6 (which I somewhat doubt) or > something else I can't think of, but setting a *deprecated* flag is > definitely not the right answer, neither is completely disabling HW > checksumming. > > So can you investigate what's going on a bit more closely please ? I > can try myself, though I have very little experience with IPV6 and > probably won't have time before next week. I did get that piece of information from Aspeed: The HW checksum generation is supported if: - The length of UDP header is always 20 bytes. - The length of TCP and IP header have 4 * N bytes (N is 5 to 15). Now these afaik are also the protocol limits, so it *should* work. Or am I missing something or some funky encaspulation/header format that can be used under some circumstances ? Cheers, Ben. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-17 23:14 ` Benjamin Herrenschmidt 2019-10-17 23:34 ` Benjamin Herrenschmidt @ 2019-10-18 0:06 ` Vijay Khemka 2019-10-18 0:32 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-10-18 0:06 UTC (permalink / raw) To: Benjamin Herrenschmidt, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On 10/17/19, 4:15 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote: On Thu, 2019-10-17 at 22:01 +0000, Vijay Khemka wrote: > > On 10/16/19, 6:29 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote: > > On Fri, 2019-10-11 at 14:30 -0700, Vijay Khemka wrote: > > HW checksum generation is not working for AST2500, specially with > > IPV6 > > over NCSI. All TCP packets with IPv6 get dropped. By disabling this > > it works perfectly fine with IPV6. As it works for IPV4 so enabled > > hw checksum back for IPV4. > > > > Verified with IPV6 enabled and can do ssh. > > So while this probably works, I don't think this is the right > approach, at least according to the comments in skbuff.h > > This is not a matter of unsupported csum, it is broken hw csum. > That's why we disable hw checksum. My guess is once we disable > Hw checksum, it will use sw checksum. So I am just disabling hw > Checksum. I don't understand what you are saying. You reported a problem with IPV6 checksums generation. The HW doesn't support it. What's "not a matter of unsupported csum" ? Your patch uses a *deprecated* bit to tell the network stack to only do HW checksum generation on IPV4. This bit is deprecated for a reason, again, see skbuff.h. The right approach, *which the driver already does*, is to tell the stack that we support HW checksuming using NETIF_F_HW_CSUM, and then, in the transmit handler, to call skb_checksum_help() to have the SW calculate the checksum if it's not a supported type. My understanding was when we enable NETIF_F_HW_CSUM means network stack enables HW checksum and doesn't calculate SW checksum. But as per this supported types HW checksum are used only for IPV4 and not for IPV6 even though driver enabled NETIF_F_HW_CSUM. For IPV6 it is always a SW generated checksum, please correct me here. This is exactly what ftgmac100_prep_tx_csum() does. It only enables HW checksum generation on supported types and uses skb_checksum_help() otherwise, supported types being protocol ETH_P_IP and IP protocol being raw IP, TCP and UDP. So this *should* have fallen back to SW for IPV6. So either something in my code there is making an incorrect assumption, or something is broken in skb_checksum_help() for IPV6 (which I somewhat doubt) or something else I can't think of, but setting a *deprecated* flag is definitely not the right answer, neither is completely disabling HW checksumming. So can you investigate what's going on a bit more closely please ? I can try myself, though I have very little experience with IPV6 and probably won't have time before next week. Cheers, Ben. > The driver should have handled unsupported csum via SW fallback > already in ftgmac100_prep_tx_csum() > > Can you check why this didn't work for you ? > > Cheers, > Ben. > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > --- > > Changes since v1: > > Enabled IPV4 hw checksum generation as it works for IPV4. > > > > drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > > b/drivers/net/ethernet/faraday/ftgmac100.c > > index 030fed65393e..0255a28d2958 100644 > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct > > platform_device *pdev) > > /* AST2400 doesn't have working HW checksum generation */ > > if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) > > netdev->hw_features &= ~NETIF_F_HW_CSUM; > > + > > + /* AST2500 doesn't have working HW checksum generation for IPV6 > > + * but it works for IPV4, so disabling hw checksum and enabling > > + * it for only IPV4. > > + */ > > + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) > > { > > + netdev->hw_features &= ~NETIF_F_HW_CSUM; > > + netdev->hw_features |= NETIF_F_IP_CSUM; > > + } > > + > > if (np && of_get_property(np, "no-hw-checksum", NULL)) > > - netdev->hw_features &= ~(NETIF_F_HW_CSUM | > > NETIF_F_RXCSUM); > > + netdev->hw_features &= ~(NETIF_F_HW_CSUM | > > NETIF_F_RXCSUM > > + | NETIF_F_IP_CSUM); > > netdev->features |= netdev->hw_features; > > > > /* register network device */ > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-18 0:06 ` Vijay Khemka @ 2019-10-18 0:32 ` Benjamin Herrenschmidt 2019-10-18 22:50 ` Vijay Khemka 0 siblings, 1 reply; 14+ messages in thread From: Benjamin Herrenschmidt @ 2019-10-18 0:32 UTC (permalink / raw) To: Vijay Khemka, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On Fri, 2019-10-18 at 00:06 +0000, Vijay Khemka wrote: > > > This is not a matter of unsupported csum, it is broken hw csum. > > That's why we disable hw checksum. My guess is once we disable > > Hw checksum, it will use sw checksum. So I am just disabling hw > > Checksum. > > I don't understand what you are saying. You reported a problem with > IPV6 checksums generation. The HW doesn't support it. What's "not a > matter of unsupported csum" ? > > Your patch uses a *deprecated* bit to tell the network stack to only do > HW checksum generation on IPV4. > > This bit is deprecated for a reason, again, see skbuff.h. The right > approach, *which the driver already does*, is to tell the stack that we > support HW checksuming using NETIF_F_HW_CSUM, and then, in the transmit > handler, to call skb_checksum_help() to have the SW calculate the > checksum if it's not a supported type. > > My understanding was when we enable NETIF_F_HW_CSUM means network > stack enables HW checksum and doesn't calculate SW checksum. But as per > this supported types HW checksum are used only for IPV4 and not for IPV6 even > though driver enabled NETIF_F_HW_CSUM. For IPV6 it is always a SW generated > checksum, please correct me here. Have you actually read the comments in skbuff.h that I pointed you to ? And the rest of my email for that matter ? > This is exactly what ftgmac100_prep_tx_csum() does. It only enables HW > checksum generation on supported types and uses skb_checksum_help() > otherwise, supported types being protocol ETH_P_IP and IP protocol > being raw IP, TCP and UDP. > > > So this *should* have fallen back to SW for IPV6. So either something > in my code there is making an incorrect assumption, or something is > broken in skb_checksum_help() for IPV6 (which I somewhat doubt) or > something else I can't think of, but setting a *deprecated* flag is > definitely not the right answer, neither is completely disabling HW > checksumming. > > So can you investigate what's going on a bit more closely please ? I > can try myself, though I have very little experience with IPV6 and > probably won't have time before next week. > > Cheers, > Ben. > > > The driver should have handled unsupported csum via SW fallback > > already in ftgmac100_prep_tx_csum() > > > > Can you check why this didn't work for you ? > > > > Cheers, > > Ben. > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > > --- > > > Changes since v1: > > > Enabled IPV4 hw checksum generation as it works for IPV4. > > > > > > drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > > > b/drivers/net/ethernet/faraday/ftgmac100.c > > > index 030fed65393e..0255a28d2958 100644 > > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > > @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct > > > platform_device *pdev) > > > /* AST2400 doesn't have working HW checksum generation */ > > > if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) > > > netdev->hw_features &= ~NETIF_F_HW_CSUM; > > > + > > > + /* AST2500 doesn't have working HW checksum generation for IPV6 > > > + * but it works for IPV4, so disabling hw checksum and enabling > > > + * it for only IPV4. > > > + */ > > > + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) > > > { > > > + netdev->hw_features &= ~NETIF_F_HW_CSUM; > > > + netdev->hw_features |= NETIF_F_IP_CSUM; > > > + } > > > + > > > if (np && of_get_property(np, "no-hw-checksum", NULL)) > > > - netdev->hw_features &= ~(NETIF_F_HW_CSUM | > > > NETIF_F_RXCSUM); > > > + netdev->hw_features &= ~(NETIF_F_HW_CSUM | > > > NETIF_F_RXCSUM > > > + | NETIF_F_IP_CSUM); > > > netdev->features |= netdev->hw_features; > > > > > > /* register network device */ > > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-18 0:32 ` Benjamin Herrenschmidt @ 2019-10-18 22:50 ` Vijay Khemka 2019-10-19 0:03 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-10-18 22:50 UTC (permalink / raw) To: Benjamin Herrenschmidt, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On 10/17/19, 5:33 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote: On Fri, 2019-10-18 at 00:06 +0000, Vijay Khemka wrote: > > > This is not a matter of unsupported csum, it is broken hw csum. > > That's why we disable hw checksum. My guess is once we disable > > Hw checksum, it will use sw checksum. So I am just disabling hw > > Checksum. > > I don't understand what you are saying. You reported a problem with > IPV6 checksums generation. The HW doesn't support it. What's "not a > matter of unsupported csum" ? > > Your patch uses a *deprecated* bit to tell the network stack to only do > HW checksum generation on IPV4. > > This bit is deprecated for a reason, again, see skbuff.h. The right > approach, *which the driver already does*, is to tell the stack that we > support HW checksuming using NETIF_F_HW_CSUM, and then, in the transmit > handler, to call skb_checksum_help() to have the SW calculate the > checksum if it's not a supported type. > > My understanding was when we enable NETIF_F_HW_CSUM means network > stack enables HW checksum and doesn't calculate SW checksum. But as per > this supported types HW checksum are used only for IPV4 and not for IPV6 even > though driver enabled NETIF_F_HW_CSUM. For IPV6 it is always a SW generated > checksum, please correct me here. Have you actually read the comments in skbuff.h that I pointed you to ? And the rest of my email for that matter ? Yes, I went through comments in skbuff.h and your comments as well. I knew about this deprecated bit that's why I have disabled NETIF_F_HW_CSUM completely in my V1 patch. Then Florian gave a comment and asked me to disable only IPV6 not IPV4 as it is working for IPV4 and issue with only IPV6. That's why I sent patch V2 accommodating his feedback. I don't have much understanding of IP Stack but I went through code details and you are right and found that it should fallback to SW calculation for IPV6 but it doesn't happen because ftgmac100_hard_start_xmit checks for CHECKSUM_PARTIAL before setting HW checksum and calling ftgmac100_prep_tx_csum function. And in my understanding, this value is set CHECKSUM_PARTIAL in IP stack. I looked up IP stack for IPV6, file net/ipv6/ip6_output.c, function __ip6_append_data: here it sets CHECKSUM_PARTIAL only for UDP packets not for TCP packets. Please look at line number 1880. This could be an issue we are seeing here as why ftgmac100_prep_tx_csum is not getting triggered for IPV6 with TCP. Please correct me if my understanding is wrong. > This is exactly what ftgmac100_prep_tx_csum() does. It only enables HW > checksum generation on supported types and uses skb_checksum_help() > otherwise, supported types being protocol ETH_P_IP and IP protocol > being raw IP, TCP and UDP. > > > So this *should* have fallen back to SW for IPV6. So either something > in my code there is making an incorrect assumption, or something is > broken in skb_checksum_help() for IPV6 (which I somewhat doubt) or > something else I can't think of, but setting a *deprecated* flag is > definitely not the right answer, neither is completely disabling HW > checksumming. > > So can you investigate what's going on a bit more closely please ? I > can try myself, though I have very little experience with IPV6 and > probably won't have time before next week. > > Cheers, > Ben. > > > The driver should have handled unsupported csum via SW fallback > > already in ftgmac100_prep_tx_csum() > > > > Can you check why this didn't work for you ? > > > > Cheers, > > Ben. > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > > --- > > > Changes since v1: > > > Enabled IPV4 hw checksum generation as it works for IPV4. > > > > > > drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > > > b/drivers/net/ethernet/faraday/ftgmac100.c > > > index 030fed65393e..0255a28d2958 100644 > > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > > @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct > > > platform_device *pdev) > > > /* AST2400 doesn't have working HW checksum generation */ > > > if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) > > > netdev->hw_features &= ~NETIF_F_HW_CSUM; > > > + > > > + /* AST2500 doesn't have working HW checksum generation for IPV6 > > > + * but it works for IPV4, so disabling hw checksum and enabling > > > + * it for only IPV4. > > > + */ > > > + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) > > > { > > > + netdev->hw_features &= ~NETIF_F_HW_CSUM; > > > + netdev->hw_features |= NETIF_F_IP_CSUM; > > > + } > > > + > > > if (np && of_get_property(np, "no-hw-checksum", NULL)) > > > - netdev->hw_features &= ~(NETIF_F_HW_CSUM | > > > NETIF_F_RXCSUM); > > > + netdev->hw_features &= ~(NETIF_F_HW_CSUM | > > > NETIF_F_RXCSUM > > > + | NETIF_F_IP_CSUM); > > > netdev->features |= netdev->hw_features; > > > > > > /* register network device */ > > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-18 22:50 ` Vijay Khemka @ 2019-10-19 0:03 ` Benjamin Herrenschmidt 2019-10-19 1:31 ` Vijay Khemka 0 siblings, 1 reply; 14+ messages in thread From: Benjamin Herrenschmidt @ 2019-10-19 0:03 UTC (permalink / raw) To: Vijay Khemka, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On Fri, 2019-10-18 at 22:50 +0000, Vijay Khemka wrote: > I don't have much understanding of IP Stack but I went through code details and > you are right and found that it should fallback to SW calculation for IPV6 but it doesn't > happen because ftgmac100_hard_start_xmit checks for CHECKSUM_PARTIAL before > setting HW checksum and calling ftgmac100_prep_tx_csum function. And in my > understanding, this value is set CHECKSUM_PARTIAL in IP stack. I looked up IP stack for > IPV6, file net/ipv6/ip6_output.c, function __ip6_append_data: here it sets > CHECKSUM_PARTIAL only for UDP packets not for TCP packets. Please look at line > number 1880. This could be an issue we are seeing here as why > ftgmac100_prep_tx_csum is not getting triggered for IPV6 with TCP. Please correct > me if my understanding is wrong. > Not entirely sure. tcp_v6_send_response() in tcp_ipv6.c does set CHECKSUM_PARTIAL as well. I don't really know how things are being handled in that part of the network stack though. From a driver perspective, if the value of ip_summed is not CHECKSUM_PARTIAL it means we should not have to calculate any checksum. At least that's my understanding here. You may need to add some traces to the driver to see what you get in there, what protocol indication etc... and analyze the corresponding packets with something like tcpdump or wireshark on the other end. Cheers, Ben. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-19 0:03 ` Benjamin Herrenschmidt @ 2019-10-19 1:31 ` Vijay Khemka 2019-10-19 10:25 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-10-19 1:31 UTC (permalink / raw) To: Benjamin Herrenschmidt, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On 10/18/19, 5:03 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote: On Fri, 2019-10-18 at 22:50 +0000, Vijay Khemka wrote: > I don't have much understanding of IP Stack but I went through code details and > you are right and found that it should fallback to SW calculation for IPV6 but it doesn't > happen because ftgmac100_hard_start_xmit checks for CHECKSUM_PARTIAL before > setting HW checksum and calling ftgmac100_prep_tx_csum function. And in my > understanding, this value is set CHECKSUM_PARTIAL in IP stack. I looked up IP stack for > IPV6, file net/ipv6/ip6_output.c, function __ip6_append_data: here it sets > CHECKSUM_PARTIAL only for UDP packets not for TCP packets. Please look at line > number 1880. This could be an issue we are seeing here as why > ftgmac100_prep_tx_csum is not getting triggered for IPV6 with TCP. Please correct > me if my understanding is wrong. > Not entirely sure. tcp_v6_send_response() in tcp_ipv6.c does set CHECKSUM_PARTIAL as well. I don't really know how things are being handled in that part of the network stack though. From a driver perspective, if the value of ip_summed is not CHECKSUM_PARTIAL it means we should not have to calculate any checksum. At least that's my understanding here. You may need to add some traces to the driver to see what you get in there, what protocol indication etc... and analyze the corresponding packets with something like tcpdump or wireshark on the other end. Thanks Ben, I will try to add some trace and test whatever possible and test it. As we don't have tcpdump into our image and I have limited understanding of networking stack so if you get some time to verify ipv6, it will be really helpful. Cheers, Ben. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 2019-10-19 1:31 ` Vijay Khemka @ 2019-10-19 10:25 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 14+ messages in thread From: Benjamin Herrenschmidt @ 2019-10-19 10:25 UTC (permalink / raw) To: Vijay Khemka, David S. Miller, Kate Stewart, Sven Van Asbroeck, Mark Brown, Bhupesh Sharma, YueHaibing, Mauro Carvalho Chehab, Luis Chamberlain, Thomas Gleixner, netdev, linux-kernel Cc: openbmc @ lists . ozlabs . org, joel, linux-aspeed, Sai Dasari On Sat, 2019-10-19 at 01:31 +0000, Vijay Khemka wrote: > Thanks Ben, > I will try to add some trace and test whatever possible and test it. > As we > don't have tcpdump into our image and I have limited understanding of > networking stack so if you get some time to verify ipv6, it will be > really > helpful. > You only need tcpdump (or wireshark) on the *other end* of the link, could even be your laptop, to look at what the generated frames look like and compare with your traces. Cheers, Ben. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-10-19 10:26 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20190911194453.2595021-1-vijaykhemka@fb.com> 2019-09-17 19:34 ` [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500 Vijay Khemka 2019-09-24 17:26 ` Vijay Khemka 2019-10-10 19:20 ` Vijay Khemka 2019-10-11 1:16 ` Jakub Kicinski [not found] <20191011213027.2110008-1-vijaykhemka@fb.com> 2019-10-17 1:28 ` Benjamin Herrenschmidt 2019-10-17 22:01 ` Vijay Khemka 2019-10-17 23:14 ` Benjamin Herrenschmidt 2019-10-17 23:34 ` Benjamin Herrenschmidt 2019-10-18 0:06 ` Vijay Khemka 2019-10-18 0:32 ` Benjamin Herrenschmidt 2019-10-18 22:50 ` Vijay Khemka 2019-10-19 0:03 ` Benjamin Herrenschmidt 2019-10-19 1:31 ` Vijay Khemka 2019-10-19 10:25 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).