[ethtool] ethtool: Add support for WAKE_FILTER

Message ID 20180717153645.7500-2-f.fainelli@gmail.com
State Superseded
Delegated to: John Linville
Headers show
Series
  • [ethtool] ethtool: Add support for WAKE_FILTER
Related show

Commit Message

Florian Fainelli July 17, 2018, 3:36 p.m.
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(-)

Comments

Florian Fainelli July 30, 2018, 10:26 p.m. | #1
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",
>
Andrew Lunn July 30, 2018, 10:30 p.m. | #2
> 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
Florian Fainelli July 30, 2018, 10:39 p.m. | #3
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.
Andrew Lunn July 30, 2018, 10:55 p.m. | #4
> 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
Andrew Lunn July 30, 2018, 11:01 p.m. | #5
> 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
David Miller Aug. 1, 2018, 4:32 p.m. | #6
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 :-/
Florian Fainelli Aug. 3, 2018, 5:57 p.m. | #7
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?
David Miller Aug. 3, 2018, 7:07 p.m. | #8
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?
Florian Fainelli Aug. 3, 2018, 7:58 p.m. | #9
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!
David Miller Aug. 3, 2018, 8:18 p.m. | #10
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.

Patch

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",