On Wed, Jan 22, 2020 at 01:29:32PM -0500, Willem de Bruijn wrote:

On Tue, Jan 21, 2020 at 12:51 PM Martin Varghese
[off-list ref] wrote:

quoted

From: Martin Varghese <redacted>

The Bareudp tunnel module provides a generic L3 encapsulation
tunnelling module for tunnelling different protocols like MPLS,
IP,NSH etc inside a UDP tunnel.

Signed-off-by: Martin Varghese <redacted>

This addresses the main points I raised. A few small points below,
nothing serious. It could use more eye balls, but beyond those Acked
from me.

quoted

---
Changes in v2:
     - Fixed documentation errors.
     - Converted documentation to rst format.
     - Moved ip tunnel rt lookup code to a common location.
     - Removed seperate v4 and v6 socket.
     - Added call to skb_ensure_writable before updating ethernet header.
     - Simplified bareudp_destroy_tunnels as deleting devices under a
       namespace is taken care be the default pernet exit code.
     - Fixed bareudp_change_mtu.

Changes in v3:
     - Re-sending the patch again.

Changes in v4:
     - Converted bareudp device to l3 device.

I didn't quite get this statement, but it encompasses the change to
ARPHRD_NONE and introduction of gro_cells, I guess?

The term l3 device is from OVS may be.What i meant is we no longer
needed the dummy ethernet header and the device works with l3 packet.
 

quoted

     - Removed redundant fields in bareudp device.

quoted

diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
index d07d985..ea3d604 100644
--- a/Documentation/networking/index.rst
+++ b/Documentation/networking/index.rst

@@ -33,6 +33,7 @@ Contents:
    tls
    tls-offload
    nfc
+   bareudp

if respinning: this list is mostly alphabetically ordened, perhaps
insert before batman-adv

quoted

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index dee7958..9726447 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig

@@ -258,6 +258,19 @@ config GENEVE
          To compile this driver as a module, choose M here: the module
          will be called geneve.

+config BAREUDP
+       tristate "Bare UDP Encapsulation"
+       depends on INET && NET_UDP_TUNNEL
+       depends on IPV6 || !IPV6
+       select NET_IP_TUNNEL
+       select GRO_CELLS

Depends on NET_UDP_TUNNEL plus selects NET_IP_TUNNEL seems odd.

NET_UDP_TUNNEL itself selects NET_IP_TUNNEL, so perhaps just select
NET_UDP_TUNNEL.

I had to make that change to be able to get it in a .config after make
defconfig.

Noted

quoted

+static int bareudp_change_mtu(struct net_device *dev, int new_mtu)
+{
+       dev->mtu = new_mtu;
+       return 0;
+}

If your ndo_change_mtu does nothing special, it can just rely on the
assignment in __dev_set_mtu

Yes we could remove the ndo_change_mtu implementation as it is redundant
But i would like to retain bareudp_change_mtu and also to add a validation 
code in the function as no validation is done in the rtnetlink layer during newlink create

quoted

+/* Initialize the device structure. */
+static void bareudp_setup(struct net_device *dev)
+{
+       dev->netdev_ops = &bareudp_netdev_ops;
+       dev->needs_free_netdev = true;
+       SET_NETDEV_DEVTYPE(dev, &bareudp_type);
+       dev->features    |= NETIF_F_SG | NETIF_F_HW_CSUM;
+       dev->features    |= NETIF_F_RXCSUM;
+       dev->features    |= NETIF_F_GSO_SOFTWARE;
+       dev->hw_features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+       dev->hw_features |= NETIF_F_GSO_SOFTWARE;
+       dev->hard_header_len = 0;
+       dev->addr_len = 0;
+       dev->mtu = 1500;

ETH_DATA_LEN?

Noted

Thanks for your time.The code look really better from the v1 version.