[PATCH net-next 0/7] net: Support Wake-on-LAN using filters

[PATCH net-next 0/7] net: Support Wake-on-LAN using filters

[Florian Fainelli]

Hi all,

This patch series adds support for allowing Wake-on-LAN to wake-up the
system using configurable filters. This is particular useful in the context
of Android where wake on MDNS is a requirement.

We support this by using the bcm_sf2 Compact Field Processor (CFP) which
supports matching packets and tagging them with an unique identifier
(Classification ID) that is added in each packet being matched through the use
of Broadcom tags. The SYSTEMPORT MAC attached to that switch is then used to
match that unique identifier and trigger a system wake-up event.

Last patch is the ethtool modifications to support that feature.

Example:

ethtool --config-nfc gphy flow-type udp4 src-ip 192.168.1.1 dst-ip 192.168.1.32 \
	src-port 1234 dst-port 5678 action 64
Added rule with ID 1

ethtool -s gphy wol f filters 0x2

To wake up the device:

nc -vz -u -p 1234 -s 192.168.1.1 192.168.1.32 5678

Florian Fainelli (7):
  net: dsa: bcm_sf2: Allow targeting CPU ports for CFP rules
  net: dsa: bcm_sf2: Disable learning while in WoL
  net: systemport: Do not re-configure upon WoL interrupt
  net: systemport: Create helper to set MPD
  ethtool: Add WAKE_FILTER bitmask
  net: systemport: Add support for WAKE_FILTER
  net: dsa: bcm_sf2: Support WAKE_FILTER

 drivers/net/dsa/bcm_sf2.c                  |  14 +++-
 drivers/net/dsa/bcm_sf2_cfp.c              |   3 +-
 drivers/net/dsa/bcm_sf2_regs.h             |   2 +
 drivers/net/ethernet/broadcom/bcmsysport.c | 129 ++++++++++++++++++++++++-----
 drivers/net/ethernet/broadcom/bcmsysport.h |  14 +++-
 include/uapi/linux/ethtool.h               |   3 +-
 6 files changed, 138 insertions(+), 27 deletions(-)

-- 
2.14.1

[PATCH ethtool] ethtool: Add support for WAKE_FILTER

[Florian Fainelli]

Allow re-purposing the wol->sopass storage area to specify a bitmask of filters
(programmed previously via ethtool::rxnfc) to be used as wake-up patterns.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 ethtool-copy.h |  1 +
 ethtool.c      | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 8cc61e9ab40b..dbfaca15dca5 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -1628,6 +1628,7 @@ static __inline__ int ethtool_validate_duplex(__u8 duplex)
 #define WAKE_ARP		(1 << 4)
 #define WAKE_MAGIC		(1 << 5)
 #define WAKE_MAGICSECURE	(1 << 6) /* only meaningful if WAKE_MAGIC */
+#define WAKE_FILTER		(1 << 7)
 
 /* L2-L4 network traffic flow types */
 #define	TCP_V4_FLOW	0x01	/* hash or spec (tcp_ip4_spec) */
diff --git a/ethtool.c b/ethtool.c
index fb93ae898312..322fc8d98ee5 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -931,6 +931,9 @@ static int parse_wolopts(char *optstr, u32 *data)
 		case 's':
 			*data |= WAKE_MAGICSECURE;
 			break;
+		case 'f':
+			*data |= WAKE_FILTER;
+			break;
 		case 'd':
 			*data = 0;
 			break;
@@ -964,6 +967,8 @@ static char *unparse_wolopts(int wolopts)
 			*p++ = 'g';
 		if (wolopts & WAKE_MAGICSECURE)
 			*p++ = 's';
+		if (wolopts & WAKE_FILTER)
+			*p++ = 'f';
 	} else {
 		*p = 'd';
 	}
@@ -989,6 +994,21 @@ static int dump_wol(struct ethtool_wolinfo *wol)
 		fprintf(stdout, "\n");
 	}
 
+	if (wol->supported & WAKE_FILTER) {
+		int i, j;
+		int delim = 0;
+		fprintf(stdout, "        Filter(s) enabled: ");
+		for (i = 0; i < SOPASS_MAX; i++) {
+			for (j = 0; j < 8; j++) {
+				if (wol->sopass[i] & (1 << j)) {
+					fprintf(stdout, "%s%d", delim?",":"", i * 8 + j);
+					delim=1;
+				}
+			}
+		}
+		fprintf(stdout, "\n");
+	}
+
 	return 0;
 }
 
@@ -2897,6 +2917,16 @@ static int do_sset(struct cmd_context *ctx)
 				exit_bad_args();
 			get_mac_addr(argp[i], sopass_wanted);
 			sopass_change = 1;
+		} else if (!strcmp(argp[i], "filters")) {
+			gwol_changed = 1;
+			i++;
+			if (i >= argc)
+				exit_bad_args();
+			if (parse_hex_u32_bitmap(argp[i],
+						 SOPASS_MAX * 8,
+						 (unsigned int *)sopass_wanted))
+				exit_bad_args();
+			sopass_change = 1;
 		} else if (!strcmp(argp[i], "msglvl")) {
 			i++;
 			if (i >= argc)
@@ -3112,8 +3142,10 @@ static int do_sset(struct cmd_context *ctx)
 		if (err < 0) {
 			if (wol_change)
 				fprintf(stderr, "  not setting wol\n");
-			if (sopass_change)
+			if (sopass_change & wol.wolopts & WAKE_MAGICSECURE)
 				fprintf(stderr, "  not setting sopass\n");
+			if (sopass_change & wol.wolopts & WAKE_FILTER)
+				fprintf(stderr, "  not setting filters\n");
 		}
 	}
 
@@ -5066,6 +5098,7 @@ static const struct option {
 	  "		[ xcvr internal|external ]\n"
 	  "		[ wol p|u|m|b|a|g|s|d... ]\n"
 	  "		[ sopass %x:%x:%x:%x:%x:%x ]\n"
+	  "		[ filters %x ]\n"
 	  "		[ msglvl %d | msglvl type on|off ... ]\n" },
 	{ "-a|--show-pause", 1, do_gpause, "Show pause options" },
 	{ "-A|--pause", 1, do_spause, "Set pause options",
-- 
2.14.1

[PATCH net-next 1/7] net: dsa: bcm_sf2: Allow targeting CPU ports for CFP rules

[Florian Fainelli]

ds->enabled_port_mask only contains a bitmask of user-facing enabled
ports, we also need to allow programming CFP rules that target CPU ports
(e.g: ports 5 and 8).

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2_cfp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
index b89acaee12d4..1a2b4e16aa13 100644
--- a/drivers/net/dsa/bcm_sf2_cfp.c
+++ b/drivers/net/dsa/bcm_sf2_cfp.c
@@ -755,7 +755,8 @@ static int bcm_sf2_cfp_rule_set(struct dsa_switch *ds, int port,
 	port_num = fs->ring_cookie / SF2_NUM_EGRESS_QUEUES;
 
 	if (fs->ring_cookie == RX_CLS_FLOW_DISC ||
-	    !dsa_is_user_port(ds, port_num) ||
+	    !(dsa_is_user_port(ds, port_num) ||
+	    dsa_is_cpu_port(ds, port_num)) ||
 	    port_num >= priv->hw_params.num_ports)
 		return -EINVAL;
 	/*
-- 
2.14.1

[PATCH net-next 2/7] net: dsa: bcm_sf2: Disable learning while in WoL

[Florian Fainelli]

When we are in Wake-on-LAN, we operate with the host sofware not running
a network stack, so we want to the switch to flood packets in order to
cause a system wake-up when matching specific filters (unicast or
multicast). This was not necessary before since we supported Magic
Packet which are targeting a broadcast MAC address which the switch
already floods.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c      | 12 +++++++++++-
 drivers/net/dsa/bcm_sf2_regs.h |  2 ++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index ac96ff40d37e..e0066adcd2f3 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -166,6 +166,11 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
 	reg &= ~P_TXQ_PSM_VDD(port);
 	core_writel(priv, reg, CORE_MEM_PSM_VDD_CTRL);
 
+	/* Enable learning */
+	reg = core_readl(priv, CORE_DIS_LEARN);
+	reg &= ~BIT(port);
+	core_writel(priv, reg, CORE_DIS_LEARN);
+
 	/* Enable Broadcom tags for that port if requested */
 	if (priv->brcm_tag_mask & BIT(port))
 		b53_brcm_hdr_setup(ds, port);
@@ -222,8 +227,13 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	u32 reg;
 
-	if (priv->wol_ports_mask & (1 << port))
+	/* Disable learning while in WoL mode */
+	if (priv->wol_ports_mask & (1 << port)) {
+		reg = core_readl(priv, CORE_DIS_LEARN);
+		reg |= BIT(port);
+		core_writel(priv, reg, CORE_DIS_LEARN);
 		return;
+	}
 
 	if (port == priv->moca_port)
 		bcm_sf2_port_intr_disable(priv, port);
diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
index 3ccd5a865dcb..0a1e530d52b7 100644
--- a/drivers/net/dsa/bcm_sf2_regs.h
+++ b/drivers/net/dsa/bcm_sf2_regs.h
@@ -168,6 +168,8 @@ enum bcm_sf2_reg_offs {
 #define CORE_SWITCH_CTRL		0x00088
 #define  MII_DUMB_FWDG_EN		(1 << 6)
 
+#define CORE_DIS_LEARN			0x000f0
+
 #define CORE_SFT_LRN_CTRL		0x000f8
 #define  SW_LEARN_CNTL(x)		(1 << (x))
 
-- 
2.14.1

[PATCH net-next 3/7] net: systemport: Do not re-configure upon WoL interrupt

[Florian Fainelli]

We already properly resume from Wake-on-LAN whether such a condition
occured or not, no need to process the WoL interrupt for functional
changes since that could race with other settings.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index eb890c4b3b2d..f152826f3d06 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1102,10 +1102,8 @@ static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
 	if (priv->irq0_stat & INTRL2_0_TX_RING_FULL)
 		bcm_sysport_tx_reclaim_all(priv);
 
-	if (priv->irq0_stat & INTRL2_0_MPD) {
+	if (priv->irq0_stat & INTRL2_0_MPD)
 		netdev_info(priv->netdev, "Wake-on-LAN interrupt!\n");
-		bcm_sysport_resume_from_wol(priv);
-	}
 
 	if (!priv->is_lite)
 		goto out;
-- 
2.14.1

[PATCH net-next 4/7] net: systemport: Create helper to set MPD

[Florian Fainelli]

Create a helper function to turn on/off MPD, this will be used to avoid
duplicating code as we are going to add additional types of wake-up
types.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index f152826f3d06..511caec7030a 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1041,17 +1041,25 @@ static int bcm_sysport_poll(struct napi_struct *napi, int budget)
 	return work_done;
 }
 
-static void bcm_sysport_resume_from_wol(struct bcm_sysport_priv *priv)
+static void mpd_enable_set(struct bcm_sysport_priv *priv, bool enable)
 {
 	u32 reg;
 
+	reg = umac_readl(priv, UMAC_MPD_CTRL);
+	if (enable)
+		reg |= MPD_EN;
+	else
+		reg &= ~MPD_EN;
+	umac_writel(priv, reg, UMAC_MPD_CTRL);
+}
+
+static void bcm_sysport_resume_from_wol(struct bcm_sysport_priv *priv)
+{
 	/* Stop monitoring MPD interrupt */
 	intrl2_0_mask_set(priv, INTRL2_0_MPD);
 
 	/* Clear the MagicPacket detection logic */
-	reg = umac_readl(priv, UMAC_MPD_CTRL);
-	reg &= ~MPD_EN;
-	umac_writel(priv, reg, UMAC_MPD_CTRL);
+	mpd_enable_set(priv, false);
 
 	netif_dbg(priv, wol, priv->netdev, "resumed from WOL\n");
 }
@@ -2447,9 +2455,7 @@ static int bcm_sysport_suspend_to_wol(struct bcm_sysport_priv *priv)
 
 	/* Do not leave the UniMAC RBUF matching only MPD packets */
 	if (!timeout) {
-		reg = umac_readl(priv, UMAC_MPD_CTRL);
-		reg &= ~MPD_EN;
-		umac_writel(priv, reg, UMAC_MPD_CTRL);
+		mpd_enable_set(priv, false);
 		netif_err(priv, wol, ndev, "failed to enter WOL mode\n");
 		return -ETIMEDOUT;
 	}
-- 
2.14.1

[PATCH net-next 5/7] ethtool: Add WAKE_FILTER bitmask

[Florian Fainelli]

Add the ability to specify that a filter, programmed through
ethtool::rxnfc will be used as a wake-up source. sopass which is a
48-bit wide storage is used to indicate which filters (as bits) can be
used for wake-up.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/uapi/linux/ethtool.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 4ca65b56084f..59e35f0ca9eb 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -201,7 +201,7 @@ struct ethtool_drvinfo {
  * @supported: Bitmask of %WAKE_* flags for supported Wake-On-Lan modes.
  *	Read-only.
  * @wolopts: Bitmask of %WAKE_* flags for enabled Wake-On-Lan modes.
- * @sopass: SecureOn(tm) password; meaningful only if %WAKE_MAGICSECURE
+ * @sopass: SecureOn(tm) password; meaningful only if %WAKE_MAGICSECURE or %WAKE_FILTER
  *	is set in @wolopts.
  */
 struct ethtool_wolinfo {
@@ -1634,6 +1634,7 @@ static inline int ethtool_validate_duplex(__u8 duplex)
 #define WAKE_ARP		(1 << 4)
 #define WAKE_MAGIC		(1 << 5)
 #define WAKE_MAGICSECURE	(1 << 6) /* only meaningful if WAKE_MAGIC */
+#define WAKE_FILTER		(1 << 7)
 
 /* L2-L4 network traffic flow types */
 #define	TCP_V4_FLOW	0x01	/* hash or spec (tcp_ip4_spec) */
-- 
2.14.1

[PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Florian Fainelli]

The SYSTEMPORT MAC allows up to 8 filters to be programmed to wake-up
from LAN. Verify that we have up to 8 filters and program them to the
appropriate RXCHK entries to be matched (along with their masks).

We need to update the entry and exit to Wake-on-LAN mode to keep the
RXCHK engine running to match during suspend, but this is otherwise
fairly similar to Magic Packet detection.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 111 +++++++++++++++++++++++++----
 drivers/net/ethernet/broadcom/bcmsysport.h |  14 +++-
 2 files changed, 109 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 511caec7030a..8d7ce3df1080 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -521,25 +521,31 @@ static void bcm_sysport_get_wol(struct net_device *dev,
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
 	u32 reg;
 
-	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE;
+	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
 	wol->wolopts = priv->wolopts;
 
-	if (!(priv->wolopts & WAKE_MAGICSECURE))
-		return;
+	if (priv->wolopts & WAKE_MAGICSECURE) {
+		/* Return the programmed SecureOn password */
+		reg = umac_readl(priv, UMAC_PSW_MS);
+		put_unaligned_be16(reg, &wol->sopass[0]);
+		reg = umac_readl(priv, UMAC_PSW_LS);
+		put_unaligned_be32(reg, &wol->sopass[2]);
+	}
 
-	/* Return the programmed SecureOn password */
-	reg = umac_readl(priv, UMAC_PSW_MS);
-	put_unaligned_be16(reg, &wol->sopass[0]);
-	reg = umac_readl(priv, UMAC_PSW_LS);
-	put_unaligned_be32(reg, &wol->sopass[2]);
+	if (priv->wolopts & WAKE_FILTER)
+		bitmap_copy((unsigned long *)wol->sopass, priv->filters,
+			    WAKE_FILTER_BITS);
 }
 
+
 static int bcm_sysport_set_wol(struct net_device *dev,
 			       struct ethtool_wolinfo *wol)
 {
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
 	struct device *kdev = &priv->pdev->dev;
-	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE;
+	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
+	unsigned int index, i = 0;
+	u32 reg;
 
 	if (!device_can_wakeup(kdev))
 		return -ENOTSUPP;
@@ -555,6 +561,32 @@ static int bcm_sysport_set_wol(struct net_device *dev,
 			    UMAC_PSW_LS);
 	}
 
+	/* We support matching up to 8 filters only */
+	if (wol->wolopts & WAKE_FILTER) {
+		bitmap_copy(priv->filters, (unsigned long *)wol->sopass,
+			    WAKE_FILTER_BITS);
+
+		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) >
+				  RXCHK_BRCM_TAG_MAX) {
+			bitmap_zero(priv->filters, WAKE_FILTER_BITS);
+			return -ENOSPC;
+		}
+
+		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) == 0)
+			return -EINVAL;
+
+		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
+			/* Write the index we want to match within the CID field */
+			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
+			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
+				 RXCHK_BRCM_TAG_CID_SHIFT);
+			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
+			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
+			rxchk_writel(priv, 0xff00ffff, RXCHK_BRCM_TAG_MASK(i));
+			i++;
+		}
+	}
+
 	/* Flag the device and relevant IRQ as wakeup capable */
 	if (wol->wolopts) {
 		device_set_wakeup_enable(kdev, 1);
@@ -1043,7 +1075,7 @@ static int bcm_sysport_poll(struct napi_struct *napi, int budget)
 
 static void mpd_enable_set(struct bcm_sysport_priv *priv, bool enable)
 {
-	u32 reg;
+	u32 reg, bit;
 
 	reg = umac_readl(priv, UMAC_MPD_CTRL);
 	if (enable)
@@ -1051,12 +1083,32 @@ static void mpd_enable_set(struct bcm_sysport_priv *priv, bool enable)
 	else
 		reg &= ~MPD_EN;
 	umac_writel(priv, reg, UMAC_MPD_CTRL);
+
+	if (priv->is_lite)
+		bit = RBUF_ACPI_EN_LITE;
+	else
+		bit = RBUF_ACPI_EN;
+
+	reg = rbuf_readl(priv, RBUF_CONTROL);
+	if (enable)
+		reg |= bit;
+	else
+		reg &= ~bit;
+	rbuf_writel(priv, reg, RBUF_CONTROL);
 }
 
 static void bcm_sysport_resume_from_wol(struct bcm_sysport_priv *priv)
 {
+	u32 reg;
+
 	/* Stop monitoring MPD interrupt */
-	intrl2_0_mask_set(priv, INTRL2_0_MPD);
+	intrl2_0_mask_set(priv, INTRL2_0_MPD | INTRL2_0_BRCM_MATCH_TAG);
+
+	/* Disable RXCHK, active filters and Broadcom tag matching */
+	reg = rxchk_readl(priv, RXCHK_CONTROL);
+	reg &= ~(RXCHK_BRCM_TAG_MATCH_MASK <<
+		 RXCHK_BRCM_TAG_MATCH_SHIFT | RXCHK_EN | RXCHK_BRCM_TAG_EN);
+	rxchk_writel(priv, reg, RXCHK_CONTROL);
 
 	/* Clear the MagicPacket detection logic */
 	mpd_enable_set(priv, false);
@@ -1085,6 +1137,7 @@ static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
 	struct bcm_sysport_priv *priv = netdev_priv(dev);
 	struct bcm_sysport_tx_ring *txr;
 	unsigned int ring, ring_bit;
+	u32 reg;
 
 	priv->irq0_stat = intrl2_0_readl(priv, INTRL2_CPU_STATUS) &
 			  ~intrl2_0_readl(priv, INTRL2_CPU_MASK_STATUS);
@@ -1111,7 +1164,14 @@ static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
 		bcm_sysport_tx_reclaim_all(priv);
 
 	if (priv->irq0_stat & INTRL2_0_MPD)
-		netdev_info(priv->netdev, "Wake-on-LAN interrupt!\n");
+		netdev_info(priv->netdev, "Wake-on-LAN (MPD) interrupt!\n");
+
+	if (priv->irq0_stat & INTRL2_0_BRCM_MATCH_TAG) {
+		reg = rxchk_readl(priv, RXCHK_BRCM_TAG_MATCH_STATUS) &
+				  RXCHK_BRCM_TAG_MATCH_MASK;
+		netdev_info(priv->netdev,
+			    "Wake-on-LAN (filters 0x%02x) interrupt!\n", reg);
+	}
 
 	if (!priv->is_lite)
 		goto out;
@@ -2434,16 +2494,39 @@ static int bcm_sysport_suspend_to_wol(struct bcm_sysport_priv *priv)
 {
 	struct net_device *ndev = priv->netdev;
 	unsigned int timeout = 1000;
+	unsigned int index, i = 0;
 	u32 reg;
 
 	/* Password has already been programmed */
 	reg = umac_readl(priv, UMAC_MPD_CTRL);
-	reg |= MPD_EN;
+	if (priv->wolopts & (WAKE_MAGIC | WAKE_MAGICSECURE))
+		reg |= MPD_EN;
 	reg &= ~PSW_EN;
 	if (priv->wolopts & WAKE_MAGICSECURE)
 		reg |= PSW_EN;
 	umac_writel(priv, reg, UMAC_MPD_CTRL);
 
+	if (priv->wolopts & WAKE_FILTER) {
+		/* Turn on ACPI matching to steal packets from RBUF */
+		reg = rbuf_readl(priv, RBUF_CONTROL);
+		if (priv->is_lite)
+			reg |= RBUF_ACPI_EN_LITE;
+		else
+			reg |= RBUF_ACPI_EN;
+		rbuf_writel(priv, reg, RBUF_CONTROL);
+
+		/* Enable RXCHK, active filters and Broadcom tag matching */
+		reg = rxchk_readl(priv, RXCHK_CONTROL);
+		reg &= ~(RXCHK_BRCM_TAG_MATCH_MASK <<
+			 RXCHK_BRCM_TAG_MATCH_SHIFT);
+		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
+			reg |= BIT(RXCHK_BRCM_TAG_MATCH_SHIFT + i);
+			i++;
+		}
+		reg |= RXCHK_EN | RXCHK_BRCM_TAG_EN;
+		rxchk_writel(priv, reg, RXCHK_CONTROL);
+	}
+
 	/* Make sure RBUF entered WoL mode as result */
 	do {
 		reg = rbuf_readl(priv, RBUF_STATUS);
@@ -2464,7 +2547,7 @@ static int bcm_sysport_suspend_to_wol(struct bcm_sysport_priv *priv)
 	umac_enable_set(priv, CMD_RX_EN, 1);
 
 	/* Enable the interrupt wake-up source */
-	intrl2_0_mask_clear(priv, INTRL2_0_MPD);
+	intrl2_0_mask_clear(priv, INTRL2_0_MPD | INTRL2_0_BRCM_MATCH_TAG);
 
 	netif_dbg(priv, wol, ndev, "entered WOL mode\n");
 
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index d6e5d0cbf3a3..6a64bd8cf787 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -11,6 +11,8 @@
 #ifndef __BCM_SYSPORT_H
 #define __BCM_SYSPORT_H
 
+#include <linux/bitmap.h>
+#include <linux/ethtool.h>
 #include <linux/if_vlan.h>
 #include <linux/net_dim.h>
 
@@ -155,14 +157,18 @@ struct bcm_rsb {
 #define  RXCHK_PARSE_AUTH		(1 << 22)
 
 #define RXCHK_BRCM_TAG0			0x04
-#define RXCHK_BRCM_TAG(i)		((i) * RXCHK_BRCM_TAG0)
+#define RXCHK_BRCM_TAG(i)		((i) * 0x4 + RXCHK_BRCM_TAG0)
 #define RXCHK_BRCM_TAG0_MASK		0x24
-#define RXCHK_BRCM_TAG_MASK(i)		((i) * RXCHK_BRCM_TAG0_MASK)
+#define RXCHK_BRCM_TAG_MASK(i)		((i) * 0x4 + RXCHK_BRCM_TAG0_MASK)
 #define RXCHK_BRCM_TAG_MATCH_STATUS	0x44
 #define RXCHK_ETHERTYPE			0x48
 #define RXCHK_BAD_CSUM_CNTR		0x4C
 #define RXCHK_OTHER_DISC_CNTR		0x50
 
+#define RXCHK_BRCM_TAG_MAX		8
+#define RXCHK_BRCM_TAG_CID_SHIFT	16
+#define RXCHK_BRCM_TAG_CID_MASK		0xff
+
 /* TXCHCK offsets and defines */
 #define SYS_PORT_TXCHK_OFFSET		0x380
 #define TXCHK_PKT_RDY_THRESH		0x00
@@ -185,6 +191,7 @@ struct bcm_rsb {
 #define  RBUF_RSB_SWAP0			(1 << 22)
 #define  RBUF_RSB_SWAP1			(1 << 23)
 #define  RBUF_ACPI_EN			(1 << 23)
+#define  RBUF_ACPI_EN_LITE		(1 << 24)
 
 #define RBUF_PKT_RDY_THRESH		0x04
 
@@ -524,6 +531,8 @@ struct dma_desc {
 
 #define WORDS_PER_DESC			(sizeof(struct dma_desc) / sizeof(u32))
 
+#define WAKE_FILTER_BITS		(SOPASS_MAX * BITS_PER_BYTE)
+
 /* Rx/Tx common counter group.*/
 struct bcm_sysport_pkt_counters {
 	u32	cnt_64;		/* RO Received/Transmited 64 bytes packet */
@@ -776,6 +785,7 @@ struct bcm_sysport_priv {
 
 	/* Ethtool */
 	u32			msg_enable;
+	DECLARE_BITMAP(filters, SOPASS_MAX * BITS_PER_BYTE);
 
 	struct bcm_sysport_stats64	stats64;
 
-- 
2.14.1

[PATCH net-next 7/7] net: dsa: bcm_sf2: Support WAKE_FILTER

[Florian Fainelli]

Propagate wol->sopass when WAKE_FILTER is set since that contains the
bitmap of filters to be enabled for wake-up.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index e0066adcd2f3..ca562cf6ffd2 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -741,7 +741,7 @@ static void bcm_sf2_sw_get_wol(struct dsa_switch *ds, int port,
 	wol->supported = pwol.supported;
 	memset(&wol->sopass, 0, sizeof(wol->sopass));
 
-	if (pwol.wolopts & WAKE_MAGICSECURE)
+	if (pwol.wolopts & (WAKE_MAGICSECURE | WAKE_FILTER))
 		memcpy(&wol->sopass, pwol.sopass, sizeof(wol->sopass));
 
 	if (priv->wol_ports_mask & (1 << port))
-- 
2.14.1

Re: [PATCH net-next 0/7] net: Support Wake-on-LAN using filters

[Andrew Lunn]

On Tue, Jul 17, 2018 at 08:36:37AM -0700, Florian Fainelli wrote:
> Hi all,
> 
> This patch series adds support for allowing Wake-on-LAN to wake-up the
> system using configurable filters. This is particular useful in the context
> of Android where wake on MDNS is a requirement.
> 
> We support this by using the bcm_sf2 Compact Field Processor (CFP) which
> supports matching packets and tagging them with an unique identifier
> (Classification ID) that is added in each packet being matched through the use
> of Broadcom tags. The SYSTEMPORT MAC attached to that switch is then used to
> match that unique identifier and trigger a system wake-up event.
> 
> Last patch is the ethtool modifications to support that feature.
> 
> Example:
> 
> ethtool --config-nfc gphy flow-type udp4 src-ip 192.168.1.1 dst-ip 192.168.1.32 \
> 	src-port 1234 dst-port 5678 action 64
> Added rule with ID 1

Hi Florian

What is action 64?
 
> ethtool -s gphy wol f filters 0x2

What does this 0x2 represent?

These magic numbers are not so nice.

      Andrew

Re: [PATCH net-next 2/7] net: dsa: bcm_sf2: Disable learning while in WoL

[Andrew Lunn]

On Tue, Jul 17, 2018 at 08:36:40AM -0700, Florian Fainelli wrote:
> When we are in Wake-on-LAN, we operate with the host sofware not running
> a network stack, so we want to the switch to flood packets

Hi Florian

Just to be sure... 

By flood, you mean from a user port to the CPU. You don't mean
flooding between user ports.

	 Andrew

Re: [PATCH net-next 2/7] net: dsa: bcm_sf2: Disable learning while in WoL

[Florian Fainelli]

On 07/17/2018 08:54 AM, Andrew Lunn wrote:
> On Tue, Jul 17, 2018 at 08:36:40AM -0700, Florian Fainelli wrote:
>> When we are in Wake-on-LAN, we operate with the host sofware not running
>> a network stack, so we want to the switch to flood packets
> 
> Hi Florian
> 
> Just to be sure... 
> 
> By flood, you mean from a user port to the CPU. You don't mean
> flooding between user ports.

Yes, flooding from an user-port to the CPU port, that is, behaving like
a fully unmanaged switch during WoL.
-- 
Florian

Re: [PATCH net-next 0/7] net: Support Wake-on-LAN using filters

[Florian Fainelli]

On 07/17/2018 08:47 AM, Andrew Lunn wrote:
> On Tue, Jul 17, 2018 at 08:36:37AM -0700, Florian Fainelli wrote:
>> Hi all,
>>
>> This patch series adds support for allowing Wake-on-LAN to wake-up the
>> system using configurable filters. This is particular useful in the context
>> of Android where wake on MDNS is a requirement.
>>
>> We support this by using the bcm_sf2 Compact Field Processor (CFP) which
>> supports matching packets and tagging them with an unique identifier
>> (Classification ID) that is added in each packet being matched through the use
>> of Broadcom tags. The SYSTEMPORT MAC attached to that switch is then used to
>> match that unique identifier and trigger a system wake-up event.
>>
>> Last patch is the ethtool modifications to support that feature.
>>
>> Example:
>>
>> ethtool --config-nfc gphy flow-type udp4 src-ip 192.168.1.1 dst-ip 192.168.1.32 \
>> 	src-port 1234 dst-port 5678 action 64
>> Added rule with ID 1
> 
> Hi Florian
> 
> What is action 64?

There are 8 egress queues per port, and we want to target port 8 here.
Number of queues is discoverable through sysfs by scanning
/sys/class/net/gphy/queues/tx-*. CPU port number is fixed at 8, we don't
have a mechanism AFAICT to expose that to users, because, of course, we
don't expose the CPU port with DSA.

>  
>> ethtool -s gphy wol f filters 0x2
> 
> What does this 0x2 represent?

0x2 = bit 1 is set, which corresponds to the filter ID that was returned
from the previous ethtool::rxnfc command invocation. If ethtool
--config-nfc returned 3, then we would have used filters 0x8, etc.
-- 
Florian

Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Andrew Lunn]

On Tue, Jul 17, 2018 at 08:36:44AM -0700, Florian Fainelli wrote:
> The SYSTEMPORT MAC allows up to 8 filters to be programmed to wake-up
> from LAN. Verify that we have up to 8 filters and program them to the
> appropriate RXCHK entries to be matched (along with their masks).
> 
> We need to update the entry and exit to Wake-on-LAN mode to keep the
> RXCHK engine running to match during suspend, but this is otherwise
> fairly similar to Magic Packet detection.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 111 +++++++++++++++++++++++++----
>  drivers/net/ethernet/broadcom/bcmsysport.h |  14 +++-
>  2 files changed, 109 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 511caec7030a..8d7ce3df1080 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -521,25 +521,31 @@ static void bcm_sysport_get_wol(struct net_device *dev,
>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>  	u32 reg;
>  
> -	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE;
> +	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
>  	wol->wolopts = priv->wolopts;
>  
> -	if (!(priv->wolopts & WAKE_MAGICSECURE))
> -		return;
> +	if (priv->wolopts & WAKE_MAGICSECURE) {
> +		/* Return the programmed SecureOn password */
> +		reg = umac_readl(priv, UMAC_PSW_MS);
> +		put_unaligned_be16(reg, &wol->sopass[0]);
> +		reg = umac_readl(priv, UMAC_PSW_LS);
> +		put_unaligned_be32(reg, &wol->sopass[2]);
> +	}
>  
> -	/* Return the programmed SecureOn password */
> -	reg = umac_readl(priv, UMAC_PSW_MS);
> -	put_unaligned_be16(reg, &wol->sopass[0]);
> -	reg = umac_readl(priv, UMAC_PSW_LS);
> -	put_unaligned_be32(reg, &wol->sopass[2]);
> +	if (priv->wolopts & WAKE_FILTER)
> +		bitmap_copy((unsigned long *)wol->sopass, priv->filters,
> +			    WAKE_FILTER_BITS);
>  }
>  
> +
>  static int bcm_sysport_set_wol(struct net_device *dev,

Two blank lines...



>  			       struct ethtool_wolinfo *wol)
>  {
>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>  	struct device *kdev = &priv->pdev->dev;
> -	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE;
> +	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
> +	unsigned int index, i = 0;
> +	u32 reg;
>  
>  	if (!device_can_wakeup(kdev))
>  		return -ENOTSUPP;
> @@ -555,6 +561,32 @@ static int bcm_sysport_set_wol(struct net_device *dev,
>  			    UMAC_PSW_LS);
>  	}
>  
> +	/* We support matching up to 8 filters only */
> +	if (wol->wolopts & WAKE_FILTER) {
> +		bitmap_copy(priv->filters, (unsigned long *)wol->sopass,
> +			    WAKE_FILTER_BITS);

Shouldn't this be done after to the two checks for errors? Otherwise
you have unexpected side effects.

> +
> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) >
> +				  RXCHK_BRCM_TAG_MAX) {
> +			bitmap_zero(priv->filters, WAKE_FILTER_BITS);
> +			return -ENOSPC;
> +		}
> +
> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) == 0)
> +			return -EINVAL;
> +
> +		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
> +			/* Write the index we want to match within the CID field */
> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
> +				 RXCHK_BRCM_TAG_CID_SHIFT);
> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
> +			rxchk_writel(priv, 0xff00ffff, RXCHK_BRCM_TAG_MASK(i));
> +			i++;
> +		}
> +	}

How do you disable filters? It looks like you cannot pass all bits set
to 0. Also, how do you disable a specific filter? The code above seems
to be additive only. There does not appear to be a first write which
disables all existing filters before writing the new set of filters.

	 Andrew

Re: [PATCH net-next 0/7] net: Support Wake-on-LAN using filters

[Andrew Lunn]

> >> ethtool -s gphy wol f filters 0x2
> > 
> > What does this 0x2 represent?
> 
> 0x2 = bit 1 is set, which corresponds to the filter ID that was returned
> from the previous ethtool::rxnfc command invocation. If ethtool
> --config-nfc returned 3, then we would have used filters 0x8, etc.

It would be a simpler for the user if you could pass the filter IDs as
a list, and let ethtool do the shift and OR.

  Andrew

Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Florian Fainelli]

On 07/17/2018 09:14 AM, Andrew Lunn wrote:
> On Tue, Jul 17, 2018 at 08:36:44AM -0700, Florian Fainelli wrote:
>> The SYSTEMPORT MAC allows up to 8 filters to be programmed to wake-up
>> from LAN. Verify that we have up to 8 filters and program them to the
>> appropriate RXCHK entries to be matched (along with their masks).
>>
>> We need to update the entry and exit to Wake-on-LAN mode to keep the
>> RXCHK engine running to match during suspend, but this is otherwise
>> fairly similar to Magic Packet detection.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/bcmsysport.c | 111 +++++++++++++++++++++++++----
>>  drivers/net/ethernet/broadcom/bcmsysport.h |  14 +++-
>>  2 files changed, 109 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index 511caec7030a..8d7ce3df1080 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -521,25 +521,31 @@ static void bcm_sysport_get_wol(struct net_device *dev,
>>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>>  	u32 reg;
>>  
>> -	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE;
>> +	wol->supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
>>  	wol->wolopts = priv->wolopts;
>>  
>> -	if (!(priv->wolopts & WAKE_MAGICSECURE))
>> -		return;
>> +	if (priv->wolopts & WAKE_MAGICSECURE) {
>> +		/* Return the programmed SecureOn password */
>> +		reg = umac_readl(priv, UMAC_PSW_MS);
>> +		put_unaligned_be16(reg, &wol->sopass[0]);
>> +		reg = umac_readl(priv, UMAC_PSW_LS);
>> +		put_unaligned_be32(reg, &wol->sopass[2]);
>> +	}
>>  
>> -	/* Return the programmed SecureOn password */
>> -	reg = umac_readl(priv, UMAC_PSW_MS);
>> -	put_unaligned_be16(reg, &wol->sopass[0]);
>> -	reg = umac_readl(priv, UMAC_PSW_LS);
>> -	put_unaligned_be32(reg, &wol->sopass[2]);
>> +	if (priv->wolopts & WAKE_FILTER)
>> +		bitmap_copy((unsigned long *)wol->sopass, priv->filters,
>> +			    WAKE_FILTER_BITS);
>>  }
>>  
>> +
>>  static int bcm_sysport_set_wol(struct net_device *dev,
> 
> Two blank lines...
> 
> 
> 
>>  			       struct ethtool_wolinfo *wol)
>>  {
>>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>>  	struct device *kdev = &priv->pdev->dev;
>> -	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE;
>> +	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
>> +	unsigned int index, i = 0;
>> +	u32 reg;
>>  
>>  	if (!device_can_wakeup(kdev))
>>  		return -ENOTSUPP;
>> @@ -555,6 +561,32 @@ static int bcm_sysport_set_wol(struct net_device *dev,
>>  			    UMAC_PSW_LS);
>>  	}
>>  
>> +	/* We support matching up to 8 filters only */
>> +	if (wol->wolopts & WAKE_FILTER) {
>> +		bitmap_copy(priv->filters, (unsigned long *)wol->sopass,
>> +			    WAKE_FILTER_BITS);
> 
> Shouldn't this be done after to the two checks for errors? Otherwise
> you have unexpected side effects.

How would you use the bitmap_* routines if you don't copy the bitmap
first? Besides, if the bitmap is too wide (next check), we zero it out,
so nothing will get programmed if we attempt a Wake-on-LAN suspend (and
priv->wolopts is not copied anyway) and the second check would reject a
zero bitmap as well.

> 
>> +
>> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) >
>> +				  RXCHK_BRCM_TAG_MAX) {
>> +			bitmap_zero(priv->filters, WAKE_FILTER_BITS);
>> +			return -ENOSPC;
>> +		}
>> +
>> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) == 0)
>> +			return -EINVAL;
>> +
>> +		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
>> +			/* Write the index we want to match within the CID field */
>> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
>> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
>> +				 RXCHK_BRCM_TAG_CID_SHIFT);
>> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
>> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
>> +			rxchk_writel(priv, 0xff00ffff, RXCHK_BRCM_TAG_MASK(i));
>> +			i++;
>> +		}
>> +	}
> 
> How do you disable filters? It looks like you cannot pass all bits set
> to 0. Also, how do you disable a specific filter? The code above seems
> to be additive only. There does not appear to be a first write which
> disables all existing filters before writing the new set of filters.

Either you disable WoL entirely (ethtool -s gphy wol d) and then we
don't put the hardware in a state that allows it to wake-up the system,
or you re-program a different set of filters by re-sending a new bitmask
of desired filters. This is not different from how you program/unprogram
MagicPacket with SecureOn password.
-- 
Florian

Re: [PATCH net-next 0/7] net: Support Wake-on-LAN using filters

[Florian Fainelli]

On 07/17/2018 09:21 AM, Andrew Lunn wrote:
>>>> ethtool -s gphy wol f filters 0x2
>>>
>>> What does this 0x2 represent?
>>
>> 0x2 = bit 1 is set, which corresponds to the filter ID that was returned
>> from the previous ethtool::rxnfc command invocation. If ethtool
>> --config-nfc returned 3, then we would have used filters 0x8, etc.
> 
> It would be a simpler for the user if you could pass the filter IDs as
> a list, and let ethtool do the shift and OR.

Sure, I can definitively add that, is a comma separator okay with you
for that? E.g:

ethtool -s gphy wol f filters 0,1,2,3,4,5,6,7
-- 
Florian

Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Andrew Lunn]

> >>  			       struct ethtool_wolinfo *wol)
> >>  {
> >>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
> >>  	struct device *kdev = &priv->pdev->dev;
> >> -	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE;
> >> +	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
> >> +	unsigned int index, i = 0;
> >> +	u32 reg;
> >>  
> >>  	if (!device_can_wakeup(kdev))
> >>  		return -ENOTSUPP;
> >> @@ -555,6 +561,32 @@ static int bcm_sysport_set_wol(struct net_device *dev,
> >>  			    UMAC_PSW_LS);
> >>  	}
> >>  
> >> +	/* We support matching up to 8 filters only */
> >> +	if (wol->wolopts & WAKE_FILTER) {
> >> +		bitmap_copy(priv->filters, (unsigned long *)wol->sopass,
> >> +			    WAKE_FILTER_BITS);
> > 
> > Shouldn't this be done after to the two checks for errors? Otherwise
> > you have unexpected side effects.
> 
> How would you use the bitmap_* routines if you don't copy the bitmap
> first? Besides, if the bitmap is too wide (next check), we zero it out,
> so nothing will get programmed if we attempt a Wake-on-LAN suspend (and
> priv->wolopts is not copied anyway) and the second check would reject a
> zero bitmap as well.

Zero'ing it is a side effect. get_wol() will now return that no
filtered are programmed. However, it appears the hardware is still
programmed with the old filters. Maybe there is a 

rxchk_writel(priv, 0, RXCHK_BRCM_TAG(i)

hiding in this code somewhere, clearing out the old bits, but i don't
see it.

> 
> > 
> >> +
> >> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) >
> >> +				  RXCHK_BRCM_TAG_MAX) {
> >> +			bitmap_zero(priv->filters, WAKE_FILTER_BITS);
> >> +			return -ENOSPC;
> >> +		}
> >> +
> >> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) == 0)
> >> +			return -EINVAL;
> >> +
> >> +		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
> >> +			/* Write the index we want to match within the CID field */
> >> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
> >> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
> >> +				 RXCHK_BRCM_TAG_CID_SHIFT);
> >> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
> >> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
> >> +			rxchk_writel(priv, 0xff00ffff, RXCHK_BRCM_TAG_MASK(i));
> >> +			i++;
> >> +		}
> >> +	}
> > 
> > How do you disable filters? It looks like you cannot pass all bits set
> > to 0. Also, how do you disable a specific filter? The code above seems
> > to be additive only. There does not appear to be a first write which
> > disables all existing filters before writing the new set of filters.
> 
> Either you disable WoL entirely (ethtool -s gphy wol d) and then we
> don't put the hardware in a state that allows it to wake-up the system,
> or you re-program a different set of filters by re-sending a new bitmask
> of desired filters.

This appears to be read-modify-write:

> >> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
> >> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
> >> +				 RXCHK_BRCM_TAG_CID_SHIFT);
> >> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
> >> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));

It looks like you can add more bits, but i don't see any way to clear
bits. As i said above, there might be an initial write of 0, but i
cannot see it. The obvious place for it would be just before the
for_each_set_bit(), or at the beginning of the function.

    Andrew

Re: [PATCH net-next 0/7] net: Support Wake-on-LAN using filters

[Andrew Lunn]

On Tue, Jul 17, 2018 at 09:28:52AM -0700, Florian Fainelli wrote:
> 
> 
> On 07/17/2018 09:21 AM, Andrew Lunn wrote:
> >>>> ethtool -s gphy wol f filters 0x2
> >>>
> >>> What does this 0x2 represent?
> >>
> >> 0x2 = bit 1 is set, which corresponds to the filter ID that was returned
> >> from the previous ethtool::rxnfc command invocation. If ethtool
> >> --config-nfc returned 3, then we would have used filters 0x8, etc.
> > 
> > It would be a simpler for the user if you could pass the filter IDs as
> > a list, and let ethtool do the shift and OR.
> 
> Sure, I can definitively add that, is a comma separator okay with you
> for that? E.g:
> 
> ethtool -s gphy wol f filters 0,1,2,3,4,5,6,7

Hi Florian

That looks good. But maybe check the man page for ethtool and see if
there is an established convention.

      Andrew

Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Florian Fainelli]

On 07/17/2018 09:49 AM, Andrew Lunn wrote:
>>>>  			       struct ethtool_wolinfo *wol)
>>>>  {
>>>>  	struct bcm_sysport_priv *priv = netdev_priv(dev);
>>>>  	struct device *kdev = &priv->pdev->dev;
>>>> -	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE;
>>>> +	u32 supported = WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER;
>>>> +	unsigned int index, i = 0;
>>>> +	u32 reg;
>>>>  
>>>>  	if (!device_can_wakeup(kdev))
>>>>  		return -ENOTSUPP;
>>>> @@ -555,6 +561,32 @@ static int bcm_sysport_set_wol(struct net_device *dev,
>>>>  			    UMAC_PSW_LS);
>>>>  	}
>>>>  
>>>> +	/* We support matching up to 8 filters only */
>>>> +	if (wol->wolopts & WAKE_FILTER) {
>>>> +		bitmap_copy(priv->filters, (unsigned long *)wol->sopass,
>>>> +			    WAKE_FILTER_BITS);
>>>
>>> Shouldn't this be done after to the two checks for errors? Otherwise
>>> you have unexpected side effects.
>>
>> How would you use the bitmap_* routines if you don't copy the bitmap
>> first? Besides, if the bitmap is too wide (next check), we zero it out,
>> so nothing will get programmed if we attempt a Wake-on-LAN suspend (and
>> priv->wolopts is not copied anyway) and the second check would reject a
>> zero bitmap as well.
> 
> Zero'ing it is a side effect. get_wol() will now return that no
> filtered are programmed. However, it appears the hardware is still
> programmed with the old filters. Maybe there is a 
> 
> rxchk_writel(priv, 0, RXCHK_BRCM_TAG(i)
> 
> hiding in this code somewhere, clearing out the old bits, but i don't
> see it.

It is not necessary to clear those registers for a number of reasons:

- they are only active if the corresponding bit to enable those is also
programmed in RXHCK_CONTROL, which is only done during
bcm_sysport_suspend_to_wol(), though I suppose for safety one could be
moving the RXCHK_BRCM_TAG_MATCH_MASK clearing outside of the WAKE_FILTER
check though again, not necessary because:
	- HW starts with those bits cleared
	- if you entered WoL once with any of those filters, we would be
clearing those bits again during WoL resume

- if WoL is disabled, we don't even enable network ports to forward
traffic and remotely allow a packet to enter the switch (see
drivers/net/dsa/bcm_sf2.c)

> 
>>
>>>
>>>> +
>>>> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) >
>>>> +				  RXCHK_BRCM_TAG_MAX) {
>>>> +			bitmap_zero(priv->filters, WAKE_FILTER_BITS);
>>>> +			return -ENOSPC;
>>>> +		}
>>>> +
>>>> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) == 0)
>>>> +			return -EINVAL;
>>>> +
>>>> +		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
>>>> +			/* Write the index we want to match within the CID field */
>>>> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
>>>> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
>>>> +				 RXCHK_BRCM_TAG_CID_SHIFT);
>>>> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
>>>> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
>>>> +			rxchk_writel(priv, 0xff00ffff, RXCHK_BRCM_TAG_MASK(i));
>>>> +			i++;
>>>> +		}
>>>> +	}
>>>
>>> How do you disable filters? It looks like you cannot pass all bits set
>>> to 0. Also, how do you disable a specific filter? The code above seems
>>> to be additive only. There does not appear to be a first write which
>>> disables all existing filters before writing the new set of filters.
>>
>> Either you disable WoL entirely (ethtool -s gphy wol d) and then we
>> don't put the hardware in a state that allows it to wake-up the system,
>> or you re-program a different set of filters by re-sending a new bitmask
>> of desired filters.
> 
> This appears to be read-modify-write:
> 
>>>> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
>>>> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
>>>> +				 RXCHK_BRCM_TAG_CID_SHIFT);
>>>> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
>>>> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
> 
> It looks like you can add more bits, but i don't see any way to clear
> bits. As i said above, there might be an initial write of 0, but i
> cannot see it. The obvious place for it would be just before the
> for_each_set_bit(), or at the beginning of the function.

We are only programming the HW to be matching bits [23:16] and with a
corresponding mask of 0xff00_ffff so even if these bits contained
garbage, they would not be matched by the HW.
-- 
Florian

Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Andrew Lunn]

> >>>> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) >
> >>>> +				  RXCHK_BRCM_TAG_MAX) {
> >>>> +			bitmap_zero(priv->filters, WAKE_FILTER_BITS);
> >>>> +			return -ENOSPC;
> >>>> +		}
> >>>> +
> >>>> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) == 0)
> >>>> +			return -EINVAL;
> >>>> +
> >>>> +		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
> >>>> +			/* Write the index we want to match within the CID field */
> >>>> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
> >>>> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
> >>>> +				 RXCHK_BRCM_TAG_CID_SHIFT);
> >>>> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
> >>>> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
> >>>> +			rxchk_writel(priv, 0xff00ffff, RXCHK_BRCM_TAG_MASK(i));
> >>>> +			i++;
> >>>> +		}
> >>>> +	}
> >>>

Just to convince me, can you dump the contents of reg. And execute the
commands:

ethtool -s gphy wol f filters 0x1
ethtool -s gphy wol f filters 0x2

After these two commands, we expect only one filter bit to be set, bit
1. Filter Bit 0 should of been cleared when the second command was
executed.

  Andrew

Re: [PATCH net-next 1/7] net: dsa: bcm_sf2: Allow targeting CPU ports for CFP rules

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 17 Jul 2018 08:36:39 -0700

> @@ -755,7 +755,8 @@ static int bcm_sf2_cfp_rule_set(struct dsa_switch *ds, int port,
>  	port_num = fs->ring_cookie / SF2_NUM_EGRESS_QUEUES;
>  
>  	if (fs->ring_cookie == RX_CLS_FLOW_DISC ||
> -	    !dsa_is_user_port(ds, port_num) ||
> +	    !(dsa_is_user_port(ds, port_num) ||
> +	    dsa_is_cpu_port(ds, port_num)) ||

I think the second new line needs to be indented by two more
spaces, but I could be wrong :-)

Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Florian Fainelli]

On 07/17/2018 10:06 AM, Andrew Lunn wrote:
>>>>>> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) >
>>>>>> +				  RXCHK_BRCM_TAG_MAX) {
>>>>>> +			bitmap_zero(priv->filters, WAKE_FILTER_BITS);
>>>>>> +			return -ENOSPC;
>>>>>> +		}
>>>>>> +
>>>>>> +		if (bitmap_weight(priv->filters, WAKE_FILTER_BITS) == 0)
>>>>>> +			return -EINVAL;
>>>>>> +
>>>>>> +		for_each_set_bit(index, priv->filters, WAKE_FILTER_BITS) {
>>>>>> +			/* Write the index we want to match within the CID field */
>>>>>> +			reg = rxchk_readl(priv, RXCHK_BRCM_TAG(i));
>>>>>> +			reg &= ~(RXCHK_BRCM_TAG_CID_MASK <<
>>>>>> +				 RXCHK_BRCM_TAG_CID_SHIFT);
>>>>>> +			reg |= index << RXCHK_BRCM_TAG_CID_SHIFT;
>>>>>> +			rxchk_writel(priv, reg, RXCHK_BRCM_TAG(i));
>>>>>> +			rxchk_writel(priv, 0xff00ffff, RXCHK_BRCM_TAG_MASK(i));
>>>>>> +			i++;
>>>>>> +		}
>>>>>> +	}
>>>>>
> 
> Just to convince me, can you dump the contents of reg. And execute the
> commands:
> 
> ethtool -s gphy wol f filters 0x1
> ethtool -s gphy wol f filters 0x2
> 
> After these two commands, we expect only one filter bit to be set, bit
> 1. Filter Bit 0 should of been cleared when the second command was
> executed.

In both of your examples, only one bit is set, what will change is the
value being programmed to RXHCK_BRCM_TAG(i), which will be either 0, or
1, but the value programmed to RXCHK_CONTROL as far as which filter is
enabled will be the same because we can use filter position 0.

What the code basically does is look at how many bits are set in the
filters bitmap, and then it starts populating the filters from filter 0
up to filter 7 with the value of the bit.

Dumps below are:
<physiscal offset>,<value>,<constant name>

ethtool -s gphy wol f filters 0x1:

0x09400300,0x01d91019,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_CONTROL
0x09400304,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_0
0x09400308,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_1
0x0940030c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_2
0x09400310,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_3
0x09400314,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_4
0x09400318,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_5
0x0940031c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_6
0x09400320,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_7
0x09400324,0xff00ffff,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_0
0x09400328,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_1
0x0940032c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_2
0x09400330,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_3
0x09400334,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_4
0x09400338,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_5
0x0940033c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_6
0x09400340,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_7
0x09400344,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MATCH_STATUS
0x09400348,0x88a88100,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_ETHERTYPE
0x0940034c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BAD_CHECKSUM_PACKET_DISCARD_COUNTER
0x09400350,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_OTHER_PACKET_DISCARD_COUNTER

ethtool -s gphy wol f filters 0x2:

0x09400300,0x01d91019,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_CONTROL
0x09400304,0x00010000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_0
0x09400308,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_1
0x0940030c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_2
0x09400310,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_3
0x09400314,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_4
0x09400318,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_5
0x0940031c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_6
0x09400320,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_7
0x09400324,0xff00ffff,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_0
0x09400328,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_1
0x0940032c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_2
0x09400330,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_3
0x09400334,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_4
0x09400338,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_5
0x0940033c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_6
0x09400340,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_7
0x09400344,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MATCH_STATUS
0x09400348,0x88a88100,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_ETHERTYPE
0x0940034c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BAD_CHECKSUM_PACKET_DISCARD_COUNTER
0x09400350,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_OTHER_PACKET_DISCARD_COUNTER

As you can see, the only thing that changes here is that we re-used
BRCM_TAG_0 but programmed a different CID value in there which is the
bit position within the filter.

ethtool -s gphy wol f filters 0x3

0x09400300,0x01d91039,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_CONTROL
0x09400304,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_0
0x09400308,0x00010000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_1
0x0940030c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_2
0x09400310,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_3
0x09400314,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_4
0x09400318,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_5
0x0940031c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_6
0x09400320,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_7
0x09400324,0xff00ffff,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_0
0x09400328,0xff00ffff,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_1
0x0940032c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_2
0x09400330,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_3
0x09400334,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_4
0x09400338,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_5
0x0940033c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_6
0x09400340,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_7
0x09400344,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MATCH_STATUS
0x09400348,0x88a88100,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_ETHERTYPE
0x0940034c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BAD_CHECKSUM_PACKET_DISCARD_COUNTER
0x09400350,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_OTHER_PACKET_DISCARD_COUNTER

Here we are using two filters, which gets reflected by programming
BRCM_TAG_0 and BRCM_TAG_1 and setting the bitmask of enabled filters in
RXHCK_CONTROL (0x30)

ethtool -s gphy wol f filters 0x1

0x09400300,0x01d91019,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_CONTROL
0x09400304,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_0
0x09400308,0x00010000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_1
0x0940030c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_2
0x09400310,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_3
0x09400314,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_4
0x09400318,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_5
0x0940031c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_6
0x09400320,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_7
0x09400324,0xff00ffff,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_0
0x09400328,0xff00ffff,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_1
0x0940032c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_2
0x09400330,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_3
0x09400334,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_4
0x09400338,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_5
0x0940033c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_6
0x09400340,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MASK_7
0x09400344,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BRCM_TAG_MATCH_STATUS
0x09400348,0x88a88100,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_ETHERTYPE
0x0940034c,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_BAD_CHECKSUM_PACKET_DISCARD_COUNTER
0x09400350,0x00000000,SYSTEMPORTLITE_TOP_1_SYSTEMPORTLITE_1_RXCHK_OTHER_PACKET_DISCARD_COUNTER

Here we left the filter that we previously programmed into BRCM_TAG_1,
but as you can see from RHCHK_CONTROL set to 0x10, we are not activating
it, so it will not be part of the wake-up process.

Register programming is always a bit expensive, which is why the code
does not try to clear previously programmed filters because we have this
active/deactive bit instead which is more efficient to manage.

Hope this is convincing you :)
-- 
Florian

Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Andrew Lunn]

> In both of your examples, only one bit is set, what will change is the
> value being programmed to RXHCK_BRCM_TAG(i), which will be either 0, or
> 1, but the value programmed to RXCHK_CONTROL as far as which filter is
> enabled will be the same because we can use filter position 0.
> 
> What the code basically does is look at how many bits are set in the
> filters bitmap, and then it starts populating the filters from filter 0
> up to filter 7 with the value of the bit.

O.K. Now it get it. Sorry for being so slow.

     Andrew

Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER

[Florian Fainelli]

On 07/19/2018 03:25 PM, Andrew Lunn wrote:
>> In both of your examples, only one bit is set, what will change is the
>> value being programmed to RXHCK_BRCM_TAG(i), which will be either 0, or
>> 1, but the value programmed to RXCHK_CONTROL as far as which filter is
>> enabled will be the same because we can use filter position 0.
>>
>> What the code basically does is look at how many bits are set in the
>> filters bitmap, and then it starts populating the filters from filter 0
>> up to filter 7 with the value of the bit.
> 
> O.K. Now it get it. Sorry for being so slow.

No worries, thanks for reviewing these changes. I have made the
requested ethtool changes and will post them when I return from vacation
on July 29th.

Thanks Andrew!
-- 
Florian

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[Florian Fainelli]

On 07/17/2018 08:36 AM, Florian Fainelli wrote:
> Allow re-purposing the wol->sopass storage area to specify a bitmask of filters
> (programmed previously via ethtool::rxnfc) to be used as wake-up patterns.

John, David, can you provide some feedback if the approach is
acceptable? I will address Andrew's comment about the user friendliness
and allow providing a comma separate list of filter identifiers.

One usability issue with this approach is that one cannot specify
wake-on-LAN using WAKE_MAGICSECURE *and* WAKE_FILTER at the same time,
since it uses the same location in the ioctl() structure that is being
passed. Do you see this as a problem?

Thanks!

> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  ethtool-copy.h |  1 +
>  ethtool.c      | 35 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/ethtool-copy.h b/ethtool-copy.h
> index 8cc61e9ab40b..dbfaca15dca5 100644
> --- a/ethtool-copy.h
> +++ b/ethtool-copy.h
> @@ -1628,6 +1628,7 @@ static __inline__ int ethtool_validate_duplex(__u8 duplex)
>  #define WAKE_ARP		(1 << 4)
>  #define WAKE_MAGIC		(1 << 5)
>  #define WAKE_MAGICSECURE	(1 << 6) /* only meaningful if WAKE_MAGIC */
> +#define WAKE_FILTER		(1 << 7)
>  
>  /* L2-L4 network traffic flow types */
>  #define	TCP_V4_FLOW	0x01	/* hash or spec (tcp_ip4_spec) */
> diff --git a/ethtool.c b/ethtool.c
> index fb93ae898312..322fc8d98ee5 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -931,6 +931,9 @@ static int parse_wolopts(char *optstr, u32 *data)
>  		case 's':
>  			*data |= WAKE_MAGICSECURE;
>  			break;
> +		case 'f':
> +			*data |= WAKE_FILTER;
> +			break;
>  		case 'd':
>  			*data = 0;
>  			break;
> @@ -964,6 +967,8 @@ static char *unparse_wolopts(int wolopts)
>  			*p++ = 'g';
>  		if (wolopts & WAKE_MAGICSECURE)
>  			*p++ = 's';
> +		if (wolopts & WAKE_FILTER)
> +			*p++ = 'f';
>  	} else {
>  		*p = 'd';
>  	}
> @@ -989,6 +994,21 @@ static int dump_wol(struct ethtool_wolinfo *wol)
>  		fprintf(stdout, "\n");
>  	}
>  
> +	if (wol->supported & WAKE_FILTER) {
> +		int i, j;
> +		int delim = 0;
> +		fprintf(stdout, "        Filter(s) enabled: ");
> +		for (i = 0; i < SOPASS_MAX; i++) {
> +			for (j = 0; j < 8; j++) {
> +				if (wol->sopass[i] & (1 << j)) {
> +					fprintf(stdout, "%s%d", delim?",":"", i * 8 + j);
> +					delim=1;
> +				}
> +			}
> +		}
> +		fprintf(stdout, "\n");
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2897,6 +2917,16 @@ static int do_sset(struct cmd_context *ctx)
>  				exit_bad_args();
>  			get_mac_addr(argp[i], sopass_wanted);
>  			sopass_change = 1;
> +		} else if (!strcmp(argp[i], "filters")) {
> +			gwol_changed = 1;
> +			i++;
> +			if (i >= argc)
> +				exit_bad_args();
> +			if (parse_hex_u32_bitmap(argp[i],
> +						 SOPASS_MAX * 8,
> +						 (unsigned int *)sopass_wanted))
> +				exit_bad_args();
> +			sopass_change = 1;
>  		} else if (!strcmp(argp[i], "msglvl")) {
>  			i++;
>  			if (i >= argc)
> @@ -3112,8 +3142,10 @@ static int do_sset(struct cmd_context *ctx)
>  		if (err < 0) {
>  			if (wol_change)
>  				fprintf(stderr, "  not setting wol\n");
> -			if (sopass_change)
> +			if (sopass_change & wol.wolopts & WAKE_MAGICSECURE)
>  				fprintf(stderr, "  not setting sopass\n");
> +			if (sopass_change & wol.wolopts & WAKE_FILTER)
> +				fprintf(stderr, "  not setting filters\n");
>  		}
>  	}
>  
> @@ -5066,6 +5098,7 @@ static const struct option {
>  	  "		[ xcvr internal|external ]\n"
>  	  "		[ wol p|u|m|b|a|g|s|d... ]\n"
>  	  "		[ sopass %x:%x:%x:%x:%x:%x ]\n"
> +	  "		[ filters %x ]\n"
>  	  "		[ msglvl %d | msglvl type on|off ... ]\n" },
>  	{ "-a|--show-pause", 1, do_gpause, "Show pause options" },
>  	{ "-A|--pause", 1, do_spause, "Set pause options",
> 


-- 
Florian

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[Andrew Lunn]

> One usability issue with this approach is that one cannot specify
> wake-on-LAN using WAKE_MAGICSECURE *and* WAKE_FILTER at the same time,
> since it uses the same location in the ioctl() structure that is being
> passed. Do you see this as a problem?

Hi Florian

Maybe you can think ahead to when ethtool gets a netlink
implementation, and you have more flexibility to pass both as
attributes? How does this affect the in kernel API?

	    Andrew

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[Florian Fainelli]

On 07/30/2018 03:30 PM, Andrew Lunn wrote:
>> One usability issue with this approach is that one cannot specify
>> wake-on-LAN using WAKE_MAGICSECURE *and* WAKE_FILTER at the same time,
>> since it uses the same location in the ioctl() structure that is being
>> passed. Do you see this as a problem?
> 
> Hi Florian
> 
> Maybe you can think ahead to when ethtool gets a netlink
> implementation, and you have more flexibility to pass both as
> attributes? How does this affect the in kernel API?

The thing is that I need this now, but when Michal's work on ethtool
being migrated to netlink settles, we should have have any issues adding
a proper storage area for specifying filters anymore. The issue here is
of course that the size and layout of ethtool_wolinfo is largely fixed
and set in stone, and therefore inflexible.
-- 
Florian

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[Andrew Lunn]

> The thing is that I need this now, but when Michal's work on ethtool
> being migrated to netlink settles, we should have have any issues adding
> a proper storage area for specifying filters anymore. The issue here is
> of course that the size and layout of ethtool_wolinfo is largely fixed
> and set in stone, and therefore inflexible.

The version in uapi/linux/ethtool.h is fixed. But i think in order to
implement this properly, you are going to have to change the internal
structure passed to ethtool_ops->set_wol/get_wol, with the filter
separated out from the sopass. ethtool_set_wol()/ethtool_get_wol()
would then need to know how to correctly unpack the structure passed
in the ioctl, where as the netlink socket code and just put attributes
into structure members.

     Andrew

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[Andrew Lunn]

> One usability issue with this approach is that one cannot specify
> wake-on-LAN using WAKE_MAGICSECURE *and* WAKE_FILTER at the same time,
> since it uses the same location in the ioctl() structure that is being
> passed. Do you see this as a problem?

Hi Florian

I think you missed adding a check for this. ethtool_set_wol() should
check if both WAKE_FILTER and WAKE_MAGICSECURE are set and return
EINVAL.

	Andrew

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 30 Jul 2018 15:26:24 -0700

> On 07/17/2018 08:36 AM, Florian Fainelli wrote:
>> Allow re-purposing the wol->sopass storage area to specify a bitmask of filters
>> (programmed previously via ethtool::rxnfc) to be used as wake-up patterns.
> 
> John, David, can you provide some feedback if the approach is
> acceptable? I will address Andrew's comment about the user friendliness
> and allow providing a comma separate list of filter identifiers.
> 
> One usability issue with this approach is that one cannot specify
> wake-on-LAN using WAKE_MAGICSECURE *and* WAKE_FILTER at the same time,
> since it uses the same location in the ioctl() structure that is being
> passed. Do you see this as a problem?

Once again we are stuck in this weird situation, a sort of limbo.

On the one hand, I don't want to block your work on the ethtool
netlink stuff being done.

However it is clear that by using netlink attributes, it would
be so much cleaner.

I honestly don't know what to say at this time.  I wish I had
a clear piece of advice and a way for everyone to move forward,
and usually I do, but this time I really don't :-/

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[Florian Fainelli]

On 08/01/2018 09:32 AM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Mon, 30 Jul 2018 15:26:24 -0700
> 
>> On 07/17/2018 08:36 AM, Florian Fainelli wrote:
>>> Allow re-purposing the wol->sopass storage area to specify a bitmask of filters
>>> (programmed previously via ethtool::rxnfc) to be used as wake-up patterns.
>>
>> John, David, can you provide some feedback if the approach is
>> acceptable? I will address Andrew's comment about the user friendliness
>> and allow providing a comma separate list of filter identifiers.
>>
>> One usability issue with this approach is that one cannot specify
>> wake-on-LAN using WAKE_MAGICSECURE *and* WAKE_FILTER at the same time,
>> since it uses the same location in the ioctl() structure that is being
>> passed. Do you see this as a problem?
> 
> Once again we are stuck in this weird situation, a sort of limbo.
> 
> On the one hand, I don't want to block your work on the ethtool
> netlink stuff being done.
> 
> However it is clear that by using netlink attributes, it would
> be so much cleaner.
> 
> I honestly don't know what to say at this time.  I wish I had
> a clear piece of advice and a way for everyone to move forward,
> and usually I do, but this time I really don't :-/
> 

That's fine, let me submit the first few patches that are per-requisite
but don't actually introduce the WAKE_FILTER support. Once Michal's
ethtool/netlink work gets merged I can quickly extend that in a way that
supports wake-on-LAN using configured filters.

Does the current approach of specifying a bitmask of filters looks
reasonable to you though?
-- 
Florian

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 3 Aug 2018 10:57:13 -0700

> Does the current approach of specifying a bitmask of filters looks
> reasonable to you though?

So, in order to answer that, I need some clarification.

The mask, as I see it, is a bit map of 48 possible positions
(SOPASS_MAX * bits_per_byte).  How do these bits map to individual
rxnfc entries?

Are they locations?  If so, how are special locations handled?

What about "special" locations, where the driver and/or hardware
are supposed to decide the location based upon the "special" type
used?

If you considered the following, and you explained why it won't
work, I apologize.  But I'm wondering why you just don't find
some way to specify this as a boolean of the flow spec in the
rxnfc request or similar?

That, at least semantically, seems to avoids several issues.  And it
is unambiguous what flow rule the wake filter boolean applies to.

Right?

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[Florian Fainelli]

On 08/03/2018 12:07 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Fri, 3 Aug 2018 10:57:13 -0700
> 
>> Does the current approach of specifying a bitmask of filters looks
>> reasonable to you though?
> 
> So, in order to answer that, I need some clarification.
> 
> The mask, as I see it, is a bit map of 48 possible positions
> (SOPASS_MAX * bits_per_byte).  How do these bits map to individual
> rxnfc entries?

Correct about the size, it is 48-bits, each bit indeed does map to a
filter location. So if you programmed a filter a location 1, you would
pass 0x2 as the wake-on filter bitmask, etc.

> 
> Are they locations?  If so, how are special locations handled?
> 
> What about "special" locations, where the driver and/or hardware
> are supposed to decide the location based upon the "special" type
> used?

I would not think they require special handling because the process is
kind of two step right now:

- first you program the desired filter (special location or not) and you
obtain an unique ID back
- second you program the desired filter mask with that ID as a bit
position that must be set

So the special location handling was kind of done by the kernel/driver
on the first filter insertion and you just pass that unique filter ID
around.

The reason why it was done as a two step process was largely because the
DSA switch driver, which is the one supporting the filter programming is
a discrete driver from the SYSTEM PORT driver which supports the
wake-on-filter thing. The two do communicate with one another through
the means of the DSA layer though.

Now that I think about it some more, see below, I prefer you approach
since it eliminates the "passing that ID around" step.

> 
> If you considered the following, and you explained why it won't
> work, I apologize.  But I'm wondering why you just don't find
> some way to specify this as a boolean of the flow spec in the
> rxnfc request or similar?
> 
> That, at least semantically, seems to avoids several issues.  And it
> is unambiguous what flow rule the wake filter boolean applies to.
> 
> Right?

Yes, it would actually remove the need for having to specify a storage
location between user-space and kernel space and we would also be able
to valid ahead of time that we are not overflowing the wake-on-LAN
filter capacity. For instance, in the current HW, you can program 128
filters through the switch, but only 8 of those could be wake-up capable
at the CPU/management (SYSTEM PORT) level.

Let me cook something that does just that and re-post.

Thanks for your feedback!
-- 
Florian

Re: [PATCH ethtool] ethtool: Add support for WAKE_FILTER

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 3 Aug 2018 12:58:12 -0700

> For instance, in the current HW, you can program 128 filters through
> the switch, but only 8 of those could be wake-up capable at the
> CPU/management (SYSTEM PORT) level.

Yes, I noticed this in the driver patches.

> Let me cook something that does just that and re-post.
> 
> Thanks for your feedback!

No problem.