[PATCH net-next 0/2] Add WAKE_MDA Wake-on-LAN option

[PATCH net-next 0/2] Add WAKE_MDA Wake-on-LAN option

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1663 bytes --]

This is a follow-up to the patch series previously submitted here:

https://lore.kernel.org/all/20230516231713.2882879-1-florian.fainelli@broadcom.com/

which is now implementing the approach suggested by Andrew. This patch
series adds a new Wake-on-LAN option (WAKE_MDA) with the single letter
'e' (all of the 'm', 'd' and 'a' were already taken) which allows us to
specify a custom destination MAC to be waking from.

For the ioctl-based interface we use an anonymous union to overlay a
6-byte MAC DA field with the existing sopass field.

This is particularly useful for systems that use Ethernet PHYs as
wake-up devices and when waking up on coarse filters like multicast,
broadcast destination MAC addresses is not enough to keep them in a low
power state.

We use this on Set-top-box environments in order to allow waking-up from
only specific IPv4/v6 multicast groups like mDNS for instance. Printers,
smart speaker and IoT devices could fall in that category as well.
Example:

ethtool -s eth0 wol e mac-da 01:00:5e:00:00:fb

ethtool patches are sent as a reply to this thread.

Florian Fainelli (2):
  ethtool: Introduce WAKE_MDA
  net: phy: broadcom: Add support for WAKE_MDA

 Documentation/networking/ethtool-netlink.rst |  7 ++++++-
 drivers/net/phy/bcm-phy-lib.c                |  7 ++++++-
 include/uapi/linux/ethtool.h                 | 10 ++++++++--
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/common.c                         |  1 +
 net/ethtool/netlink.h                        |  2 +-
 net/ethtool/wol.c                            | 21 ++++++++++++++++++++
 7 files changed, 44 insertions(+), 5 deletions(-)

-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 7693 bytes --]

Allow Ethernet PHY and MAC drivers with simplified matching logic to be
waking-up on a custom MAC destination address. This is particularly
useful for Ethernet PHYs which have limited amounts of buffering but can
still wake-up on a custom MAC destination address.

When WAKE_MDA is specified, the "sopass" field in the existing struct
ethtool_wolinfo is re-purposed to hold a custom MAC destination address
to match against.

Example:
	ethtool -s eth0 wol e mac-da 01:00:5e:00:00:fb

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 Documentation/networking/ethtool-netlink.rst |  7 ++++++-
 include/uapi/linux/ethtool.h                 | 10 ++++++++--
 include/uapi/linux/ethtool_netlink.h         |  1 +
 net/ethtool/common.c                         |  1 +
 net/ethtool/netlink.h                        |  2 +-
 net/ethtool/wol.c                            | 21 ++++++++++++++++++++
 6 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 2540c70952ff..b2b1191d5cec 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -708,11 +708,13 @@ Kernel response contents:
   ``ETHTOOL_A_WOL_HEADER``              nested  reply header
   ``ETHTOOL_A_WOL_MODES``               bitset  mask of enabled WoL modes
   ``ETHTOOL_A_WOL_SOPASS``              binary  SecureOn(tm) password
+  ``ETHTOOL_A_WOL_MAC_DA``              binary  Destination matching MAC address
   ====================================  ======  ==========================
 
 In reply, ``ETHTOOL_A_WOL_MODES`` mask consists of modes supported by the
 device, value of modes which are enabled. ``ETHTOOL_A_WOL_SOPASS`` is only
-included in reply if ``WAKE_MAGICSECURE`` mode is supported.
+included in reply if ``WAKE_MAGICSECURE`` mode is supported. ``ETHTOOL_A_WOL_MAC_DA``
+is only included in reply if ``WAKE_MDA`` mode is supported.
 
 
 WOL_SET
@@ -726,10 +728,13 @@ Request contents:
   ``ETHTOOL_A_WOL_HEADER``              nested  request header
   ``ETHTOOL_A_WOL_MODES``               bitset  enabled WoL modes
   ``ETHTOOL_A_WOL_SOPASS``              binary  SecureOn(tm) password
+  ``ETHTOOL_A_WOL_MAC_DA``              binary  Destination matching MAC address
   ====================================  ======  ==========================
 
 ``ETHTOOL_A_WOL_SOPASS`` is only allowed for devices supporting
 ``WAKE_MAGICSECURE`` mode.
+``ETHTOOL_A_WOL_MAC_DA`` is only allowed for devices supporting
+``WAKE_MDA`` mode.
 
 
 FEATURES_GET
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f7fba0dc87e5..8134ac8870bd 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -207,12 +207,17 @@ struct ethtool_drvinfo {
  * @wolopts: Bitmask of %WAKE_* flags for enabled Wake-On-Lan modes.
  * @sopass: SecureOn(tm) password; meaningful only if %WAKE_MAGICSECURE
  *	is set in @wolopts.
+ * @mac_da: Destination MAC address to match; meaningful only if
+ *	%WAKE_MDA is set in @wolopts.
  */
 struct ethtool_wolinfo {
 	__u32	cmd;
 	__u32	supported;
 	__u32	wolopts;
-	__u8	sopass[SOPASS_MAX];
+	union {
+		__u8	sopass[SOPASS_MAX];
+		__u8	mac_da[ETH_ALEN];
+	};
 };
 
 /* for passing single values */
@@ -1989,8 +1994,9 @@ static inline int ethtool_validate_duplex(__u8 duplex)
 #define WAKE_MAGIC		(1 << 5)
 #define WAKE_MAGICSECURE	(1 << 6) /* only meaningful if WAKE_MAGIC */
 #define WAKE_FILTER		(1 << 7)
+#define WAKE_MDA		(1 << 8)
 
-#define WOL_MODE_COUNT		8
+#define WOL_MODE_COUNT		9
 
 /* L2-L4 network traffic flow types */
 #define	TCP_V4_FLOW	0x01	/* hash or spec (tcp_ip4_spec) */
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 73e2c10dc2cc..237a0fc68997 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -300,6 +300,7 @@ enum {
 	ETHTOOL_A_WOL_HEADER,			/* nest - _A_HEADER_* */
 	ETHTOOL_A_WOL_MODES,			/* bitset */
 	ETHTOOL_A_WOL_SOPASS,			/* binary */
+	ETHTOOL_A_WOL_MAC_DA,			/* binary */
 
 	/* add new constants above here */
 	__ETHTOOL_A_WOL_CNT,
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index f5598c5f50de..d1c837f6094c 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -405,6 +405,7 @@ const char wol_mode_names[][ETH_GSTRING_LEN] = {
 	[const_ilog2(WAKE_MAGIC)]	= "magic",
 	[const_ilog2(WAKE_MAGICSECURE)]	= "magicsecure",
 	[const_ilog2(WAKE_FILTER)]	= "filter",
+	[const_ilog2(WAKE_MDA)]		= "mac-da",
 };
 static_assert(ARRAY_SIZE(wol_mode_names) == WOL_MODE_COUNT);
 
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 9a333a8d04c1..5958e4483ced 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -407,7 +407,7 @@ extern const struct nla_policy ethnl_linkstate_get_policy[ETHTOOL_A_LINKSTATE_HE
 extern const struct nla_policy ethnl_debug_get_policy[ETHTOOL_A_DEBUG_HEADER + 1];
 extern const struct nla_policy ethnl_debug_set_policy[ETHTOOL_A_DEBUG_MSGMASK + 1];
 extern const struct nla_policy ethnl_wol_get_policy[ETHTOOL_A_WOL_HEADER + 1];
-extern const struct nla_policy ethnl_wol_set_policy[ETHTOOL_A_WOL_SOPASS + 1];
+extern const struct nla_policy ethnl_wol_set_policy[ETHTOOL_A_WOL_MAC_DA + 1];
 extern const struct nla_policy ethnl_features_get_policy[ETHTOOL_A_FEATURES_HEADER + 1];
 extern const struct nla_policy ethnl_features_set_policy[ETHTOOL_A_FEATURES_WANTED + 1];
 extern const struct nla_policy ethnl_privflags_get_policy[ETHTOOL_A_PRIVFLAGS_HEADER + 1];
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
index 0ed56c9ac1bc..13dfcc9bb1e5 100644
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -12,6 +12,7 @@ struct wol_reply_data {
 	struct ethnl_reply_data		base;
 	struct ethtool_wolinfo		wol;
 	bool				show_sopass;
+	bool				show_mac_da;
 };
 
 #define WOL_REPDATA(__reply_base) \
@@ -41,6 +42,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
 	/* do not include password in notifications */
 	data->show_sopass = !genl_info_is_ntf(info) &&
 		(data->wol.supported & WAKE_MAGICSECURE);
+	data->show_mac_da = !genl_info_is_ntf(info) &&
+		(data->wol.supported & WAKE_MDA);
 
 	return 0;
 }
@@ -58,6 +61,8 @@ static int wol_reply_size(const struct ethnl_req_info *req_base,
 		return len;
 	if (data->show_sopass)
 		len += nla_total_size(sizeof(data->wol.sopass));
+	if (data->show_mac_da)
+		len += nla_total_size(sizeof(data->wol.mac_da));
 
 	return len;
 }
@@ -79,6 +84,10 @@ static int wol_fill_reply(struct sk_buff *skb,
 	    nla_put(skb, ETHTOOL_A_WOL_SOPASS, sizeof(data->wol.sopass),
 		    data->wol.sopass))
 		return -EMSGSIZE;
+	if (data->show_mac_da &&
+	    nla_put(skb, ETHTOOL_A_WOL_MAC_DA, sizeof(data->wol.mac_da),
+		    data->wol.mac_da))
+		return -EMSGSIZE;
 
 	return 0;
 }
@@ -91,6 +100,8 @@ const struct nla_policy ethnl_wol_set_policy[] = {
 	[ETHTOOL_A_WOL_MODES]		= { .type = NLA_NESTED },
 	[ETHTOOL_A_WOL_SOPASS]		= { .type = NLA_BINARY,
 					    .len = SOPASS_MAX },
+	[ETHTOOL_A_WOL_MAC_DA]		= { .type = NLA_BINARY,
+					    .len = ETH_ALEN }
 };
 
 static int
@@ -131,6 +142,16 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info)
 		ethnl_update_binary(wol.sopass, sizeof(wol.sopass),
 				    tb[ETHTOOL_A_WOL_SOPASS], &mod);
 	}
+	if (tb[ETHTOOL_A_WOL_MAC_DA]) {
+		if (!(wol.supported & WAKE_MDA)) {
+			NL_SET_ERR_MSG_ATTR(info->extack,
+					    tb[ETHTOOL_A_WOL_MAC_DA],
+					    "mac-da not supported, cannot set MAC");
+			return -EINVAL;
+		}
+		ethnl_update_binary(wol.mac_da, sizeof(wol.mac_da),
+				    tb[ETHTOOL_A_WOL_MAC_DA], &mod);
+	}
 
 	if (!mod)
 		return 0;
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH ethtool 1/2] update UAPI header copies for WAKE_MDA support

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 uapi/linux/ethtool.h         | 10 ++++++++--
 uapi/linux/ethtool_netlink.h |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/uapi/linux/ethtool.h b/uapi/linux/ethtool.h
index 1d0731b3d289..32eb2aac542c 100644
--- a/uapi/linux/ethtool.h
+++ b/uapi/linux/ethtool.h
@@ -205,12 +205,17 @@ struct ethtool_drvinfo {
  * @wolopts: Bitmask of %WAKE_* flags for enabled Wake-On-Lan modes.
  * @sopass: SecureOn(tm) password; meaningful only if %WAKE_MAGICSECURE
  *	is set in @wolopts.
+ * @mac_da: Destination MAC address to match; meaningful only if
+ *	%WAKE_MDA is set in @wolopts.
  */
 struct ethtool_wolinfo {
 	__u32	cmd;
 	__u32	supported;
 	__u32	wolopts;
-	__u8	sopass[SOPASS_MAX];
+	union {
+		__u8	sopass[SOPASS_MAX];
+		__u8	mac_da[ETH_ALEN];
+	};
 };
 
 /* for passing single values */
@@ -1987,8 +1992,9 @@ static __inline__ int ethtool_validate_duplex(__u8 duplex)
 #define WAKE_MAGIC		(1 << 5)
 #define WAKE_MAGICSECURE	(1 << 6) /* only meaningful if WAKE_MAGIC */
 #define WAKE_FILTER		(1 << 7)
+#define WAKE_MDA		(1 << 8)
 
-#define WOL_MODE_COUNT		8
+#define WOL_MODE_COUNT		9
 
 /* L2-L4 network traffic flow types */
 #define	TCP_V4_FLOW	0x01	/* hash or spec (tcp_ip4_spec) */
diff --git a/uapi/linux/ethtool_netlink.h b/uapi/linux/ethtool_netlink.h
index a8b0d79dad95..6e168dd7eb1c 100644
--- a/uapi/linux/ethtool_netlink.h
+++ b/uapi/linux/ethtool_netlink.h
@@ -300,6 +300,7 @@ enum {
 	ETHTOOL_A_WOL_HEADER,			/* nest - _A_HEADER_* */
 	ETHTOOL_A_WOL_MODES,			/* bitset */
 	ETHTOOL_A_WOL_SOPASS,			/* binary */
+	ETHTOOL_A_WOL_MAC_DA,			/* binary */
 
 	/* add new constants above here */
 	__ETHTOOL_A_WOL_CNT,
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH net-next 2/2] net: phy: broadcom: Add support for WAKE_MDA

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

Add support for waking-up from a custom MAC destination address which
involves programming the BCM54XX_WOL_MPD_DATA2() and
BCM54XX_WOL_MASK() register groups.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/phy/bcm-phy-lib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 876f28fd8256..00c0424f7b63 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -827,7 +827,8 @@ EXPORT_SYMBOL_GPL(bcm_phy_cable_test_get_status_rdb);
 					 WAKE_MCAST | \
 					 WAKE_BCAST | \
 					 WAKE_MAGIC | \
-					 WAKE_MAGICSECURE)
+					 WAKE_MAGICSECURE | \
+					 WAKE_MDA)
 
 int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
@@ -908,6 +909,8 @@ int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 			eth_broadcast_addr(da);
 		} else if (wol->wolopts & WAKE_MAGICSECURE) {
 			ether_addr_copy(da, wol->sopass);
+		} else if (wol->wolopts & WAKE_MDA) {
+			ether_addr_copy(da, wol->mac_da);
 		} else if (wol->wolopts & WAKE_MAGIC) {
 			memset(da, 0, sizeof(da));
 			memset(mask, 0xff, sizeof(mask));
@@ -1010,6 +1013,8 @@ void bcm_phy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 	}
 
 	if (ctl & BCM54XX_WOL_DIR_PKT_EN) {
+		memcpy(wol->mac_da, da, sizeof(da));
+		wol->wolopts |= WAKE_MDA;
 		if (is_broadcast_ether_addr(da))
 			wol->wolopts |= WAKE_BCAST;
 		else if (is_multicast_ether_addr(da))
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH ethtool 2/2] wol: Add support for WAKE_MDA

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 9091 bytes --]

Allow waking-up from a specific MAC destination address, this is
particularly useful with a number of Ethernet PHYs that have limited
buffering and packet matching abilities.

Example:

ethtool -s eth0 wol e mac-da 01:00:5e:00:00:fb

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 common.c               | 16 ++++++++++++++--
 ethtool.8.in           | 15 +++++++++++----
 ethtool.c              | 26 +++++++++++++++++++++-----
 netlink/desc-ethtool.c |  1 +
 netlink/settings.c     | 25 ++++++++++++++++++++++---
 test-cmdline.c         |  4 ++++
 6 files changed, 73 insertions(+), 14 deletions(-)

diff --git a/common.c b/common.c
index b8fd4d5bc0f4..a42d00fe3c0c 100644
--- a/common.c
+++ b/common.c
@@ -120,6 +120,8 @@ static char *unparse_wolopts(int wolopts)
 			*p++ = 's';
 		if (wolopts & WAKE_FILTER)
 			*p++ = 'f';
+		if (wolopts & WAKE_MDA)
+			*p++ = 'e';
 	} else {
 		*p = 'd';
 	}
@@ -129,13 +131,13 @@ static char *unparse_wolopts(int wolopts)
 
 int dump_wol(struct ethtool_wolinfo *wol)
 {
+	int i;
+	int delim = 0;
 	fprintf(stdout, "	Supports Wake-on: %s\n",
 		unparse_wolopts(wol->supported));
 	fprintf(stdout, "	Wake-on: %s\n",
 		unparse_wolopts(wol->wolopts));
 	if (wol->supported & WAKE_MAGICSECURE) {
-		int i;
-		int delim = 0;
 
 		fprintf(stdout, "        SecureOn password: ");
 		for (i = 0; i < SOPASS_MAX; i++) {
@@ -145,6 +147,16 @@ int dump_wol(struct ethtool_wolinfo *wol)
 		}
 		fprintf(stdout, "\n");
 	}
+	delim = 0;
+	if (wol->supported & WAKE_MDA) {
+		fprintf(stdout, "        Destination MAC: ");
+		for (i = 0; i < ETH_ALEN; i++) {
+			fprintf(stdout, "%s%02x", delim ? ":" : "",
+				wol->mac_da[i]);
+			delim = 1;
+		}
+		fprintf(stdout, "\n");
+	}
 
 	return 0;
 }
diff --git a/ethtool.8.in b/ethtool.8.in
index c0c37a427715..c7f457b9b739 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -62,7 +62,7 @@
 .\"
 .\"	\(*WO - wol flags
 .\"
-.ds WO \fBp\fP|\fBu\fP|\fBm\fP|\fBb\fP|\fBa\fP|\fBg\fP|\fBs\fP|\fBf|\fBd\fP...
+.ds WO \fBp\fP|\fBu\fP|\fBm\fP|\fBb\fP|\fBa\fP|\fBg\fP|\fBs\fP|\fBf|\fBe|\fBd\fP...
 .\"
 .\"	\(*FL - flow type values
 .\"
@@ -281,7 +281,7 @@ ethtool \- query or control network driver and hardware settings
 .B2 xcvr internal external
 .RB [ wol \ \fIN\fP[\fB/\fP\fIM\fP]
 .RB | \ wol \ \*(WO]
-.RB [ sopass \ \*(MA]
+.RB [ sopass \ \*(MA | mac-da \ \*(MA ]
 .RB [ master-slave \ \*(MS]
 .RB [ msglvl
 .IR N\fP[/\fIM\fP] \ |
@@ -949,14 +949,21 @@ a	Wake on ARP
 g	Wake on MagicPacket\[tm]
 s	Enable SecureOn\[tm] password for MagicPacket\[tm]
 f	Wake on filter(s)
+e	Wake on specific MAC destination address
 d	T{
 Disable (wake on nothing).  This option clears all previous options.
 T}
 .TE
 .TP
 .B sopass \*(MA
-Sets the SecureOn\[tm] password.  The argument to this option must be 6
-bytes in Ethernet MAC hex format (\*(MA).
+Sets the secureon\[tm] password.  The argument to this option must be 6
+bytes in ethernet mac hex format (\*(MA).
+.PP
+.TE
+.TP
+.B mac-da \*(MA
+Sets the destination MAC address to match against.  The argument to this option
+must be 6 bytes in ethernet mac hex format (\*(MA).
 .PP
 .BI msglvl \ N
 .br
diff --git a/ethtool.c b/ethtool.c
index af51220b63cc..513b4ed11623 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -973,6 +973,9 @@ static int parse_wolopts(char *optstr, u32 *data)
 		case 'f':
 			*data |= WAKE_FILTER;
 			break;
+		case 'e':
+			*data |= WAKE_MDA;
+			break;
 		case 'd':
 			*data = 0;
 			break;
@@ -2971,8 +2974,8 @@ static int do_sset(struct cmd_context *ctx)
 	int gset_changed = 0; /* did anything in GSET change? */
 	u32 wol_wanted = 0;
 	int wol_change = 0;
-	u8 sopass_wanted[SOPASS_MAX];
-	int sopass_change = 0;
+	u8 sopass_wanted[SOPASS_MAX], mda_wanted[ETH_ALEN];
+	int sopass_change = 0, mda_change = 0;
 	int gwol_changed = 0; /* did anything in GWOL change? */
 	int msglvl_changed = 0;
 	u32 msglvl_wanted = 0;
@@ -3093,6 +3096,13 @@ 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], "mac-da")) {
+			gwol_changed = 1;
+			i++;
+			if (i >= argc)
+				exit_bad_args();
+			get_mac_addr(argp[i], mda_wanted);
+			mda_change = 1;
 		} else if (!strcmp(argp[i], "msglvl")) {
 			i++;
 			if (i >= argc)
@@ -3295,14 +3305,18 @@ static int do_sset(struct cmd_context *ctx)
 		if (err < 0) {
 			perror("Cannot get current wake-on-lan settings");
 		} else {
+			int i;
 			/* Change everything the user specified. */
 			if (wol_change)
 				wol.wolopts = wol_wanted;
 			if (sopass_change) {
-				int i;
 				for (i = 0; i < SOPASS_MAX; i++)
 					wol.sopass[i] = sopass_wanted[i];
 			}
+			if (mda_change) {
+				for (i = 0; i < ETH_ALEN; i++)
+					wol.mac_da[i] = mda_wanted[i];
+			}
 
 			/* Try to perform the update. */
 			wol.cmd = ETHTOOL_SWOL;
@@ -3315,6 +3329,8 @@ static int do_sset(struct cmd_context *ctx)
 				fprintf(stderr, "  not setting wol\n");
 			if (sopass_change)
 				fprintf(stderr, "  not setting sopass\n");
+			if (mda_change)
+				fprintf(stderr, "  not setting mac-da\n");
 		}
 	}
 
@@ -5669,8 +5685,8 @@ static const struct option args[] = {
 			  "		[ advertise %x[/%x] | mode on|off ... [--] ]\n"
 			  "		[ phyad %d ]\n"
 			  "		[ xcvr internal|external ]\n"
-			  "		[ wol %d[/%d] | p|u|m|b|a|g|s|f|d... ]\n"
-			  "		[ sopass %x:%x:%x:%x:%x:%x ]\n"
+			  "		[ wol %d[/%d] | p|u|m|b|a|g|s|f|e|d... ]\n"
+			  "		[ sopass %x:%x:%x:%x:%x:%x | mac-da %x:%x:%x:%x:%x:%x ]\n"
 			  "		[ msglvl %d[/%d] | type on|off ... [--] ]\n"
 			  "		[ master-slave preferred-master|preferred-slave|forced-master|forced-slave ]\n"
 	},
diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
index 661de267262f..78732bc2719c 100644
--- a/netlink/desc-ethtool.c
+++ b/netlink/desc-ethtool.c
@@ -120,6 +120,7 @@ static const struct pretty_nla_desc __wol_desc[] = {
 	NLATTR_DESC_NESTED(ETHTOOL_A_WOL_HEADER, header),
 	NLATTR_DESC_NESTED(ETHTOOL_A_WOL_MODES, bitset),
 	NLATTR_DESC_BINARY(ETHTOOL_A_WOL_SOPASS),
+	NLATTR_DESC_BINARY(ETHTOOL_A_WOL_MAC_DA),
 };
 
 static const struct pretty_nla_desc __features_desc[] = {
diff --git a/netlink/settings.c b/netlink/settings.c
index a506618ba0a4..a13251863af1 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -810,6 +810,7 @@ int wol_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 	DECLARE_ATTR_TB_INFO(tb);
 	struct nl_context *nlctx = data;
 	struct ethtool_wolinfo wol = {};
+	unsigned int len;
 	int ret;
 
 	if (nlctx->is_dump || nlctx->is_monitor)
@@ -824,8 +825,6 @@ int wol_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 	if (tb[ETHTOOL_A_WOL_MODES])
 		walk_bitset(tb[ETHTOOL_A_WOL_MODES], NULL, wol_modes_cb, &wol);
 	if (tb[ETHTOOL_A_WOL_SOPASS]) {
-		unsigned int len;
-
 		len = mnl_attr_get_payload_len(tb[ETHTOOL_A_WOL_SOPASS]);
 		if (len != SOPASS_MAX)
 			fprintf(stderr, "invalid SecureOn password length %u (should be %u)\n",
@@ -835,6 +834,16 @@ int wol_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 			       mnl_attr_get_payload(tb[ETHTOOL_A_WOL_SOPASS]),
 			       SOPASS_MAX);
 	}
+	if (tb[ETHTOOL_A_WOL_MAC_DA]) {
+		len = mnl_attr_get_payload_len(tb[ETHTOOL_A_WOL_SOPASS]);
+		if (len != ETH_ALEN)
+			fprintf(stderr, "invalid destinatino MAC address length %u (should be %u)\n",
+				len, ETH_ALEN);
+		else
+			memcpy(wol.mac_da,
+			       mnl_attr_get_payload(tb[ETHTOOL_A_WOL_MAC_DA]),
+			       ETH_ALEN);
+	}
 	print_banner(nlctx);
 	dump_wol(&wol);
 
@@ -1050,10 +1059,11 @@ enum {
 	WAKE_MAGIC_BIT		= 5,
 	WAKE_MAGICSECURE_BIT	= 6,
 	WAKE_FILTER_BIT		= 7,
+	WAKE_MDA_BIT		= 8,
 };
 
 #define WAKE_ALL (WAKE_PHY | WAKE_UCAST | WAKE_MCAST | WAKE_BCAST | WAKE_ARP | \
-		  WAKE_MAGIC | WAKE_MAGICSECURE)
+		  WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_MDA)
 
 static const struct lookup_entry_u8 port_values[] = {
 	{ .arg = "tp",		.val = PORT_TP },
@@ -1112,6 +1122,7 @@ char wol_bit_chars[WOL_MODE_COUNT] = {
 	[WAKE_MAGIC_BIT]	= 'g',
 	[WAKE_MAGICSECURE_BIT]	= 's',
 	[WAKE_FILTER_BIT]	= 'f',
+	[WAKE_MDA_BIT]		= 'e',
 };
 
 const struct char_bitset_parser_data wol_parser_data = {
@@ -1224,6 +1235,14 @@ static const struct param_parser sset_params[] = {
 		.handler_data	= &sopass_parser_data,
 		.min_argc	= 1,
 	},
+	{
+		.arg		= "mac-da",
+		.group		= ETHTOOL_MSG_WOL_SET,
+		.type		= ETHTOOL_A_WOL_MAC_DA,
+		.handler	= nl_parse_byte_str,
+		.handler_data	= &sopass_parser_data,
+		.min_argc	= 1,
+	},
 	{
 		.arg		= "msglvl",
 		.group		= ETHTOOL_MSG_DEBUG_SET,
diff --git a/test-cmdline.c b/test-cmdline.c
index cb803ed1a93d..cfe7d24c065f 100644
--- a/test-cmdline.c
+++ b/test-cmdline.c
@@ -74,6 +74,10 @@ static struct test_case {
 	{ 1, "--change devname sopass 01:23:45:67:89:" },
 	{ 1, "-s devname sopass 01:23:45:67:89" },
 	{ 1, "--change devname sopass" },
+	{ 0, "-s devname mac-da 01:23:45:67:89:ab" },
+	{ 1, "--change devname mac-da 01:23:45:67:89:" },
+	{ 1, "-s devname mac-da 01:23:45:67:89" },
+	{ 1, "--change devname mac-da" },
 	{ 0, "-s devname msglvl 1" },
 	{ 1, "--change devname msglvl" },
 	{ 0, "-s devname msglvl hw on rx_status off" },
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 3496 bytes --]

On Wed, Oct 11, 2023 at 03:12:39PM -0700, Florian Fainelli wrote:
> Allow Ethernet PHY and MAC drivers with simplified matching logic to be
> waking-up on a custom MAC destination address. This is particularly
> useful for Ethernet PHYs which have limited amounts of buffering but can
> still wake-up on a custom MAC destination address.
> 
> When WAKE_MDA is specified, the "sopass" field in the existing struct
> ethtool_wolinfo is re-purposed to hold a custom MAC destination address
> to match against.
> 
> Example:
> 	ethtool -s eth0 wol e mac-da 01:00:5e:00:00:fb
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  Documentation/networking/ethtool-netlink.rst |  7 ++++++-
>  include/uapi/linux/ethtool.h                 | 10 ++++++++--
>  include/uapi/linux/ethtool_netlink.h         |  1 +
>  net/ethtool/common.c                         |  1 +
>  net/ethtool/netlink.h                        |  2 +-
>  net/ethtool/wol.c                            | 21 ++++++++++++++++++++
>  6 files changed, 38 insertions(+), 4 deletions(-)
> 
[...]
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f7fba0dc87e5..8134ac8870bd 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -207,12 +207,17 @@ struct ethtool_drvinfo {
>   * @wolopts: Bitmask of %WAKE_* flags for enabled Wake-On-Lan modes.
>   * @sopass: SecureOn(tm) password; meaningful only if %WAKE_MAGICSECURE
>   *	is set in @wolopts.
> + * @mac_da: Destination MAC address to match; meaningful only if
> + *	%WAKE_MDA is set in @wolopts.
>   */
>  struct ethtool_wolinfo {
>  	__u32	cmd;
>  	__u32	supported;
>  	__u32	wolopts;
> -	__u8	sopass[SOPASS_MAX];
> +	union {
> +		__u8	sopass[SOPASS_MAX];
> +		__u8	mac_da[ETH_ALEN];
> +	};
>  };

If we use the union here, we should also make sure the request cannot
set both WAKE_MAGICSECURE and WAKE_MDA, otherwise the same data will be
used for two different values (and interpreted in two different ways in
GET requests and notifications).

Another option would be keeping the existing structure for ioctl UAPI
and using another structure (like we did in other cases where we needed
new attributes beyond frozen ioctl structures) so that a combination of
WAKE_MAGICSECURE and WAKE_MDA is possible. (It doesn't seem to make much
sense to me to combine WAKE_MAGICSECURE with other bits but I can't rule
out someone might want that one day.)

[...]
> diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
> index 0ed56c9ac1bc..13dfcc9bb1e5 100644
> --- a/net/ethtool/wol.c
> +++ b/net/ethtool/wol.c
> @@ -12,6 +12,7 @@ struct wol_reply_data {
>  	struct ethnl_reply_data		base;
>  	struct ethtool_wolinfo		wol;
>  	bool				show_sopass;
> +	bool				show_mac_da;
>  };
>  
>  #define WOL_REPDATA(__reply_base) \
> @@ -41,6 +42,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
>  	/* do not include password in notifications */
>  	data->show_sopass = !genl_info_is_ntf(info) &&
>  		(data->wol.supported & WAKE_MAGICSECURE);
> +	data->show_mac_da = !genl_info_is_ntf(info) &&
> +		(data->wol.supported & WAKE_MDA);
>  
>  	return 0;
>  }

The test for !genl_info_is_ntf(info) above is meant to prevent the
sopass value (which is supposed to be secret) from being included in
notifications which can be seen by unprivileged users. Is the MAC
address to match also supposed to be secret?

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Florian Fainelli]

On 10/11/2023 4:08 PM, Michal Kubecek wrote:
> On Wed, Oct 11, 2023 at 03:12:39PM -0700, Florian Fainelli wrote:
>> Allow Ethernet PHY and MAC drivers with simplified matching logic to be
>> waking-up on a custom MAC destination address. This is particularly
>> useful for Ethernet PHYs which have limited amounts of buffering but can
>> still wake-up on a custom MAC destination address.
>>
>> When WAKE_MDA is specified, the "sopass" field in the existing struct
>> ethtool_wolinfo is re-purposed to hold a custom MAC destination address
>> to match against.
>>
>> Example:
>> 	ethtool -s eth0 wol e mac-da 01:00:5e:00:00:fb
>>
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>>   Documentation/networking/ethtool-netlink.rst |  7 ++++++-
>>   include/uapi/linux/ethtool.h                 | 10 ++++++++--
>>   include/uapi/linux/ethtool_netlink.h         |  1 +
>>   net/ethtool/common.c                         |  1 +
>>   net/ethtool/netlink.h                        |  2 +-
>>   net/ethtool/wol.c                            | 21 ++++++++++++++++++++
>>   6 files changed, 38 insertions(+), 4 deletions(-)
>>
> [...]
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index f7fba0dc87e5..8134ac8870bd 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -207,12 +207,17 @@ struct ethtool_drvinfo {
>>    * @wolopts: Bitmask of %WAKE_* flags for enabled Wake-On-Lan modes.
>>    * @sopass: SecureOn(tm) password; meaningful only if %WAKE_MAGICSECURE
>>    *	is set in @wolopts.
>> + * @mac_da: Destination MAC address to match; meaningful only if
>> + *	%WAKE_MDA is set in @wolopts.
>>    */
>>   struct ethtool_wolinfo {
>>   	__u32	cmd;
>>   	__u32	supported;
>>   	__u32	wolopts;
>> -	__u8	sopass[SOPASS_MAX];
>> +	union {
>> +		__u8	sopass[SOPASS_MAX];
>> +		__u8	mac_da[ETH_ALEN];
>> +	};
>>   };
> 
> If we use the union here, we should also make sure the request cannot
> set both WAKE_MAGICSECURE and WAKE_MDA, otherwise the same data will be
> used for two different values (and interpreted in two different ways in
> GET requests and notifications).
> 
> Another option would be keeping the existing structure for ioctl UAPI
> and using another structure (like we did in other cases where we needed
> new attributes beyond frozen ioctl structures) so that a combination of
> WAKE_MAGICSECURE and WAKE_MDA is possible. (It doesn't seem to make much
> sense to me to combine WAKE_MAGICSECURE with other bits but I can't rule
> out someone might want that one day.)

I am having some second thoughts about this proposed interface as I can 
see a few limitations:

- we can only specify an exact destination MAC address to match, but the 
HW filter underneath is typically implemented using a match + mask so 
you can actually be selective about which bits you want to match on. In 
the use case that I have in mind we would actually want to match both 
multicast MAC destination addresses corresponding to mDNS over IPv4 or IPv6

- in case a MAC/PHY/switch supports multiple filters/slots we would not 
be able to address a specific slot in the matching logic

This sort of brings me back to the original proposal which allowed this:

https://lore.kernel.org/all/20230516231713.2882879-1-florian.fainelli@broadcom.com/

Andrew, what do you think?

> 
> [...]
>> diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
>> index 0ed56c9ac1bc..13dfcc9bb1e5 100644
>> --- a/net/ethtool/wol.c
>> +++ b/net/ethtool/wol.c
>> @@ -12,6 +12,7 @@ struct wol_reply_data {
>>   	struct ethnl_reply_data		base;
>>   	struct ethtool_wolinfo		wol;
>>   	bool				show_sopass;
>> +	bool				show_mac_da;
>>   };
>>   
>>   #define WOL_REPDATA(__reply_base) \
>> @@ -41,6 +42,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
>>   	/* do not include password in notifications */
>>   	data->show_sopass = !genl_info_is_ntf(info) &&
>>   		(data->wol.supported & WAKE_MAGICSECURE);
>> +	data->show_mac_da = !genl_info_is_ntf(info) &&
>> +		(data->wol.supported & WAKE_MDA);
>>   
>>   	return 0;
>>   }
> 
> The test for !genl_info_is_ntf(info) above is meant to prevent the
> sopass value (which is supposed to be secret) from being included in
> notifications which can be seen by unprivileged users. Is the MAC
> address to match also supposed to be secret?

Either way could be considered, so I erred on the side of caution.
-- 
Florian

Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Andrew Lunn]

> I am having some second thoughts about this proposed interface as I can see
> a few limitations:
> 
> - we can only specify an exact destination MAC address to match, but the HW
> filter underneath is typically implemented using a match + mask so you can
> actually be selective about which bits you want to match on. In the use case
> that I have in mind we would actually want to match both multicast MAC
> destination addresses corresponding to mDNS over IPv4 or IPv6
> 
> - in case a MAC/PHY/switch supports multiple filters/slots we would not be
> able to address a specific slot in the matching logic
> 
> This sort of brings me back to the original proposal which allowed this:
> 
> https://lore.kernel.org/all/20230516231713.2882879-1-florian.fainelli@broadcom.com/

The Marvell PHY i just looked at supports upto 8 slots, and can match
up to the first 128 bytes of frame data. So it does seem like a more
generic and flexible interface would fit the hardware.

My previous concern was discoverability of the feature. Its not part
of ethtool -s eth0 wol. At minimum, i would suggest something in the
--help text in the wol section and man page pointing to the
alternative way to configure wol. And maybe report via the standard
wol flags that the hardware has the capability to use flow-type WoL?

The example you gave matched on Flow Type: Raw Ethernet. Is it
possible to combine flow types? If i can match on the first 128 bytes
of the frame i might want to go deeper into the frame, so want both
Ethernet and IP matching?

    Andrew

Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 3048 bytes --]

On 10/12/23 05:45, Andrew Lunn wrote:
>> I am having some second thoughts about this proposed interface as I can see
>> a few limitations:
>>
>> - we can only specify an exact destination MAC address to match, but the HW
>> filter underneath is typically implemented using a match + mask so you can
>> actually be selective about which bits you want to match on. In the use case
>> that I have in mind we would actually want to match both multicast MAC
>> destination addresses corresponding to mDNS over IPv4 or IPv6
>>
>> - in case a MAC/PHY/switch supports multiple filters/slots we would not be
>> able to address a specific slot in the matching logic
>>
>> This sort of brings me back to the original proposal which allowed this:
>>
>> https://lore.kernel.org/all/20230516231713.2882879-1-florian.fainelli@broadcom.com/
> 
> The Marvell PHY i just looked at supports upto 8 slots, and can match
> up to the first 128 bytes of frame data. So it does seem like a more
> generic and flexible interface would fit the hardware.
> 
> My previous concern was discoverability of the feature. Its not part
> of ethtool -s eth0 wol. At minimum, i would suggest something in the
> --help text in the wol section and man page pointing to the
> alternative way to configure wol. And maybe report via the standard
> wol flags that the hardware has the capability to use flow-type WoL?

WAKE_FILTER is supposed to be set by the driver if it supports waking-up 
from a network filter. That is how you would know that the device 
supports waking-up from a network filter, and then you need to configure 
the filters with ethtool -N (rxnfc).

Where this API is a good fit is that you can specify a filter location 
and the action (-2 = RX_CLS_FLOW_WAKE) to indicate where to install the 
filter and what it should do. Where it may not be such a great fit is 
that it is a two step process, where you need to make sure you install 
filter(s) plus enable WAKE_FILTER from the .set_wol() call.

At the time it was proposed it felt like a reasonable way to program, 
without having "ethtool -s eth0 wol" gain a form of packet matching 
parser. Also, it does not seem to me like we need the operations to be 
necessarily atomic in a single call to the kernel but if we feel like 
this is too difficult to use, we could consider a .set_wol() call that 
supports being passed network filter(s).

> 
> The example you gave matched on Flow Type: Raw Ethernet. Is it
> possible to combine flow types? If i can match on the first 128 bytes
> of the frame i might want to go deeper into the frame, so want both
> Ethernet and IP matching?

AFAICT with neither ethtool nor cls_flower you would be able to match on 
an arbitrary and unstructured N-bytes key + N-bytes mask. You would 
likely need to create two filters here, one for Ethernet, specified with 
the "flow-type ether" and one for each IP version...

You might have a shot with the extended flow representation which 
provides a few bytes of raw filtering, I have not really explored that 
part TBH.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Andrew Lunn]

> > My previous concern was discoverability of the feature. Its not part
> > of ethtool -s eth0 wol. At minimum, i would suggest something in the
> > --help text in the wol section and man page pointing to the
> > alternative way to configure wol. And maybe report via the standard
> > wol flags that the hardware has the capability to use flow-type WoL?
> 
> WAKE_FILTER is supposed to be set by the driver if it supports waking-up
> from a network filter. That is how you would know that the device supports
> waking-up from a network filter, and then you need to configure the filters
> with ethtool -N (rxnfc).
> 
> Where this API is a good fit is that you can specify a filter location and
> the action (-2 = RX_CLS_FLOW_WAKE) to indicate where to install the filter
> and what it should do. Where it may not be such a great fit is that it is a
> two step process, where you need to make sure you install filter(s) plus
> enable WAKE_FILTER from the .set_wol() call.
> 
> At the time it was proposed it felt like a reasonable way to program,
> without having "ethtool -s eth0 wol" gain a form of packet matching parser.
> Also, it does not seem to me like we need the operations to be necessarily
> atomic in a single call to the kernel but if we feel like this is too
> difficult to use, we could consider a .set_wol() call that supports being
> passed network filter(s).
 
I think two step is fine. I would say anybody using rxnfc is a pretty
advanced user.

But we should clearly define what we expect in terms of ordering and
maybe try to enforce it in the core. Can we make the rxnfc call return
-EBUSY or something and an extack message if WAKE_FILTER has not been
enabled first?

	Andrew

Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

On 10/12/23 13:24, Andrew Lunn wrote:
>>> My previous concern was discoverability of the feature. Its not part
>>> of ethtool -s eth0 wol. At minimum, i would suggest something in the
>>> --help text in the wol section and man page pointing to the
>>> alternative way to configure wol. And maybe report via the standard
>>> wol flags that the hardware has the capability to use flow-type WoL?
>>
>> WAKE_FILTER is supposed to be set by the driver if it supports waking-up
>> from a network filter. That is how you would know that the device supports
>> waking-up from a network filter, and then you need to configure the filters
>> with ethtool -N (rxnfc).
>>
>> Where this API is a good fit is that you can specify a filter location and
>> the action (-2 = RX_CLS_FLOW_WAKE) to indicate where to install the filter
>> and what it should do. Where it may not be such a great fit is that it is a
>> two step process, where you need to make sure you install filter(s) plus
>> enable WAKE_FILTER from the .set_wol() call.
>>
>> At the time it was proposed it felt like a reasonable way to program,
>> without having "ethtool -s eth0 wol" gain a form of packet matching parser.
>> Also, it does not seem to me like we need the operations to be necessarily
>> atomic in a single call to the kernel but if we feel like this is too
>> difficult to use, we could consider a .set_wol() call that supports being
>> passed network filter(s).
>   
> I think two step is fine. I would say anybody using rxnfc is a pretty
> advanced user.
> 
> But we should clearly define what we expect in terms of ordering and
> maybe try to enforce it in the core. Can we make the rxnfc call return
> -EBUSY or something and an extack message if WAKE_FILTER has not been
> enabled first?

It might make sense to do it the other way around, that is you must 
install filters first and if none are installed by the time we enable 
WAKE_FILTER in .set_wol(), we error out with -EINVAL?
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Andrew Lunn]

> > But we should clearly define what we expect in terms of ordering and
> > maybe try to enforce it in the core. Can we make the rxnfc call return
> > -EBUSY or something and an extack message if WAKE_FILTER has not been
> > enabled first?
> 
> It might make sense to do it the other way around, that is you must install
> filters first and if none are installed by the time we enable WAKE_FILTER in
> .set_wol(), we error out with -EINVAL?

I was thinking the other way around would be easier for the core to
enforce. When inserting an rxnfc, it can do an ethtool.get_wol(ndev)
and check WAKE_FILTER is set. That seems simpler than doing a
get_rxnfc() and having to look through the results and try to figure
out which are for WoL?

Anyway, you seem to be volunteering to implement this, so either is
fine for me, so long as we do have some central enforcement.

     Andrew

Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

On 10/12/23 14:18, Andrew Lunn wrote:
>>> But we should clearly define what we expect in terms of ordering and
>>> maybe try to enforce it in the core. Can we make the rxnfc call return
>>> -EBUSY or something and an extack message if WAKE_FILTER has not been
>>> enabled first?
>>
>> It might make sense to do it the other way around, that is you must install
>> filters first and if none are installed by the time we enable WAKE_FILTER in
>> .set_wol(), we error out with -EINVAL?
> 
> I was thinking the other way around would be easier for the core to
> enforce. When inserting an rxnfc, it can do an ethtool.get_wol(ndev)
> and check WAKE_FILTER is set. That seems simpler than doing a
> get_rxnfc() and having to look through the results and try to figure
> out which are for WoL?

What you propose would be simpler, but I believe it would be more 
logical to:

- install filter(s)
- set ethtool WAKE_FILTER

as the latter acts as a "commit", until that point the filters are 
installed, but may not be effective unless WAKE_FILTER is set and gets 
translated into the appropriate Wake-on-LAN enable bits. Of course 
nothing prevents you from doing the reverse or installing filters after 
setting WAkE_FILTER as long as the system does not go into suspend in 
between, everything should be working.

> 
> Anyway, you seem to be volunteering to implement this, so either is
> fine for me, so long as we do have some central enforcement.

Yes, patches will follow, thanks!
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]