| Message ID | 20180717153645.7500-2-f.fainelli@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | John Linville |
| Headers | show |
| Series | [ethtool] ethtool: Add support for WAKE_FILTER | expand |
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", >
> 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
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.
> 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
> 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
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 :-/
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?
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?
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!
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.
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",
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(-)