Re: [PATCH net-next v4 1/2] net: UDP tunnel encapsulation module for

[Martin Varghese <martinvarghesenokia@gmail.com>]

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
> <martinvarghesenokia@gmail.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@nokia.com>
> >
> > 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 <martin.varghese@nokia.com>
> 
> 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.
> 
> > ---
> > 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.
 
> >      - Removed redundant fields in bareudp device.
> 
> > 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
> 
> > 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
> > +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

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