2022-05-13 17:17:44

by Dylan Hung

[permalink] [raw]
Subject: RE: [PATCH net v2] net: ftgmac100: Disable hardware checksum on AST2600

> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Joel
> Stanley
> Sent: Friday, May 13, 2022 7:20 AM
> To: David S . Miller <[email protected]>; Jakub Kicinski
> <[email protected]>; Paolo Abeni <[email protected]>; Benjamin
> Herrenschmidt <[email protected]>; Dylan Hung
> <[email protected]>; David Wilder <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; David Wilder <[email protected]>
> Subject: [PATCH net v2] net: ftgmac100: Disable hardware checksum on
> AST2600
>
> The AST2600 when using the i210 NIC over NC-SI has been observed to
> produce incorrect checksum results with specific MTU values. This was first
> observed when sending data across a long distance set of networks.
>
> On a local network, the following test was performed using a 1MB file of
> random data.
>
> On the receiver run this script:
>
> #!/bin/bash
> while [ 1 ]; do
> # Zero the stats
> nstat -r > /dev/null
> nc -l 9899 > test-file
> # Check for checksum errors
> TcpInCsumErrors=$(nstat | grep TcpInCsumErrors)
> if [ -z "$TcpInCsumErrors" ]; then
> echo No TcpInCsumErrors
> else
> echo TcpInCsumErrors = $TcpInCsumErrors
> fi
> done
>
> On an AST2600 system:
>
> # nc <IP of receiver host> 9899 < test-file
>
> The test was repeated with various MTU values:
>
> # ip link set mtu 1410 dev eth0
>
> The observed results:
>
> 1500 - good
> 1434 - bad
> 1400 - good
> 1410 - bad
> 1420 - good
>
> The test was repeated after disabling tx checksumming:
>
> # ethtool -K eth0 tx-checksumming off
>
> And all MTU values tested resulted in transfers without error.
>
> An issue with the driver cannot be ruled out, however there has been no bug
> discovered so far.
>
> David has done the work to take the original bug report of slow data transfer
> between long distance connections and triaged it down to this test case.
>
> The vendor suspects this this is a hardware issue when using NC-SI. The fixes
> line refers to the patch that introduced AST2600 support.
>
> Fixes: 39bfab8844a0 ("net: ftgmac100: Add support for DT phy-handle
> property")
> Reported-by: David Wilder <[email protected]>
> Signed-off-by: Joel Stanley <[email protected]>
> ---
> v2 updates the commit message with confirmation form the vendor that this is
> a hardware issue, and clarifes why the commit used in the fixes tag was
> chosen.
>
> drivers/net/ethernet/faraday/ftgmac100.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index caf48023f8ea..5231818943c6 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1928,6 +1928,11 @@ 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;
> +
> + /* AST2600 tx checksum with NC-SI is broken */
> + if (priv->use_ncsi && of_device_is_compatible(np,
> "aspeed,ast2600-mac"))
> + netdev->hw_features &= ~NETIF_F_HW_CSUM;
> +
> if (np && of_get_property(np, "no-hw-checksum", NULL))
> netdev->hw_features &= ~(NETIF_F_HW_CSUM |
> NETIF_F_RXCSUM);
> netdev->features |= netdev->hw_features;
> --
> 2.35.1
Reviewed-by: Dylan Hung <[email protected]>


2022-05-17 17:17:38

by Joel Stanley

[permalink] [raw]
Subject: Re: [PATCH net v2] net: ftgmac100: Disable hardware checksum on AST2600

On Fri, 13 May 2022 at 01:46, Dylan Hung <[email protected]> wrote:
>
> > -----Original Message-----
> > From: [email protected] [mailto:[email protected]] On Behalf Of Joel
> > Stanley
> > Sent: Friday, May 13, 2022 7:20 AM
> > To: David S . Miller <[email protected]>; Jakub Kicinski
> > <[email protected]>; Paolo Abeni <[email protected]>; Benjamin
> > Herrenschmidt <[email protected]>; Dylan Hung
> > <[email protected]>; David Wilder <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; David Wilder <[email protected]>
> > Subject: [PATCH net v2] net: ftgmac100: Disable hardware checksum on
> > AST2600
> >
> > The AST2600 when using the i210 NIC over NC-SI has been observed to
> > produce incorrect checksum results with specific MTU values. This was first
> > observed when sending data across a long distance set of networks.
> >
> > On a local network, the following test was performed using a 1MB file of
> > random data.
> >
> > On the receiver run this script:
> >
> > #!/bin/bash
> > while [ 1 ]; do
> > # Zero the stats
> > nstat -r > /dev/null
> > nc -l 9899 > test-file
> > # Check for checksum errors
> > TcpInCsumErrors=$(nstat | grep TcpInCsumErrors)
> > if [ -z "$TcpInCsumErrors" ]; then
> > echo No TcpInCsumErrors
> > else
> > echo TcpInCsumErrors = $TcpInCsumErrors
> > fi
> > done
> >
> > On an AST2600 system:
> >
> > # nc <IP of receiver host> 9899 < test-file
> >
> > The test was repeated with various MTU values:
> >
> > # ip link set mtu 1410 dev eth0
> >
> > The observed results:
> >
> > 1500 - good
> > 1434 - bad
> > 1400 - good
> > 1410 - bad
> > 1420 - good
> >
> > The test was repeated after disabling tx checksumming:
> >
> > # ethtool -K eth0 tx-checksumming off
> >
> > And all MTU values tested resulted in transfers without error.
> >
> > An issue with the driver cannot be ruled out, however there has been no bug
> > discovered so far.
> >
> > David has done the work to take the original bug report of slow data transfer
> > between long distance connections and triaged it down to this test case.
> >
> > The vendor suspects this this is a hardware issue when using NC-SI. The fixes
> > line refers to the patch that introduced AST2600 support.
> >
> > Fixes: 39bfab8844a0 ("net: ftgmac100: Add support for DT phy-handle
> > property")
> > Reported-by: David Wilder <[email protected]>
> > Signed-off-by: Joel Stanley <[email protected]>

> Reviewed-by: Dylan Hung <[email protected]>

Thank you Dylan. I've added your r-b to v3, as the only changes are to
the wrapping of the commit message.