Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH bpf-next v4 1/2] libbpf: add support for using AF_XDP sockets
From: Maciej Fijalkowski @ 2019-02-18 11:21 UTC (permalink / raw)
  To: Magnus Karlsson
  Cc: Daniel Borkmann, Magnus Karlsson, Björn Töpel, ast,
	Network Development, Jakub Kicinski, Björn Töpel,
	Zhang, Qi Z, Jesper Dangaard Brouer, xiaolong.ye
In-Reply-To: <CAJ8uoz1xjv8OBAn==S3EBiGvTqp_dd_t7YQqp2TOSR6JSOdrnA@mail.gmail.com>

On Mon, 18 Feb 2019 09:59:30 +0100
Magnus Karlsson <magnus.karlsson@gmail.com> wrote:

> On Fri, Feb 15, 2019 at 6:09 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 02/08/2019 02:05 PM, Magnus Karlsson wrote:  
> > > This commit adds AF_XDP support to libbpf. The main reason for this is
> > > to facilitate writing applications that use AF_XDP by offering
> > > higher-level APIs that hide many of the details of the AF_XDP
> > > uapi. This is in the same vein as libbpf facilitates XDP adoption by
> > > offering easy-to-use higher level interfaces of XDP
> > > functionality. Hopefully this will facilitate adoption of AF_XDP, make
> > > applications using it simpler and smaller, and finally also make it
> > > possible for applications to benefit from optimizations in the AF_XDP
> > > user space access code. Previously, people just copied and pasted the
> > > code from the sample application into their application, which is not
> > > desirable.
> > >
> > > The interface is composed of two parts:
> > >
> > > * Low-level access interface to the four rings and the packet
> > > * High-level control plane interface for creating and setting
> > >   up umems and af_xdp sockets as well as a simple XDP program.
> > >
> > > Tested-by: Björn Töpel <bjorn.topel@intel.com>
> > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>  
> > [...]  
> > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > > new file mode 100644
> > > index 0000000..a982a76
> > > --- /dev/null
> > > +++ b/tools/lib/bpf/xsk.c
> > > @@ -0,0 +1,742 @@
> > > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > > +
> > > +/*
> > > + * AF_XDP user-space access library.
> > > + *
> > > + * Copyright(c) 2018 - 2019 Intel Corporation.
> > > + *
> > > + * Author(s): Magnus Karlsson <magnus.karlsson@intel.com>
> > > + */
> > > +
> > > +#include <errno.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <unistd.h>
> > > +#include <arpa/inet.h>
> > > +#include <asm/barrier.h>
> > > +#include <linux/compiler.h>
> > > +#include <linux/filter.h>
> > > +#include <linux/if_ether.h>
> > > +#include <linux/if_link.h>
> > > +#include <linux/if_packet.h>
> > > +#include <linux/if_xdp.h>
> > > +#include <linux/rtnetlink.h>
> > > +#include <net/if.h>
> > > +#include <sys/mman.h>
> > > +#include <sys/socket.h>
> > > +#include <sys/types.h>
> > > +
> > > +#include "bpf.h"
> > > +#include "libbpf.h"
> > > +#include "libbpf_util.h"
> > > +#include "nlattr.h"
> > > +#include "xsk.h"
> > > +
> > > +#ifndef SOL_XDP
> > > + #define SOL_XDP 283
> > > +#endif
> > > +
> > > +#ifndef AF_XDP
> > > + #define AF_XDP 44
> > > +#endif
> > > +
> > > +#ifndef PF_XDP
> > > + #define PF_XDP AF_XDP
> > > +#endif
> > > +
> > > +struct xsk_umem {
> > > +     struct xsk_ring_prod *fill;
> > > +     struct xsk_ring_cons *comp;
> > > +     char *umem_area;
> > > +     struct xsk_umem_config config;
> > > +     int fd;
> > > +     int refcount;
> > > +};
> > > +
> > > +struct xsk_socket {
> > > +     struct xsk_ring_cons *rx;
> > > +     struct xsk_ring_prod *tx;
> > > +     __u64 outstanding_tx;
> > > +     struct xsk_umem *umem;
> > > +     struct xsk_socket_config config;
> > > +     int fd;
> > > +     int xsks_map;
> > > +     int ifindex;
> > > +     int prog_fd;
> > > +     int qidconf_map_fd;
> > > +     int xsks_map_fd;
> > > +     __u32 queue_id;
> > > +};
> > > +
> > > +struct xsk_nl_info {
> > > +     bool xdp_prog_attached;
> > > +     int ifindex;
> > > +     int fd;
> > > +};
> > > +
> > > +#define MAX_QUEUES 128  
> >
> > Why is this a fixed constant here, shouldn't this be dynamic due to being NIC
> > specific anyway?  
> 
> It was only here for simplicity. If a NIC had more queues, it would
> require a recompile of the lib. Obviously, not desirable in a distro.
> What I could do is to read the max "combined" queues (pre-set maximum
> in the ethtool output) from the same interface as ethool uses and size
> the array after that. Or is there a simpler way? What to do if the NIC
> does not have a "combined", or is there no such NIC (seems the common
> HW ones set this)?
> 
> > [...]  
> > > +void *xsk_umem__get_data(struct xsk_umem *umem, __u64 addr)
> > > +{
> > > +     return &((char *)(umem->umem_area))[addr];
> > > +}  
> >
> > There's also a xsk_umem__get_data_raw() doing the same. Why having both, resp.
> > when to choose which? ;)  
> 
> There is enough to have the xsk_umem__get_data_raw() function.
> xsk_umem__get_data() is just a convenience function for which the
> application does not have to store the beginning of the umem. But as
> the application always has to provide this anyway in the
> xsk_umem__create() function, it might as well store this pointer. I
> will delete xsk_umem__get_data() and rename xsk_umem__get_data_raw()
> to xsk_umem__get_data().
> 
> > > +int xsk_umem__fd(const struct xsk_umem *umem)
> > > +{
> > > +     return umem ? umem->fd : -EINVAL;
> > > +}
> > > +
> > > +int xsk_socket__fd(const struct xsk_socket *xsk)
> > > +{
> > > +     return xsk ? xsk->fd : -EINVAL;
> > > +}
> > > +
> > > +static bool xsk_page_aligned(void *buffer)
> > > +{
> > > +     unsigned long addr = (unsigned long)buffer;
> > > +
> > > +     return !(addr & (getpagesize() - 1));
> > > +}
> > > +
> > > +static void xsk_set_umem_config(struct xsk_umem_config *cfg,
> > > +                             const struct xsk_umem_config *usr_cfg)
> > > +{
> > > +     if (!usr_cfg) {
> > > +             cfg->fill_size = XSK_RING_PROD__DEFAULT_NUM_DESCS;
> > > +             cfg->comp_size = XSK_RING_CONS__DEFAULT_NUM_DESCS;
> > > +             cfg->frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
> > > +             cfg->frame_headroom = XSK_UMEM__DEFAULT_FRAME_HEADROOM;
> > > +             return;
> > > +     }
> > > +
> > > +     cfg->fill_size = usr_cfg->fill_size;
> > > +     cfg->comp_size = usr_cfg->comp_size;
> > > +     cfg->frame_size = usr_cfg->frame_size;
> > > +     cfg->frame_headroom = usr_cfg->frame_headroom;  
> >
> > Just optional nit, might be a bit nicer to have it in this form:
> >
> >         cfg->fill_size = usr_cfg ? usr_cfg->fill_size :
> >                          XSK_RING_PROD__DEFAULT_NUM_DESCS;  
> 
> I actually think the current form is clearer when there are multiple
> lines. If there was only one line, I would agree with you.
> 
> > > +}
> > > +
> > > +static void xsk_set_xdp_socket_config(struct xsk_socket_config *cfg,
> > > +                                   const struct xsk_socket_config *usr_cfg)
> > > +{
> > > +     if (!usr_cfg) {
> > > +             cfg->rx_size = XSK_RING_CONS__DEFAULT_NUM_DESCS;
> > > +             cfg->tx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS;
> > > +             cfg->libbpf_flags = 0;
> > > +             cfg->xdp_flags = 0;
> > > +             cfg->bind_flags = 0;
> > > +             return;
> > > +     }
> > > +
> > > +     cfg->rx_size = usr_cfg->rx_size;
> > > +     cfg->tx_size = usr_cfg->tx_size;
> > > +     cfg->libbpf_flags = usr_cfg->libbpf_flags;
> > > +     cfg->xdp_flags = usr_cfg->xdp_flags;
> > > +     cfg->bind_flags = usr_cfg->bind_flags;  
> >
> > (Ditto)
> >  
> > > +}
> > > +
> > > +int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size,
> > > +                  struct xsk_ring_prod *fill, struct xsk_ring_cons *comp,
> > > +                  const struct xsk_umem_config *usr_config)
> > > +{
> > > +     struct xdp_mmap_offsets off;
> > > +     struct xdp_umem_reg mr;
> > > +     struct xsk_umem *umem;
> > > +     socklen_t optlen;
> > > +     void *map;
> > > +     int err;
> > > +
> > > +     if (!umem_area || !umem_ptr || !fill || !comp)
> > > +             return -EFAULT;
> > > +     if (!size && !xsk_page_aligned(umem_area))
> > > +             return -EINVAL;
> > > +
> > > +     umem = calloc(1, sizeof(*umem));
> > > +     if (!umem)
> > > +             return -ENOMEM;
> > > +
> > > +     umem->fd = socket(AF_XDP, SOCK_RAW, 0);
> > > +     if (umem->fd < 0) {
> > > +             err = -errno;
> > > +             goto out_umem_alloc;
> > > +     }
> > > +
> > > +     umem->umem_area = umem_area;
> > > +     xsk_set_umem_config(&umem->config, usr_config);
> > > +
> > > +     mr.addr = (uintptr_t)umem_area;
> > > +     mr.len = size;
> > > +     mr.chunk_size = umem->config.frame_size;
> > > +     mr.headroom = umem->config.frame_headroom;
> > > +
> > > +     err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_REG, &mr, sizeof(mr));
> > > +     if (err) {
> > > +             err = -errno;
> > > +             goto out_socket;
> > > +     }
> > > +     err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_FILL_RING,
> > > +                      &umem->config.fill_size,
> > > +                      sizeof(umem->config.fill_size));
> > > +     if (err) {
> > > +             err = -errno;
> > > +             goto out_socket;
> > > +     }
> > > +     err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_COMPLETION_RING,
> > > +                      &umem->config.comp_size,
> > > +                      sizeof(umem->config.comp_size));
> > > +     if (err) {
> > > +             err = -errno;
> > > +             goto out_socket;
> > > +     }
> > > +
> > > +     optlen = sizeof(off);
> > > +     err = getsockopt(umem->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
> > > +     if (err) {
> > > +             err = -errno;
> > > +             goto out_socket;
> > > +     }
> > > +
> > > +     map = xsk_mmap(NULL, off.fr.desc +
> > > +                    umem->config.fill_size * sizeof(__u64),
> > > +                    PROT_READ | PROT_WRITE, MAP_SHARED | MAP_POPULATE,
> > > +                    umem->fd, XDP_UMEM_PGOFF_FILL_RING);
> > > +     if (map == MAP_FAILED) {
> > > +             err = -errno;
> > > +             goto out_socket;
> > > +     }
> > > +
> > > +     umem->fill = fill;
> > > +     fill->mask = umem->config.fill_size - 1;
> > > +     fill->size = umem->config.fill_size;
> > > +     fill->producer = map + off.fr.producer;
> > > +     fill->consumer = map + off.fr.consumer;
> > > +     fill->ring = map + off.fr.desc;
> > > +     fill->cached_cons = umem->config.fill_size;
> > > +
> > > +     map = xsk_mmap(NULL,
> > > +                    off.cr.desc + umem->config.comp_size * sizeof(__u64),
> > > +                    PROT_READ | PROT_WRITE, MAP_SHARED | MAP_POPULATE,
> > > +                    umem->fd, XDP_UMEM_PGOFF_COMPLETION_RING);
> > > +     if (map == MAP_FAILED) {
> > > +             err = -errno;
> > > +             goto out_mmap;
> > > +     }
> > > +
> > > +     umem->comp = comp;
> > > +     comp->mask = umem->config.comp_size - 1;
> > > +     comp->size = umem->config.comp_size;
> > > +     comp->producer = map + off.cr.producer;
> > > +     comp->consumer = map + off.cr.consumer;
> > > +     comp->ring = map + off.cr.desc;
> > > +
> > > +     *umem_ptr = umem;
> > > +     return 0;
> > > +
> > > +out_mmap:
> > > +     munmap(umem->fill,
> > > +            off.fr.desc + umem->config.fill_size * sizeof(__u64));
> > > +out_socket:
> > > +     close(umem->fd);
> > > +out_umem_alloc:
> > > +     free(umem);
> > > +     return err;
> > > +}
> > > +
> > > +static int xsk_parse_nl(void *cookie, void *msg, struct nlattr **tb)
> > > +{
> > > +     struct nlattr *tb_parsed[IFLA_XDP_MAX + 1];
> > > +     struct xsk_nl_info *nl_info = cookie;
> > > +     struct ifinfomsg *ifinfo = msg;
> > > +     unsigned char mode;
> > > +     int err;
> > > +
> > > +     if (nl_info->ifindex && nl_info->ifindex != ifinfo->ifi_index)
> > > +             return 0;
> > > +
> > > +     if (!tb[IFLA_XDP])
> > > +             return 0;
> > > +
> > > +     err = libbpf_nla_parse_nested(tb_parsed, IFLA_XDP_MAX, tb[IFLA_XDP],
> > > +                                   NULL);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     if (!tb_parsed[IFLA_XDP_ATTACHED] || !tb_parsed[IFLA_XDP_FD])
> > > +             return 0;
> > > +
> > > +     mode = libbpf_nla_getattr_u8(tb_parsed[IFLA_XDP_ATTACHED]);
> > > +     if (mode == XDP_ATTACHED_NONE)
> > > +             return 0;
> > > +
> > > +     nl_info->xdp_prog_attached = true;
> > > +     nl_info->fd = libbpf_nla_getattr_u32(tb_parsed[IFLA_XDP_FD]);  
> >
> > Hm, I don't think this works if I read the intention of this helper correctly.
> >
> > IFLA_XDP_FD is never set for retrieving the prog from the kernel. So the
> > above is a bug.
> >
> > We also have bpf_get_link_xdp_id(). This should probably just be reused in
> > this context here.  
> 
> If bpf_get_link_xdp_id() will fit my bill, I will happily use it. I
> will check it out and hopefully I can drop all this code. Thanks.
>
I see that all you need to know is whether there's already attached XDP program
to xsk socket's related interface, no?
If so, then within the xsk_setup_xdp_prog, you could do something like:

	u32 prog_id = 0;

	bpf_get_link_xdp_id(xsk->ifindex, &prog_id, xsk->config.xdp_flags);
	if (!prog_id) {
		// create maps
		// load xdp prog
	} else {
		xsk->fd = prog_id;
	}

	xsk_update_bpf_maps(xsk, true, xsk->fd);

If that's ok then xsk_xdp_prog_attached and xsk_parse_nl could be dropped.

> > > +     return 0;
> > > +}
> > > +
> > > +static bool xsk_xdp_prog_attached(struct xsk_socket *xsk)
> > > +{
> > > +     struct xsk_nl_info nl_info;
> > > +     unsigned int nl_pid;
> > > +     char err_buf[256];
> > > +     int sock, err;
> > > +
> > > +     sock = libbpf_netlink_open(&nl_pid);
> > > +     if (sock < 0)
> > > +             return false;
> > > +
> > > +     nl_info.xdp_prog_attached = false;
> > > +     nl_info.ifindex = xsk->ifindex;
> > > +     nl_info.fd = -1;
> > > +
> > > +     err = libbpf_nl_get_link(sock, nl_pid, xsk_parse_nl, &nl_info);
> > > +     if (err) {
> > > +             libbpf_strerror(err, err_buf, sizeof(err_buf));
> > > +             pr_warning("Error:\n%s\n", err_buf);
> > > +             close(sock);
> > > +             return false;
> > > +     }
> > > +
> > > +     close(sock);
> > > +     xsk->prog_fd = nl_info.fd;
> > > +     return nl_info.xdp_prog_attached;
> > > +}  
> >
> > (See bpf_get_link_xdp_id().)
> >  
> > > +
> > > +static int xsk_load_xdp_prog(struct xsk_socket *xsk)
> > > +{
> > > +     char bpf_log_buf[BPF_LOG_BUF_SIZE];
> > > +     int err, prog_fd;
> > > +
> > > +     /* This is the C-program:
> > > +      * SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx)
> > > +      * {
> > > +      *     int *qidconf, index = ctx->rx_queue_index;  
> > [...]  
> > > +
> > > +int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
> > > +                    __u32 queue_id, struct xsk_umem *umem,
> > > +                    struct xsk_ring_cons *rx, struct xsk_ring_prod *tx,
> > > +                    const struct xsk_socket_config *usr_config)
> > > +{
> > > +     struct sockaddr_xdp sxdp = {};
> > > +     struct xdp_mmap_offsets off;
> > > +     struct xsk_socket *xsk;
> > > +     socklen_t optlen;
> > > +     void *map;
> > > +     int err;
> > > +
> > > +     if (!umem || !xsk_ptr || !rx || !tx)
> > > +             return -EFAULT;
> > > +
> > > +     if (umem->refcount) {
> > > +             pr_warning("Error: shared umems not supported by libbpf.\n");
> > > +             return -EBUSY;
> > > +     }
> > > +
> > > +     xsk = calloc(1, sizeof(*xsk));
> > > +     if (!xsk)
> > > +             return -ENOMEM;
> > > +
> > > +     if (umem->refcount++ > 0) {  
> >
> > Should this refcount rather be atomic actually?  
> 
> Neither our config nor data plane interfaces are reentrant for
> performance reasons. Any concurrency has to be handled explicitly on
> the application level. This so that it only penalizes apps that really
> need this.
> 
> Thanks for all your reviews: Magnus
> 
> > > +             xsk->fd = socket(AF_XDP, SOCK_RAW, 0);
> > > +             if (xsk->fd < 0) {
> > > +                     err = -errno;
> > > +                     goto out_xsk_alloc;
> > > +             }
> > > +     } else {
> > > +             xsk->fd = umem->fd;
> > > +     }
> > > +
> > > +     xsk->outstanding_tx = 0;
> > > +     xsk->queue_id = queue_id;
> > > +     xsk->umem = umem;
> > > +     xsk->ifindex = if_nametoindex(ifname);
> > > +     if (!xsk->ifindex) {
> > > +             err = -errno;
> > > +             goto out_socket;
> > > +     }
> > > +
> > > +     xsk_set_xdp_socket_config(&xsk->config, usr_config);  
> > [...]  


^ permalink raw reply

* Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-18 11:23 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: Andrew Lunn, Vivien Didelot, David S. Miller, netdev
In-Reply-To: <20190218005031.wqj54o6ilqfng5nb@shell.armlinux.org.uk>

On Mon, Feb 18, 2019 at 12:50:31AM +0000, Russell King - ARM Linux admin wrote:
> From what I can see, the port_vlan_add()/port_vlan_del() implementation
> is far from ideal, just like "always enabling flooding for CPU/DSA
> ports" is not ideal.

There also seems to be a discrepency between what net/dsa wants to do
and some of the implementations in drivers/net/dsa:

dsa_switch_vlan_add() does this:

        bitmap_zero(ds->bitmap, ds->num_ports);
        if (ds->index == info->sw_index)
                set_bit(info->port, ds->bitmap);
        for (port = 0; port < ds->num_ports; port++)
                if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
                        set_bit(port, ds->bitmap);

We then call ds->ops->port_vlan_add() for each port in ds->bitmap,
which will include DSA and CPU ports on every switch in the tree.

For rtl8366, this calls into rtl8366_vlan_add(), which does:

        if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
                dev_err(smi->dev, "port is DSA or CPU port\n");

which is surely a guaranteed error message if we have any CPU or DSA
ports specified on a rtl8366.  The example in the DT documentation
for this driver does suggest that it is capable of having CPU ports.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply

* Re: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
From: Russell King - ARM Linux admin @ 2019-02-18 11:28 UTC (permalink / raw)
  To: Stefan Chulski
  Cc: Antoine Tenart, davem@davemloft.net, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com,
	maxime.chevallier@bootlin.com, gregory.clement@bootlin.com,
	miquel.raynal@bootlin.com, Nadav Haklai, Yan Markman,
	mw@semihalf.com
In-Reply-To: <DM5PR18MB1067E1DD47E7094E02043A58B0630@DM5PR18MB1067.namprd18.prod.outlook.com>

Hi Stefan,

On Mon, Feb 18, 2019 at 11:08:34AM +0000, Stefan Chulski wrote:
> HW recommendation upon Serdes reconfiguration are the following:
> 
> 1. Disable port(CTRL0_REG - in XLG/GMAC) 
> 2. Put port in reset (both XLG/GMAC)
> 3. For KR - put in reset MPCS (MAC control clock, RX SD clock, TX SD clock), XPSC is RXAUI/XAUI clock domain
> 4. Power down Serdes lane
> 
> Do reconfiguration of Serdes.
> 
> 5. Enable Serdes lane
> 6. Disable MPCS reset for KR
> 7. Disable port reset (both XLG/GMAC)
> 8. Enable port  (both XLG/GMAC)

For clarity, presumably either the XLG or the GMAC should be released
from reset and enabled at any one time depending on the configured mode,
but never both together?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply

* RE: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
From: Stefan Chulski @ 2019-02-18 11:33 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Antoine Tenart, davem@davemloft.net, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com,
	maxime.chevallier@bootlin.com, gregory.clement@bootlin.com,
	miquel.raynal@bootlin.com, Nadav Haklai, Yan Markman,
	mw@semihalf.com
In-Reply-To: <20190218112815.ec3pyuvzpphdance@shell.armlinux.org.uk>



> -----Original Message-----
> From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Sent: Monday, February 18, 2019 1:28 PM
> To: Stefan Chulski <stefanc@marvell.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; davem@davemloft.net;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> thomas.petazzoni@bootlin.com; maxime.chevallier@bootlin.com;
> gregory.clement@bootlin.com; miquel.raynal@bootlin.com; Nadav Haklai
> <nadavh@marvell.com>; Yan Markman <ymarkman@marvell.com>;
> mw@semihalf.com
> Subject: Re: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS
> while reconfiguring the serdes lanes
> 
> Hi Stefan,
> 
> On Mon, Feb 18, 2019 at 11:08:34AM +0000, Stefan Chulski wrote:
> > HW recommendation upon Serdes reconfiguration are the following:
> >
> > 1. Disable port(CTRL0_REG - in XLG/GMAC) 2. Put port in reset (both
> > XLG/GMAC) 3. For KR - put in reset MPCS (MAC control clock, RX SD
> > clock, TX SD clock), XPSC is RXAUI/XAUI clock domain 4. Power down
> > Serdes lane
> >
> > Do reconfiguration of Serdes.
> >
> > 5. Enable Serdes lane
> > 6. Disable MPCS reset for KR
> > 7. Disable port reset (both XLG/GMAC)
> > 8. Enable port  (both XLG/GMAC)
> 
> For clarity, presumably either the XLG or the GMAC should be released from
> reset and enabled at any one time depending on the configured mode, but
> never both together?

Yes.
Only one of them the XLG or the GMAC ,depending on the configured mode.

Best Regards.

^ permalink raw reply

* Re: [PATCH net-next v2 0/3] net: dsa: mv88e6xxx: fix IPv6
From: Russell King - ARM Linux admin @ 2019-02-18 11:34 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Vivien Didelot; +Cc: David S. Miller, netdev
In-Reply-To: <20190217163114.yomawlljyxlqy3ob@shell.armlinux.org.uk>

On Sun, Feb 17, 2019 at 04:31:14PM +0000, Russell King - ARM Linux admin wrote:
> We have had some emails in private over this issue, this is my current
> patch set rebased on top of net-next which provides working IPv6 (and
> probably other protocols as well) over mv88e6xxx DSA switches.
> 
> The problem comes down to mv88e6xxx defaulting to not flood unknown
> unicast and multicast datagrams, as they would be by dumb switches,
> and as the Linux bridge code does by default.
> 
> These flood settings can be disabled via the Linux bridge code if it's
> desired to make the switch behave more like a managed switch, eg, by
> enabling the multicast querier.  However, the multicast querier
> defaults to being disabled which effectively means that by default,
> mv88e6xxx switches block all multicast traffic.  This is at odds with
> the Linux bridge documentation, and the defaults that the Linux bridge
> code adopts.
> 
> So, this patch set adds DSA support for Linux bridge flags, adds
> mv88e6xxx support for the unicast and multicast flooding flags, and
> lastly enables flooding of these frames by default to match the
> Linux bridge defaults.

While looking at some of the other DSA drivers, I've noticed that
others are also programmed to forward unknown frames to the CPU
port.  Does this not end up breaking stuff?

If I tcpdump the ethernet interface for the CPU port, what I see
is:

11:21:21.901127 00:22:68:15:37:dd (oui Unknown) > 52:54:00:00:06:25 (oui
Unknown), ethertype MEDSA (0xdada), length 126: Forward, untagged,
dev.port:vlan 0.4:0, pri 0: ethertype IPv6 (0x86dd)
e0022681537dd.dyn.armlinux.org.uk > tftp.armlinux.org.uk: ICMP6, echo
request, seq 1, length 64

which is the unknown frame being delivered to the CPU port.  It seems
nothing else happens with the frame - it is ignored.  Before my fixes
for mv88e6xxx, that frame (and the following frames for the same MAC
address) would end up being forwarded only to the CPU port and dropped
on the floor, never making their way to their intended destination.

It seems that "the hardware doesn't know what to do, forward it to
Linux to sort out" doesn't actually work.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply

* Re: [PATCH net] sock: return uapi errno in sock_setsockopt() for SO_ZEROCOPY
From: Alexey Kodanev @ 2019-02-18 11:35 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: Network Development, Petr Vorel, David Miller
In-Reply-To: <CA+FuTSeRV3Qy_S3c4qayTe3K1FkaPF7cZnV7_tNVKnS3cEuxmA@mail.gmail.com>

On 15.02.2019 19:58, Willem de Bruijn wrote:
> On Fri, Feb 15, 2019 at 11:51 AM Alexey Kodanev
> <alexey.kodanev@oracle.com> wrote:
>>
>> For unsupported protocols, setsockopt() with SO_ZEROCOPY
>> option sets errno to ENOTSUPP(524). But this number is
>> not defined anywhere in the include/uapi/ headers.
>>
>> To make sure userspace sees the known number, replace
>> ENOTSUPP(524) with EOPNOTSUPP(95).
>>
>> Fixes: 76851d1212c1 ("sock: add SOCK_ZEROCOPY sockopt")
>> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
>> Reported-by: Petr Vorel <pvorel@suse.cz>
> 
> This code has been there since 4.14. I think it's too late to change
> system call behavior.
> 

'ENOTSUPP' define is solely for an internal usage, it may be replaced
with another one or the number associated with it may be changed one day,
implicitly changing the behavior of setsockopt().

^ permalink raw reply

* Re: [RFC] net: dsa: qca8k: implement rgmii-id mode
From: Michal Vokáč @ 2019-02-18 11:54 UTC (permalink / raw)
  To: Vinod Koul, Andrew Lunn
  Cc: David S. Miller, netdev, linux-kernel@vger.kernel.org,
	Florian Fainelli
In-Reply-To: <20190218104539.GL21884@vkoul-mobl>

On 18. 02. 19 11:45, Vinod Koul wrote:
> On 15-02-19, 16:23, Andrew Lunn wrote:
>> On Fri, Feb 15, 2019 at 04:01:08PM +0100, Michal Vokáč wrote:
>>> Hi,
>>>
>>> networking on my boards [1], which are currently in linux-next, suddently
>>> stopped working. I tracked it down to this commit 5ecdd77c61c8 ("net: dsa:
>>> qca8k: disable delay for RGMII mode") [2].
>>>
>>> So I think the rgmii-id mode is obviously needed in my case.
>>> I was able to find a couple drivers that read tx/rx-delay or
>>> tx/rx-internal-delay from device tree. Namely:
>>>
>>>    drivers/net/ethernet/apm/xgene/xgene_enet_main.c
>>>    drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
>>>    drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>>>    drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
>>>    drivers/net/phy/dp83867.c
>>>
>>> I would appreciate any hints how to add similar function to qca8k driver
>>> if that is the correct way to go. Can I take some of the above mentioned
>>> drivers as a good example for that? How should the binding look like?
>>>
>>> I would expect something like this:
>>>
>>> 	switch@0 {
>>> 		compatible = "qca,qca8334";
>>> 		reg = <0>;
>>>
>>> 		switch_ports: ports {
>>> 			#address-cells = <1>;
>>> 			#size-cells = <0>;
>>>
>>> 			ethphy0: port@0 {
>>> 				reg = <0>;
>>> 				label = "cpu";
>>> 				phy-mode = "rgmii-id";
>>> 				qca,tx-delay = <3>;
>>> 				qca,rx-delay = <3>;
>>> 				ethernet = <&fec>;
>>> 		};
>>
>> Hi Michal
>>
>> Your submission used:
>>
>> +				ethphy0: port@0 {
>> +					reg = <0>;
>> +					label = "cpu";
>> +					phy-mode = "rgmii";
>> +					ethernet = <&fec>;
>> +
>> +					fixed-link {
>> +						speed = <1000>;
>> +						full-duplex;
>> +					};
>> +				};
>>
>> This is good. If you have a fixed-link you can pass a phy-mode.

Yes, I am using fixed-link and the plan was to implement the rgmii-id
mode.

>>
>> The comment that was removed was:
>>
>> -               /* According to the datasheet, RGMII delay is enabled through
>> -                * PORT5_PAD_CTRL for all ports, rather than individual port
>> -                * registers
>> -                */
>>
>> Is it possible to enable delays per port? Ideally, you want to enable
>> delays for just selected ports. Add another case for
>> PHY_INTERFACE_MODE_RGMII_ID to enable the delays.

I am still trying to collect all the relevant notes and bits from the
horrible docs to understand how this is done. The problem is different
parts and different versions of the documentation provide diffrerent
details.

For example the QCA8334 model does not have PORT5 and so that registers
are not documented. Though some application note document says:

   """
   The MAC0 timing control is in the Port0 PAD Mode Control Register (offset 0x0004):

   1. RGMII timing delay for the output path of QCA8337(N) is enabled by 0x8[24].
      Set 1 to add 2ns delay in 1000 mode for all RGMII interfaces.

   2. Bit [21:20]: select the delay time for the output path in 10/100 mode.

   3. Bit 25: enable the timing delay for the input path of QCA8337(N) in 1000 mode.

   4. Bit [23:22]: select the delay time for the input path in 1000 mode.
     00: 0.2ns
     01: 1.2ns
     10: 2.1ns
     11: 3.1ns
   """

That is in line with the removed comment. The bit 24 at address 0x8 is
used to enable/disable delays globally. And obviously it is needed on the
QCA8334 model as well even though it should not have the PORT5.

> In the hindsight I should not have removed the comment, let me ressurect
> that as well as add handling of the RGMII modes...

OK, thanks.

> Please do test

Sure, will do. Just let me know when you have something ready.
Thank you!

Michal

^ permalink raw reply

* Re: [PATCH] net: sched: matchall: verify that filter is not NULL in mall_walk()
From: Vlad Buslov @ 2019-02-18 12:00 UTC (permalink / raw)
  To: Cong Wang
  Cc: Ido Schimmel, Linux Kernel Network Developers, Jamal Hadi Salim,
	Jiri Pirko, David Miller
In-Reply-To: <CAM_iQpW4WD2KpQM344DLjtzUYamR9-r398qYev1c=kgFLJhu5A@mail.gmail.com>


On Sat 16 Feb 2019 at 00:24, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Feb 15, 2019 at 4:11 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Check that filter is not NULL before passing it to tcf_walker->fn()
>> callback. This can happen when mall_change() failed to offload filter to
>> hardware.
>>
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>> ---
>>  net/sched/cls_matchall.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
>> index a37137430e61..1f9d481b0fbb 100644
>> --- a/net/sched/cls_matchall.c
>> +++ b/net/sched/cls_matchall.c
>> @@ -247,6 +247,9 @@ static void mall_walk(struct tcf_proto *tp, struct tcf_walker *arg,
>>
>>         if (arg->count < arg->skip)
>>                 goto skip;
>> +
>> +       if (!head)
>> +               return;
>
> So head==NULL still counts one given that you check NULL after
> checking arg->count. Is this expected?

My intention was to fix the problem (arg->fn() call with NULL filter)
without changing any other functionality, and always incrementing
arg->count once seemed to be the intended behavior. However, since
mall_delete() just returns -EOPNOTSUPP, it might be the case that author
of matchall expected to always have single filter configured when cls
API calls mall_walk(). What would you suggest?

^ permalink raw reply

* [PATCH net-next] bnx2x: Remove set but not used variable 'mfw_vn'
From: YueHaibing @ 2019-02-18 12:19 UTC (permalink / raw)
  To: Ariel Elior, Sudarsana Kalluru, David S . Miller
  Cc: YueHaibing, GR-everest-linux-l2, netdev, kernel-janitors

Fixes gcc '-Wunused-but-set-variable' warning:

drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c: In function 'bnx2x_get_hwinfo':
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:11940:10: warning:
 variable 'mfw_vn' set but not used [-Wunused-but-set-variable]

It's never used since introduction.

Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 3b5b47e98c73..7c47be215a34 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -11998,7 +11998,7 @@ static void validate_set_si_mode(struct bnx2x *bp)
 static int bnx2x_get_hwinfo(struct bnx2x *bp)
 {
 	int /*abs*/func = BP_ABS_FUNC(bp);
-	int vn, mfw_vn;
+	int vn;
 	u32 val = 0, val2 = 0;
 	int rc = 0;
 
@@ -12083,12 +12083,10 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)
 	/*
 	 * Initialize MF configuration
 	 */
-
 	bp->mf_ov = 0;
 	bp->mf_mode = 0;
 	bp->mf_sub_mode = 0;
 	vn = BP_VN(bp);
-	mfw_vn = BP_FW_MB_IDX(bp);
 
 	if (!CHIP_IS_E1(bp) && !BP_NOMCP(bp)) {
 		BNX2X_DEV_INFO("shmem2base 0x%x, size %d, mfcfg offset %d\n",




^ permalink raw reply related

* Re: [PATCH] tcp: Namespace-ify sysctl_tcp_rmem and sysctl_tcp_wmem
From: Greg KH @ 2019-02-18 12:22 UTC (permalink / raw)
  To: Alakesh Haloi
  Cc: stable, David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Eric Dumazet, netdev, linux-kernel
In-Reply-To: <20190214011159.GA35034@dev-dsk-alakeshh-2c-f8a3e6e0.us-west-2.amazon.com>

On Thu, Feb 14, 2019 at 01:12:08AM +0000, Alakesh Haloi wrote:
> [ Upstream commit 356d1833b638bd465672aefeb71def3ab93fc17d ]
> 
> Note that when a new netns is created, it inherits its
> sysctl_tcp_rmem and sysctl_tcp_wmem from initial netns.
> 
> This change is needed so that we can refine TCP rcvbuf autotuning,
> to take RTT into consideration.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [alakeshh: backport to v4.14: The patch does not apply to v4.14
> directly and hence needed manual backport. Function signature for
> the function tcp_select_initial_window had to be changed to be able
> to pass pointer to struct sock.]
> Signed-off-by: Alakesh Haloi <alakeshh@amazon.com>

Like Sasha said, why is this needed in 4.14.y?  It looks like a new
feature.  What is keeping you from just using 4.19.y if you want this
feature?

thanks,

greg k-h

^ permalink raw reply

* [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled
From: Nikolay Aleksandrov @ 2019-02-18 12:21 UTC (permalink / raw)
  To: netdev
  Cc: roopa, linus.luessing, idosch, f.fainelli, bridge,
	Nikolay Aleksandrov
In-Reply-To: <20190215130427.29824-1-nikolay@cumulusnetworks.com>

This is v2 of the RFC patch which aims to forward packets to known
mdsts' ports only (the no querier case). After v1 I've kept
the previous behaviour when it comes to unregistered traffic or when
a querier is present. All of this is of course only with snooping
enabled. So with this patch the following changes should occur:
 - No querier: forward known mdst traffic to its registered ports,
               no change about unknown mcast (flood)
 - Querier present: no change

The reason to do this is simple - we want to respect the user's mdb
configuration in both cases, that is if the user adds static mdb entries
manually then we should use that information about forwarding traffic.

What do you think ?

* Notes
Traffic that is currently marked as mrouters_only:
 - IPv4: non-local mcast traffic, igmp reports
 - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports

Simple use case:
 $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
 $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
 - without a querier currently traffic for 239.0.0.1 will still be flooded,
   with this change it will be forwarded only to swp1

Ido, I know this doesn't solve the issue you brought up, maybe we should
have a separate discussion about acting on querier changes in the switch
driver or alternative solutions (e.g. always-flood-unknown-mcast knob).
Perhaps the bridge can notify the drivers on querier state changes.

This patch is meant about discussing the best way to solve the issue,
it's not thoroughly tested, in case we settle about the details I'll run
more tests.

Thanks,

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_device.c | 5 +++--
 net/bridge/br_input.c  | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 013323b6dbe4..e8c01409a7e7 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -96,8 +96,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 
 		mdst = br_mdb_get(br, skb, vid);
-		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		    br_multicast_querier_exists(br, eth_hdr(skb)))
+		if (mdst ||
+		    (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) &&
+		     br_multicast_querier_exists(br, eth_hdr(skb))))
 			br_multicast_flood(mdst, skb, false, true);
 		else
 			br_flood(br, skb, BR_PKT_MULTICAST, false, true);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 5ea7e56119c1..8777566f7b6d 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -136,8 +136,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	switch (pkt_type) {
 	case BR_PKT_MULTICAST:
 		mdst = br_mdb_get(br, skb, vid);
-		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-		    br_multicast_querier_exists(br, eth_hdr(skb))) {
+		if (mdst ||
+		    (BR_INPUT_SKB_CB_MROUTERS_ONLY(skb) &&
+		     br_multicast_querier_exists(br, eth_hdr(skb)))) {
 			if ((mdst && mdst->host_joined) ||
 			    br_multicast_is_router(br)) {
 				local_rcv = true;
-- 
2.20.1


^ permalink raw reply related

* Re: [PATCH net-next] net: sched: potential NULL dereference in tcf_block_find()
From: Vlad Buslov @ 2019-02-18 12:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
	netdev@vger.kernel.org, kernel-janitors@vger.kernel.org
In-Reply-To: <20190218092632.GB7712@kadam>

On Mon 18 Feb 2019 at 09:26, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> The error code isn't set on this path so it would result in returning
> ERR_PTR(0) and a NULL dereference in the caller.
>
> Fixes: 18d3eefb17cf ("net: sched: refactor tcf_block_find() into standalone functions")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Hi Dan,

Thank you for finding and fixing this!

Regards,
Vlad

^ permalink raw reply

* Re: [PATCH net-next v6] ipmr: ip6mr: Create new sockopt to clear mfc cache or vifs
From: Nikolay Aleksandrov @ 2019-02-18 12:31 UTC (permalink / raw)
  To: Callum Sinclair, davem, kuznet, yoshfuji, netdev, linux-kernel
  Cc: nicolas.dichtel
In-Reply-To: <20190217210752.20914-2-callum.sinclair@alliedtelesis.co.nz>

On 17/02/2019 23:07, Callum Sinclair wrote:
> Currently the only way to clear the forwarding cache was to delete the
> entries one by one using the MRT_DEL_MFC socket option or to destroy and
> recreate the socket.
> 
> Create a new socket option which with the use of optional flags can
> clear any combination of multicast entries (static or not static) and
> multicast vifs (static or not static).
> 
> Calling the new socket option MRT_FLUSH with the flags MRT_FLUSH_MFC and
> MRT_FLUSH_VIFS will clear all entries and vifs on the socket except for
> static entries.
> 
> Signed-off-by: Callum Sinclair <callum.sinclair@alliedtelesis.co.nz>
> ---
> v1 -> v2:
>   Implemented additional flags for static entries
> v2 -> v3:
>   Cleaned up flag logic so any combination of routes can be cleared.
>   Fixed style errors
>   Fixed incorrect flag values
> v3 -> v4:
>   Fixed style errors
>   Fixed incorrect flag (MRT_FLUSH was used instead of MRT_FLUSH_VIFS)
> v4 -> v5:
>   Only clear the unresolved queue when MRT_FLUSH_MFC flag is set.
> v5 -> v6:
>   Renamed MRT6_FLUSH_VIFS to MRT6_FLUSH_MIFS
> 
>  include/uapi/linux/mroute.h  |  9 ++++-
>  include/uapi/linux/mroute6.h |  9 ++++-
>  net/ipv4/ipmr.c              | 75 +++++++++++++++++++++-------------
>  net/ipv6/ip6mr.c             | 78 +++++++++++++++++++++++-------------
>  4 files changed, 115 insertions(+), 56 deletions(-)
> 

Looks good to me,
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>



^ permalink raw reply

* Re: stmmac / meson8b-dwmac
From: Simon Huelck @ 2019-02-18 12:33 UTC (permalink / raw)
  To: Jose Abreu, Martin Blumenstingl
  Cc: Emiliano Ingrassia, Gpeppe.cavallaro, alexandre.torgue,
	linux-amlogic, netdev
In-Reply-To: <c80c1e5b-fed9-25ce-eb1b-258ac0135ce7@synopsys.com>

Hi,


i recognize the followin on my ethernet stats:

     threshold: 1
     tx_pkt_n: 1457325
     rx_pkt_n: 5022405
     normal_irq_n: 671738
     rx_normal_irq_n: 606573
     napi_poll: 784439
     tx_normal_irq_n: 61061
     tx_clean: 784439
     tx_set_ic_bit: 58293
     irq_receive_pmt_irq_n: 0
     mmc_tx_irq_n: 0
     mmc_rx_irq_n: 0
     mmc_rx_csum_offload_irq_n: 0
->      irq_tx_path_in_lpi_mode_n: 382037
->      irq_tx_path_exit_lpi_mode_n: 382021


Is this normal ?


regards,
Simon



Am 18.02.2019 um 09:45 schrieb Jose Abreu:
> On 2/18/2019 8:42 AM, Jose Abreu wrote:
>> On 2/17/2019 2:48 PM, Martin Blumenstingl wrote:
>>> Jose, is my command for disabling offloading correct?
>> Yes, I think so. What about NIC statistics and logs ? ("ethtool
>> -S eth0", "dmesg | grep -i stmmac")
> And, "ifconfig eth0" after running the test.
>
>> Thanks,
>> Jose Miguel Abreu
>>


^ permalink raw reply

* [PATCH net] iptunnel: NULL pointer deref for ip_md_tunnel_xmit
From: Alan Maguire @ 2019-02-18 12:36 UTC (permalink / raw)
  To: netdev, davem; +Cc: kuznet, yoshfuji, ast, daniel, kafai, songliubraving, yhs

Naresh Kamboju noted the following oops during execution of selftest
tools/testing/selftests/bpf/test_tunnel.sh on x86_64:

[  274.120445] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000000
[  274.128285] #PF error: [INSTR]
[  274.131351] PGD 8000000414a0e067 P4D 8000000414a0e067 PUD 3b6334067 PMD 0
[  274.138241] Oops: 0010 [#1] SMP PTI
[  274.141734] CPU: 1 PID: 11464 Comm: ping Not tainted
5.0.0-rc4-next-20190129 #1
[  274.149046] Hardware name: Supermicro SYS-5019S-ML/X11SSH-F, BIOS
2.0b 07/27/2017
[  274.156526] RIP: 0010:          (null)
[  274.160280] Code: Bad RIP value.
[  274.163509] RSP: 0018:ffffbc9681f83540 EFLAGS: 00010286
[  274.168726] RAX: 0000000000000000 RBX: ffffdc967fa80a18 RCX: 0000000000000000
[  274.175851] RDX: ffff9db2ee08b540 RSI: 000000000000000e RDI: ffffdc967fa809a0
[  274.182974] RBP: ffffbc9681f83580 R08: ffff9db2c4d62690 R09: 000000000000000c
[  274.190098] R10: 0000000000000000 R11: ffff9db2ee08b540 R12: ffff9db31ce7c000
[  274.197222] R13: 0000000000000001 R14: 000000000000000c R15: ffff9db3179cf400
[  274.204346] FS:  00007ff4ae7c5740(0000) GS:ffff9db31fa80000(0000)
knlGS:0000000000000000
[  274.212424] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  274.218162] CR2: ffffffffffffffd6 CR3: 00000004574da004 CR4: 00000000003606e0
[  274.225292] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  274.232416] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  274.239541] Call Trace:
[  274.241988]  ? tnl_update_pmtu+0x296/0x3b0
[  274.246085]  ip_md_tunnel_xmit+0x1bc/0x520
[  274.250176]  gre_fb_xmit+0x330/0x390
[  274.253754]  gre_tap_xmit+0x128/0x180
[  274.257414]  dev_hard_start_xmit+0xb7/0x300
[  274.261598]  sch_direct_xmit+0xf6/0x290
[  274.265430]  __qdisc_run+0x15d/0x5e0
[  274.269007]  __dev_queue_xmit+0x2c5/0xc00
[  274.273011]  ? dev_queue_xmit+0x10/0x20
[  274.276842]  ? eth_header+0x2b/0xc0
[  274.280326]  dev_queue_xmit+0x10/0x20
[  274.283984]  ? dev_queue_xmit+0x10/0x20
[  274.287813]  arp_xmit+0x1a/0xf0
[  274.290952]  arp_send_dst.part.19+0x46/0x60
[  274.295138]  arp_solicit+0x177/0x6b0
[  274.298708]  ? mod_timer+0x18e/0x440
[  274.302281]  neigh_probe+0x57/0x70
[  274.305684]  __neigh_event_send+0x197/0x2d0
[  274.309862]  neigh_resolve_output+0x18c/0x210
[  274.314212]  ip_finish_output2+0x257/0x690
[  274.318304]  ip_finish_output+0x219/0x340
[  274.322314]  ? ip_finish_output+0x219/0x340
[  274.326493]  ip_output+0x76/0x240
[  274.329805]  ? ip_fragment.constprop.53+0x80/0x80
[  274.334510]  ip_local_out+0x3f/0x70
[  274.337992]  ip_send_skb+0x19/0x40
[  274.341391]  ip_push_pending_frames+0x33/0x40
[  274.345740]  raw_sendmsg+0xc15/0x11d0
[  274.349403]  ? __might_fault+0x85/0x90
[  274.353151]  ? _copy_from_user+0x6b/0xa0
[  274.357070]  ? rw_copy_check_uvector+0x54/0x130
[  274.361604]  inet_sendmsg+0x42/0x1c0
[  274.365179]  ? inet_sendmsg+0x42/0x1c0
[  274.368937]  sock_sendmsg+0x3e/0x50
[  274.372460]  ___sys_sendmsg+0x26f/0x2d0
[  274.376293]  ? lock_acquire+0x95/0x190
[  274.380043]  ? __handle_mm_fault+0x7ce/0xb70
[  274.384307]  ? lock_acquire+0x95/0x190
[  274.388053]  ? __audit_syscall_entry+0xdd/0x130
[  274.392586]  ? ktime_get_coarse_real_ts64+0x64/0xc0
[  274.397461]  ? __audit_syscall_entry+0xdd/0x130
[  274.401989]  ? trace_hardirqs_on+0x4c/0x100
[  274.406173]  __sys_sendmsg+0x63/0xa0
[  274.409744]  ? __sys_sendmsg+0x63/0xa0
[  274.413488]  __x64_sys_sendmsg+0x1f/0x30
[  274.417405]  do_syscall_64+0x55/0x190
[  274.421064]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  274.426113] RIP: 0033:0x7ff4ae0e6e87
[  274.429686] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00
00 00 00 8b 05 ca d9 2b 00 48 63 d2 48 63 ff 85 c0 75 10 b8 2e 00 00
00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 53 48 89 f3 48 83 ec 10 48 89 7c
24 08
[  274.448422] RSP: 002b:00007ffcd9b76db8 EFLAGS: 00000246 ORIG_RAX:
000000000000002e
[  274.455978] RAX: ffffffffffffffda RBX: 0000000000000040 RCX: 00007ff4ae0e6e87
[  274.463104] RDX: 0000000000000000 RSI: 00000000006092e0 RDI: 0000000000000003
[  274.470228] RBP: 0000000000000000 R08: 00007ffcd9bc40a0 R09: 00007ffcd9bc4080
[  274.477349] R10: 000000000000060a R11: 0000000000000246 R12: 0000000000000003
[  274.484475] R13: 0000000000000016 R14: 00007ffcd9b77fa0 R15: 00007ffcd9b78da4
[  274.491602] Modules linked in: cls_bpf sch_ingress iptable_filter
ip_tables algif_hash af_alg x86_pkg_temp_thermal fuse [last unloaded:
test_bpf]
[  274.504634] CR2: 0000000000000000
[  274.507976] ---[ end trace 196d18386545eae1 ]---
[  274.512588] RIP: 0010:          (null)
[  274.516334] Code: Bad RIP value.
[  274.519557] RSP: 0018:ffffbc9681f83540 EFLAGS: 00010286
[  274.524775] RAX: 0000000000000000 RBX: ffffdc967fa80a18 RCX: 0000000000000000
[  274.531921] RDX: ffff9db2ee08b540 RSI: 000000000000000e RDI: ffffdc967fa809a0
[  274.539082] RBP: ffffbc9681f83580 R08: ffff9db2c4d62690 R09: 000000000000000c
[  274.546205] R10: 0000000000000000 R11: ffff9db2ee08b540 R12: ffff9db31ce7c000
[  274.553329] R13: 0000000000000001 R14: 000000000000000c R15: ffff9db3179cf400
[  274.560456] FS:  00007ff4ae7c5740(0000) GS:ffff9db31fa80000(0000)
knlGS:0000000000000000
[  274.568541] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  274.574277] CR2: ffffffffffffffd6 CR3: 00000004574da004 CR4: 00000000003606e0
[  274.581403] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  274.588535] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  274.595658] Kernel panic - not syncing: Fatal exception in interrupt
[  274.602046] Kernel Offset: 0x14400000 from 0xffffffff81000000
(relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[  274.612827] ---[ end Kernel panic - not syncing: Fatal exception in
interrupt ]---
[  274.620387] ------------[ cut here ]------------

I'm also seeing the same failure on x86_64, and it reproduces
consistently.

From poking around it looks like the skb's dst entry is being used
to calculate the mtu in:

mtu = skb_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;

...but because that dst_entry  has an "ops" value set to md_dst_ops,
the various ops (including mtu) are not set:

crash> struct sk_buff._skb_refdst ffff928f87447700 -x
      _skb_refdst = 0xffffcd6fbf5ea590
crash> struct dst_entry.ops 0xffffcd6fbf5ea590
  ops = 0xffffffffa0193800
crash> struct dst_ops.mtu 0xffffffffa0193800
  mtu = 0x0
crash>

I confirmed that the dst entry also has dst->input set to
dst_md_discard, so it looks like it's an entry that's been
initialized via __metadata_dst_init alright.

I think the fix here is to use skb_valid_dst(skb) - it checks
for  DST_METADATA also, and with that fix in place, the
problem - which was previously 100% reproducible - disappears.

The below patch resolves the panic and all bpf tunnel tests pass
without incident.

Fixes: c8b34e680a09 ("ip_tunnel: Add tnl_update_pmtu in ip_md_tunnel_xmit")

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 net/ipv4/ip_tunnel.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 893f013..5dcf50c 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -515,9 +515,10 @@ static int tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb,
 		mtu = dst_mtu(&rt->dst) - dev->hard_header_len
 					- sizeof(struct iphdr) - tunnel_hlen;
 	else
-		mtu = skb_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;
+		mtu = skb_valid_dst(skb) ? dst_mtu(skb_dst(skb)) : dev->mtu;
 
-	skb_dst_update_pmtu(skb, mtu);
+	if (skb_valid_dst(skb))
+		skb_dst_update_pmtu(skb, mtu);
 
 	if (skb->protocol == htons(ETH_P_IP)) {
 		if (!skb_is_gso(skb) &&
@@ -530,9 +531,11 @@ static int tnl_update_pmtu(struct net_device *dev, struct sk_buff *skb,
 	}
 #if IS_ENABLED(CONFIG_IPV6)
 	else if (skb->protocol == htons(ETH_P_IPV6)) {
-		struct rt6_info *rt6 = (struct rt6_info *)skb_dst(skb);
+		struct rt6_info *rt6;
 		__be32 daddr;
 
+		rt6 = skb_valid_dst(skb) ? (struct rt6_info *)skb_dst(skb) :
+					   NULL;
 		daddr = md ? dst : tunnel->parms.iph.daddr;
 
 		if (rt6 && mtu < dst_mtu(skb_dst(skb)) &&
-- 
1.8.3.1


^ permalink raw reply related

* Re: stmmac / meson8b-dwmac
From: Jose Abreu @ 2019-02-18 12:41 UTC (permalink / raw)
  To: Simon Huelck, Jose Abreu, Martin Blumenstingl
  Cc: Emiliano Ingrassia, Gpeppe.cavallaro, alexandre.torgue,
	linux-amlogic, netdev
In-Reply-To: <7afdaa0d-d0ef-b223-1f6e-2b52bfa44784@gmx.de>

On 2/18/2019 12:33 PM, Simon Huelck wrote:
> Hi,
> 
> 
> i recognize the followin on my ethernet stats:
> 
>      threshold: 1
>      tx_pkt_n: 1457325
>      rx_pkt_n: 5022405
>      normal_irq_n: 671738
>      rx_normal_irq_n: 606573
>      napi_poll: 784439
>      tx_normal_irq_n: 61061
>      tx_clean: 784439
>      tx_set_ic_bit: 58293
>      irq_receive_pmt_irq_n: 0
>      mmc_tx_irq_n: 0
>      mmc_rx_irq_n: 0
>      mmc_rx_csum_offload_irq_n: 0
> ->      irq_tx_path_in_lpi_mode_n: 382037
> ->      irq_tx_path_exit_lpi_mode_n: 382021
> 
> 
> Is this normal ?

Did you try disabling EEE ? ("ethtool --set-eee eth0 eee off")

Thanks,
Jose Miguel Abreu

^ permalink raw reply

* Re: [PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding
From: Russell King - ARM Linux admin @ 2019-02-18 12:53 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Vivien Didelot
  Cc: Heiner Kallweit, David S. Miller, netdev
In-Reply-To: <E1gvPMu-0000Ax-Ba@rmk-PC.armlinux.org.uk>

On Sun, Feb 17, 2019 at 04:32:40PM +0000, Russell King wrote:
> Switches work by learning the MAC address for each attached station by
> monitoring traffic from each station.  When a station sends a packet,
> the switch records which port the MAC address is connected to.
> 
> With IPv4 networking, before communication commences with a neighbour,
> an ARP packet is broadcasted to all stations asking for the MAC address
> corresponding with the IPv4.  The desired station responds with an ARP
> reply, and the ARP reply causes the switch to learn which port the
> station is connected to.
> 
> With IPv6 networking, the situation is rather different.  Rather than
> broadcasting ARP packets, a "neighbour solicitation" is multicasted
> rather than broadcasted.  This multicast needs to reach the intended
> station in order for the neighbour to be discovered.
> 
> Once a neighbour has been discovered, and entered into the sending
> stations neighbour cache, communication can restart at a point later
> without sending a new neighbour solicitation, even if the entry in
> the neighbour cache is marked as stale.  This can be after the MAC
> address has expired from the forwarding cache of the DSA switch -
> when that occurs, there is a long pause in communication.
> 
> Our DSA implementation for mv88e6xxx switches has defaulted to having
> multicast and unicast flooding disabled.  As per the above description,
> this is fine for IPv4 networking, since the broadcasted ARP queries
> will be sent to and received by all stations on the same network.
> However, this breaks IPv6 very badly - blocking neighbour solicitations
> and later causing connections to stall.
> 
> The defaults that the Linux bridge code expect from bridges are that
> unknown unicast frames and unknown multicast frames are flooded to
> all stations, which is at odds to the defaults adopted by our DSA
> implementation for mv88e6xxx switches.
> 
> This commit enables by default flooding of both unknown unicast and
> unknown multicast frames.  This means that mv88e6xxx DSA switches now
> behave as per the bridge(8) man page, and IPv6 works flawlessly through
> such a switch.

Thinking about this a bit more, this approach probably isn't the best.
If we have a port that goes through this life-cycle:

1. assigned to a bridge
2. configured not to flood
3. reassigned to a new bridge

the port will retain its settings from the first bridge, which will be
at odds with the settings that the Linux bridge code expects and the
settings visible to the user.

So, how about this, which basically reverts this patch and applies the
flood settings each time a port joins a bridge, and clears them when
the port leaves a bridge.

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5ac2c93b7cee..6f68c35d416f 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1962,6 +1962,13 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
 
 	mutex_lock(&chip->reg_lock);
 	err = mv88e6xxx_bridge_map(chip, br);
+	/* Linux bridges are expected to flood unknown multicast and
+	 * unicast frames to all ports - as per the defaults specified
+	 * in the iproute2 bridge(8) man page. Not doing this causes
+	 * stalls and failures with IPv6 over Marvell bridges. */
+	if (err == 0 && chip->info->ops->port_set_egress_floods)
+		err = chip->info->ops->port_set_egress_floods(chip, port,
+							      true, true);
 	mutex_unlock(&chip->reg_lock);
 
 	return err;
@@ -1976,6 +1983,9 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port,
 	if (mv88e6xxx_bridge_map(chip, br) ||
 	    mv88e6xxx_port_vlan_map(chip, port))
 		dev_err(ds->dev, "failed to remap in-chip Port VLAN\n");
+	if (chip->info->ops->port_set_egress_floods &&
+	    chip->info->ops->port_set_egress_floods(chip, port, false, false))
+		dev_err(ds->dev, "failed to disable port floods\n");
 	mutex_unlock(&chip->reg_lock);
 }
 
@@ -2134,13 +2144,14 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
 
 static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 {
-	/* Linux bridges are expected to flood unknown multicast and
-	 * unicast frames to all ports - as per the defaults specified
-	 * in the iproute2 bridge(8) man page. Not doing this causes
-	 * stalls and failures with IPv6 over Marvell bridges. */
+	struct dsa_switch *ds = chip->ds;
+	bool flood;
+
+	/* Upstream ports flood frames with unknown unicast or multicast DA */
+	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
 	if (chip->info->ops->port_set_egress_floods)
 		return chip->info->ops->port_set_egress_floods(chip, port,
-							       true, true);
+							       flood, flood);
 
 	return 0;
 }

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply related

* Re: [PATCH][next] Bluetooth: hci_event: Use struct_size() helper
From: Marcel Holtmann @ 2019-02-18 13:00 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Johan Hedberg, David S. Miller, linux-bluetooth, netdev,
	linux-kernel
In-Reply-To: <20190208004033.GA15609@embeddedor>

Hi Gustavo,

> Make use of the struct_size() helper instead of an open-coded version
> in order to avoid any potential type mistakes, in particular in the
> context in which this code is being used.
> 
> So, change the following form:
> 
> sizeof(*ev) + ev->num_hndl * sizeof(struct hci_comp_pkts_info)
> 
> to :
> 
> struct_size(ev, handles, ev->num_hndl)
> 
> This code was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> net/bluetooth/hci_event.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply

* Re: [PATCH][next] Bluetooth: a2mp: Use struct_size() helper
From: Marcel Holtmann @ 2019-02-18 13:02 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Johan Hedberg, David S. Miller, linux-bluetooth, netdev,
	linux-kernel
In-Reply-To: <20190208002817.GA15338@embeddedor>

Hi Gustavo,

> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
> 
> struct foo {
>    int stuff;
>    struct boo entry[];
> };
> 
> size = sizeof(struct foo) + count * sizeof(struct boo);
> instance = alloc(size, GFP_KERNEL)
> 
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
> 
> size = struct_size(instance, entry, count);
> instance = alloc(size, GFP_KERNEL)
> 
> This code was detected with the help of Coccinelle.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> net/bluetooth/a2mp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply

* Re: stmmac / meson8b-dwmac
From: Jose Abreu @ 2019-02-18 13:02 UTC (permalink / raw)
  To: Simon Huelck, Jose Abreu, Martin Blumenstingl
  Cc: Emiliano Ingrassia, Gpeppe.cavallaro, alexandre.torgue,
	linux-amlogic, netdev
In-Reply-To: <6f25da2b-7f28-b888-3240-7178ac75c90c@synopsys.com>

On 2/18/2019 12:41 PM, Jose Abreu wrote:
> On 2/18/2019 12:33 PM, Simon Huelck wrote:
>> Hi,
>>
>>
>> i recognize the followin on my ethernet stats:
>>
>>      threshold: 1
>>      tx_pkt_n: 1457325
>>      rx_pkt_n: 5022405
>>      normal_irq_n: 671738
>>      rx_normal_irq_n: 606573
>>      napi_poll: 784439
>>      tx_normal_irq_n: 61061
>>      tx_clean: 784439
>>      tx_set_ic_bit: 58293
>>      irq_receive_pmt_irq_n: 0
>>      mmc_tx_irq_n: 0
>>      mmc_rx_irq_n: 0
>>      mmc_rx_csum_offload_irq_n: 0
>> ->      irq_tx_path_in_lpi_mode_n: 382037
>> ->      irq_tx_path_exit_lpi_mode_n: 382021
>>
>>
>> Is this normal ?
> 
> Did you try disabling EEE ? ("ethtool --set-eee eth0 eee off")

BTW, do not try to re-enable it. There is a race in the enable
function. I will send a fix ASAP.

> 
> Thanks,
> Jose Miguel Abreu
> 

^ permalink raw reply

* Re: [RFC] net: dsa: qca8k: implement rgmii-id mode
From: Vinod Koul @ 2019-02-18 13:03 UTC (permalink / raw)
  To: Michal Vokáč
  Cc: Andrew Lunn, David S. Miller, netdev,
	linux-kernel@vger.kernel.org, Florian Fainelli
In-Reply-To: <40d0e8b0-b16d-85ef-e647-944bc7dbd571@ysoft.com>

On 18-02-19, 12:54, Michal Vokáč wrote:
> On 18. 02. 19 11:45, Vinod Koul wrote:
> > On 15-02-19, 16:23, Andrew Lunn wrote:
> > > On Fri, Feb 15, 2019 at 04:01:08PM +0100, Michal Vokáč wrote:
> > > > Hi,
> > > > 
> > > > networking on my boards [1], which are currently in linux-next, suddently
> > > > stopped working. I tracked it down to this commit 5ecdd77c61c8 ("net: dsa:
> > > > qca8k: disable delay for RGMII mode") [2].
> > > > 
> > > > So I think the rgmii-id mode is obviously needed in my case.
> > > > I was able to find a couple drivers that read tx/rx-delay or
> > > > tx/rx-internal-delay from device tree. Namely:
> > > > 
> > > >    drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> > > >    drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> > > >    drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> > > >    drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> > > >    drivers/net/phy/dp83867.c
> > > > 
> > > > I would appreciate any hints how to add similar function to qca8k driver
> > > > if that is the correct way to go. Can I take some of the above mentioned
> > > > drivers as a good example for that? How should the binding look like?
> > > > 
> > > > I would expect something like this:
> > > > 
> > > > 	switch@0 {
> > > > 		compatible = "qca,qca8334";
> > > > 		reg = <0>;
> > > > 
> > > > 		switch_ports: ports {
> > > > 			#address-cells = <1>;
> > > > 			#size-cells = <0>;
> > > > 
> > > > 			ethphy0: port@0 {
> > > > 				reg = <0>;
> > > > 				label = "cpu";
> > > > 				phy-mode = "rgmii-id";
> > > > 				qca,tx-delay = <3>;
> > > > 				qca,rx-delay = <3>;
> > > > 				ethernet = <&fec>;
> > > > 		};
> > > 
> > > Hi Michal
> > > 
> > > Your submission used:
> > > 
> > > +				ethphy0: port@0 {
> > > +					reg = <0>;
> > > +					label = "cpu";
> > > +					phy-mode = "rgmii";
> > > +					ethernet = <&fec>;
> > > +
> > > +					fixed-link {
> > > +						speed = <1000>;
> > > +						full-duplex;
> > > +					};
> > > +				};
> > > 
> > > This is good. If you have a fixed-link you can pass a phy-mode.
> 
> Yes, I am using fixed-link and the plan was to implement the rgmii-id
> mode.
> 
> > > 
> > > The comment that was removed was:
> > > 
> > > -               /* According to the datasheet, RGMII delay is enabled through
> > > -                * PORT5_PAD_CTRL for all ports, rather than individual port
> > > -                * registers
> > > -                */
> > > 
> > > Is it possible to enable delays per port? Ideally, you want to enable
> > > delays for just selected ports. Add another case for
> > > PHY_INTERFACE_MODE_RGMII_ID to enable the delays.
> 
> I am still trying to collect all the relevant notes and bits from the
> horrible docs to understand how this is done. The problem is different
> parts and different versions of the documentation provide diffrerent
> details.
> 
> For example the QCA8334 model does not have PORT5 and so that registers
> are not documented. Though some application note document says:
> 
>   """
>   The MAC0 timing control is in the Port0 PAD Mode Control Register (offset 0x0004):
> 
>   1. RGMII timing delay for the output path of QCA8337(N) is enabled by 0x8[24].
>      Set 1 to add 2ns delay in 1000 mode for all RGMII interfaces.
> 
>   2. Bit [21:20]: select the delay time for the output path in 10/100 mode.
> 
>   3. Bit 25: enable the timing delay for the input path of QCA8337(N) in 1000 mode.
> 
>   4. Bit [23:22]: select the delay time for the input path in 1000 mode.
>     00: 0.2ns
>     01: 1.2ns
>     10: 2.1ns
>     11: 3.1ns
>   """
> 
> That is in line with the removed comment. The bit 24 at address 0x8 is
> used to enable/disable delays globally. And obviously it is needed on the
> QCA8334 model as well even though it should not have the PORT5.
> 
> > In the hindsight I should not have removed the comment, let me ressurect
> > that as well as add handling of the RGMII modes...
> 
> OK, thanks.

Since it used to work for you before, you need older code in RGMII_ID
mode, I have added that along with the comment

> 
> > Please do test
> 
> Sure, will do. Just let me know when you have something ready.
> Thank you!

Sending it shortly and ccing you as well

Thanks
-- 
~Vinod

^ permalink raw reply

* [PATCH] net: dsa: qca8k: Enable delay for RGMII_ID mode
From: Vinod Koul @ 2019-02-18 13:03 UTC (permalink / raw)
  To: David S Miller
  Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, netdev, Niklas Cassel,
	Andrew Lunn, Florian Fainelli, Michal Vokáč

RGMII_ID specifies that we should have internal delay, so resurrect the
delay addition routine but under the RGMII_ID mode.

Fixes: 40269aa9f40a ("net: dsa: qca8k: disable delay for RGMII mode")
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/net/dsa/qca8k.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index a4b6cda38016..aa1f7f1b20d3 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -443,6 +443,18 @@ qca8k_set_pad_ctrl(struct qca8k_priv *priv, int port, int mode)
 		val = QCA8K_PORT_PAD_RGMII_EN;
 		qca8k_write(priv, reg, val);
 		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		/* RGMII_ID needs internal delay. This is enabled through
+		 * PORT5_PAD_CTRL for all ports, rather than individual port
+		 * registers
+		 */
+		qca8k_write(priv, reg,
+			    QCA8K_PORT_PAD_RGMII_EN |
+			    QCA8K_PORT_PAD_RGMII_TX_DELAY(3) |
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY(3));
+		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
+			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
+		break;
 	case PHY_INTERFACE_MODE_SGMII:
 		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
 		break;
-- 
2.20.1


^ permalink raw reply related

* [ANNOUNCE] Linux IPsec workshop 2019
From: Steffen Klassert @ 2019-02-18 13:15 UTC (permalink / raw)
  To: netdev; +Cc: lwn

This is an announce of the Linux IPsec workshop 2019. The workshop
will take place in Prague, Czech Republic, from 18th to 20th March 2019.

The workshop is invitation based and limited to ca. 20 - 25 IPsec
developers from user and kernel space. We almost reached the limit,
but still have a few spare places. If you think you can contribute
with a discussion topic or presentation, please send me a mail with
your proposal.

The main focus of this workshop is IPsec, but also other network
security/crypto related related topics are welcome. This workshop
should bring together userspace and kernel IPsec developers, as well as
hardware vendors that develop hardware with network security offload
capabilities. The aim is to discuss the ongoing IPsec related development.
It should be also a platform to discuss new ideas and to get feedback
from different points of view.


^ permalink raw reply

* [PATCH net 0/2] qed: iWARP - fix some syn related issues.
From: Michal Kalderon @ 2019-02-18 13:24 UTC (permalink / raw)
  To: michal.kalderon, ariel.elior, davem; +Cc: netdev, linux-rdma

This series fixes two bugs related to iWARP syn processing flow. 

Michal Kalderon (2):
  qed: Fix iWARP buffer size provided for syn packet processing.
  qed: Fix iWARP syn packet mac address validation.

 drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 21 +++++++++++++++------
 drivers/net/ethernet/qlogic/qed/qed_iwarp.h |  1 -
 2 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.9.5


^ permalink raw reply

* [PATCH net 1/2] qed: Fix iWARP buffer size provided for syn packet processing.
From: Michal Kalderon @ 2019-02-18 13:24 UTC (permalink / raw)
  To: michal.kalderon, ariel.elior, davem; +Cc: netdev, linux-rdma
In-Reply-To: <20190218132403.12487-1-michal.kalderon@marvell.com>

The assumption that the maximum size of a syn packet is 128 bytes
is wrong. Tunneling headers were not accounted for.
Allocate buffers large enough for mtu.

Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
---
 drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 12 ++++++------
 drivers/net/ethernet/qlogic/qed/qed_iwarp.h |  1 -
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
index beb8e5d..e84fb01 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
@@ -2605,7 +2605,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
 	struct qed_iwarp_info *iwarp_info;
 	struct qed_ll2_acquire_data data;
 	struct qed_ll2_cbs cbs;
-	u32 mpa_buff_size;
+	u32 buff_size;
 	u16 n_ooo_bufs;
 	int rc = 0;
 	int i;
@@ -2632,7 +2632,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
 
 	memset(&data, 0, sizeof(data));
 	data.input.conn_type = QED_LL2_TYPE_IWARP;
-	data.input.mtu = QED_IWARP_MAX_SYN_PKT_SIZE;
+	data.input.mtu = params->max_mtu;
 	data.input.rx_num_desc = QED_IWARP_LL2_SYN_RX_SIZE;
 	data.input.tx_num_desc = QED_IWARP_LL2_SYN_TX_SIZE;
 	data.input.tx_max_bds_per_packet = 1;	/* will never be fragmented */
@@ -2654,9 +2654,10 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
 		goto err;
 	}
 
+	buff_size = QED_IWARP_MAX_BUF_SIZE(params->max_mtu);
 	rc = qed_iwarp_ll2_alloc_buffers(p_hwfn,
 					 QED_IWARP_LL2_SYN_RX_SIZE,
-					 QED_IWARP_MAX_SYN_PKT_SIZE,
+					 buff_size,
 					 iwarp_info->ll2_syn_handle);
 	if (rc)
 		goto err;
@@ -2710,10 +2711,9 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
 	if (rc)
 		goto err;
 
-	mpa_buff_size = QED_IWARP_MAX_BUF_SIZE(params->max_mtu);
 	rc = qed_iwarp_ll2_alloc_buffers(p_hwfn,
 					 data.input.rx_num_desc,
-					 mpa_buff_size,
+					 buff_size,
 					 iwarp_info->ll2_mpa_handle);
 	if (rc)
 		goto err;
@@ -2726,7 +2726,7 @@ qed_iwarp_ll2_start(struct qed_hwfn *p_hwfn,
 
 	iwarp_info->max_num_partial_fpdus = (u16)p_hwfn->p_rdma_info->num_qps;
 
-	iwarp_info->mpa_intermediate_buf = kzalloc(mpa_buff_size, GFP_KERNEL);
+	iwarp_info->mpa_intermediate_buf = kzalloc(buff_size, GFP_KERNEL);
 	if (!iwarp_info->mpa_intermediate_buf)
 		goto err;
 
diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.h b/drivers/net/ethernet/qlogic/qed/qed_iwarp.h
index b8f612d..7ac95903 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.h
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.h
@@ -46,7 +46,6 @@ enum qed_iwarp_qp_state qed_roce2iwarp_state(enum qed_roce_qp_state state);
 
 #define QED_IWARP_LL2_SYN_TX_SIZE       (128)
 #define QED_IWARP_LL2_SYN_RX_SIZE       (256)
-#define QED_IWARP_MAX_SYN_PKT_SIZE      (128)
 
 #define QED_IWARP_LL2_OOO_DEF_TX_SIZE   (256)
 #define QED_IWARP_MAX_OOO		(16)
-- 
2.9.5


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox