Fully agree on converging.
I missed Rafal's switchdev effort.
Before starting on this work, I first tried to look into net/dsa in
the Linux kernel.
I did notice your patches there and started on that.
I got to a decent point, but then I was stuck at some issues.
I later found out that I needed to adjust the MTU and manually pad
potential runt packets.
I also found out the switchdev API, which seems like it's in an rather
infant state.
In any case, I think DSA is the better approach, so my vote is for b53
+ DSA (or DSA-like) in the kernel.
For this swconfig effort, the reasoning is that our current firmware
is @ kernel 3.10, and most of our current target setup logic relies on
swconfig; I backported DSA, but because of time/effort constraints to
also port, test & validate any b53 PHY setup logic in DSA, we decided
that it's easier to adapt the current b53 driver, and later re-visit DSA.
And because we have time constraints, there's no telling if we'll have
the time to port b53 to DSA, so this patchset is at least meant to
share our effort/findings regarding the b53 + LAN ports + Broadcom tag.
If things get settled/more calm, I might try to do an effort to do b53
+ DSA (if nobody does that before me).
Regarding this patchset, I have no objections regarding whether it
gets accepted/dropped.
Either way, we'll keep it in our tree for a while, and re-evaluate any
other DSA (or DSA-like) frameworks later.
Thanks
Alex
On Thu, Feb 26, 2015 at 8:24 PM, Florian Fainelli
<[email protected] <mailto:[email protected]>> wrote:
On 25/02/15 07:24, Alexandru Ardelean wrote:
> Feature implemented and tested on BCM53128.
>
> Slave devices logic copied from the Linux kernel from Marvell's DSA
> driver ( linux/net/dsa/ ).
> Also the logic for the Broadcom tag processing has been copied
from there.
There are different efforts here going on, and I would like to at
least
3 different people (you, Rafal and myself) can converge to an
identical
solution that fits everybody here.
What net-next supports today is:
- broadcom tags in net/dsa/tag_brcm.c, 4-bytes format, identical to
yours AFAICT
- HW bridging support for bcm_sf2 (roboswitch successor)
What's missing:
- adding VLAN configuration, which is what Rafal has been doing using
here: http://thread.gmane.org/gmane.linux.network/351503
There are a number of things that I want to rework in DSA such that we
can almost immediately leverage OpenWrt's switch drivers, where the
entry point is a phy_driver, and have them register as switches
(DSA or
something wider) eventually [1], such that DSA handles the slave
devices
creation, and also handles the transmission/reception of Broadcom tags
for us. This is work in progress, but I expect the patches would be
ready by the end of this week.
[1]: http://www.spinics.net/lists/netdev/msg295942.html
>
> OpenWRT's eth_mangle_rx/tx() patch/code is being used to tap into
> the packets to/from the ethernet chip since it's convenient.
>
> This code will create lanX (X = 1..B53_N_PORTS) devices.
> All traffic from the ethX device will be forwarded the proper
lanX device.
> So, sw_port0_traffic == lan1_traffic and so on.
>
> The slave devices logic has been put into it's own file.
> Should this logic be desired to be extended to swconfig or other
> switch chips, it should be convenient to just move the slave.c/h
files.
>
> Note: if enable_vlan == 1, be sure to configure VLAN per lanX device
> in '/etc/config/network'
>
> Signed-off-by: Alexandru Ardelean <[email protected]
<mailto:[email protected]>>
> ---
> .../generic/files/drivers/net/phy/b53/Makefile | 2 +-
> .../generic/files/drivers/net/phy/b53/b53_common.c | 3 +
> .../generic/files/drivers/net/phy/b53/b53_hdr.c | 114
+++++++++++-
> .../generic/files/drivers/net/phy/b53/b53_priv.h | 2 +
> .../generic/files/drivers/net/phy/b53/slave.c | 196
+++++++++++++++++++++
> .../generic/files/drivers/net/phy/b53/slave.h | 38 ++++
> 6 files changed, 352 insertions(+), 3 deletions(-)
> create mode 100644
target/linux/generic/files/drivers/net/phy/b53/slave.c
> create mode 100644
target/linux/generic/files/drivers/net/phy/b53/slave.h
>
> diff --git
a/target/linux/generic/files/drivers/net/phy/b53/Makefile
b/target/linux/generic/files/drivers/net/phy/b53/Makefile
> index 6c809f9..c74f82b 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/Makefile
> +++ b/target/linux/generic/files/drivers/net/phy/b53/Makefile
> @@ -1,5 +1,5 @@
> obj-$(CONFIG_B53) += b53_common.o
> -obj-$(CONFIG_B53_HDR) += b53_hdr.o
> +obj-$(CONFIG_B53_HDR) += b53_hdr.o slave.o
>
> obj-$(CONFIG_B53_PHY_FIXUP) += b53_phy_fixup.o
>
> diff --git
a/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
b/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> index 9459b22..3da9efe 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> @@ -1377,6 +1377,9 @@ static int b53_global_reset_switch(struct
switch_dev *dev)
> priv->enable_management = 0;
> #ifdef CONFIG_B53_HDR
> priv->enable_brcm_hdr = 0;
> + /* Call this function before the memset on the priv->ports,
> + * otherwise we may leak devices */
> + b53_unregister_netdevs(priv);
> #endif
>
> memset(priv->vlans, 0, sizeof(priv->vlans) * dev->vlans);
> diff --git
a/target/linux/generic/files/drivers/net/phy/b53/b53_hdr.c
b/target/linux/generic/files/drivers/net/phy/b53/b53_hdr.c
> index 2a562a9..8fa7929 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_hdr.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_hdr.c
> @@ -21,14 +21,32 @@
>
> #include "b53_regs.h"
> #include "b53_priv.h"
> +#include "slave.h"
>
> /* This tag length is 4 bytes, older ones were 6 bytes, we do not
> * handle them
> */
> #define BRCM_HDR_LEN 4
> #define MIN_FRAME_LEN 64
> +#define BRCM_IG_DSTMAP2_MASK 1
> +#define BRCM_IG_DSTMAP1_MASK 0xff
> +#define BRCM_EG_PID_MASK 0x1f
>
> -static struct sk_buff *b53_mangle_tx(struct net_device *dev,
struct sk_buff *skb)
> +/* Ingress and egress opcodes */
> +#define BRCM_OPCODE_SHIFT 5
> +
> +static inline void b53_brcm_tag_set(struct sk_buff *skb, int
port_idx)
> +{
> + u8 *brcm_tag = skb->data + 2 * ETH_ALEN;
> +
> + brcm_tag[0] = (1 << BRCM_OPCODE_SHIFT);
> + brcm_tag[1] = 0;
> + brcm_tag[2] = (port_idx == 8) ? BRCM_IG_DSTMAP2_MASK : 0;
> + brcm_tag[3] = (1 << port_idx) & BRCM_IG_DSTMAP1_MASK;
> +}
> +
> +static struct sk_buff *b53_mangle_tx_port(struct net_device *dev,
> + struct sk_buff *skb, struct slave_priv* p)
> {
> if (unlikely(skb_headroom(skb) < BRCM_HDR_LEN)) {
> if (pskb_expand_head(skb, BRCM_HDR_LEN, 0,
GFP_ATOMIC) < 0)
> @@ -40,7 +58,20 @@ static struct sk_buff *b53_mangle_tx(struct
net_device *dev, struct sk_buff *skb
> memmove(skb->data, skb->data + BRCM_HDR_LEN, 2 * ETH_ALEN);
>
> /* Build the tag after the MAC Source Address */
> - memset(skb->data + 2 * ETH_ALEN, 0, BRCM_HDR_LEN);
> + if (!p) {
> + memset(skb->data + 2 * ETH_ALEN, 0, BRCM_HDR_LEN);
> + } else {
> + /* Register some TX stats for this device before
> + * passing the skb to the parent device */
> + dev->stats.tx_packets++;
> + dev->stats.tx_bytes += skb->len;
> +
> + b53_brcm_tag_set(skb, p->port_idx);
> +
> + skb->dev = p->parent_dev;
> + skb->protocol = htons(ETH_P_DSA);
> + }
> +
> #ifdef CONFIG_B53_HDR_TX_SW_PADDING
> /* FIXME: we're doing some padding here for runt ( < 64
bytes) packets;
> * some drivers/hw refuse to add hw padding for us
after we add
> @@ -64,12 +95,88 @@ out_err:
> return NULL;
> }
>
> +static struct sk_buff *b53_mangle_tx(struct net_device *dev,
struct sk_buff *skb)
> +{
> + /* The b53_mangle_tx() function can get called twice now
that there are
> + * slave devices: once for the lanX device, and one for
the ethY device
> + * which means that the Broadcom Header would get added twice.
> + * Which is why the packet type has been marked by the
slave lanX device
> + * to tell us that we've tagged this packet already. */
> + if (unlikely(skb->protocol == htons(ETH_P_DSA)))
> + return skb;
> + return b53_mangle_tx_port(dev, skb, NULL);
> +}
> +
> static void b53_mangle_rx(struct net_device *dev, struct
sk_buff *skb)
> {
> + struct b53_device *p = dev->phy_ptr;
> + u8 *brcm_tag;
> + u8 source_port;
> +
> + /* We're only interested in the 4th byte of the Broadcom
Header right now */
> + brcm_tag = skb->data + (2 * ETH_ALEN) + 3;
> + source_port = *brcm_tag & BRCM_EG_PID_MASK;
> +
> skb_pull(skb, BRCM_HDR_LEN);
> memmove(skb->data,
> skb->data - BRCM_HDR_LEN,
> 2 * ETH_ALEN);
> +
> + if ((source_port < B53_N_PORTS) &&
(p->ports[source_port].port_dev)) {
> + skb->dev = p->ports[source_port].port_dev;
> + skb->dev->stats.rx_packets++;
> + skb->dev->stats.rx_bytes += skb->len;
> + }
> +}
> +
> +static void b53_register_netdevs(struct b53_device *dev)
> +{
> + int i;
> + bool unlock = false;
> +
> + if (!rtnl_is_locked()) {
> + rtnl_lock();
> + unlock = true;
> + }
> + b53_for_each_port(dev, i) {
> + if (is_cpu_port(dev, i))
> + continue;
> + if (!(dev->enabled_ports & BIT(i))) {
> + if (dev->ports[i].port_dev) {
> + unregister_netdevice(dev->ports[i].port_dev);
> + dev->ports[i].port_dev = NULL;
> + }
> + continue;
> + }
> + /* Check if devices already exist for these devices */
> + if (!dev->ports[i].port_dev &&
> + !(dev->ports[i].port_dev =
slave_create(dev->eth_dev, i, b53_mangle_tx_port))) {
> + pr_warn("%s: can't create slave device for
port %d\n",
> + dev->sw_dev.name
<http://sw_dev.name>, i);
> + continue;
> + }
> + }
> + if (unlock)
> + rtnl_unlock();
> +}
> +
> +void b53_unregister_netdevs(struct b53_device *dev)
> +{
> + int i;
> + bool unlock = false;
> +
> + if (!rtnl_is_locked()) {
> + rtnl_lock();
> + unlock = true;
> + }
> + b53_for_each_port(dev, i) {
> + if (!dev->ports[i].port_dev)
> + continue;
> + unregister_netdevice(dev->ports[i].port_dev);
> + dev->ports[i].port_dev = NULL;
> + }
> + if (unlock)
> + rtnl_unlock();
> }
>
> void b53_enable_brcm_hdr(struct b53_device *dev)
> @@ -86,6 +193,7 @@ void b53_enable_brcm_hdr(struct b53_device *dev)
> }
> brcm_hdr_ctrl &= ~B53_BRCM_HDR_EN;
> if (!dev->eth_dev) {
> + b53_unregister_netdevs(dev);
> goto out;
> }
>
> @@ -93,6 +201,8 @@ void b53_enable_brcm_hdr(struct b53_device *dev)
> if (!dev->enable_management)
> goto out;
>
> + pr_info("%s: registering lan devices\n", dev->sw_dev.name
<http://sw_dev.name>);
> + b53_register_netdevs(dev);
> dev->eth_dev->phy_ptr = dev;
> dev->eth_dev->priv_flags |= IFF_NO_IP_ALIGN;
> dev->eth_dev->eth_mangle_rx = b53_mangle_rx;
> diff --git
a/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
b/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> index f487bf2..167e042 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> @@ -359,8 +359,10 @@ int b53_global_get_brcm_hdr(struct
switch_dev *dev,
> int b53_global_set_brcm_hdr(struct switch_dev *dev,
> const struct switch_attr *attr,
> struct switch_val *val);
> +void b53_unregister_netdevs(struct b53_device *dev);
> #else
> #define b53_enable_brcm_hdr(x)
> +#define b53_unregister_netdevs(x)
> #endif
>
> #ifdef CONFIG_BCM47XX
> diff --git
a/target/linux/generic/files/drivers/net/phy/b53/slave.c
b/target/linux/generic/files/drivers/net/phy/b53/slave.c
> new file mode 100644
> index 0000000..b8cd2dc
> --- /dev/null
> +++ b/target/linux/generic/files/drivers/net/phy/b53/slave.c
> @@ -0,0 +1,196 @@
> +/*
> + * Slave devices logic.
> + * Adapted/copied from the Linux kernel DSA driver (net/dsa).
> + *
> + * Permission to use, copy, modify, and/or distribute this
software for any
> + * purpose with or without fee is hereby granted, provided that
the above
> + * copyright notice and this permission notice appear in all
copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS
ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE
LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR
ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS,
WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <linux/rtnetlink.h>
> +#include <linux/etherdevice.h>
> +
> +#include "slave.h"
> +
> +static int slave_init(struct net_device *dev)
> +{
> + struct slave_priv *p = netdev_priv(dev);
> + dev->iflink = p->parent_dev->ifindex;
> + return 0;
> +}
> +
> +static int slave_open(struct net_device *dev)
> +{
> + struct slave_priv *p = netdev_priv(dev);
> + struct net_device *parent = p->parent_dev;
> + int err;
> +
> + if (!(parent->flags & IFF_UP))
> + return -ENETDOWN;
> +
> + if (!ether_addr_equal(dev->dev_addr, parent->dev_addr)) {
> + err = dev_uc_add(parent, dev->dev_addr);
> + if (err < 0)
> + goto out;
> + }
> +
> + if (dev->flags & IFF_ALLMULTI) {
> + err = dev_set_allmulti(parent, 1);
> + if (err < 0)
> + goto del_unicast;
> + }
> + if (dev->flags & IFF_PROMISC) {
> + err = dev_set_promiscuity(parent, 1);
> + if (err < 0)
> + goto clear_allmulti;
> + }
> +
> + return 0;
> +
> +clear_allmulti:
> + if (dev->flags & IFF_ALLMULTI)
> + dev_set_allmulti(parent, -1);
> +del_unicast:
> + if (!ether_addr_equal(dev->dev_addr, parent->dev_addr))
> + dev_uc_del(parent, dev->dev_addr);
> +out:
> + return err;
> +}
> +
> +static int slave_close(struct net_device *dev)
> +{
> + struct slave_priv *p = netdev_priv(dev);
> + struct net_device *parent = p->parent_dev;
> +
> + dev_mc_unsync(parent, dev);
> + dev_uc_unsync(parent, dev);
> + if (dev->flags & IFF_ALLMULTI)
> + dev_set_allmulti(parent, -1);
> + if (dev->flags & IFF_PROMISC)
> + dev_set_promiscuity(parent, -1);
> +
> + if (!ether_addr_equal(dev->dev_addr, parent->dev_addr))
> + dev_uc_del(parent, dev->dev_addr);
> +
> + return 0;
> +}
> +
> +static void slave_change_rx_flags(struct net_device *dev, int
change)
> +{
> + struct slave_priv *p = netdev_priv(dev);
> + struct net_device *parent = p->parent_dev;
> +
> + if (change & IFF_ALLMULTI)
> + dev_set_allmulti(parent, dev->flags & IFF_ALLMULTI
? 1 : -1);
> + if (change & IFF_PROMISC)
> + dev_set_promiscuity(parent, dev->flags &
IFF_PROMISC ? 1 : -1);
> +}
> +
> +static void slave_set_rx_mode(struct net_device *dev)
> +{
> + struct slave_priv *p = netdev_priv(dev);
> + struct net_device *parent = p->parent_dev;
> +
> + dev_mc_sync(parent, dev);
> + dev_uc_sync(parent, dev);
> +}
> +
> +static int slave_set_mac_address(struct net_device *dev, void *a)
> +{
> + struct slave_priv *p = netdev_priv(dev);
> + struct net_device *parent = p->parent_dev;
> + struct sockaddr *addr = a;
> + int err;
> +
> + if (!is_valid_ether_addr(addr->sa_data))
> + return -EADDRNOTAVAIL;
> +
> + if (!(dev->flags & IFF_UP))
> + goto out;
> +
> + if (!ether_addr_equal(addr->sa_data, parent->dev_addr)) {
> + err = dev_uc_add(parent, addr->sa_data);
> + if (err < 0)
> + return err;
> + }
> +
> + if (!ether_addr_equal(dev->dev_addr, parent->dev_addr))
> + dev_uc_del(parent, dev->dev_addr);
> +
> +out:
> + memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
> +
> + return 0;
> +}
> +
> +static netdev_tx_t slave_xmit(struct sk_buff *skb, struct
net_device *dev)
> +{
> + struct slave_priv *p = netdev_priv(dev);
> +
> + skb->dev = p->parent_dev;
> + skb = p->eth_mangle_tx_port(dev, skb, p);
> + dev_queue_xmit(skb);
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops slave_netdev_ops = {
> + .ndo_init = slave_init,
> + .ndo_open = slave_open,
> + .ndo_stop = slave_close,
> + .ndo_start_xmit = slave_xmit,
> + .ndo_change_rx_flags = slave_change_rx_flags,
> + .ndo_set_rx_mode = slave_set_rx_mode,
> + .ndo_set_mac_address = slave_set_mac_address,
> +};
> +
> +struct net_device *slave_create(struct net_device *parent, int
port,
> + eth_mangle_tx_port_t eth_mangle_tx_port)
> +{
> + /* Parent dev should not be null according to the code
path below */
> + struct net_device *slave_dev = NULL;
> + struct slave_priv *p;
> + char port_name[] = "lan1000";
> +
> + snprintf(port_name, sizeof(port_name), "lan%d", (port + 1));
> + slave_dev = alloc_netdev(sizeof(struct slave_priv),
> + port_name, ether_setup);
> + if (!slave_dev) {
> + pr_err("Failed to allocate memory for slave device");
> + goto out;
> + }
> +
> + slave_dev->features = parent->vlan_features;
> + eth_hw_addr_inherit(slave_dev, parent);
> + slave_dev->tx_queue_len = 0;
> + slave_dev->netdev_ops = &slave_netdev_ops;
> +
> + SET_NETDEV_DEV(slave_dev, &parent->dev);
> + slave_dev->vlan_features = parent->vlan_features;
> +
> + p = netdev_priv(slave_dev);
> + p->parent_dev = parent;
> + p->port_dev = slave_dev;
> + p->port_idx = port;
> + p->eth_mangle_tx_port = eth_mangle_tx_port;
> +
> + if (register_netdevice(slave_dev)) {
> + free_netdev(slave_dev);
> + slave_dev = NULL;
> + goto out;
> + }
> +
> + netif_carrier_on(slave_dev);
> +
> +out:
> + return slave_dev;
> +}
> +
> diff --git
a/target/linux/generic/files/drivers/net/phy/b53/slave.h
b/target/linux/generic/files/drivers/net/phy/b53/slave.h
> new file mode 100644
> index 0000000..3cfe7c7
> --- /dev/null
> +++ b/target/linux/generic/files/drivers/net/phy/b53/slave.h
> @@ -0,0 +1,38 @@
> +/*
> + * Slave devices logic.
> + * Adapted/copied from the Linux kernel DSA driver (net/dsa).
> + *
> + * Permission to use, copy, modify, and/or distribute this
software for any
> + * purpose with or without fee is hereby granted, provided that
the above
> + * copyright notice and this permission notice appear in all
copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS
ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE
LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR
ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS,
WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#ifndef __SLAVE_H__
> +#define __SLAVE_H__
> +
> +#include <linux/etherdevice.h>
> +
> +struct slave_priv;
> +typedef struct sk_buff *(*eth_mangle_tx_port_t)(struct
net_device *dev,
> + struct sk_buff *skb, struct slave_priv* p);
> +
> +struct slave_priv {
> + struct net_device *parent_dev;
> + struct net_device *port_dev;
> + eth_mangle_tx_port_t eth_mangle_tx_port;
> + int port_idx;
> +};
> +
> +/* For the moment, the rtnl_lock() needs to be called by the
caller */
> +struct net_device *slave_create(struct net_device *parent, int
port,
> + eth_mangle_tx_port_t eth_mangle_tx_port);
> +
> +#endif /* __SLAVE_H__ */
>
--
Florian
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
