Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net] Documentation/networking: fix default_ttl typo in mpls-sysctl
From: David Ahern @ 2019-07-01 16:21 UTC (permalink / raw)
  To: Hangbin Liu, netdev; +Cc: David Miller
In-Reply-To: <20190701084528.25872-1-liuhangbin@gmail.com>

On 7/1/19 2:45 AM, Hangbin Liu wrote:
> default_ttl should be integer instead of bool
> 
> Reported-by: Ying Xu <yinxu@redhat.com>
> Fixes: a59166e47086 ("mpls: allow TTL propagation from IP packets to be configured")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  Documentation/networking/mpls-sysctl.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Reviewed-by: David Ahern <dsahern@gmail.com>


^ permalink raw reply

* Re: [PATCH v5 net-next 6/6] net: ethernet: ti: cpsw: add XDP support
From: Jesper Dangaard Brouer @ 2019-07-01 16:19 UTC (permalink / raw)
  To: Ivan Khoronzhuk
  Cc: grygorii.strashko, davem, ast, linux-kernel, linux-omap,
	ilias.apalodimas, netdev, daniel, jakub.kicinski, john.fastabend,
	brouer
In-Reply-To: <20190630172348.5692-7-ivan.khoronzhuk@linaro.org>

On Sun, 30 Jun 2019 20:23:48 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:

> +static int cpsw_ndev_create_xdp_rxq(struct cpsw_priv *priv, int ch)
> +{
> +	struct cpsw_common *cpsw = priv->cpsw;
> +	int ret, new_pool = false;
> +	struct xdp_rxq_info *rxq;
> +
> +	rxq = &priv->xdp_rxq[ch];
> +
> +	ret = xdp_rxq_info_reg(rxq, priv->ndev, ch);
> +	if (ret)
> +		return ret;
> +
> +	if (!cpsw->page_pool[ch]) {
> +		ret =  cpsw_create_rx_pool(cpsw, ch);
> +		if (ret)
> +			goto err_rxq;
> +
> +		new_pool = true;
> +	}
> +
> +	ret = xdp_rxq_info_reg_mem_model(rxq, MEM_TYPE_PAGE_POOL,
> +					 cpsw->page_pool[ch]);
> +	if (!ret)
> +		return 0;
> +
> +	if (new_pool) {
> +		page_pool_free(cpsw->page_pool[ch]);
> +		cpsw->page_pool[ch] = NULL;
> +	}
> +
> +err_rxq:
> +	xdp_rxq_info_unreg(rxq);
> +	return ret;
> +}

Looking at this, and Ilias'es XDP-netsec error handling path, it might
be a mistake that I removed page_pool_destroy() and instead put the
responsibility on xdp_rxq_info_unreg().

As here, we have to detect if page_pool_create() was a success, and then
if xdp_rxq_info_reg_mem_model() was a failure, explicitly call
page_pool_free() because the xdp_rxq_info_unreg() call cannot "free"
the page_pool object given it was not registered.  

Ivan's patch in[1], might be a better approach, which forced all
drivers to explicitly call page_pool_free(), even-though it just
dec-refcnt and the real call to page_pool_free() happened via
xdp_rxq_info_unreg().

To better handle error path, I would re-introduce page_pool_destroy(),
as a driver API, that would gracefully handle NULL-pointer case, and
then call page_pool_free() with the atomic_dec_and_test().  (It should
hopefully simplify the error handling code a bit)

[1] https://lore.kernel.org/netdev/20190625175948.24771-2-ivan.khoronzhuk@linaro.org/


> +void cpsw_ndev_destroy_xdp_rxqs(struct cpsw_priv *priv)
> +{
> +	struct cpsw_common *cpsw = priv->cpsw;
> +	struct xdp_rxq_info *rxq;
> +	int i;
> +
> +	for (i = 0; i < cpsw->rx_ch_num; i++) {
> +		rxq = &priv->xdp_rxq[i];
> +		if (xdp_rxq_info_is_reg(rxq))
> +			xdp_rxq_info_unreg(rxq);
> +	}
> +}

Are you sure you need to test xdp_rxq_info_is_reg() here?

You should just call xdp_rxq_info_unreg(rxq), if you know that this rxq
should be registered.  If your assumption failed, you will get a
WARNing, and discover your driver level bug.  This is one of the ways
the API is designed to "detect" misuse of the API.  (I found this
rather useful, when I converted the approx 12 drivers using this
xdp_rxq_info API).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

^ permalink raw reply

* Re: [PATCH net] ipv4: don't set IPv6 only flags to IPv4 addresses
From: Matteo Croce @ 2019-07-01 16:13 UTC (permalink / raw)
  To: Joe Perches
  Cc: netdev, LKML, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI
In-Reply-To: <42624f83da71354a5daef959a4749cb75516d37f.camel@perches.com>

On Mon, Jul 1, 2019 at 6:10 PM Joe Perches <joe@perches.com> wrote:
>
> On Mon, 2019-07-01 at 18:08 +0200, Matteo Croce wrote:
> > Avoid the situation where an IPV6 only flag is applied to an IPv4 address:
> []
> > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> []
> > @@ -468,6 +473,9 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh,
> >       ifa->ifa_flags &= ~IFA_F_SECONDARY;
> >       last_primary = &in_dev->ifa_list;
> >
> > +     /* Don't set IPv6 only flags to IPv6 addresses */
>
> umm, IPv4 addresses?
>
>

Ouch, right.

/* Don't set IPv6 only flags to IPv4 addresses */

Can this be edidet on patchwork instead of spamming with a v2?

-- 
Matteo Croce
per aspera ad upstream

^ permalink raw reply

* Re: [PATCH 3/4] net: dsa: vsc73xx: add support for parallel mode
From: Andrew Lunn @ 2019-07-01 16:11 UTC (permalink / raw)
  To: Pawel Dembicki
  Cc: linus.walleij, Vivien Didelot, Florian Fainelli, David S. Miller,
	Rob Herring, Mark Rutland, netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-3-paweldembicki@gmail.com>

On Mon, Jul 01, 2019 at 05:27:22PM +0200, Pawel Dembicki wrote:
> +static int vsc73xx_platform_read(struct vsc73xx *vsc, u8 block, u8 subblock,
> +				 u8 reg, u32 *val)
> +{
> +	struct vsc73xx_platform *vsc_platform = vsc->priv;
> +	u32 offset;
> +
> +	if (!vsc73xx_is_addr_valid(block, subblock))
> +		return -EINVAL;
> +
> +	offset = vsc73xx_make_addr(block, subblock, reg);
> +
> +	mutex_lock(&vsc->lock);
> +		*val = ioread32be(vsc_platform->base_addr + offset);
> +	mutex_unlock(&vsc->lock);

Hi Pawel

What is this mutex protecting?

Plus the indentation is wrong.

Thanks
	Andrew

^ permalink raw reply

* Re: [PATCH v4 bpf-next 0/9] libbpf: add bpf_link and tracing attach APIs
From: Stanislav Fomichev @ 2019-07-01 16:10 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: andrii.nakryiko, bpf, netdev, ast, daniel, kernel-team,
	songliubraving
In-Reply-To: <20190629034906.1209916-1-andriin@fb.com>

On 06/28, Andrii Nakryiko wrote:
> This patchset adds the following APIs to allow attaching BPF programs to
> tracing entities:
> - bpf_program__attach_perf_event for attaching to any opened perf event FD,
>   allowing users full control;
> - bpf_program__attach_kprobe for attaching to kernel probes (both entry and
>   return probes);
> - bpf_program__attach_uprobe for attaching to user probes (both entry/return);
> - bpf_program__attach_tracepoint for attaching to kernel tracepoints;
> - bpf_program__attach_raw_tracepoint for attaching to raw kernel tracepoint
>   (wrapper around bpf_raw_tracepoint_open);
> 
> This set of APIs makes libbpf more useful for tracing applications.
> 
> All attach APIs return abstract struct bpf_link that encapsulates logic of
> detaching BPF program. See patch #2 for details. bpf_assoc was considered as
> an alternative name for this opaque "handle", but bpf_link seems to be
> appropriate semantically and is nice and short.
> 
> Pre-patch #1 makes internal libbpf_strerror_r helper function work w/ negative
> error codes, lifting the burder off callers to keep track of error sign.
> Patch #2 adds bpf_link abstraction.
> Patch #3 adds attach_perf_event, which is the base for all other APIs.
> Patch #4 adds kprobe/uprobe APIs.
> Patch #5 adds tracepoint API.
> Patch #6 adds raw_tracepoint API.
> Patch #7 converts one existing test to use attach_perf_event.
> Patch #8 adds new kprobe/uprobe tests.
> Patch #9 converts some selftests currently using tracepoint to new APIs.
> 
> v3->v4:
> - proper errno handling (Stanislav);
> - bpf_fd -> prog_fd (Stanislav);
> - switch to fprintf (Song);
Reviewed-by: Stanislav Fomichev <sdf@google.com>

Thanks!

> v2->v3:
> - added bpf_link concept (Daniel);
> - didn't add generic bpf_link__attach_program for reasons described in [0];
> - dropped Stanislav's Reviewed-by from patches #2-#6, in case he doesn't like
>   the change;
> v1->v2:
> - preserve errno before close() call (Stanislav);
> - use libbpf_perf_event_disable_and_close in selftest (Stanislav);
> - remove unnecessary memset (Stanislav);
> 
> [0] https://lore.kernel.org/bpf/CAEf4BzZ7EM5eP2eaZn7T2Yb5QgVRiwAs+epeLR1g01TTx-6m6Q@mail.gmail.com/
> 
> Andrii Nakryiko (9):
>   libbpf: make libbpf_strerror_r agnostic to sign of error
>   libbpf: introduce concept of bpf_link
>   libbpf: add ability to attach/detach BPF program to perf event
>   libbpf: add kprobe/uprobe attach API
>   libbpf: add tracepoint attach API
>   libbpf: add raw tracepoint attach API
>   selftests/bpf: switch test to new attach_perf_event API
>   selftests/bpf: add kprobe/uprobe selftests
>   selftests/bpf: convert existing tracepoint tests to new APIs
> 
>  tools/lib/bpf/libbpf.c                        | 359 ++++++++++++++++++
>  tools/lib/bpf/libbpf.h                        |  21 +
>  tools/lib/bpf/libbpf.map                      |   8 +-
>  tools/lib/bpf/str_error.c                     |   2 +-
>  .../selftests/bpf/prog_tests/attach_probe.c   | 155 ++++++++
>  .../bpf/prog_tests/stacktrace_build_id.c      |  50 +--
>  .../bpf/prog_tests/stacktrace_build_id_nmi.c  |  31 +-
>  .../selftests/bpf/prog_tests/stacktrace_map.c |  43 +--
>  .../bpf/prog_tests/stacktrace_map_raw_tp.c    |  15 +-
>  .../selftests/bpf/progs/test_attach_probe.c   |  55 +++
>  10 files changed, 644 insertions(+), 95 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/attach_probe.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_attach_probe.c
> 
> -- 
> 2.17.1
> 

^ permalink raw reply

* Re: [PATCH net] ipv4: don't set IPv6 only flags to IPv4 addresses
From: Joe Perches @ 2019-07-01 16:10 UTC (permalink / raw)
  To: Matteo Croce, netdev
  Cc: linux-kernel, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI
In-Reply-To: <20190701160805.32404-1-mcroce@redhat.com>

On Mon, 2019-07-01 at 18:08 +0200, Matteo Croce wrote:
> Avoid the situation where an IPV6 only flag is applied to an IPv4 address:
[]
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
[]
> @@ -468,6 +473,9 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh,
>  	ifa->ifa_flags &= ~IFA_F_SECONDARY;
>  	last_primary = &in_dev->ifa_list;
>  
> +	/* Don't set IPv6 only flags to IPv6 addresses */

umm, IPv4 addresses?



^ permalink raw reply

* [PATCH net] ipv4: don't set IPv6 only flags to IPv4 addresses
From: Matteo Croce @ 2019-07-01 16:08 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI

Avoid the situation where an IPV6 only flag is applied to an IPv4 address:

    # ip addr add 192.0.2.1/24 dev dummy0 nodad home mngtmpaddr noprefixroute
    # ip -4 addr show dev dummy0
    2: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
        inet 192.0.2.1/24 scope global noprefixroute dummy0
           valid_lft forever preferred_lft forever

Or worse, by sending a malicious netlink command:

    # ip -4 addr show dev dummy0
    2: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
        inet 192.0.2.1/24 scope global nodad optimistic dadfailed home tentative mngtmpaddr noprefixroute stable-privacy dummy0
           valid_lft forever preferred_lft forever

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 net/ipv4/devinet.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index c6bd0f7a020a..f40ccdcf4cfe 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -62,6 +62,11 @@
 #include <net/net_namespace.h>
 #include <net/addrconf.h>
 
+#define IPV6ONLY_FLAGS	\
+		(IFA_F_NODAD | IFA_F_OPTIMISTIC | IFA_F_DADFAILED | \
+		 IFA_F_HOMEADDRESS | IFA_F_TENTATIVE | \
+		 IFA_F_MANAGETEMPADDR | IFA_F_STABLE_PRIVACY)
+
 static struct ipv4_devconf ipv4_devconf = {
 	.data = {
 		[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
@@ -468,6 +473,9 @@ static int __inet_insert_ifa(struct in_ifaddr *ifa, struct nlmsghdr *nlh,
 	ifa->ifa_flags &= ~IFA_F_SECONDARY;
 	last_primary = &in_dev->ifa_list;
 
+	/* Don't set IPv6 only flags to IPv6 addresses */
+	ifa->ifa_flags &= ~IPV6ONLY_FLAGS;
+
 	for (ifap = &in_dev->ifa_list; (ifa1 = *ifap) != NULL;
 	     ifap = &ifa1->ifa_next) {
 		if (!(ifa1->ifa_flags & IFA_F_SECONDARY) &&
-- 
2.21.0


^ permalink raw reply related

* Re: [PATCH bpf-next 1/2] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Stanislav Fomichev @ 2019-07-01 16:04 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Yonghong Song, Stanislav Fomichev, netdev@vger.kernel.org,
	bpf@vger.kernel.org, davem@davemloft.net, ast@kernel.org,
	daniel@iogearbox.net, Andrii Nakryiko, kernel test robot
In-Reply-To: <CAEf4BzbT7h2oDapgSwQr8gSMnunCssqu88KMdymMjgBGpZpA4Q@mail.gmail.com>

On 07/01, Andrii Nakryiko wrote:
> On Sat, Jun 29, 2019 at 10:53 PM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> > > Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
> > > that it can do a single u64 store into user_ip6[2] instead of two
> > > separate u32 ones:
> > >
> > >   #  17: (18) r2 = 0x100000000000000
> > >   #  ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
> > >   #  19: (7b) *(u64 *)(r1 +16) = r2
> > >   #  invalid bpf_context access off=16 size=8
> > >
> > >  From the compiler point of view it does look like a correct thing
> > > to do, so let's support it on the kernel side.
> > >
> > > Credit to Andrii Nakryiko for a proper implementation of
> > > bpf_ctx_wide_store_ok.
> > >
> > > Cc: Andrii Nakryiko <andriin@fb.com>
> > > Cc: Yonghong Song <yhs@fb.com>
> > > Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
> > > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> >
> > The change looks good to me with the following nits:
> >    1. could you add a cover letter for the patch set?
> >       typically if the number of patches is more than one,
> >       it would be a good practice with a cover letter.
> >       See bpf_devel_QA.rst .
> >    2. with this change, the comments in uapi bpf.h
> >       are not accurate any more.
> >          __u32 user_ip6[4];      /* Allows 1,2,4-byte read an 4-byte write.
> >                                   * Stored in network byte order.
> >
> >                                   */
> >          __u32 msg_src_ip6[4];   /* Allows 1,2,4-byte read an 4-byte write.
> >                                   * Stored in network byte order.
> >                                   */
> >       now for stores, aligned 8-byte write is permitted.
> >       could you update this as well?
> >
> >  From the typical usage pattern, I did not see a need
> > for 8-tye read of user_ip6 and msg_src_ip6 yet. So let
> > us just deal with write for now.
> 
> But I guess it's still possible for clang to optimize two consecutive
> 4-byte reads into single 8-byte read in some circumstances? If that's
> the case, maybe it's a good idea to have corresponding read checks as
> well?
I guess clang can do those kinds of optimizations. I can put it on my
todo and address later (or when we actually see it out in the wild).

> But overall this looks good to me:
> 
> Acked-by: Andrii Nakryiko <andriin@fb.com>
Thanks for a review!

> >
> > With the above two nits,
> > Acked-by: Yonghong Song <yhs@fb.com>
> >
> > > ---
> > >   include/linux/filter.h |  6 ++++++
> > >   net/core/filter.c      | 22 ++++++++++++++--------
> > >   2 files changed, 20 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > > index 340f7d648974..3901007e36f1 100644
> > > --- a/include/linux/filter.h
> > > +++ b/include/linux/filter.h
> > > @@ -746,6 +746,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
> > >       return size <= size_default && (size & (size - 1)) == 0;
> > >   }
> > >
> > > +#define bpf_ctx_wide_store_ok(off, size, type, field)                        \
> > > +     (size == sizeof(__u64) &&                                       \
> > > +     off >= offsetof(type, field) &&                                 \
> > > +     off + sizeof(__u64) <= offsetofend(type, field) &&              \
> > > +     off % sizeof(__u64) == 0)
> > > +
> > >   #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
> > >
> > >   static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index dc8534be12fc..5d33f2146dab 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -6849,6 +6849,16 @@ static bool sock_addr_is_valid_access(int off, int size,
> > >                       if (!bpf_ctx_narrow_access_ok(off, size, size_default))
> > >                               return false;
> > >               } else {
> > > +                     if (bpf_ctx_wide_store_ok(off, size,
> > > +                                               struct bpf_sock_addr,
> > > +                                               user_ip6))
> > > +                             return true;
> > > +
> > > +                     if (bpf_ctx_wide_store_ok(off, size,
> > > +                                               struct bpf_sock_addr,
> > > +                                               msg_src_ip6))
> > > +                             return true;
> > > +
> > >                       if (size != size_default)
> > >                               return false;
> > >               }
> > > @@ -7689,9 +7699,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > >   /* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
> > >    * SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
> > >    *
> > > - * It doesn't support SIZE argument though since narrow stores are not
> > > - * supported for now.
> > > - *
> > >    * In addition it uses Temporary Field TF (member of struct S) as the 3rd
> > >    * "register" since two registers available in convert_ctx_access are not
> > >    * enough: we can't override neither SRC, since it contains value to store, nor
> > > @@ -7699,7 +7706,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > >    * instructions. But we need a temporary place to save pointer to nested
> > >    * structure whose field we want to store to.
> > >    */
> > > -#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF)                     \
> > > +#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF)               \
> > >       do {                                                                   \
> > >               int tmp_reg = BPF_REG_9;                                       \
> > >               if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg)          \
> > > @@ -7710,8 +7717,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > >                                     offsetof(S, TF));                        \
> > >               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg,         \
> > >                                     si->dst_reg, offsetof(S, F));            \
> > > -             *insn++ = BPF_STX_MEM(                                         \
> > > -                     BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg,        \
> > > +             *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg,              \
> > >                       bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF),           \
> > >                                      target_size)                            \
> > >                               + OFF);                                        \
> > > @@ -7723,8 +7729,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > >                                                     TF)                      \
> > >       do {                                                                   \
> > >               if (type == BPF_WRITE) {                                       \
> > > -                     SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF,    \
> > > -                                                      TF);                  \
> > > +                     SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE,   \
> > > +                                                      OFF, TF);             \
> > >               } else {                                                       \
> > >                       SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(                  \
> > >                               S, NS, F, NF, SIZE, OFF);  \
> > >

^ permalink raw reply

* Re: [PATCH 2/4] net: dsa: vsc73xx: Split vsc73xx driver
From: Andrew Lunn @ 2019-07-01 16:03 UTC (permalink / raw)
  To: Pawel Dembicki
  Cc: linus.walleij, Vivien Didelot, Florian Fainelli, David S. Miller,
	Rob Herring, Mark Rutland, netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-2-paweldembicki@gmail.com>

> @@ -495,12 +380,12 @@ static int vsc73xx_update_bits(struct vsc73xx *vsc, u8 block, u8 subblock,
>  	int ret;
>  
>  	/* Same read-modify-write algorithm as e.g. regmap */
> -	ret = vsc73xx_read(vsc, block, subblock, reg, &orig);
> +	ret = vsc->ops->read(vsc, block, subblock, reg, &orig);
>  	if (ret)
>  		return ret;
>  	tmp = orig & ~mask;
>  	tmp |= val & mask;
> -	return vsc73xx_write(vsc, block, subblock, reg, tmp);
> +	return vsc->ops->write(vsc, block, subblock, reg, tmp);

This patch would be a lot less invasive and smaller if you hid the
difference between SPI and platform inside vsc73xx_write() and
vsc73xx_read().

> -static int vsc73xx_probe(struct spi_device *spi)
> +int vsc73xx_probe(struct vsc73xx *vsc)
>  {
> -	struct device *dev = &spi->dev;

  struct device *dev = vsc->dev;

and then a lot of the changes you make here go away.

In general, think about how to make the changes small. It saves your
time from actually making changes, and reviewer time since the patch
it smaller.

    Andrew

^ permalink raw reply

* Re: [PATCH bpf-next 1/2] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Stanislav Fomichev @ 2019-07-01 16:01 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Stanislav Fomichev, netdev@vger.kernel.org, bpf@vger.kernel.org,
	davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
	Andrii Nakryiko, kernel test robot
In-Reply-To: <be223396-b181-e587-d63c-2b15eaca3721@fb.com>

On 06/30, Yonghong Song wrote:
> 
> 
> On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> > Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
> > that it can do a single u64 store into user_ip6[2] instead of two
> > separate u32 ones:
> > 
> >   #  17: (18) r2 = 0x100000000000000
> >   #  ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
> >   #  19: (7b) *(u64 *)(r1 +16) = r2
> >   #  invalid bpf_context access off=16 size=8
> > 
> >  From the compiler point of view it does look like a correct thing
> > to do, so let's support it on the kernel side.
> > 
> > Credit to Andrii Nakryiko for a proper implementation of
> > bpf_ctx_wide_store_ok.
> > 
> > Cc: Andrii Nakryiko <andriin@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
> > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> 
> The change looks good to me with the following nits:
>    1. could you add a cover letter for the patch set?
>       typically if the number of patches is more than one,
>       it would be a good practice with a cover letter.
>       See bpf_devel_QA.rst .
>    2. with this change, the comments in uapi bpf.h
>       are not accurate any more.
>          __u32 user_ip6[4];      /* Allows 1,2,4-byte read an 4-byte write.
>                                   * Stored in network byte order. 
> 
>                                   */
>          __u32 msg_src_ip6[4];   /* Allows 1,2,4-byte read an 4-byte write.
>                                   * Stored in network byte order.
>                                   */
>       now for stores, aligned 8-byte write is permitted.
>       could you update this as well?
> 
>  From the typical usage pattern, I did not see a need
> for 8-tye read of user_ip6 and msg_src_ip6 yet. So let
> us just deal with write for now.
> 
> With the above two nits,
> Acked-by: Yonghong Song <yhs@fb.com>
Thank you for a review, will follow up with a v2 shortly with both
things addressed!

> > ---
> >   include/linux/filter.h |  6 ++++++
> >   net/core/filter.c      | 22 ++++++++++++++--------
> >   2 files changed, 20 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index 340f7d648974..3901007e36f1 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -746,6 +746,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
> >   	return size <= size_default && (size & (size - 1)) == 0;
> >   }
> >   
> > +#define bpf_ctx_wide_store_ok(off, size, type, field)			\
> > +	(size == sizeof(__u64) &&					\
> > +	off >= offsetof(type, field) &&					\
> > +	off + sizeof(__u64) <= offsetofend(type, field) &&		\
> > +	off % sizeof(__u64) == 0)
> > +
> >   #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
> >   
> >   static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index dc8534be12fc..5d33f2146dab 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -6849,6 +6849,16 @@ static bool sock_addr_is_valid_access(int off, int size,
> >   			if (!bpf_ctx_narrow_access_ok(off, size, size_default))
> >   				return false;
> >   		} else {
> > +			if (bpf_ctx_wide_store_ok(off, size,
> > +						  struct bpf_sock_addr,
> > +						  user_ip6))
> > +				return true;
> > +
> > +			if (bpf_ctx_wide_store_ok(off, size,
> > +						  struct bpf_sock_addr,
> > +						  msg_src_ip6))
> > +				return true;
> > +
> >   			if (size != size_default)
> >   				return false;
> >   		}
> > @@ -7689,9 +7699,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >   /* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
> >    * SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
> >    *
> > - * It doesn't support SIZE argument though since narrow stores are not
> > - * supported for now.
> > - *
> >    * In addition it uses Temporary Field TF (member of struct S) as the 3rd
> >    * "register" since two registers available in convert_ctx_access are not
> >    * enough: we can't override neither SRC, since it contains value to store, nor
> > @@ -7699,7 +7706,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >    * instructions. But we need a temporary place to save pointer to nested
> >    * structure whose field we want to store to.
> >    */
> > -#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF)		       \
> > +#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF)	       \
> >   	do {								       \
> >   		int tmp_reg = BPF_REG_9;				       \
> >   		if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg)	       \
> > @@ -7710,8 +7717,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >   				      offsetof(S, TF));			       \
> >   		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg,	       \
> >   				      si->dst_reg, offsetof(S, F));	       \
> > -		*insn++ = BPF_STX_MEM(					       \
> > -			BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg,	       \
> > +		*insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg,	       \
> >   			bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF),	       \
> >   				       target_size)			       \
> >   				+ OFF);					       \
> > @@ -7723,8 +7729,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >   						      TF)		       \
> >   	do {								       \
> >   		if (type == BPF_WRITE) {				       \
> > -			SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF,    \
> > -							 TF);		       \
> > +			SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE,   \
> > +							 OFF, TF);	       \
> >   		} else {						       \
> >   			SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(		       \
> >   				S, NS, F, NF, SIZE, OFF);  \
> > 

^ permalink raw reply

* Re: [PATCH bpf-next 2/2] selftests/bpf: add verifier tests for wide stores
From: Stanislav Fomichev @ 2019-07-01 16:00 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Stanislav Fomichev, netdev@vger.kernel.org, bpf@vger.kernel.org,
	davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
	Andrii Nakryiko
In-Reply-To: <8e469767-a108-ba42-f8c8-6fd505393699@fb.com>

On 06/30, Yonghong Song wrote:
> 
> 
> On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> > Make sure that wide stores are allowed at proper (aligned) addresses.
> > Note that user_ip6 is naturally aligned on 8-byte boundary, so
> > correct addresses are user_ip6[0] and user_ip6[2]. msg_src_ip6 is,
> > however, aligned on a 4-byte bondary, so only msg_src_ip6[1]
> > can be wide-stored.
> > 
> > Cc: Andrii Nakryiko <andriin@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >   tools/testing/selftests/bpf/test_verifier.c   | 17 ++++++--
> >   .../selftests/bpf/verifier/wide_store.c       | 40 +++++++++++++++++++
> >   2 files changed, 54 insertions(+), 3 deletions(-)
> >   create mode 100644 tools/testing/selftests/bpf/verifier/wide_store.c
> > 
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index c5514daf8865..b0773291012a 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -105,6 +105,7 @@ struct bpf_test {
> >   			__u64 data64[TEST_DATA_LEN / 8];
> >   		};
> >   	} retvals[MAX_TEST_RUNS];
> > +	enum bpf_attach_type expected_attach_type;
> >   };
> >   
> >   /* Note we want this to be 64 bit aligned so that the end of our array is
> > @@ -850,6 +851,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >   	int fd_prog, expected_ret, alignment_prevented_execution;
> >   	int prog_len, prog_type = test->prog_type;
> >   	struct bpf_insn *prog = test->insns;
> > +	struct bpf_load_program_attr attr;
> >   	int run_errs, run_successes;
> >   	int map_fds[MAX_NR_MAPS];
> >   	const char *expected_err;
> > @@ -881,8 +883,17 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >   		pflags |= BPF_F_STRICT_ALIGNMENT;
> >   	if (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS)
> >   		pflags |= BPF_F_ANY_ALIGNMENT;
> > -	fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
> > -				     "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
> > +
> > +	memset(&attr, 0, sizeof(attr));
> > +	attr.prog_type = prog_type;
> > +	attr.expected_attach_type = test->expected_attach_type;
> > +	attr.insns = prog;
> > +	attr.insns_cnt = prog_len;
> > +	attr.license = "GPL";
> > +	attr.log_level = 4;
> > +	attr.prog_flags = pflags;
> > +
> > +	fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
> >   	if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
> >   		printf("SKIP (unsupported program type %d)\n", prog_type);
> >   		skips++;
> > @@ -912,7 +923,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >   			printf("FAIL\nUnexpected success to load!\n");
> >   			goto fail_log;
> >   		}
> > -		if (!strstr(bpf_vlog, expected_err)) {
> > +		if (!expected_err || !strstr(bpf_vlog, expected_err)) {
> >   			printf("FAIL\nUnexpected error message!\n\tEXP: %s\n\tRES: %s\n",
> >   			      expected_err, bpf_vlog);
> >   			goto fail_log;
> > diff --git a/tools/testing/selftests/bpf/verifier/wide_store.c b/tools/testing/selftests/bpf/verifier/wide_store.c
> > new file mode 100644
> > index 000000000000..c6385f45b114
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/verifier/wide_store.c
> > @@ -0,0 +1,40 @@
> > +#define BPF_SOCK_ADDR(field, off, res, err) \
> > +{ \
> > +	"wide store to bpf_sock_addr." #field "[" #off "]", \
> > +	.insns = { \
> > +	BPF_MOV64_IMM(BPF_REG_0, 1), \
> > +	BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, \
> > +		    offsetof(struct bpf_sock_addr, field[off])), \
> > +	BPF_EXIT_INSN(), \
> > +	}, \
> > +	.result = res, \
> > +	.prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR, \
> > +	.expected_attach_type = BPF_CGROUP_UDP6_SENDMSG, \
> > +	.errstr = err, \
> > +}
> > +
> > +/* user_ip6[0] is u64 aligned */
> > +BPF_SOCK_ADDR(user_ip6, 0, ACCEPT,
> > +	      NULL),
> > +BPF_SOCK_ADDR(user_ip6, 1, REJECT,
> > +	      "invalid bpf_context access off=12 size=8"),
> > +BPF_SOCK_ADDR(user_ip6, 2, ACCEPT,
> > +	      NULL),
> > +BPF_SOCK_ADDR(user_ip6, 3, REJECT,
> > +	      "invalid bpf_context access off=20 size=8"),
> > +BPF_SOCK_ADDR(user_ip6, 4, REJECT,
> > +	      "invalid bpf_context access off=24 size=8"),
> 
> With offset 4, we have
> #968/p wide store to bpf_sock_addr.user_ip6[4] OK
> 
> This test case can be removed. user code typically
> won't write bpf_sock_addr.user_ip6[4], and compiler
> typically will give a warning since it is out of
> array bound. Any particular reason you want to
> include this one?
Agreed on both, I'm being overly cautious here. They should
be caught by the outer switch and be rejected because of
other reasons.

> > +
> > +/* msg_src_ip6[0] is _not_ u64 aligned */
> > +BPF_SOCK_ADDR(msg_src_ip6, 0, REJECT,
> > +	      "invalid bpf_context access off=44 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 1, ACCEPT,
> > +	      NULL),
> > +BPF_SOCK_ADDR(msg_src_ip6, 2, REJECT,
> > +	      "invalid bpf_context access off=52 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 3, REJECT,
> > +	      "invalid bpf_context access off=56 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 4, REJECT,
> > +	      "invalid bpf_context access off=60 size=8"),
> 
> The same as above, offset=4 case can be removed?
> 
> > +
> > +#undef BPF_SOCK_ADDR
> > 

^ permalink raw reply

* Re: [PATCH 1/4] net: dsa: Change DT bindings for Vitesse VSC73xx switches
From: Andrew Lunn @ 2019-07-01 15:55 UTC (permalink / raw)
  To: Pawel Dembicki
  Cc: linus.walleij, Vivien Didelot, Florian Fainelli, David S. Miller,
	Rob Herring, Mark Rutland, netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-1-paweldembicki@gmail.com>

On Mon, Jul 01, 2019 at 05:27:20PM +0200, Pawel Dembicki wrote:
> This commit document changes after split vsc73xx driver into core and
> spi part. The change of DT bindings is required for support the same
> vsc73xx chip, which need PI bus to communicate with CPU. It also

SPI

> introduce how to use vsc73xx platform driver.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
>  .../bindings/net/dsa/vitesse,vsc73xx.txt      | 74 ++++++++++++++++---
>  1 file changed, 64 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.txt b/Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.txt
> index ed4710c40641..c6a4cd85891c 100644
> --- a/Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/vitesse,vsc73xx.txt
> @@ -2,8 +2,8 @@ Vitesse VSC73xx Switches
>  ========================
>  
>  This defines device tree bindings for the Vitesse VSC73xx switch chips.
> -The Vitesse company has been acquired by Microsemi and Microsemi in turn
> -acquired by Microchip but retains this vendor branding.
> +The Vitesse company has been acquired by Microsemi and Microsemi has
> +been acquired Microchip but retains this vendor branding.
>  
>  The currently supported switch chips are:
>  Vitesse VSC7385 SparX-G5 5+1-port Integrated Gigabit Ethernet Switch
> @@ -11,16 +11,26 @@ Vitesse VSC7388 SparX-G8 8-port Integrated Gigabit Ethernet Switch
>  Vitesse VSC7395 SparX-G5e 5+1-port Integrated Gigabit Ethernet Switch
>  Vitesse VSC7398 SparX-G8e 8-port Integrated Gigabit Ethernet Switch
>  
> -The device tree node is an SPI device so it must reside inside a SPI bus
> -device tree node, see spi/spi-bus.txt
> +This switch could have two different management interface.
> +
> +If SPI interface is used, the device tree node is an SPI device so it must
> +reside inside a SPI bus device tree node, see spi/spi-bus.txt
> +
> +If Platform driver is used, the device tree node is an platform device so it
> +must reside inside a platform bus device tree node.
>  
>  Required properties:
>  
> -- compatible: must be exactly one of:
> -	"vitesse,vsc7385"
> -	"vitesse,vsc7388"
> -	"vitesse,vsc7395"
> -	"vitesse,vsc7398"

You cannot remove these. It will break backwards compatibility.
Adding new compatible strings is fine, but you cannot remove existing
ones.

	Andrew


^ permalink raw reply

* Re: [PATCH net-next 8/8] net: mscc: PTP Hardware Clock (PHC) support
From: Eric Dumazet @ 2019-07-01 15:54 UTC (permalink / raw)
  To: Willem de Bruijn, Antoine Tenart
  Cc: David Miller, Richard Cochran, alexandre.belloni, UNGLinuxDriver,
	ralf, paul.burton, jhogan, Network Development, linux-mips,
	thomas.petazzoni, allan.nielsen
In-Reply-To: <CA+FuTSecj3FYGd5xnybgNFH7ndceLu9Orsa9O4RFp0U5bpNy7w@mail.gmail.com>



On 7/1/19 8:12 AM, Willem de Bruijn wrote:
> On Mon, Jul 1, 2019 at 6:05 AM Antoine Tenart
> <antoine.tenart@bootlin.com> wrote:
>>
>> This patch adds support for PTP Hardware Clock (PHC) to the Ocelot
>> switch for both PTP 1-step and 2-step modes.
>>
>> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> 
>>  void ocelot_deinit(struct ocelot *ocelot)
>>  {
>> +       struct ocelot_port *port;
>> +       struct ocelot_skb *entry;
>> +       struct list_head *pos;
>> +       int i;
>> +
>>         destroy_workqueue(ocelot->stats_queue);
>>         mutex_destroy(&ocelot->stats_lock);
>>         ocelot_ace_deinit();
>> +
>> +       for (i = 0; i < ocelot->num_phys_ports; i++) {
>> +               port = ocelot->ports[i];
>> +
>> +               list_for_each(pos, &port->skbs) {
>> +                       entry = list_entry(pos, struct ocelot_skb, head);
>> +
>> +                       list_del(pos);
> 
> list_for_each_safe

Also entry->skb seems to be leaked ?

dev_kfree_skb_any(entry->skb) seems to be needed


> 
>> +                       kfree(entry);


^ permalink raw reply

* [RFC PATCH v2 1/2] Documentation: net: dsa: Describe DSA switch configuration
From: Benedikt Spranger @ 2019-07-01 15:42 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, Sebastian Andrzej Siewior, Kurt Kanzenbach, Andrew Lunn,
	Vivien Didelot
In-Reply-To: <20190701154209.27656-1-b.spranger@linutronix.de>

Document DSA tagged and VLAN based switch configuration by showcases.

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
---
 .../networking/dsa/configuration.rst          | 292 ++++++++++++++++++
 Documentation/networking/dsa/index.rst        |   1 +
 2 files changed, 293 insertions(+)
 create mode 100644 Documentation/networking/dsa/configuration.rst

diff --git a/Documentation/networking/dsa/configuration.rst b/Documentation/networking/dsa/configuration.rst
new file mode 100644
index 000000000000..55d6dce6500d
--- /dev/null
+++ b/Documentation/networking/dsa/configuration.rst
@@ -0,0 +1,292 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=======================================
+DSA switch configuration from userspace
+=======================================
+
+The DSA switch configuration is not integrated into the main userspace
+network configuration suites by now and has to be performed manualy.
+
+.. _dsa-config-showcases:
+
+Configuration showcases
+-----------------------
+
+To configure a DSA switch a couple of commands need to be executed. In this
+documentation some common configuration scenarios are handled as showcases:
+
+*single port*
+  Every switch port acts as a different configurable Ethernet port
+
+*bridge*
+  Every switch port is part of one configurable Ethernet bridge
+
+*gateway*
+  Every switch port except one upstream port is part of a configurable
+  Ethernet bridge.
+  The upstream port acts as different configurable Ethernet port.
+
+All configurations are performed with tools from iproute2, wich is available at
+https://www.kernel.org/pub/linux/utils/net/iproute2/
+
+Through DSA every port of a switch is handled like a normal linux Ethernet
+interface. The CPU port is the switch port connected to an Ethernet MAC chip.
+The corresponding linux Ethernet interface is called the master interface.
+All other corresponding linux interfaces are called slave interfaces.
+
+The slave interfaces depend on the master interface. They can only brought up,
+when the master interface is up.
+
+In this documentation the following Ethernet interfaces are used:
+
+*eth0*
+  the master interface
+
+*lan1*
+  a slave interface
+
+*lan2*
+  another slave interface
+
+*lan3*
+  a third slave interface
+
+*wan*
+  A slave interface dedicated for upstream traffic
+
+Further Ethernet interfaces can be configured similar.
+The configured IPs and networks are:
+
+*single port*
+  * lan1: 192.0.2.1/30 (192.0.2.0 - 192.0.2.3)
+  * lan2: 192.0.2.5/30 (192.0.2.4 - 192.0.2.7)
+  * lan3: 192.0.2.9/30 (192.0.2.8 - 192.0.2.11)
+
+*bridge*
+  * br0: 192.0.2.129/25 (192.0.2.128 - 192.0.2.255)
+
+*gateway*
+  * br0: 192.0.2.129/25 (192.0.2.128 - 192.0.2.255)
+  * wan: 192.0.2.1/30 (192.0.2.0 - 192.0.2.3)
+
+.. _dsa-tagged-configuration:
+
+Configuration with tagging support
+----------------------------------
+
+The tagging based configuration is desired and supported by the majority of
+DSA switches. These switches are capable to tag incoming and outgoing traffic
+without using a VLAN based configuration.
+    
+single port
+~~~~~~~~~~~
+
+.. code-block:: sh
+
+  # configure each interface
+  ip addr add 192.0.2.1/30 dev lan1
+  ip addr add 192.0.2.5/30 dev lan2
+  ip addr add 192.0.2.9/30 dev lan3
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+
+  # bring up the slave interfaces
+  ip link set lan1 up
+  ip link set lan2 up
+  ip link set lan3 up
+
+bridge
+~~~~~~
+
+.. code-block:: sh
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+
+  # bring up the slave interfaces
+  ip link set lan1 up
+  ip link set lan2 up
+  ip link set lan3 up
+
+  # create bridge
+  ip link add name br0 type bridge
+
+  # add ports to bridge
+  ip link set dev lan1 master br0
+  ip link set dev lan2 master br0
+  ip link set dev lan3 master br0
+
+  # configure the bridge
+  ip addr add 192.0.2.129/25 dev br0
+
+  # bring up the bridge
+  ip link set dev br0 up
+
+gateway
+~~~~~~~
+
+.. code-block:: sh
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+
+  # bring up the slave interfaces
+  ip link set wan up
+  ip link set lan1 up
+  ip link set lan2 up
+
+  # configure the upstream port
+  ip addr add 192.0.2.1/30 dev wan
+
+  # create bridge
+  ip link add name br0 type bridge
+
+  # add ports to bridge
+  ip link set dev lan1 master br0
+  ip link set dev lan2 master br0
+
+  # configure the bridge
+  ip addr add 192.0.2.129/25 dev br0
+
+  # bring up the bridge
+  ip link set dev br0 up
+
+.. _dsa-vlan-configuration:
+
+Configuration without tagging support
+-------------------------------------
+
+A minority of switches are not capable to use a taging protocol
+(DSA_TAG_PROTO_NONE). These switches can be configured by a VLAN based
+configuration.
+
+single port
+~~~~~~~~~~~
+The configuration can only be set up via VLAN tagging and bridge setup.
+
+.. code-block:: sh
+
+  # tag traffic on CPU port
+  ip link add link eth0 name eth0.1 type vlan id 1
+  ip link add link eth0 name eth0.2 type vlan id 2
+  ip link add link eth0 name eth0.3 type vlan id 3
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+  ip link set eth0.1 up
+  ip link set eth0.2 up
+  ip link set eth0.3 up
+
+  # bring up the slave interfaces
+  ip link set lan1 up
+  ip link set lan1 up
+  ip link set lan3 up
+
+  # create bridge
+  ip link add name br0 type bridge
+
+  # activate VLAN filtering
+  ip link set dev br0 type bridge vlan_filtering 1
+
+  # add ports to bridges
+  ip link set dev lan1 master br0
+  ip link set dev lan2 master br0
+  ip link set dev lan3 master br0
+
+  # tag traffic on ports
+  bridge vlan add dev lan1 vid 1 pvid untagged
+  bridge vlan add dev lan2 vid 2 pvid untagged
+  bridge vlan add dev lan3 vid 3 pvid untagged
+
+  # configure the VLANs
+  ip addr add 192.0.2.1/30 dev eth0.1
+  ip addr add 192.0.2.5/30 dev eth0.2
+  ip addr add 192.0.2.9/30 dev eth0.3
+
+  # bring up the bridge devices
+  ip link set br0 up
+
+
+bridge
+~~~~~~
+
+.. code-block:: sh
+
+  # tag traffic on CPU port
+  ip link add link eth0 name eth0.1 type vlan id 1
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+  ip link set eth0.1 up
+
+  # bring up the slave interfaces
+  ip link set lan1 up
+  ip link set lan2 up
+  ip link set lan3 up
+
+  # create bridge
+  ip link add name br0 type bridge
+
+  # activate VLAN filtering
+  ip link set dev br0 type bridge vlan_filtering 1
+
+  # add ports to bridge
+  ip link set dev lan1 master br0
+  ip link set dev lan2 master br0
+  ip link set dev lan3 master br0
+  ip link set eth0.1 master br0
+
+  # tag traffic on ports
+  bridge vlan add dev lan1 vid 1 pvid untagged
+  bridge vlan add dev lan2 vid 1 pvid untagged
+  bridge vlan add dev lan3 vid 1 pvid untagged
+
+  # configure the bridge
+  ip addr add 192.0.2.129/25 dev br0
+
+  # bring up the bridge
+  ip link set dev br0 up
+
+gateway
+~~~~~~~
+
+.. code-block:: sh
+
+  # tag traffic on CPU port
+  ip link add link eth0 name eth0.1 type vlan id 1
+  ip link add link eth0 name eth0.2 type vlan id 2
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+  ip link set eth0.1 up
+  ip link set eth0.2 up
+
+  # bring up the slave interfaces
+  ip link set wan up
+  ip link set lan1 up
+  ip link set lan2 up
+
+  # create bridge
+  ip link add name br0 type bridge
+
+  # activate VLAN filtering
+  ip link set dev br0 type bridge vlan_filtering 1
+
+  # add ports to bridges
+  ip link set dev wan master br0
+  ip link set eth0.1 master br0
+  ip link set dev lan1 master br0
+  ip link set dev lan2 master br0
+
+  # tag traffic on ports
+  bridge vlan add dev lan1 vid 1 pvid untagged
+  bridge vlan add dev lan2 vid 1 pvid untagged
+  bridge vlan add dev wan vid 2 pvid untagged
+
+  # configure the VLANs
+  ip addr add 192.0.2.1/30 dev eth0.2
+  ip addr add 192.0.2.129/25 dev br0
+
+  # bring up the bridge devices
+  ip link set br0 up
diff --git a/Documentation/networking/dsa/index.rst b/Documentation/networking/dsa/index.rst
index 0e5b7a9be406..c279cfbf9083 100644
--- a/Documentation/networking/dsa/index.rst
+++ b/Documentation/networking/dsa/index.rst
@@ -9,3 +9,4 @@ Distributed Switch Architecture
    bcm_sf2
    lan9303
    sja1105
+   configuration
-- 
2.20.1


^ permalink raw reply related

* [RFC PATCH v2 2/2] Documentation: net: dsa: b53: Describe b53 configuration
From: Benedikt Spranger @ 2019-07-01 15:42 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, Sebastian Andrzej Siewior, Kurt Kanzenbach, Andrew Lunn,
	Vivien Didelot
In-Reply-To: <20190701154209.27656-1-b.spranger@linutronix.de>

Document the different needs of documentation for the b53 driver.

Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
---
 Documentation/networking/dsa/b53.rst   | 174 +++++++++++++++++++++++++
 Documentation/networking/dsa/index.rst |   1 +
 2 files changed, 175 insertions(+)
 create mode 100644 Documentation/networking/dsa/b53.rst

diff --git a/Documentation/networking/dsa/b53.rst b/Documentation/networking/dsa/b53.rst
new file mode 100644
index 000000000000..23f1d79a6258
--- /dev/null
+++ b/Documentation/networking/dsa/b53.rst
@@ -0,0 +1,174 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==========================================
+Broadcom RoboSwitch Ethernet switch driver
+==========================================
+
+The Broadcom RoboSwitch Ethernet switch family is used in quite a range of
+xDSL router, cable modems and other multimedia devices.
+
+The actual implementation supports the devices BCM5325E, BCM5365, BCM539x,
+BCM53115 and BCM53125 as well as BCM63XX.
+
+Implementation details
+======================
+
+The driver is located in ``drivers/net/dsa/b53/`` and is implemented as a
+DSA driver; see ``Documentation/networking/dsa/dsa.rst`` for details on the
+subsystem and what it provides.
+
+The switch is, if possible, configured to enable a Broadcom specific 4-bytes
+switch tag which gets inserted by the switch for every packet forwarded to the
+CPU interface, conversely, the CPU network interface should insert a similar
+tag for packets entering the CPU port. The tag format is described in
+``net/dsa/tag_brcm.c``.
+
+The configuration of the device depends on whether or not tagging is
+supported.
+
+The interface names and example network configuration are used according the
+configuration described in the :ref:`dsa-config-showcases`.
+
+Configuration with tagging support
+----------------------------------
+
+The tagging based configuration is desired. It is not specific to the b53
+DSA driver and will work like all DSA drivers which supports tagging.
+
+See :ref:`dsa-tagged-configuration`.
+
+Configuration without tagging support
+-------------------------------------
+
+Older models (5325, 5365) support a different tag format that is not supported
+yet. 539x and 531x5 require managed mode and some special handling, which is
+also not yet supported. The tagging support is disabled in these cases and the
+switch need a different configuration.
+
+The configuration slightly differ from the :ref:`dsa-vlan-configuration`.
+
+single port
+~~~~~~~~~~~
+The configuration can only be set up via VLAN tagging and bridge setup.
+By default packages are tagged with vid 1:
+
+.. code-block:: sh
+
+  # tag traffic on CPU port
+  ip link add link eth0 name eth0.1 type vlan id 1
+  ip link add link eth0 name eth0.2 type vlan id 2
+  ip link add link eth0 name eth0.3 type vlan id 3
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+  ip link set eth0.1 up
+  ip link set eth0.2 up
+  ip link set eth0.3 up
+
+  # bring up the slave interfaces
+  ip link set wan up
+  ip link set lan1 up
+  ip link set lan2 up
+
+  # create bridge
+  ip link add name br0 type bridge
+
+  # activate VLAN filtering
+  ip link set dev br0 type bridge vlan_filtering 1
+
+  # add ports to bridges
+  ip link set dev wan master br0
+  ip link set dev lan1 master br0
+  ip link set dev lan2 master br0
+
+  # tag traffic on ports
+  bridge vlan add dev lan1 vid 2 pvid untagged
+  bridge vlan del dev lan1 vid 1
+  bridge vlan add dev lan2 vid 3 pvid untagged
+  bridge vlan del dev lan2 vid 1
+
+  # configure the VLANs
+  ip addr add 192.0.2.1/30 dev eth0.1
+  ip addr add 192.0.2.5/30 dev eth0.2
+  ip addr add 192.0.2.9/30 dev eth0.3
+
+  # bring up the bridge devices
+  ip link set br0 up
+
+
+bridge
+~~~~~~
+
+.. code-block:: sh
+
+  # tag traffic on CPU port
+  ip link add link eth0 name eth0.1 type vlan id 1
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+  ip link set eth0.1 up
+
+  # bring up the slave interfaces
+  ip link set wan up
+  ip link set lan1 up
+  ip link set lan2 up
+
+  # create bridge
+  ip link add name br0 type bridge
+
+  # activate VLAN filtering
+  ip link set dev br0 type bridge vlan_filtering 1
+
+  # add ports to bridge
+  ip link set dev wan master br0
+  ip link set dev lan1 master br0
+  ip link set dev lan2 master br0
+  ip link set eth0.1 master br0
+
+  # configure the bridge
+  ip addr add 192.0.2.129/25 dev br0
+
+  # bring up the bridge
+  ip link set dev br0 up
+
+gateway
+~~~~~~~
+
+.. code-block:: sh
+
+  # tag traffic on CPU port
+  ip link add link eth0 name eth0.1 type vlan id 1
+  ip link add link eth0 name eth0.2 type vlan id 2
+
+  # The master interface needs to be brought up before the slave ports.
+  ip link set eth0 up
+  ip link set eth0.1 up
+  ip link set eth0.2 up
+
+  # bring up the slave interfaces
+  ip link set wan up
+  ip link set lan1 up
+  ip link set lan2 up
+
+  # create bridge
+  ip link add name br0 type bridge
+
+  # activate VLAN filtering
+  ip link set dev br0 type bridge vlan_filtering 1
+
+  # add ports to bridges
+  ip link set dev wan master br0
+  ip link set eth0.1 master br0
+  ip link set dev lan1 master br0
+  ip link set dev lan2 master br0
+
+  # tag traffic on ports
+  bridge vlan add dev wan vid 2 pvid untagged
+  bridge vlan del dev wan vid 1
+
+  # configure the VLANs
+  ip addr add 192.0.2.1/30 dev eth0.2
+  ip addr add 192.0.2.129/25 dev br0
+
+  # bring up the bridge devices
+  ip link set br0 up
diff --git a/Documentation/networking/dsa/index.rst b/Documentation/networking/dsa/index.rst
index c279cfbf9083..ee631e2d646f 100644
--- a/Documentation/networking/dsa/index.rst
+++ b/Documentation/networking/dsa/index.rst
@@ -6,6 +6,7 @@ Distributed Switch Architecture
    :maxdepth: 1
 
    dsa
+   b53
    bcm_sf2
    lan9303
    sja1105
-- 
2.20.1


^ permalink raw reply related

* [RFC PATCH v2 0/2] Document the configuration of b53
From: Benedikt Spranger @ 2019-07-01 15:42 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: netdev, Sebastian Andrzej Siewior, Kurt Kanzenbach, Andrew Lunn,
	Vivien Didelot

Hi,

this is the second RFC to document the configuration of a b53 supported
switch.

Thanks for the comments.

Regards
    Bene Spranger

v1..v2:
- split out generic parts of the configuration.
- target comments by Andrew Lunn and Florian Fainelli.
- make changes visible to build system

Benedikt Spranger (2):
  Documentation: net: dsa: Describe DSA switch configuration
  Documentation: net: dsa: b53: Describe b53 configuration

 Documentation/networking/dsa/b53.rst          | 174 +++++++++++
 .../networking/dsa/configuration.rst          | 292 ++++++++++++++++++
 Documentation/networking/dsa/index.rst        |   2 +
 3 files changed, 468 insertions(+)
 create mode 100644 Documentation/networking/dsa/b53.rst
 create mode 100644 Documentation/networking/dsa/configuration.rst

-- 
2.20.1


^ permalink raw reply

* Re: [PATCH net-next v4 4/5] net: sched: add mpls manipulation actions to TC
From: Willem de Bruijn @ 2019-07-01 15:48 UTC (permalink / raw)
  To: John Hurley
  Cc: Network Development, David Miller, jiri, Cong Wang, David Ahern,
	simon.horman, Jakub Kicinski, oss-drivers
In-Reply-To: <1561984257-9798-5-git-send-email-john.hurley@netronome.com>

On Mon, Jul 1, 2019 at 8:31 AM John Hurley <john.hurley@netronome.com> wrote:
>
> Currently, TC offers the ability to match on the MPLS fields of a packet
> through the use of the flow_dissector_key_mpls struct. However, as yet, TC
> actions do not allow the modification or manipulation of such fields.
>
> Add a new module that registers TC action ops to allow manipulation of
> MPLS. This includes the ability to push and pop headers as well as modify
> the contents of new or existing headers. A further action to decrement the
> TTL field of an MPLS header is also provided.
>
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>

> +static __be32 tcf_mpls_get_lse(struct mpls_shim_hdr *lse,
> +                              struct tcf_mpls_params *p, bool set_bos)
> +{
> +       u32 new_lse = 0;
> +
> +       if (lse)
> +               new_lse = be32_to_cpu(lse->label_stack_entry);
> +
> +       if (p->tcfm_label) {
> +               new_lse &= ~MPLS_LS_LABEL_MASK;
> +               new_lse |= p->tcfm_label << MPLS_LS_LABEL_SHIFT;
> +       }
> +       if (p->tcfm_ttl) {
> +               new_lse &= ~MPLS_LS_TTL_MASK;
> +               new_lse |= p->tcfm_ttl << MPLS_LS_TTL_SHIFT;
> +       }
> +       if (p->tcfm_tc != ACT_MPLS_TC_NOT_SET) {
> +               new_lse &= ~MPLS_LS_TC_MASK;
> +               new_lse |= p->tcfm_tc << MPLS_LS_TC_SHIFT;
> +       }
> +       if (p->tcfm_bos != ACT_MPLS_BOS_NOT_SET) {
> +               new_lse &= ~MPLS_LS_S_MASK;
> +               new_lse |= p->tcfm_bos << MPLS_LS_S_SHIFT;
> +       } else if (set_bos) {
> +               new_lse |= 1 << MPLS_LS_S_SHIFT;
> +       }

not necessarily for this patchset, but perhaps it would make code more
readable to add a struct mpls_label_type with integer bit fields to avoid
all this explicit masking and shifting.

> +static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a,
> +                       struct tcf_result *res)
> +{
> +       struct tcf_mpls *m = to_mpls(a);
> +       struct mpls_shim_hdr *pkt_lse;
> +       struct tcf_mpls_params *p;
> +       __be32 new_lse;
> +       u32 cpu_lse;
> +       int ret;
> +       u8 ttl;
> +
> +       tcf_lastuse_update(&m->tcf_tm);
> +       bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
> +
> +       /* Ensure 'data' points at mac_header prior calling mpls manipulating
> +        * functions.
> +        */
> +       if (skb_at_tc_ingress(skb))
> +               skb_push_rcsum(skb, skb->mac_len);
> +
> +       ret = READ_ONCE(m->tcf_action);
> +
> +       p = rcu_dereference_bh(m->mpls_p);
> +
> +       switch (p->tcfm_action) {
> +       case TCA_MPLS_ACT_POP:
> +               if (skb_mpls_pop(skb, p->tcfm_proto))
> +                       goto drop;
> +               break;
> +       case TCA_MPLS_ACT_PUSH:
> +               new_lse = tcf_mpls_get_lse(NULL, p, !eth_p_mpls(skb->protocol));
> +               if (skb_mpls_push(skb, new_lse, p->tcfm_proto))
> +                       goto drop;
> +               break;
> +       case TCA_MPLS_ACT_MODIFY:
> +               new_lse = tcf_mpls_get_lse(mpls_hdr(skb), p, false);
> +               if (skb_mpls_update_lse(skb, new_lse))
> +                       goto drop;
> +               break;
> +       case TCA_MPLS_ACT_DEC_TTL:
> +               pkt_lse = mpls_hdr(skb);
> +               cpu_lse = be32_to_cpu(pkt_lse->label_stack_entry);
> +               ttl = (cpu_lse & MPLS_LS_TTL_MASK) >> MPLS_LS_TTL_SHIFT;
> +               if (!--ttl)
> +                       goto drop;
> +
> +               cpu_lse &= ~MPLS_LS_TTL_MASK;
> +               cpu_lse |= ttl << MPLS_LS_TTL_SHIFT;

this could perhaps use a helper of its own?


> +               if (skb_mpls_update_lse(skb, cpu_to_be32(cpu_lse)))
> +                       goto drop;
> +               break;
> +       }

^ permalink raw reply

* Re: [PATCH net-next v4 4/5] net: sched: add mpls manipulation actions to TC
From: David Ahern @ 2019-07-01 15:47 UTC (permalink / raw)
  To: John Hurley, netdev
  Cc: davem, jiri, xiyou.wangcong, simon.horman, jakub.kicinski,
	oss-drivers
In-Reply-To: <1561984257-9798-5-git-send-email-john.hurley@netronome.com>

On 7/1/19 6:30 AM, John Hurley wrote:
> Currently, TC offers the ability to match on the MPLS fields of a packet
> through the use of the flow_dissector_key_mpls struct. However, as yet, TC
> actions do not allow the modification or manipulation of such fields.
> 
> Add a new module that registers TC action ops to allow manipulation of
> MPLS. This includes the ability to push and pop headers as well as modify
> the contents of new or existing headers. A further action to decrement the
> TTL field of an MPLS header is also provided.

Would be good to document an example here and how to handle a label
stack. The same example can be used with the iproute2 patch (I presume
this one ;-)).


> +static int valid_label(const struct nlattr *attr,
> +		       struct netlink_ext_ack *extack)
> +{
> +	const u32 *label = nla_data(attr);
> +
> +	if (!*label || *label & ~MPLS_LABEL_MASK) {
> +		NL_SET_ERR_MSG_MOD(extack, "MPLS label out of range");
> +		return -EINVAL;
> +	}

core MPLS code (nla_get_labels) checks for MPLS_LABEL_IMPLNULL as well.


> +
> +	return 0;
> +}
> +
> +static const struct nla_policy mpls_policy[TCA_MPLS_MAX + 1] = {
> +	[TCA_MPLS_UNSPEC]	= { .strict_start_type = TCA_MPLS_UNSPEC + 1 },
> +	[TCA_MPLS_PARMS]	= NLA_POLICY_EXACT_LEN(sizeof(struct tc_mpls)),
> +	[TCA_MPLS_PROTO]	= { .type = NLA_U16 },
> +	[TCA_MPLS_LABEL]	= NLA_POLICY_VALIDATE_FN(NLA_U32, valid_label),
> +	[TCA_MPLS_TC]		= NLA_POLICY_RANGE(NLA_U8, 0, 7),
> +	[TCA_MPLS_TTL]		= NLA_POLICY_MIN(NLA_U8, 1),
> +	[TCA_MPLS_BOS]		= NLA_POLICY_RANGE(NLA_U8, 0, 1),
> +};
> +
> +static int tcf_mpls_init(struct net *net, struct nlattr *nla,
> +			 struct nlattr *est, struct tc_action **a,
> +			 int ovr, int bind, bool rtnl_held,
> +			 struct tcf_proto *tp, struct netlink_ext_ack *extack)
> +{
> +	struct tc_action_net *tn = net_generic(net, mpls_net_id);
> +	struct nlattr *tb[TCA_MPLS_MAX + 1];
> +	struct tcf_chain *goto_ch = NULL;
> +	struct tcf_mpls_params *p;
> +	struct tc_mpls *parm;
> +	bool exists = false;
> +	struct tcf_mpls *m;
> +	int ret = 0, err;
> +	u8 mpls_ttl = 0;
> +
> +	if (!nla) {
> +		NL_SET_ERR_MSG_MOD(extack, "Missing netlink attributes");
> +		return -EINVAL;
> +	}
> +
> +	err = nla_parse_nested(tb, TCA_MPLS_MAX, nla, mpls_policy, extack);
> +	if (err < 0)
> +		return err;
> +
> +	if (!tb[TCA_MPLS_PARMS]) {
> +		NL_SET_ERR_MSG_MOD(extack, "No MPLS params");
> +		return -EINVAL;
> +	}
> +	parm = nla_data(tb[TCA_MPLS_PARMS]);
> +
> +	/* Verify parameters against action type. */
> +	switch (parm->m_action) {
> +	case TCA_MPLS_ACT_POP:
> +		if (!tb[TCA_MPLS_PROTO] ||
> +		    !eth_proto_is_802_3(nla_get_be16(tb[TCA_MPLS_PROTO]))) {
> +			NL_SET_ERR_MSG_MOD(extack, "Invalid protocol type for MPLS pop");

would be better to call out '!tb[TCA_MPLS_PROTO]' with its own 'Protocol
must be set given for pop' message.


^ permalink raw reply

* Re: [PATCH bpf-next 2/2] selftests/bpf: add verifier tests for wide stores
From: Andrii Nakryiko @ 2019-07-01 15:44 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Stanislav Fomichev, netdev@vger.kernel.org, bpf@vger.kernel.org,
	davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
	Andrii Nakryiko
In-Reply-To: <8e469767-a108-ba42-f8c8-6fd505393699@fb.com>

On Sat, Jun 29, 2019 at 11:02 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> > Make sure that wide stores are allowed at proper (aligned) addresses.
> > Note that user_ip6 is naturally aligned on 8-byte boundary, so
> > correct addresses are user_ip6[0] and user_ip6[2]. msg_src_ip6 is,
> > however, aligned on a 4-byte bondary, so only msg_src_ip6[1]
> > can be wide-stored.
> >
> > Cc: Andrii Nakryiko <andriin@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >   tools/testing/selftests/bpf/test_verifier.c   | 17 ++++++--
> >   .../selftests/bpf/verifier/wide_store.c       | 40 +++++++++++++++++++
> >   2 files changed, 54 insertions(+), 3 deletions(-)
> >   create mode 100644 tools/testing/selftests/bpf/verifier/wide_store.c
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index c5514daf8865..b0773291012a 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -105,6 +105,7 @@ struct bpf_test {
> >                       __u64 data64[TEST_DATA_LEN / 8];
> >               };
> >       } retvals[MAX_TEST_RUNS];
> > +     enum bpf_attach_type expected_attach_type;
> >   };
> >
> >   /* Note we want this to be 64 bit aligned so that the end of our array is
> > @@ -850,6 +851,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >       int fd_prog, expected_ret, alignment_prevented_execution;
> >       int prog_len, prog_type = test->prog_type;
> >       struct bpf_insn *prog = test->insns;
> > +     struct bpf_load_program_attr attr;
> >       int run_errs, run_successes;
> >       int map_fds[MAX_NR_MAPS];
> >       const char *expected_err;
> > @@ -881,8 +883,17 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >               pflags |= BPF_F_STRICT_ALIGNMENT;
> >       if (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS)
> >               pflags |= BPF_F_ANY_ALIGNMENT;
> > -     fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
> > -                                  "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
> > +
> > +     memset(&attr, 0, sizeof(attr));
> > +     attr.prog_type = prog_type;
> > +     attr.expected_attach_type = test->expected_attach_type;
> > +     attr.insns = prog;
> > +     attr.insns_cnt = prog_len;
> > +     attr.license = "GPL";
> > +     attr.log_level = 4;
> > +     attr.prog_flags = pflags;
> > +
> > +     fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
> >       if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
> >               printf("SKIP (unsupported program type %d)\n", prog_type);
> >               skips++;
> > @@ -912,7 +923,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >                       printf("FAIL\nUnexpected success to load!\n");
> >                       goto fail_log;
> >               }
> > -             if (!strstr(bpf_vlog, expected_err)) {
> > +             if (!expected_err || !strstr(bpf_vlog, expected_err)) {
> >                       printf("FAIL\nUnexpected error message!\n\tEXP: %s\n\tRES: %s\n",
> >                             expected_err, bpf_vlog);
> >                       goto fail_log;
> > diff --git a/tools/testing/selftests/bpf/verifier/wide_store.c b/tools/testing/selftests/bpf/verifier/wide_store.c
> > new file mode 100644
> > index 000000000000..c6385f45b114
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/verifier/wide_store.c
> > @@ -0,0 +1,40 @@
> > +#define BPF_SOCK_ADDR(field, off, res, err) \
> > +{ \
> > +     "wide store to bpf_sock_addr." #field "[" #off "]", \
> > +     .insns = { \
> > +     BPF_MOV64_IMM(BPF_REG_0, 1), \
> > +     BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, \
> > +                 offsetof(struct bpf_sock_addr, field[off])), \
> > +     BPF_EXIT_INSN(), \
> > +     }, \
> > +     .result = res, \
> > +     .prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR, \
> > +     .expected_attach_type = BPF_CGROUP_UDP6_SENDMSG, \
> > +     .errstr = err, \
> > +}
> > +
> > +/* user_ip6[0] is u64 aligned */
> > +BPF_SOCK_ADDR(user_ip6, 0, ACCEPT,
> > +           NULL),
> > +BPF_SOCK_ADDR(user_ip6, 1, REJECT,
> > +           "invalid bpf_context access off=12 size=8"),
> > +BPF_SOCK_ADDR(user_ip6, 2, ACCEPT,
> > +           NULL),
> > +BPF_SOCK_ADDR(user_ip6, 3, REJECT,
> > +           "invalid bpf_context access off=20 size=8"),
> > +BPF_SOCK_ADDR(user_ip6, 4, REJECT,
> > +           "invalid bpf_context access off=24 size=8"),
>
> With offset 4, we have
> #968/p wide store to bpf_sock_addr.user_ip6[4] OK
>
> This test case can be removed. user code typically
> won't write bpf_sock_addr.user_ip6[4], and compiler
> typically will give a warning since it is out of
> array bound. Any particular reason you want to
> include this one?

I agree, user_ip6[4] is essentially 8-byte write to user_port field.

>
>
> > +
> > +/* msg_src_ip6[0] is _not_ u64 aligned */
> > +BPF_SOCK_ADDR(msg_src_ip6, 0, REJECT,
> > +           "invalid bpf_context access off=44 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 1, ACCEPT,
> > +           NULL),
> > +BPF_SOCK_ADDR(msg_src_ip6, 2, REJECT,
> > +           "invalid bpf_context access off=52 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 3, REJECT,
> > +           "invalid bpf_context access off=56 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 4, REJECT,
> > +           "invalid bpf_context access off=60 size=8"),
>
> The same as above, offset=4 case can be removed?

And this one is a write into a struct hole, which should be rejected
even without wide-store check, right?

>
> > +
> > +#undef BPF_SOCK_ADDR
> >

^ permalink raw reply

* Re: [PATCH bpf-next 1/2] bpf: allow wide (u64) aligned stores for some fields of bpf_sock_addr
From: Andrii Nakryiko @ 2019-07-01 15:36 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Stanislav Fomichev, netdev@vger.kernel.org, bpf@vger.kernel.org,
	davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
	Andrii Nakryiko, kernel test robot
In-Reply-To: <be223396-b181-e587-d63c-2b15eaca3721@fb.com>

On Sat, Jun 29, 2019 at 10:53 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> > Since commit cd17d7770578 ("bpf/tools: sync bpf.h") clang decided
> > that it can do a single u64 store into user_ip6[2] instead of two
> > separate u32 ones:
> >
> >   #  17: (18) r2 = 0x100000000000000
> >   #  ; ctx->user_ip6[2] = bpf_htonl(DST_REWRITE_IP6_2);
> >   #  19: (7b) *(u64 *)(r1 +16) = r2
> >   #  invalid bpf_context access off=16 size=8
> >
> >  From the compiler point of view it does look like a correct thing
> > to do, so let's support it on the kernel side.
> >
> > Credit to Andrii Nakryiko for a proper implementation of
> > bpf_ctx_wide_store_ok.
> >
> > Cc: Andrii Nakryiko <andriin@fb.com>
> > Cc: Yonghong Song <yhs@fb.com>
> > Fixes: cd17d7770578 ("bpf/tools: sync bpf.h")
> > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
>
> The change looks good to me with the following nits:
>    1. could you add a cover letter for the patch set?
>       typically if the number of patches is more than one,
>       it would be a good practice with a cover letter.
>       See bpf_devel_QA.rst .
>    2. with this change, the comments in uapi bpf.h
>       are not accurate any more.
>          __u32 user_ip6[4];      /* Allows 1,2,4-byte read an 4-byte write.
>                                   * Stored in network byte order.
>
>                                   */
>          __u32 msg_src_ip6[4];   /* Allows 1,2,4-byte read an 4-byte write.
>                                   * Stored in network byte order.
>                                   */
>       now for stores, aligned 8-byte write is permitted.
>       could you update this as well?
>
>  From the typical usage pattern, I did not see a need
> for 8-tye read of user_ip6 and msg_src_ip6 yet. So let
> us just deal with write for now.

But I guess it's still possible for clang to optimize two consecutive
4-byte reads into single 8-byte read in some circumstances? If that's
the case, maybe it's a good idea to have corresponding read checks as
well?

But overall this looks good to me:

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
> With the above two nits,
> Acked-by: Yonghong Song <yhs@fb.com>
>
> > ---
> >   include/linux/filter.h |  6 ++++++
> >   net/core/filter.c      | 22 ++++++++++++++--------
> >   2 files changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index 340f7d648974..3901007e36f1 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -746,6 +746,12 @@ bpf_ctx_narrow_access_ok(u32 off, u32 size, u32 size_default)
> >       return size <= size_default && (size & (size - 1)) == 0;
> >   }
> >
> > +#define bpf_ctx_wide_store_ok(off, size, type, field)                        \
> > +     (size == sizeof(__u64) &&                                       \
> > +     off >= offsetof(type, field) &&                                 \
> > +     off + sizeof(__u64) <= offsetofend(type, field) &&              \
> > +     off % sizeof(__u64) == 0)
> > +
> >   #define bpf_classic_proglen(fprog) (fprog->len * sizeof(fprog->filter[0]))
> >
> >   static inline void bpf_prog_lock_ro(struct bpf_prog *fp)
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index dc8534be12fc..5d33f2146dab 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -6849,6 +6849,16 @@ static bool sock_addr_is_valid_access(int off, int size,
> >                       if (!bpf_ctx_narrow_access_ok(off, size, size_default))
> >                               return false;
> >               } else {
> > +                     if (bpf_ctx_wide_store_ok(off, size,
> > +                                               struct bpf_sock_addr,
> > +                                               user_ip6))
> > +                             return true;
> > +
> > +                     if (bpf_ctx_wide_store_ok(off, size,
> > +                                               struct bpf_sock_addr,
> > +                                               msg_src_ip6))
> > +                             return true;
> > +
> >                       if (size != size_default)
> >                               return false;
> >               }
> > @@ -7689,9 +7699,6 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >   /* SOCK_ADDR_STORE_NESTED_FIELD_OFF() has semantic similar to
> >    * SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF() but for store operation.
> >    *
> > - * It doesn't support SIZE argument though since narrow stores are not
> > - * supported for now.
> > - *
> >    * In addition it uses Temporary Field TF (member of struct S) as the 3rd
> >    * "register" since two registers available in convert_ctx_access are not
> >    * enough: we can't override neither SRC, since it contains value to store, nor
> > @@ -7699,7 +7706,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >    * instructions. But we need a temporary place to save pointer to nested
> >    * structure whose field we want to store to.
> >    */
> > -#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF, TF)                     \
> > +#define SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE, OFF, TF)               \
> >       do {                                                                   \
> >               int tmp_reg = BPF_REG_9;                                       \
> >               if (si->src_reg == tmp_reg || si->dst_reg == tmp_reg)          \
> > @@ -7710,8 +7717,7 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >                                     offsetof(S, TF));                        \
> >               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg,         \
> >                                     si->dst_reg, offsetof(S, F));            \
> > -             *insn++ = BPF_STX_MEM(                                         \
> > -                     BPF_FIELD_SIZEOF(NS, NF), tmp_reg, si->src_reg,        \
> > +             *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg,              \
> >                       bpf_target_off(NS, NF, FIELD_SIZEOF(NS, NF),           \
> >                                      target_size)                            \
> >                               + OFF);                                        \
> > @@ -7723,8 +7729,8 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >                                                     TF)                      \
> >       do {                                                                   \
> >               if (type == BPF_WRITE) {                                       \
> > -                     SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, OFF,    \
> > -                                                      TF);                  \
> > +                     SOCK_ADDR_STORE_NESTED_FIELD_OFF(S, NS, F, NF, SIZE,   \
> > +                                                      OFF, TF);             \
> >               } else {                                                       \
> >                       SOCK_ADDR_LOAD_NESTED_FIELD_SIZE_OFF(                  \
> >                               S, NS, F, NF, SIZE, OFF);  \
> >

^ permalink raw reply

* Re: [PATCH] net: usb: asix: init MAC address buffers
From: Dan Williams @ 2019-07-01 15:29 UTC (permalink / raw)
  To: Phong Tran, syzbot+8a3fc6674bbc3978ed4e
  Cc: davem, glider, linux-kernel, linux-usb, lynxis, marcel.ziswiler,
	netdev, syzkaller-bugs, yang.wei9, zhang.run, skhan,
	linux-kernel-mentees
In-Reply-To: <20190630234533.15089-1-tranmanphong@gmail.com>

On Mon, 2019-07-01 at 06:45 +0700, Phong Tran wrote:
> This is for fixing bug KMSAN: uninit-value in ax88772_bind
> 
> Tested by
> https://groups.google.com/d/msg/syzkaller-bugs/aFQurGotng4/cFe9nxMCCwAJ
> 
> Reported-by: syzbot+8a3fc6674bbc3978ed4e@syzkaller.appspotmail.com
> 
> syzbot found the following crash on:
> 
> HEAD commit:    f75e4cfe kmsan: use kmsan_handle_urb() in urb.c
> git tree:       kmsan
> console output: 
> https://syzkaller.appspot.com/x/log.txt?x=136d720ea00000
> kernel config:
> https://syzkaller.appspot.com/x/.config?x=602468164ccdc30a
> dashboard link:
> https://syzkaller.appspot.com/bug?extid=8a3fc6674bbc3978ed4e
> compiler:       clang version 9.0.0 (/home/glider/llvm/clang
> 06d00afa61eef8f7f501ebdb4e8612ea43ec2d78)
> syz repro:
> https://syzkaller.appspot.com/x/repro.syz?x=12788316a00000
> C reproducer:   
> https://syzkaller.appspot.com/x/repro.c?x=120359aaa00000
> 
> ==================================================================
> BUG: KMSAN: uninit-value in is_valid_ether_addr
> include/linux/etherdevice.h:200 [inline]
> BUG: KMSAN: uninit-value in asix_set_netdev_dev_addr
> drivers/net/usb/asix_devices.c:73 [inline]
> BUG: KMSAN: uninit-value in ax88772_bind+0x93d/0x11e0
> drivers/net/usb/asix_devices.c:724
> CPU: 0 PID: 3348 Comm: kworker/0:2 Not tainted 5.1.0+ #1
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS
> Google 01/01/2011
> Workqueue: usb_hub_wq hub_event
> Call Trace:
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0x191/0x1f0 lib/dump_stack.c:113
>   kmsan_report+0x130/0x2a0 mm/kmsan/kmsan.c:622
>   __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:310
>   is_valid_ether_addr include/linux/etherdevice.h:200 [inline]
>   asix_set_netdev_dev_addr drivers/net/usb/asix_devices.c:73 [inline]
>   ax88772_bind+0x93d/0x11e0 drivers/net/usb/asix_devices.c:724
>   usbnet_probe+0x10f5/0x3940 drivers/net/usb/usbnet.c:1728
>   usb_probe_interface+0xd66/0x1320 drivers/usb/core/driver.c:361
>   really_probe+0xdae/0x1d80 drivers/base/dd.c:513
>   driver_probe_device+0x1b3/0x4f0 drivers/base/dd.c:671
>   __device_attach_driver+0x5b8/0x790 drivers/base/dd.c:778
>   bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:454
>   __device_attach+0x454/0x730 drivers/base/dd.c:844
>   device_initial_probe+0x4a/0x60 drivers/base/dd.c:891
>   bus_probe_device+0x137/0x390 drivers/base/bus.c:514
>   device_add+0x288d/0x30e0 drivers/base/core.c:2106
>   usb_set_configuration+0x30dc/0x3750 drivers/usb/core/message.c:2027
>   generic_probe+0xe7/0x280 drivers/usb/core/generic.c:210
>   usb_probe_device+0x14c/0x200 drivers/usb/core/driver.c:266
>   really_probe+0xdae/0x1d80 drivers/base/dd.c:513
>   driver_probe_device+0x1b3/0x4f0 drivers/base/dd.c:671
>   __device_attach_driver+0x5b8/0x790 drivers/base/dd.c:778
>   bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:454
>   __device_attach+0x454/0x730 drivers/base/dd.c:844
>   device_initial_probe+0x4a/0x60 drivers/base/dd.c:891
>   bus_probe_device+0x137/0x390 drivers/base/bus.c:514
>   device_add+0x288d/0x30e0 drivers/base/core.c:2106
>   usb_new_device+0x23e5/0x2ff0 drivers/usb/core/hub.c:2534
>   hub_port_connect drivers/usb/core/hub.c:5089 [inline]
>   hub_port_connect_change drivers/usb/core/hub.c:5204 [inline]
>   port_event drivers/usb/core/hub.c:5350 [inline]
>   hub_event+0x48d1/0x7290 drivers/usb/core/hub.c:5432
>   process_one_work+0x1572/0x1f00 kernel/workqueue.c:2269
>   process_scheduled_works kernel/workqueue.c:2331 [inline]
>   worker_thread+0x189c/0x2460 kernel/workqueue.c:2417
>   kthread+0x4b5/0x4f0 kernel/kthread.c:254
>   ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
> 
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
> ---
>  drivers/net/usb/asix_devices.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/usb/asix_devices.c
> b/drivers/net/usb/asix_devices.c
> index c9bc96310ed4..f514d19316b1 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -230,6 +230,7 @@ static int ax88172_bind(struct usbnet *dev,
> struct usb_interface *intf)
>  	int i;
>  	unsigned long gpio_bits = dev->driver_info->data;
>  
> +	memset(buf, 0, sizeof(buf));

For array variables defined in the function itself, isn't this usually
done with:

         int ret = 0;
-        u8 buf[ETH_ALEN];
+        u8 buf[ETH_ALEN] = {0};
         int i;
         unsigned long gpio_bits = dev->driver_info->data;

eg make the compiler do it (though maybe it's smart enough to elide the
memset, I don't know). See drivers/net/ethernet/intel/igb/e1000_mac.c
for an example.

Dan

>  	usbnet_get_endpoints(dev,intf);
>  
>  	/* Toggle the GPIOs in a manufacturer/model specific way */
> @@ -681,6 +682,7 @@ static int ax88772_bind(struct usbnet *dev,
> struct usb_interface *intf)
>  	u32 phyid;
>  	struct asix_common_private *priv;
>  
> +	memset(buf, 0, sizeof(buf));
>  	usbnet_get_endpoints(dev, intf);
>  
>  	/* Maybe the boot loader passed the MAC address via device tree
> */
> @@ -1063,6 +1065,7 @@ static int ax88178_bind(struct usbnet *dev,
> struct usb_interface *intf)
>  	int ret;
>  	u8 buf[ETH_ALEN];
>  
> +	memset(buf, 0, sizeof(buf));
>  	usbnet_get_endpoints(dev,intf);
>  
>  	/* Get the MAC address */


^ permalink raw reply

* Re: [PATCH net-next v4 3/5] net: core: add MPLS update core helper and use in OvS
From: Willem de Bruijn @ 2019-07-01 15:28 UTC (permalink / raw)
  To: John Hurley
  Cc: Network Development, David Miller, jiri, Cong Wang, David Ahern,
	simon.horman, Jakub Kicinski, oss-drivers
In-Reply-To: <1561984257-9798-4-git-send-email-john.hurley@netronome.com>

On Mon, Jul 1, 2019 at 8:31 AM John Hurley <john.hurley@netronome.com> wrote:
>
> Open vSwitch allows the updating of an existing MPLS header on a packet.
> In preparation for supporting similar functionality in TC, move this to a
> common skb helper function.
>
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
>  /**
> + * skb_mpls_update_lse() - modify outermost MPLS header and update csum
> + *
> + * @skb: buffer
> + * @mpls_lse: new MPLS label stack entry to update to
> + *
> + * Expects skb->data at mac header.
> + *
> + * Returns 0 on success, -errno otherwise.
> + */
> +int skb_mpls_update_lse(struct sk_buff *skb, __be32 mpls_lse)
> +{
> +       struct mpls_shim_hdr *old_lse = mpls_hdr(skb);
> +       int err;
> +
> +       if (unlikely(!eth_p_mpls(skb->protocol)))
> +               return -EINVAL;
> +
> +       err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN);
> +       if (unlikely(err))
> +               return err;
> +
> +       if (skb->ip_summed == CHECKSUM_COMPLETE) {
> +               __be32 diff[] = { ~old_lse->label_stack_entry, mpls_lse };
> +
> +               skb->csum = csum_partial((char *)diff, sizeof(diff), skb->csum);
> +       }
> +
> +       old_lse->label_stack_entry = mpls_lse;

skb_ensure_writable may have reallocated the skb linear. old_lse needs
to be loaded after. Or, safer:

  mpls_hdr(skb)->label_stack_entry = mpls_lse;

^ permalink raw reply

* [PATCH 2/4] net: dsa: vsc73xx: Split vsc73xx driver
From: Pawel Dembicki @ 2019-07-01 15:27 UTC (permalink / raw)
  Cc: linus.walleij, paweldembicki, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, David S. Miller, Rob Herring, Mark Rutland,
	netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-1-paweldembicki@gmail.com>

This driver (currently) only takes control of the switch chip over
SPI and configures it to route packages around when connected to a
CPU port. But Vitesse chip support also parallel interface.

This patch split driver into two parts: core and spi. It is required
for add support to another managing interface.

Tested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 arch/arm/boot/dts/gemini-sl93512r.dts         |   2 +-
 arch/arm/boot/dts/gemini-sq201.dts            |   2 +-
 drivers/net/dsa/Kconfig                       |  11 +-
 drivers/net/dsa/Makefile                      |   3 +-
 ...tesse-vsc73xx.c => vitesse-vsc73xx-core.c} | 265 ++++--------------
 drivers/net/dsa/vitesse-vsc73xx-spi.c         | 201 +++++++++++++
 drivers/net/dsa/vitesse-vsc73xx.h             |  30 ++
 7 files changed, 297 insertions(+), 217 deletions(-)
 rename drivers/net/dsa/{vitesse-vsc73xx.c => vitesse-vsc73xx-core.c} (85%)
 create mode 100644 drivers/net/dsa/vitesse-vsc73xx-spi.c
 create mode 100644 drivers/net/dsa/vitesse-vsc73xx.h

diff --git a/arch/arm/boot/dts/gemini-sl93512r.dts b/arch/arm/boot/dts/gemini-sl93512r.dts
index 2bb953440793..87f5340387a4 100644
--- a/arch/arm/boot/dts/gemini-sl93512r.dts
+++ b/arch/arm/boot/dts/gemini-sl93512r.dts
@@ -94,7 +94,7 @@
 		num-chipselects = <1>;
 
 		switch@0 {
-			compatible = "vitesse,vsc7385";
+			compatible = "vitesse,vsc7385-spi";
 			reg = <0>;
 			/* Specified for 2.5 MHz or below */
 			spi-max-frequency = <2500000>;
diff --git a/arch/arm/boot/dts/gemini-sq201.dts b/arch/arm/boot/dts/gemini-sq201.dts
index 239dfacaae4d..4fcb20f87ba0 100644
--- a/arch/arm/boot/dts/gemini-sq201.dts
+++ b/arch/arm/boot/dts/gemini-sq201.dts
@@ -79,7 +79,7 @@
 		num-chipselects = <1>;
 
 		switch@0 {
-			compatible = "vitesse,vsc7395";
+			compatible = "vitesse,vsc7395-spi";
 			reg = <0>;
 			/* Specified for 2.5 MHz or below */
 			spi-max-frequency = <2500000>;
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index b91e78e3598f..4ab2aa09e2e4 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -99,8 +99,8 @@ config NET_DSA_SMSC_LAN9303_MDIO
 	  for MDIO managed mode.
 
 config NET_DSA_VITESSE_VSC73XX
-	tristate "Vitesse VSC7385/7388/7395/7398 support"
-	depends on OF && SPI
+	tristate
+	depends on OF
 	depends on NET_DSA
 	select FIXED_PHY
 	select VITESSE_PHY
@@ -109,4 +109,11 @@ config NET_DSA_VITESSE_VSC73XX
 	  This enables support for the Vitesse VSC7385, VSC7388,
 	  VSC7395 and VSC7398 SparX integrated ethernet switches.
 
+config NET_DSA_VITESSE_VSC73XX_SPI
+	tristate "Vitesse VSC7385/7388/7395/7398 SPI mode support"
+	depends on SPI
+	select NET_DSA_VITESSE_VSC73XX
+	---help---
+	  This enables support for the Vitesse VSC7385, VSC7388, VSC7395
+	  and VSC7398 SparX integrated ethernet switches in SPI managed mode.
 endmenu
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index fefb6aaa82ba..117bf78be211 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -14,7 +14,8 @@ realtek-objs			:= realtek-smi.o rtl8366.o rtl8366rb.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303_I2C) += lan9303_i2c.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
-obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx.o
+obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
+obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
 obj-y				+= b53/
 obj-y				+= microchip/
 obj-y				+= mv88e6xxx/
diff --git a/drivers/net/dsa/vitesse-vsc73xx.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
similarity index 85%
rename from drivers/net/dsa/vitesse-vsc73xx.c
rename to drivers/net/dsa/vitesse-vsc73xx-core.c
index d4780610ea8a..9975446cdc66 100644
--- a/drivers/net/dsa/vitesse-vsc73xx.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -10,10 +10,6 @@
  * handling the switch in a memory-mapped manner by connecting to that external
  * CPU's memory bus.
  *
- * This driver (currently) only takes control of the switch chip over SPI and
- * configures it to route packages around when connected to a CPU port. The
- * chip has embedded PHYs and VLAN support so we model it using DSA.
- *
  * Copyright (C) 2018 Linus Wallej <linus.walleij@linaro.org>
  * Includes portions of code from the firmware uploader by:
  * Copyright (C) 2009 Gabor Juhos <juhosg@openwrt.org>
@@ -24,8 +20,6 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
-#include <linux/platform_device.h>
-#include <linux/spi/spi.h>
 #include <linux/bitops.h>
 #include <linux/if_bridge.h>
 #include <linux/etherdevice.h>
@@ -34,6 +28,8 @@
 #include <linux/random.h>
 #include <net/dsa.h>
 
+#include "vitesse-vsc73xx.h"
+
 #define VSC73XX_BLOCK_MAC	0x1 /* Subblocks 0-4, 6 (CPU port) */
 #define VSC73XX_BLOCK_ANALYZER	0x2 /* Only subblock 0 */
 #define VSC73XX_BLOCK_MII	0x3 /* Subblocks 0 and 1 */
@@ -255,13 +251,6 @@
 #define VSC73XX_GLORESET_PHY_RESET	BIT(1)
 #define VSC73XX_GLORESET_MASTER_RESET	BIT(0)
 
-#define VSC73XX_CMD_MODE_READ		0
-#define VSC73XX_CMD_MODE_WRITE		1
-#define VSC73XX_CMD_MODE_SHIFT		4
-#define VSC73XX_CMD_BLOCK_SHIFT		5
-#define VSC73XX_CMD_BLOCK_MASK		0x7
-#define VSC73XX_CMD_SUBBLOCK_MASK	0xf
-
 #define VSC7385_CLOCK_DELAY		((3 << 4) | 3)
 #define VSC7385_CLOCK_DELAY_MASK	((3 << 4) | 3)
 
@@ -274,20 +263,6 @@
 				 VSC73XX_ICPU_CTRL_CLK_EN | \
 				 VSC73XX_ICPU_CTRL_SRST)
 
-/**
- * struct vsc73xx - VSC73xx state container
- */
-struct vsc73xx {
-	struct device		*dev;
-	struct gpio_desc	*reset;
-	struct spi_device	*spi;
-	struct dsa_switch	*ds;
-	struct gpio_chip	gc;
-	u16			chipid;
-	u8			addr[ETH_ALEN];
-	struct mutex		lock; /* Protects SPI traffic */
-};
-
 #define IS_7385(a) ((a)->chipid == VSC73XX_CHIPID_ID_7385)
 #define IS_7388(a) ((a)->chipid == VSC73XX_CHIPID_ID_7388)
 #define IS_7395(a) ((a)->chipid == VSC73XX_CHIPID_ID_7395)
@@ -365,7 +340,7 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = {
 	{ 29, "TxQoSClass3" }, /* non-standard counter */
 };
 
-static int vsc73xx_is_addr_valid(u8 block, u8 subblock)
+int vsc73xx_is_addr_valid(u8 block, u8 subblock)
 {
 	switch (block) {
 	case VSC73XX_BLOCK_MAC:
@@ -396,97 +371,7 @@ static int vsc73xx_is_addr_valid(u8 block, u8 subblock)
 
 	return 0;
 }
-
-static u8 vsc73xx_make_addr(u8 mode, u8 block, u8 subblock)
-{
-	u8 ret;
-
-	ret = (block & VSC73XX_CMD_BLOCK_MASK) << VSC73XX_CMD_BLOCK_SHIFT;
-	ret |= (mode & 1) << VSC73XX_CMD_MODE_SHIFT;
-	ret |= subblock & VSC73XX_CMD_SUBBLOCK_MASK;
-
-	return ret;
-}
-
-static int vsc73xx_read(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
-			u32 *val)
-{
-	struct spi_transfer t[2];
-	struct spi_message m;
-	u8 cmd[4];
-	u8 buf[4];
-	int ret;
-
-	if (!vsc73xx_is_addr_valid(block, subblock))
-		return -EINVAL;
-
-	spi_message_init(&m);
-
-	memset(&t, 0, sizeof(t));
-
-	t[0].tx_buf = cmd;
-	t[0].len = sizeof(cmd);
-	spi_message_add_tail(&t[0], &m);
-
-	t[1].rx_buf = buf;
-	t[1].len = sizeof(buf);
-	spi_message_add_tail(&t[1], &m);
-
-	cmd[0] = vsc73xx_make_addr(VSC73XX_CMD_MODE_READ, block, subblock);
-	cmd[1] = reg;
-	cmd[2] = 0;
-	cmd[3] = 0;
-
-	mutex_lock(&vsc->lock);
-	ret = spi_sync(vsc->spi, &m);
-	mutex_unlock(&vsc->lock);
-
-	if (ret)
-		return ret;
-
-	*val = (buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3];
-
-	return 0;
-}
-
-static int vsc73xx_write(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
-			 u32 val)
-{
-	struct spi_transfer t[2];
-	struct spi_message m;
-	u8 cmd[2];
-	u8 buf[4];
-	int ret;
-
-	if (!vsc73xx_is_addr_valid(block, subblock))
-		return -EINVAL;
-
-	spi_message_init(&m);
-
-	memset(&t, 0, sizeof(t));
-
-	t[0].tx_buf = cmd;
-	t[0].len = sizeof(cmd);
-	spi_message_add_tail(&t[0], &m);
-
-	t[1].tx_buf = buf;
-	t[1].len = sizeof(buf);
-	spi_message_add_tail(&t[1], &m);
-
-	cmd[0] = vsc73xx_make_addr(VSC73XX_CMD_MODE_WRITE, block, subblock);
-	cmd[1] = reg;
-
-	buf[0] = (val >> 24) & 0xff;
-	buf[1] = (val >> 16) & 0xff;
-	buf[2] = (val >> 8) & 0xff;
-	buf[3] = val & 0xff;
-
-	mutex_lock(&vsc->lock);
-	ret = spi_sync(vsc->spi, &m);
-	mutex_unlock(&vsc->lock);
-
-	return ret;
-}
+EXPORT_SYMBOL(vsc73xx_is_addr_valid);
 
 static int vsc73xx_update_bits(struct vsc73xx *vsc, u8 block, u8 subblock,
 			       u8 reg, u32 mask, u32 val)
@@ -495,12 +380,12 @@ static int vsc73xx_update_bits(struct vsc73xx *vsc, u8 block, u8 subblock,
 	int ret;
 
 	/* Same read-modify-write algorithm as e.g. regmap */
-	ret = vsc73xx_read(vsc, block, subblock, reg, &orig);
+	ret = vsc->ops->read(vsc, block, subblock, reg, &orig);
 	if (ret)
 		return ret;
 	tmp = orig & ~mask;
 	tmp |= val & mask;
-	return vsc73xx_write(vsc, block, subblock, reg, tmp);
+	return vsc->ops->write(vsc, block, subblock, reg, tmp);
 }
 
 static int vsc73xx_detect(struct vsc73xx *vsc)
@@ -512,7 +397,7 @@ static int vsc73xx_detect(struct vsc73xx *vsc)
 	int ret;
 	u32 id;
 
-	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
+	ret = vsc->ops->read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
 			   VSC73XX_ICPU_MBOX_VAL, &val);
 	if (ret) {
 		dev_err(vsc->dev, "unable to read mailbox (%d)\n", ret);
@@ -530,7 +415,7 @@ static int vsc73xx_detect(struct vsc73xx *vsc)
 		/* Wait 20ms according to datasheet table 245 */
 		msleep(20);
 
-		ret = vsc73xx_read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
+		ret = vsc->ops->read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
 				   VSC73XX_ICPU_MBOX_VAL, &val);
 		if (val == 0xffffffff) {
 			dev_err(vsc->dev, "seems not to help, giving up\n");
@@ -538,7 +423,7 @@ static int vsc73xx_detect(struct vsc73xx *vsc)
 		}
 	}
 
-	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
+	ret = vsc->ops->read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
 			   VSC73XX_CHIPID, &val);
 	if (ret) {
 		dev_err(vsc->dev, "unable to read chip id (%d)\n", ret);
@@ -563,7 +448,7 @@ static int vsc73xx_detect(struct vsc73xx *vsc)
 		VSC73XX_CHIPID_REV_MASK;
 	dev_info(vsc->dev, "VSC%04X (rev: %d) switch found\n", id, rev);
 
-	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
+	ret = vsc->ops->read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
 			   VSC73XX_ICPU_CTRL, &val);
 	if (ret) {
 		dev_err(vsc->dev, "unable to read iCPU control\n");
@@ -611,11 +496,11 @@ static int vsc73xx_phy_read(struct dsa_switch *ds, int phy, int regnum)
 
 	/* Setting bit 26 means "read" */
 	cmd = BIT(26) | (phy << 21) | (regnum << 16);
-	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, 0, 1, cmd);
+	ret = vsc->ops->write(vsc, VSC73XX_BLOCK_MII, 0, 1, cmd);
 	if (ret)
 		return ret;
 	msleep(2);
-	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MII, 0, 2, &val);
+	ret = vsc->ops->read(vsc, VSC73XX_BLOCK_MII, 0, 2, &val);
 	if (ret)
 		return ret;
 	if (val & BIT(16)) {
@@ -650,7 +535,7 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
 	}
 
 	cmd = (phy << 21) | (regnum << 16);
-	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_MII, 0, 1, cmd);
+	ret = vsc->ops->write(vsc, VSC73XX_BLOCK_MII, 0, 1, cmd);
 	if (ret)
 		return ret;
 
@@ -682,7 +567,7 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 	dev_info(vsc->dev, "set up the switch\n");
 
 	/* Issue RESET */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
 		      VSC73XX_GLORESET_MASTER_RESET);
 	usleep_range(125, 200);
 
@@ -695,7 +580,7 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 	 */
 	for (i = 0; i <= 15; i++) {
 		if (i != 6 && i != 7) {
-			vsc73xx_write(vsc, VSC73XX_BLOCK_MEMINIT,
+			vsc->ops->write(vsc, VSC73XX_BLOCK_MEMINIT,
 				      2,
 				      0, 0x1010400 + i);
 			mdelay(1);
@@ -704,12 +589,12 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 	mdelay(30);
 
 	/* Clear MAC table */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
 		      VSC73XX_MACACCESS,
 		      VSC73XX_MACACCESS_CMD_CLEAR_TABLE);
 
 	/* Clear VLAN table */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
 		      VSC73XX_VLANACCESS,
 		      VSC73XX_VLANACCESS_VLAN_TBL_CMD_CLEAR_TABLE);
 
@@ -721,7 +606,7 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 	 * Port "31" is "all ports".
 	 */
 	if (IS_739X(vsc))
-		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 0x1f,
+		vsc->ops->write(vsc, VSC73XX_BLOCK_MAC, 0x1f,
 			      VSC73XX_Q_MISC_CONF,
 			      VSC73XX_Q_MISC_CONF_EXTENT_MEM);
 
@@ -729,25 +614,25 @@ static int vsc73xx_setup(struct dsa_switch *ds)
 	for (i = 0; i < 7; i++) {
 		if (i == 5)
 			continue;
-		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, 4,
+		vsc->ops->write(vsc, VSC73XX_BLOCK_MAC, 4,
 			      VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
 	}
 
 	/* MII delay, set both GTX and RX delay to 2 ns */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY,
 		      VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS |
 		      VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS);
 	/* Enable reception of frames on all ports */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_RECVMASK,
 		      0x5f);
 	/* IP multicast flood mask (table 144) */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK,
 		      0xff);
 
 	mdelay(50);
 
 	/* Release reset from the internal PHYs */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GLORESET,
 		      VSC73XX_GLORESET_PHY_RESET);
 
 	udelay(4);
@@ -760,7 +645,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
 	u32 val;
 
 	/* MAC configure, first reset the port and then write defaults */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port,
 		      VSC73XX_MAC_CFG,
 		      VSC73XX_MAC_CFG_RESET);
@@ -774,7 +659,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
 	else
 		val = VSC73XX_MAC_CFG_1000M_F_PHY;
 
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port,
 		      VSC73XX_MAC_CFG,
 		      val |
@@ -787,7 +672,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
 	 * frames, so just enable it. It is clear from the application note
 	 * that "9.6 kilobytes" == 9600 bytes.
 	 */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port,
 		      VSC73XX_MAXLEN, 9600);
 
@@ -795,7 +680,7 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
 	 * Use a zero delay pause frame when pause condition is left
 	 * Obey pause control frames
 	 */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port,
 		      VSC73XX_FCCONF,
 		      VSC73XX_FCCONF_ZERO_PAUSE_EN |
@@ -811,19 +696,19 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
 	else
 		val = VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE;
 	val |= VSC73XX_Q_MISC_CONF_EXTENT_MEM;
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port,
 		      VSC73XX_Q_MISC_CONF,
 		      val);
 
 	/* Flow control MAC: a MAC address used in flow control frames */
 	val = (vsc->addr[5] << 16) | (vsc->addr[4] << 8) | (vsc->addr[3]);
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port,
 		      VSC73XX_FCMACHI,
 		      val);
 	val = (vsc->addr[2] << 16) | (vsc->addr[1] << 8) | (vsc->addr[0]);
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port,
 		      VSC73XX_FCMACLO,
 		      val);
@@ -831,13 +716,13 @@ static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
 	/* Tell the categorizer to forward pause frames, not control
 	 * frame. Do not drop anything.
 	 */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port,
 		      VSC73XX_CAT_DROP,
 		      VSC73XX_CAT_DROP_FWD_PAUSE_ENA);
 
 	/* Clear all counters */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 		      port, VSC73XX_C_RX0, 0);
 }
 
@@ -850,21 +735,21 @@ static void vsc73xx_adjust_enable_port(struct vsc73xx *vsc,
 
 	/* Reset this port FIXME: break out subroutine */
 	val |= VSC73XX_MAC_CFG_RESET;
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, val);
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, val);
 
 	/* Seed the port randomness with randomness */
 	get_random_bytes(&seed, 1);
 	val |= seed << VSC73XX_MAC_CFG_SEED_OFFSET;
 	val |= VSC73XX_MAC_CFG_SEED_LOAD;
 	val |= VSC73XX_MAC_CFG_WEXC_DIS;
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, val);
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, val);
 
 	/* Flow control for the PHY facing ports:
 	 * Use a zero delay pause frame when pause condition is left
 	 * Obey pause control frames
 	 * When generating pause frames, use 0xff as pause value
 	 */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_FCCONF,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_FCCONF,
 		      VSC73XX_FCCONF_ZERO_PAUSE_EN |
 		      VSC73XX_FCCONF_FLOW_CTRL_OBEY |
 		      0xff);
@@ -895,7 +780,7 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
 		 * Enable the GMII GTX external clock
 		 * Use double data rate (DDR mode)
 		 */
-		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC,
+		vsc->ops->write(vsc, VSC73XX_BLOCK_MAC,
 			      CPU_PORT,
 			      VSC73XX_ADVPORTM,
 			      VSC73XX_ADVPORTM_EXT_PORT |
@@ -922,11 +807,11 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
 				    VSC73XX_ARBDISC, BIT(port), BIT(port));
 
 		/* Wait until queue is empty */
-		vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
+		vsc->ops->read(vsc, VSC73XX_BLOCK_ARBITER, 0,
 			     VSC73XX_ARBEMPTY, &val);
 		while (!(val & BIT(port))) {
 			msleep(1);
-			vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
+			vsc->ops->read(vsc, VSC73XX_BLOCK_ARBITER, 0,
 				     VSC73XX_ARBEMPTY, &val);
 			if (--maxloop == 0) {
 				dev_err(vsc->dev,
@@ -937,7 +822,7 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
 		}
 
 		/* Put this port into reset */
-		vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
+		vsc->ops->write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
 			      VSC73XX_MAC_CFG_RESET);
 
 		/* Accept packets again */
@@ -1018,7 +903,7 @@ static void vsc73xx_port_disable(struct dsa_switch *ds, int port)
 	struct vsc73xx *vsc = ds->priv;
 
 	/* Just put the port into reset */
-	vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port,
+	vsc->ops->write(vsc, VSC73XX_BLOCK_MAC, port,
 		      VSC73XX_MAC_CFG, VSC73XX_MAC_CFG_RESET);
 }
 
@@ -1063,7 +948,7 @@ static void vsc73xx_get_strings(struct dsa_switch *ds, int port, u32 stringset,
 	if (stringset != ETH_SS_STATS)
 		return;
 
-	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
+	ret = vsc->ops->read(vsc, VSC73XX_BLOCK_MAC, port,
 			   VSC73XX_C_CFG, &val);
 	if (ret)
 		return;
@@ -1137,7 +1022,7 @@ static void vsc73xx_get_ethtool_stats(struct dsa_switch *ds, int port,
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(regs); i++) {
-		ret = vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
+		ret = vsc->ops->read(vsc, VSC73XX_BLOCK_MAC, port,
 				   regs[i], &val);
 		if (ret) {
 			dev_err(vsc->dev, "error reading counter %d\n", i);
@@ -1166,7 +1051,7 @@ static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	u32 val;
 	int ret;
 
-	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
+	ret = vsc->ops->read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
 			   VSC73XX_GPIO, &val);
 	if (ret)
 		return ret;
@@ -1212,7 +1097,7 @@ static int vsc73xx_gpio_get_direction(struct gpio_chip *chip,
 	u32 val;
 	int ret;
 
-	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
+	ret = vsc->ops->read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
 			   VSC73XX_GPIO, &val);
 	if (ret)
 		return ret;
@@ -1245,42 +1130,25 @@ static int vsc73xx_gpio_probe(struct vsc73xx *vsc)
 	return 0;
 }
 
-static int vsc73xx_probe(struct spi_device *spi)
+int vsc73xx_probe(struct vsc73xx *vsc)
 {
-	struct device *dev = &spi->dev;
-	struct vsc73xx *vsc;
 	int ret;
 
-	vsc = devm_kzalloc(dev, sizeof(*vsc), GFP_KERNEL);
-	if (!vsc)
-		return -ENOMEM;
-
-	spi_set_drvdata(spi, vsc);
-	vsc->spi = spi_dev_get(spi);
-	vsc->dev = dev;
 	mutex_init(&vsc->lock);
 
 	/* Release reset, if any */
-	vsc->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	vsc->reset = devm_gpiod_get_optional(vsc->dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(vsc->reset)) {
-		dev_err(dev, "failed to get RESET GPIO\n");
+		dev_err(vsc->dev, "failed to get RESET GPIO\n");
 		return PTR_ERR(vsc->reset);
 	}
 	if (vsc->reset)
 		/* Wait 20ms according to datasheet table 245 */
 		msleep(20);
 
-	spi->mode = SPI_MODE_0;
-	spi->bits_per_word = 8;
-	ret = spi_setup(spi);
-	if (ret < 0) {
-		dev_err(dev, "spi setup failed.\n");
-		return ret;
-	}
-
 	ret = vsc73xx_detect(vsc);
 	if (ret) {
-		dev_err(dev, "no chip found (%d)\n", ret);
+		dev_err(vsc->dev, "no chip found (%d)\n", ret);
 		return -ENODEV;
 	}
 
@@ -1301,7 +1169,7 @@ static int vsc73xx_probe(struct spi_device *spi)
 	 * We allocate 8 ports and avoid access to the nonexistant
 	 * ports.
 	 */
-	vsc->ds = dsa_switch_alloc(dev, 8);
+	vsc->ds = dsa_switch_alloc(vsc->dev, 8);
 	if (!vsc->ds)
 		return -ENOMEM;
 	vsc->ds->priv = vsc;
@@ -1309,7 +1177,7 @@ static int vsc73xx_probe(struct spi_device *spi)
 	vsc->ds->ops = &vsc73xx_ds_ops;
 	ret = dsa_register_switch(vsc->ds);
 	if (ret) {
-		dev_err(dev, "unable to register switch (%d)\n", ret);
+		dev_err(vsc->dev, "unable to register switch (%d)\n", ret);
 		return ret;
 	}
 
@@ -1321,43 +1189,16 @@ static int vsc73xx_probe(struct spi_device *spi)
 
 	return 0;
 }
+EXPORT_SYMBOL(vsc73xx_probe);
 
-static int vsc73xx_remove(struct spi_device *spi)
+int vsc73xx_remove(struct vsc73xx *vsc)
 {
-	struct vsc73xx *vsc = spi_get_drvdata(spi);
-
 	dsa_unregister_switch(vsc->ds);
 	gpiod_set_value(vsc->reset, 1);
 
 	return 0;
 }
-
-static const struct of_device_id vsc73xx_of_match[] = {
-	{
-		.compatible = "vitesse,vsc7385",
-	},
-	{
-		.compatible = "vitesse,vsc7388",
-	},
-	{
-		.compatible = "vitesse,vsc7395",
-	},
-	{
-		.compatible = "vitesse,vsc7398",
-	},
-	{ },
-};
-MODULE_DEVICE_TABLE(of, vsc73xx_of_match);
-
-static struct spi_driver vsc73xx_driver = {
-	.probe = vsc73xx_probe,
-	.remove = vsc73xx_remove,
-	.driver = {
-		.name = "vsc73xx",
-		.of_match_table = vsc73xx_of_match,
-	},
-};
-module_spi_driver(vsc73xx_driver);
+EXPORT_SYMBOL(vsc73xx_remove);
 
 MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
 MODULE_DESCRIPTION("Vitesse VSC7385/7388/7395/7398 driver");
diff --git a/drivers/net/dsa/vitesse-vsc73xx-spi.c b/drivers/net/dsa/vitesse-vsc73xx-spi.c
new file mode 100644
index 000000000000..dab8c0e69cf9
--- /dev/null
+++ b/drivers/net/dsa/vitesse-vsc73xx-spi.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: GPL-2.0
+/* DSA driver for:
+ * Vitesse VSC7385 SparX-G5 5+1-port Integrated Gigabit Ethernet Switch
+ * Vitesse VSC7388 SparX-G8 8-port Integrated Gigabit Ethernet Switch
+ * Vitesse VSC7395 SparX-G5e 5+1-port Integrated Gigabit Ethernet Switch
+ * Vitesse VSC7398 SparX-G8e 8-port Integrated Gigabit Ethernet Switch
+ *
+ * This driver takes control of the switch chip over SPI and
+ * configures it to route packages around when connected to a CPU port.
+ *
+ * Copyright (C) 2018 Linus Wallej <linus.walleij@linaro.org>
+ * Includes portions of code from the firmware uploader by:
+ * Copyright (C) 2009 Gabor Juhos <juhosg@openwrt.org>
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/spi/spi.h>
+
+#include "vitesse-vsc73xx.h"
+
+#define VSC73XX_CMD_SPI_MODE_READ		0
+#define VSC73XX_CMD_SPI_MODE_WRITE		1
+#define VSC73XX_CMD_SPI_MODE_SHIFT		4
+#define VSC73XX_CMD_SPI_BLOCK_SHIFT		5
+#define VSC73XX_CMD_SPI_BLOCK_MASK		0x7
+#define VSC73XX_CMD_SPI_SUBBLOCK_MASK	0xf
+
+/**
+ * struct vsc73xx_spi - VSC73xx SPI state container
+ */
+struct vsc73xx_spi {
+	struct spi_device	*spi;
+	struct vsc73xx vsc;
+};
+
+static const struct vsc73xx_ops vsc73xx_spi_ops;
+
+static u8 vsc73xx_make_addr(u8 mode, u8 block, u8 subblock)
+{
+	u8 ret;
+
+	ret = (block & VSC73XX_CMD_SPI_BLOCK_MASK)
+					<< VSC73XX_CMD_SPI_BLOCK_SHIFT;
+	ret |= (mode & 1) << VSC73XX_CMD_SPI_MODE_SHIFT;
+	ret |= subblock & VSC73XX_CMD_SPI_SUBBLOCK_MASK;
+
+	return ret;
+}
+
+static int vsc73xx_spi_read(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
+			    u32 *val)
+{
+	struct vsc73xx_spi *vsc_spi = vsc->priv;
+	struct spi_transfer t[2];
+	struct spi_message m;
+	u8 cmd[4];
+	u8 buf[4];
+	int ret;
+
+	if (!vsc73xx_is_addr_valid(block, subblock))
+		return -EINVAL;
+
+	spi_message_init(&m);
+
+	memset(&t, 0, sizeof(t));
+
+	t[0].tx_buf = cmd;
+	t[0].len = sizeof(cmd);
+	spi_message_add_tail(&t[0], &m);
+
+	t[1].rx_buf = buf;
+	t[1].len = sizeof(buf);
+	spi_message_add_tail(&t[1], &m);
+
+	cmd[0] = vsc73xx_make_addr(VSC73XX_CMD_SPI_MODE_READ, block, subblock);
+	cmd[1] = reg;
+	cmd[2] = 0;
+	cmd[3] = 0;
+
+	mutex_lock(&vsc->lock);
+	ret = spi_sync(vsc_spi->spi, &m);
+	mutex_unlock(&vsc->lock);
+
+	if (ret)
+		return ret;
+
+	*val = (buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3];
+
+	return 0;
+}
+
+static int vsc73xx_spi_write(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
+			     u32 val)
+{
+	struct vsc73xx_spi *vsc_spi = vsc->priv;
+	struct spi_transfer t[2];
+	struct spi_message m;
+	u8 cmd[2];
+	u8 buf[4];
+	int ret;
+
+	if (!vsc73xx_is_addr_valid(block, subblock))
+		return -EINVAL;
+
+	spi_message_init(&m);
+
+	memset(&t, 0, sizeof(t));
+
+	t[0].tx_buf = cmd;
+	t[0].len = sizeof(cmd);
+	spi_message_add_tail(&t[0], &m);
+
+	t[1].tx_buf = buf;
+	t[1].len = sizeof(buf);
+	spi_message_add_tail(&t[1], &m);
+
+	cmd[0] = vsc73xx_make_addr(VSC73XX_CMD_SPI_MODE_WRITE, block, subblock);
+	cmd[1] = reg;
+
+	buf[0] = (val >> 24) & 0xff;
+	buf[1] = (val >> 16) & 0xff;
+	buf[2] = (val >> 8) & 0xff;
+	buf[3] = val & 0xff;
+
+	mutex_lock(&vsc->lock);
+	ret = spi_sync(vsc_spi->spi, &m);
+	mutex_unlock(&vsc->lock);
+
+	return ret;
+}
+
+static int vsc73xx_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct vsc73xx_spi *vsc_spi;
+	int ret;
+
+	vsc_spi = devm_kzalloc(dev, sizeof(*vsc_spi), GFP_KERNEL);
+	if (!vsc_spi)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, vsc_spi);
+	vsc_spi->spi = spi_dev_get(spi);
+	vsc_spi->vsc.dev = dev;
+	vsc_spi->vsc.priv = vsc_spi;
+	vsc_spi->vsc.ops = &vsc73xx_spi_ops;
+
+	spi->mode = SPI_MODE_0;
+	spi->bits_per_word = 8;
+	ret = spi_setup(spi);
+	if (ret < 0) {
+		dev_err(dev, "spi setup failed.\n");
+		return ret;
+	}
+
+	return vsc73xx_probe(&vsc_spi->vsc);
+}
+
+static int vsc73xx_spi_remove(struct spi_device *spi)
+{
+	struct vsc73xx_spi *vsc_spi = spi_get_drvdata(spi);
+
+	return vsc73xx_remove(&vsc_spi->vsc);
+}
+
+static const struct vsc73xx_ops vsc73xx_spi_ops = {
+	.read = vsc73xx_spi_read,
+	.write = vsc73xx_spi_write,
+};
+
+static const struct of_device_id vsc73xx_spi_of_match[] = {
+	{
+		.compatible = "vitesse,vsc7385-spi",
+	},
+	{
+		.compatible = "vitesse,vsc7388-spi",
+	},
+	{
+		.compatible = "vitesse,vsc7395-spi",
+	},
+	{
+		.compatible = "vitesse,vsc7398-spi",
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, vsc73xx_spi_of_match);
+
+static struct spi_driver vsc73xx_spi_driver = {
+	.probe = vsc73xx_spi_probe,
+	.remove = vsc73xx_spi_remove,
+	.driver = {
+		.name = "vsc73xx-spi",
+		.of_match_table = vsc73xx_spi_of_match,
+	},
+};
+module_spi_driver(vsc73xx_spi_driver);
+
+MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>");
+MODULE_DESCRIPTION("Vitesse VSC7385/7388/7395/7398 SPI driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
new file mode 100644
index 000000000000..a0e2e5b8f0ce
--- /dev/null
+++ b/drivers/net/dsa/vitesse-vsc73xx.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/device.h>
+#include <linux/etherdevice.h>
+#include <linux/gpio/driver.h>
+
+/**
+ * struct vsc73xx - VSC73xx state container
+ */
+struct vsc73xx {
+	struct device		*dev;
+	struct gpio_desc	*reset;
+	struct dsa_switch	*ds;
+	struct gpio_chip	gc;
+	u16			chipid;
+	u8			addr[ETH_ALEN];
+	struct mutex		lock; /* Protects SPI traffic */
+	const struct vsc73xx_ops *ops;
+	void *priv;
+};
+
+struct vsc73xx_ops {
+	int (*read)(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
+		    u32 *val);
+	int (*write)(struct vsc73xx *vsc, u8 block, u8 subblock, u8 reg,
+		     u32 val);
+};
+
+int vsc73xx_is_addr_valid(u8 block, u8 subblock);
+int vsc73xx_probe(struct vsc73xx *vsc);
+int vsc73xx_remove(struct vsc73xx *vsc);
-- 
2.20.1


^ permalink raw reply related

* [PATCH 4/4] net: dsa: vsc73xx: Assert reset if iCPU is enabled
From: Pawel Dembicki @ 2019-07-01 15:27 UTC (permalink / raw)
  Cc: linus.walleij, paweldembicki, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, David S. Miller, Rob Herring, Mark Rutland,
	netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-1-paweldembicki@gmail.com>

Driver allow to use devices with disabled iCPU only.

Some devices have pre-initialised iCPU by bootloader.
That state make switch unmanaged. This patch force reset
if device is in unmanaged state. In the result chip lost
internal firmware from RAM and it can be managed.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/vitesse-vsc73xx-core.c | 36 ++++++++++++--------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 9975446cdc66..5cdf91849b5d 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -405,22 +405,8 @@ static int vsc73xx_detect(struct vsc73xx *vsc)
 	}
 
 	if (val == 0xffffffff) {
-		dev_info(vsc->dev, "chip seems dead, assert reset\n");
-		gpiod_set_value_cansleep(vsc->reset, 1);
-		/* Reset pulse should be 20ns minimum, according to datasheet
-		 * table 245, so 10us should be fine
-		 */
-		usleep_range(10, 100);
-		gpiod_set_value_cansleep(vsc->reset, 0);
-		/* Wait 20ms according to datasheet table 245 */
-		msleep(20);
-
-		ret = vsc->ops->read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
-				   VSC73XX_ICPU_MBOX_VAL, &val);
-		if (val == 0xffffffff) {
-			dev_err(vsc->dev, "seems not to help, giving up\n");
-			return -ENODEV;
-		}
+		dev_info(vsc->dev, "chip seems dead.\n");
+		return -EAGAIN;
 	}
 
 	ret = vsc->ops->read(vsc, VSC73XX_BLOCK_SYSTEM, 0,
@@ -471,9 +457,8 @@ static int vsc73xx_detect(struct vsc73xx *vsc)
 	}
 	if (icpu_si_boot_en && !icpu_pi_en) {
 		dev_err(vsc->dev,
-			"iCPU enabled boots from SI, no external memory\n");
-		dev_err(vsc->dev, "no idea how to deal with this\n");
-		return -ENODEV;
+			"iCPU enabled boots from PI/SI, no external memory\n");
+		return -EAGAIN;
 	}
 	if (!icpu_si_boot_en && icpu_pi_en) {
 		dev_err(vsc->dev,
@@ -1147,6 +1132,19 @@ int vsc73xx_probe(struct vsc73xx *vsc)
 		msleep(20);
 
 	ret = vsc73xx_detect(vsc);
+	if (ret == -EAGAIN) {
+		dev_err(vsc->dev,
+			"Chip seams to be out of control. Assert reset and try again.\n");
+		gpiod_set_value_cansleep(vsc->reset, 1);
+		/* Reset pulse should be 20ns minimum, according to datasheet
+		 * table 245, so 10us should be fine
+		 */
+		usleep_range(10, 100);
+		gpiod_set_value_cansleep(vsc->reset, 0);
+		/* Wait 20ms according to datasheet table 245 */
+		msleep(20);
+		ret = vsc73xx_detect(vsc);
+	}
 	if (ret) {
 		dev_err(vsc->dev, "no chip found (%d)\n", ret);
 		return -ENODEV;
-- 
2.20.1


^ permalink raw reply related

* [PATCH 3/4] net: dsa: vsc73xx: add support for parallel mode
From: Pawel Dembicki @ 2019-07-01 15:27 UTC (permalink / raw)
  Cc: linus.walleij, paweldembicki, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, David S. Miller, Rob Herring, Mark Rutland,
	netdev, devicetree, linux-kernel
In-Reply-To: <20190701152723.624-1-paweldembicki@gmail.com>

This patch add platform part of vsc73xx driver.
It allows to use chip connected by PI interface.

Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 drivers/net/dsa/Kconfig                    |   8 +
 drivers/net/dsa/Makefile                   |   1 +
 drivers/net/dsa/vitesse-vsc73xx-platform.c | 166 +++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 drivers/net/dsa/vitesse-vsc73xx-platform.c

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 4ab2aa09e2e4..80965808949d 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -116,4 +116,12 @@ config NET_DSA_VITESSE_VSC73XX_SPI
 	---help---
 	  This enables support for the Vitesse VSC7385, VSC7388, VSC7395
 	  and VSC7398 SparX integrated ethernet switches in SPI managed mode.
+
+config NET_DSA_VITESSE_VSC73XX_PLATFORM
+	tristate "Vitesse VSC7385/7388/7395/7398 Platform mode support"
+	depends on HAS_IOMEM
+	select NET_DSA_VITESSE_VSC73XX
+	---help---
+	  This enables support for the Vitesse VSC7385, VSC7388, VSC7395
+	  and VSC7398 SparX integrated ethernet switches in Platform managed mode.
 endmenu
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index 117bf78be211..d5e4c668ac03 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303_I2C) += lan9303_i2c.o
 obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
 obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
+obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
 obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
 obj-y				+= b53/
 obj-y				+= microchip/
diff --git a/drivers/net/dsa/vitesse-vsc73xx-platform.c b/drivers/net/dsa/vitesse-vsc73xx-platform.c
new file mode 100644
index 000000000000..b2e5da0ffde3
--- /dev/null
+++ b/drivers/net/dsa/vitesse-vsc73xx-platform.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+/* DSA driver for:
+ * Vitesse VSC7385 SparX-G5 5+1-port Integrated Gigabit Ethernet Switch
+ * Vitesse VSC7388 SparX-G8 8-port Integrated Gigabit Ethernet Switch
+ * Vitesse VSC7395 SparX-G5e 5+1-port Integrated Gigabit Ethernet Switch
+ * Vitesse VSC7398 SparX-G8e 8-port Integrated Gigabit Ethernet Switch
+ *
+ * This driver takes control of the switch chip over Platform and
+ * configures it to route packages around when connected to a CPU port.
+ *
+ * Copyright (C) 2019 pawel Dembicki <paweldembicki@gmail.com>
+ * Based on vitesse-vsc-spi.c by:
+ * Copyright (C) 2018 Linus Wallej <linus.walleij@linaro.org>
+ * Includes portions of code from the firmware uploader by:
+ * Copyright (C) 2009 Gabor Juhos <juhosg@openwrt.org>
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include "vitesse-vsc73xx.h"
+
+#define VSC73XX_CMD_PLATFORM_BLOCK_SHIFT		14
+#define VSC73XX_CMD_PLATFORM_BLOCK_MASK		0x7
+#define VSC73XX_CMD_PLATFORM_SUBBLOCK_SHIFT		10
+#define VSC73XX_CMD_PLATFORM_SUBBLOCK_MASK	0xf
+#define VSC73XX_CMD_PLATFORM_REGISTER_SHIFT		2
+
+/**
+ * struct vsc73xx_platform - VSC73xx Platform state container
+ */
+struct vsc73xx_platform {
+	struct platform_device *pdev;
+	void __iomem *base_addr;
+	struct vsc73xx vsc;
+};
+
+static const struct vsc73xx_ops vsc73xx_platform_ops;
+
+static u32 vsc73xx_make_addr(u8 block, u8 subblock, u8 reg)
+{
+	u32 ret;
+
+	ret = (block & VSC73XX_CMD_PLATFORM_BLOCK_MASK)
+		<< VSC73XX_CMD_PLATFORM_BLOCK_SHIFT;
+	ret |= (subblock & VSC73XX_CMD_PLATFORM_SUBBLOCK_MASK)
+		<< VSC73XX_CMD_PLATFORM_SUBBLOCK_SHIFT;
+	ret |= reg << VSC73XX_CMD_PLATFORM_REGISTER_SHIFT;
+
+	return ret;
+}
+
+static int vsc73xx_platform_read(struct vsc73xx *vsc, u8 block, u8 subblock,
+				 u8 reg, u32 *val)
+{
+	struct vsc73xx_platform *vsc_platform = vsc->priv;
+	u32 offset;
+
+	if (!vsc73xx_is_addr_valid(block, subblock))
+		return -EINVAL;
+
+	offset = vsc73xx_make_addr(block, subblock, reg);
+
+	mutex_lock(&vsc->lock);
+		*val = ioread32be(vsc_platform->base_addr + offset);
+	mutex_unlock(&vsc->lock);
+
+	return 0;
+}
+
+static int vsc73xx_platform_write(struct vsc73xx *vsc, u8 block, u8 subblock,
+				  u8 reg, u32 val)
+{
+	struct vsc73xx_platform *vsc_platform = vsc->priv;
+	u32 offset;
+
+	if (!vsc73xx_is_addr_valid(block, subblock))
+		return -EINVAL;
+
+	offset = vsc73xx_make_addr(block, subblock, reg);
+
+	mutex_lock(&vsc->lock);
+		iowrite32be(val, vsc_platform->base_addr + offset);
+	mutex_unlock(&vsc->lock);
+
+	return 0;
+}
+
+static int vsc73xx_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct vsc73xx_platform *vsc_platform;
+	struct resource *res = NULL;
+	int ret;
+
+	vsc_platform = devm_kzalloc(dev, sizeof(*vsc_platform), GFP_KERNEL);
+	if (!vsc_platform)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, vsc_platform);
+	vsc_platform->pdev = pdev;
+	vsc_platform->vsc.dev = dev;
+	vsc_platform->vsc.priv = vsc_platform;
+	vsc_platform->vsc.ops = &vsc73xx_platform_ops;
+
+	/* obtain I/O memory space */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot obtain I/O memory space\n");
+		ret = -ENXIO;
+		return ret;
+	}
+
+	vsc_platform->base_addr = devm_ioremap_resource(&pdev->dev, res);
+	if (!vsc_platform->base_addr) {
+		dev_err(&pdev->dev, "cannot request I/O memory space\n");
+		ret = -ENXIO;
+		return ret;
+	}
+
+	return vsc73xx_probe(&vsc_platform->vsc);
+}
+
+static int vsc73xx_platform_remove(struct platform_device *pdev)
+{
+	struct vsc73xx_platform *vsc_platform = platform_get_drvdata(pdev);
+
+	return vsc73xx_remove(&vsc_platform->vsc);
+}
+
+static const struct vsc73xx_ops vsc73xx_platform_ops = {
+	.read = vsc73xx_platform_read,
+	.write = vsc73xx_platform_write,
+};
+
+static const struct of_device_id vsc73xx_platform_of_match[] = {
+	{
+		.compatible = "vitesse,vsc7385-platform",
+	},
+	{
+		.compatible = "vitesse,vsc7388-platform",
+	},
+	{
+		.compatible = "vitesse,vsc7395-platform",
+	},
+	{
+		.compatible = "vitesse,vsc7398-platform",
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, vsc73xx_platform_of_match);
+
+static struct platform_driver vsc73xx_platform_driver = {
+	.probe = vsc73xx_platform_probe,
+	.remove = vsc73xx_platform_remove,
+	.driver = {
+		.name = "vsc73xx-platform",
+		.of_match_table = vsc73xx_platform_of_match,
+	},
+};
+module_platform_driver(vsc73xx_platform_driver);
+
+MODULE_AUTHOR("Pawel Dembicki <paweldembicki@gmail.com>");
+MODULE_DESCRIPTION("Vitesse VSC7385/7388/7395/7398 Platform driver");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1


^ 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