On Mon, Dec 4, 2017 at 2:15 PM, Yuval Mintz [off-list ref] wrote:

quoted

@@ -96,6 +98,7 @@ enum {
 #define NETIF_F_FRAGLIST     __NETIF_F(FRAGLIST)
 #define NETIF_F_FSO          __NETIF_F(FSO)
 #define NETIF_F_GRO          __NETIF_F(GRO)
+#define NETIF_F_GRO_HW               __NETIF_F(GRO_HW)
 #define NETIF_F_GSO          __NETIF_F(GSO)
 #define NETIF_F_GSO_ROBUST   __NETIF_F(GSO_ROBUST)
 #define NETIF_F_HIGHDMA              __NETIF_F(HIGHDMA)

@@ -193,7 +196,7 @@ enum {
  * If upper/master device has these features disabled, they must be disabled
  * on all lower/slave devices as well.
  */
-#define NETIF_F_UPPER_DISABLES       NETIF_F_LRO
+#define NETIF_F_UPPER_DISABLES       (NETIF_F_LRO | NETIF_F_GRO_HW)

 /* changeable features with no special hardware requirements */
 #define NETIF_F_SOFT_FEATURES        (NETIF_F_GSO | NETIF_F_GRO)

diff --git a/net/core/dev.c b/net/core/dev.c
index 30b5fe3..09c2ad0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -7392,6 +7392,19 @@ static netdev_features_t

netdev_fix_features(struct net_device *dev,
              features &= ~dev->gso_partial_features;
      }

+     if (features & NETIF_F_GRO_HW) {
+             /* Hardware GRO depends on GRO. */
+             if (!(features & NETIF_F_GRO)) {

While at it, perhaps also make it dependent on NETIF_F_RXCSUM?

OK.  Makes sense.

quoted

+                     netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since
no GRO feature.\n");
+                     features &= ~NETIF_F_GRO_HW;
+             }
+             /* Hardware GRO and LRO are mutually exclusive. */
+             if (features & NETIF_F_LRO) {
+                     netdev_dbg(dev, "Dropping NETIF_F_LRO since
GRO_HW is set.\n");
+                     features &= ~NETIF_F_LRO;

Isn't this considered to be breaking an existing API?
After this, while NETIF_F_GRO_HW is published an application trying to
set NETIF_F_LRO and then query its state would discover it failed
[while previously it could have succeeded, such as for bnx2]

While I understand the need to make sure core doesn't enable
two competing aggregation offloads, why make GRO_HW > LRO?
I understand it's probably the better one, but until LRO gets deprecated
isn't it safer to do this limitation the opposite way?
I.e., make sure NETIF_F_GRO_HW can't be set as long as NETIF_F_LRO is set?

I am just following precedents in the netdev_fix_features() logic to
drop incompatible features.  I can make LRO and GRO_HW have equal
standing by dropping the other when one is set.  So if I do that, user
will be able to always enable LRO.  The code will just drop GRO_HW if
it is set.