[OpenWrt-Devel] [PATCH 2/3][RESEND] b53: implement port mirroring feature
Jonas Gorski
jogo at openwrt.org
Fri Feb 27 14:58:00 EST 2015
Hi,
On Mon, Feb 23, 2015 at 4:55 PM, Alexandru Ardelean
<ardeleanalex at gmail.com> wrote:
> From: Alexandru Ardelean <ardeleanalex at gmail.com>
>
> We consider this a debugging feature of the B53 hardware, since
> you'd normally not need to mirror traffic from ports to a single
> capture port, unless debugging or doing some port snooping.
>
> Because the feature is a bit complex in hardware, string parsing
> is used to make this a bit more comfortable when setting it
> from user space.
>
> To enable/disable port mirroring (or capture):
> swconfig dev switch0 set port_capture on [or off]
> swconfig dev switch0 set apply
>
> Before calling 'set apply', the current port capture configuration
> can be read, before applying it to the registers.
> swconfig dev switch0 get port_capture
>
> The output will be:
> Mirror Capture: On
> Capture Port: 8
> Block Not Mirrored Traffic: No
> Incoming Filter:
> Address Filter: None
> Mirrored Ports: None
> Outgoing Filter:
> Address Filter: None
> Mirrored Ports: None
>
> The 'off' parameter takes no arguments.
>
> The 'on' parameter takes these arguments:
> swconfig dev switch0 set port_capture \
> "on cap_port 8 \
> block_not_mirrored \
> in_filter:(da XX:XX:XX:XX:XX:XX ports 1ff div 3ffff) \
> out_filter:(sa YY:YY:YY:YY:YY:YY ports 0ff div 4)" \
>
> Above is an example of how this would be called.
> Backslashes mean that the command should be a single line.
> The parsing is stupid simple, so quotes, cases & spaces are important.
> - cap_port - the port on which to forward all the captured traffic
> - block_not_mirrored - block the traffic that is not mirrored
I think these two could be separate global options, as well as a
global "on/off" switch.
> - in_filter/out_filter - filters for incoming/outgoing traffic capture
> default is for no filters, and no mirrored ports
> - da (or sa) - but not both; filter by MAC address; not specifying it
> should capture all packets
> - div - capture every Nth packet; default (0) captures all
These are fine as a string to be parsed.
> - ports - bitmask port list, which ports to capture traffic
And this could be a per-port property.
The less string parsing to be done in the kernel, the better.
> Once mirrored ports are set, traffic will be forwarded to the
> capture port based on filter parameters.
>
> Signed-off-by: Alexandru Ardelean <ardeleanalex at gmail.com>
> ---
> .../generic/files/drivers/net/phy/b53/Kconfig | 13 +
> .../generic/files/drivers/net/phy/b53/b53_common.c | 294 +++++++++++++++++++++
> .../generic/files/drivers/net/phy/b53/b53_priv.h | 25 ++
> .../generic/files/drivers/net/phy/b53/b53_regs.h | 24 ++
> 4 files changed, 356 insertions(+)
>
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/Kconfig b/target/linux/generic/files/drivers/net/phy/b53/Kconfig
> index 67e053e..545814a 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/Kconfig
> +++ b/target/linux/generic/files/drivers/net/phy/b53/Kconfig
> @@ -35,3 +35,16 @@ config B53_SRAB_DRIVER
>
> config B53_PHY_FIXUP
> bool
> + depends on B53
> +
> +config B53_HW_DEBUG_FEATURES
If you introduce a new config symbol, you need to make sure that the
appropriate target configs get updated, else you will break the build
for everyone using b53.
> + depends on B53
> + bool "B53 hw debug features"
Usually depends/selects follow the bool/tristate, not before it.
> + default n
n is already the default, no need to set it again.
> + help
> + Various features that are supported by the switch chip and are
> + not needed for normal functioning of the switch chip.
> + They could come in handy for debugging
> + So far they've been tested on BCM53128, and should work on BCM53125
> + since that's a 4 port variant.
I haven't tested it, but bcm53115/53118 is AFAIK essentially the same
except for EEE support. BCM539x should also work as-is.
BCM5365 is mostly the same, except that it does not know the
block_not_mirrored bit, and the capture port field is a bitmask, but
still only allows one bit to be set. Apart from that it seems to be
the same.
No Idea about BCM5325, maybe Florian knows something about it.
> +
> 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 b82bc93..4abc8c3 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
> @@ -404,6 +404,70 @@ static void b53_enable_ports(struct b53_device *dev)
> }
> }
>
> +#ifdef CONFIG_B53_HW_DEBUG_FEATURES
> +static void b53_enable_port_capture_filter(struct b53_device *dev,
This seems to both enable and disable, so I think
b53_set_port_capture_filter would be a more appropriate name.
> + struct b53_port_capture_filter *flt, u32 reg_off)
Please align the start with the opening (.
> +{
> + u16 flt_ctrl;
> +
> + b53_read16(dev, B53_MGMT_PAGE, B53_MIRROR_CTRL_FILTER(reg_off), &flt_ctrl);
> +
> + flt_ctrl &= ~(B53_MIRROR_FILTER_BY_DA | B53_MIRROR_FILTER_BY_SA);
> + if (flt && (flt->mode == B53_CAPTURE_BY_DA))
Unnecessary ().
> + flt_ctrl |= B53_MIRROR_FILTER_BY_DA;
> + else if (flt && (flt->mode == B53_CAPTURE_BY_SA))
Unnecessary ().
> + flt_ctrl |= B53_MIRROR_FILTER_BY_SA;
> +
> + if (flt_ctrl & (B53_MIRROR_FILTER_BY_DA | B53_MIRROR_FILTER_BY_SA))
> + b53_write48(dev, B53_MGMT_PAGE, B53_MIRROR_MAC_ADDR(reg_off),
> + b53_mac_array_to_u64(flt->mac));
> +
> + if (flt && flt->nth) {
> + b53_write16(dev, B53_MGMT_PAGE, B53_MIRROR_DIVIDER(reg_off),
> + B53_MIRROR_DIVIDER_VALUE(flt->divider));
> + flt_ctrl |= B53_MIRROR_DIV_EN;
> + } else
> + flt_ctrl &= ~B53_MIRROR_DIV_EN;
Please use braces for both branches.
> +
> + flt_ctrl &= ~0x1ff;
Please add a macro for that.
> + if (flt)
> + flt_ctrl |= B53_MIRROR_PORTS_MASK(flt->ports_mask);
> +
> + b53_write16(dev, B53_MGMT_PAGE, B53_MIRROR_CTRL_FILTER(reg_off), flt_ctrl);
> +}
> +
> +static void b53_enable_port_capture(struct b53_device *dev)
This also seems to do both.
> +{
> + u16 port_mirror_ctrl;
> +
> + b53_read16(dev, B53_MGMT_PAGE, B53_MIRROR_CTRL, &port_mirror_ctrl);
> +
> + if (!dev->port_capture.enable) {
> + port_mirror_ctrl &= ~B53_MIRROR_CTRL_EN;
> + goto out;
Do we really need a goto here? Just do the write and return.
> + } else
> + port_mirror_ctrl |= B53_MIRROR_CTRL_EN;
> +
> + if (dev->port_capture.block_not_mirrored)
> + port_mirror_ctrl |= B53_BLOCK_NOT_MIR;
> + else
> + port_mirror_ctrl &= ~B53_BLOCK_NOT_MIR;
> +
> + B53_CAP_PORT_SET(port_mirror_ctrl, dev->port_capture.capture_port);
I don't like these macros, just use
foo &= ~MASK;
foo |= (bar & ~MASK);
> +
> + b53_enable_port_capture_filter(dev, dev->port_capture.in_filter,
> + B53_MIR_IN_FILTER_OFFSET);
> + b53_enable_port_capture_filter(dev, dev->port_capture.out_filter,
> + B53_MIR_OUT_FILTER_OFFSET);
> +
> +out:
> + b53_write16(dev, B53_MGMT_PAGE, B53_MIRROR_CTRL, port_mirror_ctrl);
> +}
> +
> +#else
> +#define b53_enable_port_capture(x)
Please use a static inline function for that so the arguments can
still be checked for correctness.
> +#endif /* CONFIG_B53_HW_DEBUG_FEATURES */
> +
> static void b53_enable_mib(struct b53_device *dev)
> {
> u8 gc;
> @@ -452,6 +516,7 @@ static int b53_apply(struct b53_device *dev)
> }
>
> b53_enable_ports(dev);
> + b53_enable_port_capture(dev);
>
> if (!is5325(dev) && !is5365(dev))
> b53_set_jumbo(dev, dev->enable_jumbo, 1);
> @@ -506,6 +571,8 @@ static int b53_switch_reset(struct b53_device *dev)
>
> /* enable all ports */
> b53_enable_ports(dev);
> + /* disable port capture (if enabled) */
> + b53_enable_port_capture(dev);
>
> /* configure MII port if necessary */
> if (is5325(dev)) {
> @@ -610,6 +677,222 @@ static int b53_global_set_4095_enable(struct switch_dev *dev,
> return 0;
> }
>
> +#ifdef CONFIG_B53_HW_DEBUG_FEATURES
> +static int b53_global_get_port_capture_filter(struct b53_device *dev,
> + int len, const char *type,
> + struct b53_port_capture_filter *flt)
> +{
> + int i, ports;
> + const char *flt_mode = "None";
> +
> + if (!flt)
> + goto print;
This goto seems unnecessary, just do
if (!flt)
flt_mode = "None";
else if (flt->mode == ... )
> +
> + if (flt->mode == B53_CAPTURE_BY_DA)
> + flt_mode = "By Destination Address";
> + else if (flt->mode == B53_CAPTURE_BY_SA)
> + flt_mode = "By Source Address";
> +
> +print:
> + len += snprintf(dev->buf + len, (B53_BUF_SIZE - len),
> + "%s Filter:\n Address Filter: %s\n", type, flt_mode);
> + if (flt && flt->mode != B53_CAPTURE_ALL)
> + len += snprintf(dev->buf + len, (B53_BUF_SIZE - len),
> + " MAC: %02x:%02x:%02x:%02x:%02x:%02x\n",
> + flt->mac[0],flt->mac[1],flt->mac[2],
> + flt->mac[3],flt->mac[4],flt->mac[5]);
Use %M for mac addresses.
> + if (flt && flt->nth)
> + len += snprintf(dev->buf + len, (B53_BUF_SIZE - len),
> + " Capture Nth: %u\n", flt->divider);
> +
> + len += snprintf(dev->buf + len,(B53_BUF_SIZE - len)," Mirrored Ports:");
> + ports = 0;
> + b53_for_each_port(dev, i) {
> + if (flt && (flt->ports_mask & BIT(i))) {
> + len += snprintf(dev->buf + len, (B53_BUF_SIZE - len)," %u", i);
> + ports++;
> + }
> + }
> + len += snprintf(dev->buf + len, (B53_BUF_SIZE - len),"%s\n",
> + (ports ? "" : " None"));
> +
> + return len;
> +}
> +
> +static int b53_global_get_port_capture(struct switch_dev *dev,
> + const struct switch_attr *attr,
> + struct switch_val *val)
> +{
> + struct b53_device *priv = sw_to_b53(dev);
> + int len;
> +
> + len = snprintf(priv->buf, B53_BUF_SIZE, "Mirror Capture: %s\n",
> + priv->port_capture.enable ? "On" : "Off");
> + if (!priv->port_capture.enable)
> + goto out;
> + len += snprintf(priv->buf + len,B53_BUF_SIZE - len," Capture Port: %u\n",
> + priv->port_capture.capture_port);
> + len += snprintf(priv->buf + len,B53_BUF_SIZE - len,
> + " Block Not Mirrored Traffic: %s\n",
> + priv->port_capture.block_not_mirrored ? "Yes" : "No");
> +
> + len = b53_global_get_port_capture_filter(priv, len, "Incoming",
> + priv->port_capture.in_filter);
> + len = b53_global_get_port_capture_filter(priv, len, "Outgoing",
> + priv->port_capture.out_filter);
> +out:
> + val->len = len;
> + val->value.s = priv->buf;
> +
> + return 0;
> +}
> +
> +static bool b53_mac_from_string(const char *s, u8 *dst)
> +{
> + if (6 == sscanf(s, "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx",
> + &dst[0], &dst[1], &dst[2], &dst[3], &dst[4], &dst[5]))
> + return true;
> + if (6 == sscanf(s, "%02hhX:%02hhX:%02hhX:%02hhX:%02hhx:%02hhX",
> + &dst[0], &dst[1], &dst[2], &dst[3], &dst[4], &dst[5]))
> + return true;
> +
> + return false;
mac_pton() already does this.
> +}
> +
> +static int b53_ports_from_string(const char *s, const char *endp, u16 *ports)
> +{
> + u16 p;
> +
> + /* read ports mask (if specified) */
> + if (!(s = strstr(s, "ports")))
> + return 0;
> + if (endp && s >= endp)
> + return 0;
> +
> + if (sscanf(s + 6, "%03hX", &p) < 1 && sscanf(s + 6, "%03hx", &p) < 1)
> + return -EINVAL;
> + if (p > 0x1ff)
> + return -EINVAL;
> + *ports = p;
> +
> + return 1;
> +}
> +
> +static int b53_global_set_port_capture_filter(struct b53_device *dev,
> + struct b53_port_capture_filter **flt, const char *s)
> +{
> + const char *s1, *endp;
> + u16 ports_mask;
> + u32 div;
> + int rc = -EINVAL;
> +
> + /* check end paranthesis; where our params should end */
> + if (!(endp = strstr(s, ")")))
> + goto out;
> +
> + if (!*flt)
> + *flt = devm_kzalloc(dev->dev,
> + sizeof(struct b53_port_capture_filter),
> + GFP_KERNEL);
> + if (!*flt)
> + return -ENOMEM;
> +
> + /* first read if we filter by DA or SA */
> + if ((s1 = strstr(s, "da")) && s1 < endp)
> + (*flt)->mode = B53_CAPTURE_BY_DA;
Unnecessary ().
> + else if ((s1 = strstr(s, "sa")) && s1 < endp)
> + (*flt)->mode = B53_CAPTURE_BY_SA;
Unnecessary ().
> + else
> + (*flt)->mode = B53_CAPTURE_ALL;
Unnecessary ().
> +
> + if ((*flt)->mode != B53_CAPTURE_ALL &&
Unnecessary ().
> + !b53_mac_from_string(s1 + 3, (*flt)->mac)) {
Unnecessary ().
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + /* read divider (if specified) */
> + if ((s1 = strstr(s, "div")) && s1 >= endp)
> + s1 = NULL;
> + if (s1 && (sscanf(s1 + 4, "%u", &div) < 1 || div > 0x3ffff)) {
The devider is limited to 0x1ff, so I wonder where this 0x3ffff value
comes from. Also please avoid magic values.
> + rc = -EINVAL;
> + goto out;
Just directly return -EINVAL;
> + }
> + if (s1) {
> + (*flt)->nth = 1;
> + (*flt)->divider = div;
Unecessary ().
> + }
> +
> + /* read ports mask (if specified) */
> + if (b53_ports_from_string(s, endp, &ports_mask) > 0)
> + (*flt)->ports_mask = ports_mask;
Unecessary ().
> + else
> + (*flt)->ports_mask = 0;
Unecessary ().
> +
> + return 0;
> +
> +out:
> + return rc;
> +}
> +
> +static inline void b53_global_port_capture_cleanup(struct b53_device *dev)
> +{
> + if (dev->port_capture.in_filter)
> + devm_kfree(dev->dev, dev->port_capture.in_filter);
> + if (dev->port_capture.out_filter)
> + devm_kfree(dev->dev, dev->port_capture.out_filter);
If you allocate with devm_, then you aren't supposed to free it
yourself; this will produce errors on unload, so drop the free calls.
> + memset(&dev->port_capture, 0, sizeof(dev->port_capture));
> +}
> +
> +static int b53_global_set_port_capture(struct switch_dev *dev,
> + const struct switch_attr *attr,
> + struct switch_val *val)
> +{
> + struct b53_device *priv = sw_to_b53(dev);
> + const char *s;
> + unsigned int cap_port;
> + int rc = 0;
> +
> + if (strstr(val->value.s, "off"))
> + goto out_cleanup;
> +
> + if (strstr(val->value.s, "on"))
> + priv->port_capture.enable = 1;
> + priv->port_capture.block_not_mirrored = !!strstr(val->value.s, "block_not_mirrored");
> +
> + /* read capture port (if specified) */
> + if ((s = strstr(val->value.s, "cap_port")) &&
> + sscanf(s + 9, "%u", &cap_port) < 1 &&
> + cap_port > B53_N_PORTS) {
> + rc = -EINVAL;
> + goto out_cleanup;
> + }
> + priv->port_capture.capture_port = (s) ? cap_port : dev->cpu_port;
Unnecessary ().
> +
> + if ((s = strstr(val->value.s, "in_filter:")) && (s = strstr(s, "(")))
> + rc = b53_global_set_port_capture_filter(priv,
> + &priv->port_capture.in_filter, s);
Please no assignment in conditions.
> + if (rc)
> + goto out_cleanup;
> +
> + if ((s = strstr(val->value.s, "out_filter:")) && (s = strstr(s, "(")))
> + rc = b53_global_set_port_capture_filter(priv,
> + &priv->port_capture.out_filter, s);
Please no assignment in conditions.
> + if (!rc)
> + goto out;
> +
> +out_cleanup:
> + b53_global_port_capture_cleanup(priv);
> +out:
> + return rc;
> +}
> +
> +#else
> +
> +#define b53_global_port_capture_cleanup(x)
> +
> +#endif /* CONFIG_B53_HW_DEBUG_FEATURES */
> +
> static int b53_global_get_ports(struct switch_dev *dev,
> const struct switch_attr *attr,
> struct switch_val *val)
> @@ -778,6 +1061,8 @@ static int b53_global_reset_switch(struct switch_dev *dev)
> memset(priv->vlans, 0, sizeof(priv->vlans) * dev->vlans);
> memset(priv->ports, 0, sizeof(priv->ports) * dev->ports);
>
> + b53_global_port_capture_cleanup(priv);
> +
> return b53_switch_reset(priv);
> }
>
> @@ -939,6 +1224,15 @@ static struct switch_attr b53_global_ops[] = {
> .get = b53_global_get_4095_enable,
> .max = 1,
> },
> +#ifdef CONFIG_B53_HW_DEBUG_FEATURES
> + {
> + .type = SWITCH_TYPE_STRING,
> + .name = "port_capture",
> + .description = "Ports Capture Traffic",
> + .set = b53_global_set_port_capture,
> + .get = b53_global_get_port_capture,
> + },
> +#endif /* CONFIG_B53_HW_DEBUG_FEATURES */
> };
>
> static struct switch_attr b53_port_ops[] = {
> 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 bc9b533..1edad71 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
> @@ -22,6 +22,7 @@
> #include <linux/kernel.h>
> #include <linux/mutex.h>
> #include <linux/switch.h>
> +#include <linux/if_ether.h>
>
> struct b53_device;
>
> @@ -67,6 +68,21 @@ struct b53_port {
> unsigned int pvid:12;
> };
>
> +#ifdef CONFIG_B53_HW_DEBUG_FEATURES
> +enum {
> + B53_CAPTURE_ALL = 0,
> + B53_CAPTURE_BY_DA,
> + B53_CAPTURE_BY_SA,
> +};
> +struct b53_port_capture_filter {
> + unsigned mode:2;
> + unsigned nth:1;
> + unsigned ports_mask:9;
> + u8 mac[ETH_ALEN];
> + unsigned divider:10;
Please switch the order of mac and devider, so the compiler can merge
the fields can merge the fields.
> +};
> +#endif
> +
> struct b53_device {
> struct switch_dev sw_dev;
> struct b53_platform_data *pdata;
> @@ -95,6 +111,15 @@ struct b53_device {
> unsigned enable_vlan:1;
> unsigned enable_jumbo:1;
> unsigned allow_vid_4095:1;
> +#ifdef CONFIG_B53_HW_DEBUG_FEATURES
> + struct {
> + unsigned enable:1;
> + unsigned block_not_mirrored:1;
> + unsigned capture_port:4;
> + struct b53_port_capture_filter *in_filter;
> + struct b53_port_capture_filter *out_filter;
Maybe let's not use pointers here but embed them. You never have more
than one each, they aren't that big (~10 byte), and would simplify a
few things. You can then replace if (!flt) with if
(!dev->port_capture_enabled).
> + } port_capture;
> +#endif
>
> struct b53_port *ports;
> struct b53_vlan *vlans;
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h b/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h
> index 4379c58..28361e6 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_regs.h
> @@ -166,6 +166,30 @@ static inline u64 b53_mac_array_to_u64(const u8 *u8_arr) {
> #define GC_FRM_MGMT_PORT_04 0x00
> #define GC_FRM_MGMT_PORT_MII 0x80
>
> +/* Mirror Capture Control Register (16 bit) */
> +#define B53_MIRROR_CTRL 0x10
> +#define B53_CAP_PORT_SET(r,p) (r = (r & ~0x0f) | (p & 0x0f))
Please don't use these kind of macros, IMHO they reduce readability.
Also always enclose arugments in () to avoid unwanted sideffects with
complex arguments. This applies to all other macros with arguments.
> +#define B53_BLOCK_NOT_MIR BIT(14)
> +#define B53_MIRROR_CTRL_EN BIT(15)
> +
> +/* Offset for groups of filter registers: 0x0 = In, 0xA = Out */
> +#define B53_MIR_IN_FILTER_OFFSET 0
> +#define B53_MIR_OUT_FILTER_OFFSET 0x0A
> +
> +/* In/Out Mirror Control Registers (16 bit) */
> +#define B53_MIRROR_CTRL_FILTER(o) (0x12 + o)
> +#define B53_MIRROR_PORTS_MASK(p) (0x1ff & p)
Same here, just "use" in the code as f &= B53_MIRROR_PORTS_MASK (
> +#define B53_MIRROR_DIV_EN BIT(13)
> +#define B53_MIRROR_FILTER_BY_DA BIT(14) /* Do not use both DA/SA */
> +#define B53_MIRROR_FILTER_BY_SA BIT(15)
> +
> +/* In/Out Divider Registers (16 bit) */
> +#define B53_MIRROR_DIVIDER(o) (0x14 + o)
> +#define B53_MIRROR_DIVIDER_VALUE(n) (0x1ff & n)
If the field is 10 bit wide, shouldn't be the maximum value 0x3ff? Or
the divider field of port_capture_filter is one bit too wide.
> +
> +/* In/Out MAC Address Registers (48 bit) */
> +#define B53_MIRROR_MAC_ADDR(o) (0x16 + o)
> +
> /* Device ID register (8 or 32 bit) */
> #define B53_DEVICE_ID 0x30
Jonas
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list