lkml.org 
[lkml]   [2019]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 net-next 18/24] net: dsa: sja1105: Add support for traffic through standalone ports
On Sat, Apr 13, 2019 at 04:28:16AM +0300, Vladimir Oltean wrote:
> In order to support this, we are creating a make-shift switch tag out of
> a VLAN trunk configured on the CPU port. Termination of normal traffic
> on switch ports only works when not under a vlan_filtering bridge.
> Termination of management (PTP, BPDU) traffic works under all
> circumstances because it uses a different tagging mechanism
> (incl_srcpt). We are making use of the generic CONFIG_NET_DSA_TAG_8021Q
> code and leveraging it from our own CONFIG_NET_DSA_TAG_SJA1105.
>
> There are two types of traffic: regular and link-local.
> The link-local traffic received on the CPU port is trapped from the
> switch's regular forwarding decisions because it matched one of the two
> DMAC filters for management traffic.
> On transmission, the switch requires special massaging for these
> link-local frames. Due to a weird implementation of the switching IP, by
> default it drops link-local frames that originate on the CPU port. It
> needs to be told where to forward them to, through an SPI command
> ("management route") that is valid for only a single frame.
> So when we're sending link-local traffic, we need to clone skb's from
> DSA and send them in our custom xmit worker that also performs SPI access.
>
> For that purpose, the DSA xmit handler and the xmit worker communicate
> through a per-port "skb ring" software structure, with a producer and a
> consumer index. At the moment this structure is rather fragile
> (ping-flooding to a link-local DMAC would cause most of the frames to
> get dropped). I would like to move the management traffic on a separate
> netdev queue that I can stop when the skb ring got full and hardware is
> busy processing, so that we are not forced to drop traffic.
>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v3:
> Made management traffic be receivable on the DSA netdevices even when
> switch tagging is disabled, as well as regular traffic be receivable on
> the master netdevice in the same scenario. Both are accomplished using
> the sja1105_filter() function and some small touch-ups in the .rcv
> callback.

It seems like you made major changes to this. When you do that, you
should drop any reviewed-by tags you have. They are no longer valid
because of the major changes.

> /* This callback needs to be present */
> @@ -1141,7 +1158,11 @@ static int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled)
> if (rc)
> dev_err(ds->dev, "Failed to change VLAN Ethertype\n");
>
> - return rc;
> + /* Switch port identification based on 802.1Q is only passable

possible, not passable.

> + * if we are not under a vlan_filtering bridge. So make sure
> + * the two configurations are mutually exclusive.
> + */
> + return sja1105_setup_8021q_tagging(ds, !enabled);
> }
>
> static void sja1105_vlan_add(struct dsa_switch *ds, int port,
> @@ -1233,9 +1254,107 @@ static int sja1105_setup(struct dsa_switch *ds)
> */
> ds->vlan_filtering_is_global = true;
>
> + /* The DSA/switchdev model brings up switch ports in standalone mode by
> + * default, and that means vlan_filtering is 0 since they're not under
> + * a bridge, so it's safe to set up switch tagging at this time.
> + */
> + return sja1105_setup_8021q_tagging(ds, true);
> +}
> +
> +#include "../../../net/dsa/dsa_priv.h"

No. Don't use relative includes like this.

What do you need from the header? Maybe move it into
include/linux/net/dsa.h

> +/* Deferred work is unfortunately necessary because setting up the management
> + * route cannot be done from atomit context (SPI transfer takes a sleepable
> + * lock on the bus)
> + */
> +static void sja1105_xmit_work_handler(struct work_struct *work)
> +{
> + struct sja1105_port *sp = container_of(work, struct sja1105_port,
> + xmit_work);
> + struct sja1105_private *priv = sp->dp->ds->priv;
> + struct net_device *slave = sp->dp->slave;
> + struct net_device *master = dsa_slave_to_master(slave);
> + int port = (uintptr_t)(sp - priv->ports);
> + struct sk_buff *skb;
> + int i, rc;
> +
> + while ((i = sja1105_skb_ring_get(&sp->xmit_ring, &skb)) >= 0) {
> + struct sja1105_mgmt_entry mgmt_route = { 0 };
> + struct ethhdr *hdr;
> + int timeout = 10;
> + int skb_len;
> +
> + skb_len = skb->len;
> + hdr = eth_hdr(skb);
> +
> + mgmt_route.macaddr = ether_addr_to_u64(hdr->h_dest);
> + mgmt_route.destports = BIT(port);
> + mgmt_route.enfport = 1;
> + mgmt_route.tsreg = 0;
> + mgmt_route.takets = false;
> +
> + rc = sja1105_dynamic_config_write(priv, BLK_IDX_MGMT_ROUTE,
> + port, &mgmt_route, true);
> + if (rc < 0) {
> + kfree_skb(skb);
> + slave->stats.tx_dropped++;
> + continue;
> + }
> +
> + /* Transfer skb to the host port. */
> + skb->dev = master;
> + dev_queue_xmit(skb);
> +
> + /* Wait until the switch has processed the frame */
> + do {
> + rc = sja1105_dynamic_config_read(priv, BLK_IDX_MGMT_ROUTE,
> + port, &mgmt_route);
> + if (rc < 0) {
> + slave->stats.tx_errors++;
> + dev_err(priv->ds->dev,
> + "xmit: failed to poll for mgmt route\n");
> + continue;
> + }
> +
> + /* UM10944: The ENFPORT flag of the respective entry is
> + * cleared when a match is found. The host can use this
> + * flag as an acknowledgment.
> + */
> + cpu_relax();
> + } while (mgmt_route.enfport && --timeout);
> +
> + if (!timeout) {
> + dev_err(priv->ds->dev, "xmit timed out\n");
> + slave->stats.tx_errors++;
> + continue;
> + }
> +
> + slave->stats.tx_packets++;
> + slave->stats.tx_bytes += skb_len;
> + }
> +}
> +
> +static int sja1105_port_enable(struct dsa_switch *ds, int port,
> + struct phy_device *phydev)
> +{
> + struct sja1105_private *priv = ds->priv;
> + struct sja1105_port *sp = &priv->ports[port];
> +
> + sp->dp = &ds->ports[port];
> + INIT_WORK(&sp->xmit_work, sja1105_xmit_work_handler);
> return 0;
> }

I think i'm missing something here. You have a per port queue of link
local frames which need special handling. And you have a per-port work
queue. To send such a frame, you need to write some register, send the
frame, and then wait until the mgmt_route.enfport is reset.

Why are you doing this per port? How do you stop two ports/work queues
running at the same time? It seems like one queue, with one work queue
would be a better structure.

Also, please move all this code into the tagger. Just add exports for
sja1105_dynamic_config_write() and sja1105_dynamic_config_read().

> +static void sja1105_port_disable(struct dsa_switch *ds, int port)
> +{
> + struct sja1105_private *priv = ds->priv;
> + struct sja1105_port *sp = &priv->ports[port];
> + struct sk_buff *skb;
> +
> + cancel_work_sync(&sp->xmit_work);
> + while (sja1105_skb_ring_get(&sp->xmit_ring, &skb) >= 0)
> + kfree_skb(skb);
> +}
> +
> diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
> new file mode 100644
> index 000000000000..5c76a06c9093
> --- /dev/null
> +++ b/net/dsa/tag_sja1105.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019, Vladimir Oltean <olteanv@gmail.com>
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/if_vlan.h>
> +#include <linux/dsa/sja1105.h>
> +#include "../../drivers/net/dsa/sja1105/sja1105.h"

Again, no, don't do this.

> +
> +#include "dsa_priv.h"
> +
> +/* Similar to is_link_local_ether_addr(hdr->h_dest) but also covers PTP */
> +static inline bool sja1105_is_link_local(const struct sk_buff *skb)
> +{
> + const struct ethhdr *hdr = eth_hdr(skb);
> + u64 dmac = ether_addr_to_u64(hdr->h_dest);
> +
> + if ((dmac & SJA1105_LINKLOCAL_FILTER_A_MASK) ==
> + SJA1105_LINKLOCAL_FILTER_A)
> + return true;
> + if ((dmac & SJA1105_LINKLOCAL_FILTER_B_MASK) ==
> + SJA1105_LINKLOCAL_FILTER_B)
> + return true;
> + return false;
> +}
> +
> +static bool sja1105_filter(const struct sk_buff *skb, struct net_device *dev)
> +{
> + if (sja1105_is_link_local(skb))
> + return true;
> + if (!dev->dsa_ptr->vlan_filtering)
> + return true;
> + return false;
> +}

Please add a comment here about what frames cannot be handled by the
tagger. However, i'm not too happy about this design...

> +
> +static struct sk_buff *sja1105_xmit(struct sk_buff *skb,
> + struct net_device *netdev)
> +{
> + struct dsa_port *dp = dsa_slave_to_port(netdev);
> + struct dsa_switch *ds = dp->ds;
> + struct sja1105_private *priv = ds->priv;
> + struct sja1105_port *sp = &priv->ports[dp->index];
> + struct sk_buff *clone;
> +
> + if (likely(!sja1105_is_link_local(skb))) {
> + /* Normal traffic path. */
> + u16 tx_vid = dsa_tagging_tx_vid(ds, dp->index);
> + u8 pcp = skb->priority;
> +
> + /* If we are under a vlan_filtering bridge, IP termination on
> + * switch ports based on 802.1Q tags is simply too brittle to
> + * be passable. So just defer to the dsa_slave_notag_xmit
> + * implementation.
> + */
> + if (dp->vlan_filtering)
> + return skb;
> +
> + return dsa_8021q_xmit(skb, netdev, ETH_P_EDSA,
> + ((pcp << VLAN_PRIO_SHIFT) | tx_vid));

Please don't reuse ETH_P_EDSA. Define an ETH_P_SJA1105.

> + }
> +
> + /* Code path for transmitting management traffic. This does not rely
> + * upon switch tagging, but instead SPI-installed management routes.
> + */
> + clone = skb_clone(skb, GFP_ATOMIC);
> + if (!clone) {
> + dev_err(ds->dev, "xmit: failed to clone skb\n");
> + return NULL;
> + }
> +
> + if (sja1105_skb_ring_add(&sp->xmit_ring, clone) < 0) {
> + dev_err(ds->dev, "xmit: skb ring full\n");
> + kfree_skb(clone);
> + return NULL;
> + }
> +
> + if (sp->xmit_ring.count == SJA1105_SKB_RING_SIZE)
> + /* TODO setup a dedicated netdev queue for management traffic
> + * so that we can selectively apply backpressure and not be
> + * required to stop the entire traffic when the software skb
> + * ring is full. This requires hooking the ndo_select_queue
> + * from DSA and matching on mac_fltres.
> + */
> + dev_err(ds->dev, "xmit: reached maximum skb ring size\n");

This should be rate limited.

Andrew

> +
> + schedule_work(&sp->xmit_work);
> + /* Let DSA free its reference to the skb and we will free
> + * the clone in the deferred worker
> + */
> + return NULL;
> +}
> +
> +static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
> + struct net_device *netdev,
> + struct packet_type *pt)
> +{
> + unsigned int source_port, switch_id;
> + struct ethhdr *hdr = eth_hdr(skb);
> + struct sk_buff *nskb;
> + u16 tpid, vid, tci;
> + bool is_tagged;
> +
> + nskb = dsa_8021q_rcv(skb, netdev, pt, &tpid, &tci);
> + is_tagged = (nskb && tpid == ETH_P_EDSA);
> +
> + skb->priority = (tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
> + vid = tci & VLAN_VID_MASK;
> +
> + skb->offload_fwd_mark = 1;
> +
> + if (likely(!sja1105_is_link_local(skb))) {
> + /* Normal traffic path. */
> + source_port = dsa_tagging_rx_source_port(vid);
> + switch_id = dsa_tagging_rx_switch_id(vid);
> + } else {
> + /* Management traffic path. Switch embeds the switch ID and
> + * port ID into bytes of the destination MAC, courtesy of
> + * the incl_srcpt options.
> + */
> + source_port = hdr->h_dest[3];
> + switch_id = hdr->h_dest[4];
> + /* Clear the DMAC bytes that were mangled by the switch */
> + hdr->h_dest[3] = 0;
> + hdr->h_dest[4] = 0;
> + }
> +
> + skb->dev = dsa_master_find_slave(netdev, switch_id, source_port);
> + if (!skb->dev) {
> + netdev_warn(netdev, "Couldn't decode source port\n");
> + return NULL;
> + }
> +
> + /* Delete/overwrite fake VLAN header, DSA expects to not find
> + * it there, see dsa_switch_rcv: skb_push(skb, ETH_HLEN).
> + */
> + if (is_tagged)
> + memmove(skb->data - ETH_HLEN, skb->data - ETH_HLEN - VLAN_HLEN,
> + ETH_HLEN - VLAN_HLEN);
> +
> + return skb;
> +}
> +
> +const struct dsa_device_ops sja1105_netdev_ops = {
> + .xmit = sja1105_xmit,
> + .rcv = sja1105_rcv,
> + .filter = sja1105_filter,
> + .overhead = VLAN_HLEN,
> +};
> +
> --
> 2.17.1
>

\
 
 \ /
  Last update: 2019-04-13 18:39    [W:0.305 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site