Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Vijay Khemka]

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
    
    

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Vijay Khemka]

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
        
        
    
    

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Vijay Khemka]

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
    
    

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Jakub Kicinski]

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>

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Benjamin Herrenschmidt]

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 */

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Vijay Khemka]

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 */
    
    

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Benjamin Herrenschmidt]

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 */
>     
>     
> 

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Benjamin Herrenschmidt]

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.

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Vijay Khemka]

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 */
    >     
    >     
    > 
    
    

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Benjamin Herrenschmidt]

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 */
>     >     
>     >     
>     > 
>     
>     
> 

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Vijay Khemka]

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 */
    >     >     
    >     >     
    >     > 
    >     
    >     
    > 
    
    

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Benjamin Herrenschmidt]

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.

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Vijay Khemka]

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.
    
    
    

Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500

[Benjamin Herrenschmidt]

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.