[PATCH RFC net-next] net: ag71xx: disable GRO by default

[PATCH RFC net-next] net: ag71xx: disable GRO by default

[Rosen Penev]

```
Currently this is handled in userspace with ethtool. Not sure if this
should be done in the kernel or if this is even the proper place for it.
```

ag71xx is usually paired with qca8k or ar9331, both DSA drivers. DSA
internally uses GRO cells to speed up transactions. But this speed up
only works if hardware checksumming is supported. Unfortunately for
ag71xx, this is unsupported. On newer QCA devices, there is an external
HWNAT module that can be used to provide this, but the necessary code
has not been written. Mainly because at the time, the proper netfilter
code adding the proper APIs was not present.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/atheros/ag71xx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index b74856760be3..95da34c71b34 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -1770,6 +1770,15 @@ static int ag71xx_change_mtu(struct net_device *ndev, int new_mtu)
 	return 0;
 }
 
+static netdev_features_t ag71xx_fix_features(struct net_device *ndev,
+					     netdev_features_t features)
+{
+	/* remove GRO. Hardware checksumming is needed to avoid a massive
+	 * reduction in switching speed */
+	features &= ~NETIF_F_SOFT_FEATURES;
+	return features;
+}
+
 static const struct net_device_ops ag71xx_netdev_ops = {
 	.ndo_open		= ag71xx_open,
 	.ndo_stop		= ag71xx_stop,
@@ -1777,6 +1786,7 @@ static const struct net_device_ops ag71xx_netdev_ops = {
 	.ndo_eth_ioctl		= phy_do_ioctl,
 	.ndo_tx_timeout		= ag71xx_tx_timeout,
 	.ndo_change_mtu		= ag71xx_change_mtu,
+	.ndo_fix_features	= ag71xx_fix_features,
 	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_validate_addr	= eth_validate_addr,
 };
-- 
2.46.0

Re: [PATCH RFC net-next] net: ag71xx: disable GRO by default

[Andrew Lunn]

On Fri, Aug 16, 2024 at 03:47:33PM -0700, Rosen Penev wrote:
> ```
> Currently this is handled in userspace with ethtool. Not sure if this
> should be done in the kernel or if this is even the proper place for it.
> ```

Comments like this should be placed under the ---. If the patch is
merged, anything in the commit message under the --- is then
discarded.

> ag71xx is usually paired with qca8k or ar9331, both DSA drivers.

Can it be used without a switch? It looks like this option will
disable offloads which are useful when there is no switch.

	Andrew

Re: [PATCH RFC net-next] net: ag71xx: disable GRO by default

[Rosen Penev]

On Sat, Aug 17, 2024 at 11:39 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Aug 16, 2024 at 03:47:33PM -0700, Rosen Penev wrote:
> > ```
> > Currently this is handled in userspace with ethtool. Not sure if this
> > should be done in the kernel or if this is even the proper place for it.
> > ```
>
> Comments like this should be placed under the ---. If the patch is
> merged, anything in the commit message under the --- is then
> discarded.
Well, I don't mean for the patch to be merged. I'm mostly trying to
get feedback on it. From what I see in the tree, it's not common to
disable NETIF_F_SOFT_FEATURES.
>
> > ag71xx is usually paired with qca8k or ar9331, both DSA drivers.
>
> Can it be used without a switch? It looks like this option will
> disable offloads which are useful when there is no switch.
Most of the time it is used with a switch. Even in cases where only
one port is available it still goes through the switch (QCA9531 single
port device using AR9331 DSA).

Some older devices might not go through the switch. I'm not sure.
>
>         Andrew