[PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Maciej Żenczykowski]

In order to save power (battery), most network hardware
designed for low power environments (ie. battery powered
devices) supports varying types of hardware/firmware offload
(filtering and/or generating replies) of incoming packets.

The goal being to prevent device wakeups caused by ingress 'spam'.

This is particularly true for wifi (especially phones/tablets),
but isn't actually wifi specific.  It can also be implemented
in wired nics (TV) or usb ethernet dongles.

For examples TVs require this to keep power consumption
under (the EU mandated) 2 Watts while idle (display off),
while still being discoverable on the network.

This may include things like: ARP/IPv6 ND, IGMP/MLD, ping, mdns,
but various other possibilities are also possible,
for example:
  ethertype filtering (discarding non-IP ethertypes),
  nat-t keepalive (discarding ingress, automating periodic egress),
  tcp keepalive (generation/processing/filtering),
  tethering (forwarding) offload

In many ways, in its goals, it is somewhat similar to the
relatively standard destination mac filtering most wired nics
have supported for ages: reduce the amount of traffic the host
must handle.

While working on Android we've discovered that there is
no device/driver agnostic way to disable these offloads.

This patch is an attempt to rectify this.

It does not add an API to configure these offloads, as usually
this isn't needed as the driver will just fetch the required
configuration information straight from the kernel.

What it does do is add a simple API to allow disabling (masking)
them, either for debugging or for test purposes.

Cc: "Kory Maincent (Dent Project)" <kory.maincent@bootlin.com>
Cc: Ahmed Zaki <ahmed.zaki@intel.com>
Cc: Edward Cree <ecree.xilinx@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Yuyang Huang <yuyanghuang@google.com>
Cc: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 include/uapi/linux/ethtool.h | 15 +++++++++++++++
 net/ethtool/common.c         |  1 +
 net/ethtool/ioctl.c          |  5 +++++
 3 files changed, 21 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 4a0a6e703483..7b58860c3731 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -224,12 +224,27 @@ struct ethtool_value {
 #define PFC_STORM_PREVENTION_AUTO	0xffff
 #define PFC_STORM_PREVENTION_DISABLE	0
 
+/* For power/wakeup (*not* performance) related offloads */
+enum tunable_fw_offload_disable {
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_ALL,
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_ARP,
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_ND,
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_PING,
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_PING,
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_IGMP,
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_MLD,
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_MDNS,
+	ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_MDNS,
+	/* 55 bits remaining for future use */
+};
+
 enum tunable_id {
 	ETHTOOL_ID_UNSPEC,
 	ETHTOOL_RX_COPYBREAK,
 	ETHTOOL_TX_COPYBREAK,
 	ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
 	ETHTOOL_TX_COPYBREAK_BUF_SIZE,
+	ETHTOOL_FW_OFFLOAD_DISABLE, /* u64 bits numbered from LSB per tunable_fw_offload_disable */
 	/*
 	 * Add your fresh new tunable attribute above and remember to update
 	 * tunable_strings[] in net/ethtool/common.c
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 7257ae272296..8dc0c084fce5 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -91,6 +91,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
 	[ETHTOOL_TX_COPYBREAK]	= "tx-copybreak",
 	[ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout",
 	[ETHTOOL_TX_COPYBREAK_BUF_SIZE] = "tx-copybreak-buf-size",
+	[ETHTOOL_FW_OFFLOAD_DISABLE] = "fw-offload-disable",
 };
 
 const char
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 18cf9fa32ae3..31318ded17a7 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2733,6 +2733,11 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
 static int ethtool_tunable_valid(const struct ethtool_tunable *tuna)
 {
 	switch (tuna->id) {
+	case ETHTOOL_FW_OFFLOAD_DISABLE:
+		if (tuna->len != sizeof(u64) ||
+		    tuna->type_id != ETHTOOL_TUNABLE_U64)
+			return -EINVAL;
+		break;
 	case ETHTOOL_RX_COPYBREAK:
 	case ETHTOOL_TX_COPYBREAK:
 	case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
-- 
2.46.0.76.ge559c4bf1a-goog

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Jakub Kicinski]

On Tue, 13 Aug 2024 15:33:25 -0700 Maciej Żenczykowski wrote:
> In order to save power (battery), most network hardware
> designed for low power environments (ie. battery powered
> devices) supports varying types of hardware/firmware offload
> (filtering and/or generating replies) of incoming packets.
> 
> The goal being to prevent device wakeups caused by ingress 'spam'.
> 
> This is particularly true for wifi (especially phones/tablets),
> but isn't actually wifi specific.  It can also be implemented
> in wired nics (TV) or usb ethernet dongles.
> 
> For examples TVs require this to keep power consumption
> under (the EU mandated) 2 Watts while idle (display off),
> while still being discoverable on the network.

Sounds sane, adding Florian, he mentioned MDNS at last netconf.
Tho, wasn't there supposed to be a more granular API in Android
to control such protocol offloads?

You gotta find an upstream driver which implements this for us to merge.
If Florian doesn't have any quick uses -- I think Intel ethernet drivers
have private flags for enabling/disabling an LLDP agent. That could be
another way..
-- 
pw-bot: cr

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Florian Fainelli]

CC Doug, Justin,

On 8/14/2024 5:32 PM, Jakub Kicinski wrote:
> On Tue, 13 Aug 2024 15:33:25 -0700 Maciej Żenczykowski wrote:
>> In order to save power (battery), most network hardware
>> designed for low power environments (ie. battery powered
>> devices) supports varying types of hardware/firmware offload
>> (filtering and/or generating replies) of incoming packets.
>>
>> The goal being to prevent device wakeups caused by ingress 'spam'.
>>
>> This is particularly true for wifi (especially phones/tablets),
>> but isn't actually wifi specific.  It can also be implemented
>> in wired nics (TV) or usb ethernet dongles.
>>
>> For examples TVs require this to keep power consumption
>> under (the EU mandated) 2 Watts while idle (display off),
>> while still being discoverable on the network.
> 
> Sounds sane, adding Florian, he mentioned MDNS at last netconf.

Yes this looks fine to me:

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>

> Tho, wasn't there supposed to be a more granular API in Android
> to control such protocol offloads?

I still am unable to find a public link for the 
com.google.tv.mdnsoffload package that describes how we can offload mDNS 
records in ATV, but that was what I mentioned during netconf that we 
needed to program into the hardware. Ideally using an ethtool API TBD, 
or a cfg80211 one, rather than some custom ioctls() and what not.

> 
> You gotta find an upstream driver which implements this for us to merge.
> If Florian doesn't have any quick uses -- I think Intel ethernet drivers
> have private flags for enabling/disabling an LLDP agent. That could be
> another way..

Currently we have both bcmgenet and bcmasp support the WAKE_FILTER 
Wake-on-LAN specifier. Our configuration is typically done in user-space 
for mDNS with something like:

ethtool -N eth0 flow-type ether dst 33:33:00:00:00:fb action 
0xfffffffffffffffe user-def 0x320000 m 0xffffffffff000fff
ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb action 
0xfffffffffffffffe user-def 0x1e0000 m 0xffffffffff000fff
ethtool -s eth0 wol f

I would offer that we wire up the tunable into bcmgenet and bcmasp and 
we'd make sure on our side that the respective firmware implementations 
behave accordingly, but the respective firmware implementations 
currently look at whether any network filter have been programmed into 
the hardware, and if so, they are using those for offload. So we do not 
really need the tunable in a way, but if we were to add it, then we 
would need to find a way to tell the firmware not to use the network 
filters. We liked our design because there is no kernel <=> firmware 
communication.

Hummm
-- 
Florian

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Jakub Kicinski]

On Wed, 14 Aug 2024 20:08:05 -0700 Florian Fainelli wrote:
> > You gotta find an upstream driver which implements this for us to merge.
> > If Florian doesn't have any quick uses -- I think Intel ethernet drivers
> > have private flags for enabling/disabling an LLDP agent. That could be
> > another way..  
> 
> Currently we have both bcmgenet and bcmasp support the WAKE_FILTER 
> Wake-on-LAN specifier. Our configuration is typically done in user-space 
> for mDNS with something like:
> 
> ethtool -N eth0 flow-type ether dst 33:33:00:00:00:fb action 
> 0xfffffffffffffffe user-def 0x320000 m 0xffffffffff000fff
> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb action 
> 0xfffffffffffffffe user-def 0x1e0000 m 0xffffffffff000fff
> ethtool -s eth0 wol f
> 
> I would offer that we wire up the tunable into bcmgenet and bcmasp and 
> we'd make sure on our side that the respective firmware implementations 
> behave accordingly, but the respective firmware implementations 
> currently look at whether any network filter have been programmed into 
> the hardware, and if so, they are using those for offload. So we do not 
> really need the tunable in a way, but if we were to add it, then we 
> would need to find a way to tell the firmware not to use the network 
> filters. We liked our design because there is no kernel <=> firmware 
> communication.

Hm, I may be lacking the practical exposure to such systems to say
anything sensible, but when has that ever stopped me..
IIUC you're saying that FW enables MLD offload if the wake filter is 
in place. But for ping/arp/igmp offload there's no need to wake, ever,
so there wouldn't be a filter?

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Nelson, Shannon]

On 8/13/2024 3:33 PM, Maciej Żenczykowski wrote:
> 
> In order to save power (battery), most network hardware
> designed for low power environments (ie. battery powered
> devices) supports varying types of hardware/firmware offload
> (filtering and/or generating replies) of incoming packets.
> 
> The goal being to prevent device wakeups caused by ingress 'spam'.
> 
> This is particularly true for wifi (especially phones/tablets),
> but isn't actually wifi specific.  It can also be implemented
> in wired nics (TV) or usb ethernet dongles.
> 
> For examples TVs require this to keep power consumption
> under (the EU mandated) 2 Watts while idle (display off),
> while still being discoverable on the network.
> 
> This may include things like: ARP/IPv6 ND, IGMP/MLD, ping, mdns,
> but various other possibilities are also possible,
> for example:
>    ethertype filtering (discarding non-IP ethertypes),
>    nat-t keepalive (discarding ingress, automating periodic egress),
>    tcp keepalive (generation/processing/filtering),
>    tethering (forwarding) offload
> 
> In many ways, in its goals, it is somewhat similar to the
> relatively standard destination mac filtering most wired nics
> have supported for ages: reduce the amount of traffic the host
> must handle.
> 
> While working on Android we've discovered that there is
> no device/driver agnostic way to disable these offloads.
> 
> This patch is an attempt to rectify this.
> 
> It does not add an API to configure these offloads, as usually
> this isn't needed as the driver will just fetch the required
> configuration information straight from the kernel.
> 
> What it does do is add a simple API to allow disabling (masking)
> them, either for debugging or for test purposes.

I can see how this would be useful for test/debug, but it seems to me it 
is only half of a solution.  Wouldn't there also be a need to re-enable 
the offloads without having to reboot/restart the gizmo being tested? 
Or even query the current status?

sln

> 
> Cc: "Kory Maincent (Dent Project)" <kory.maincent@bootlin.com>
> Cc: Ahmed Zaki <ahmed.zaki@intel.com>
> Cc: Edward Cree <ecree.xilinx@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Yuyang Huang <yuyanghuang@google.com>
> Cc: Lorenzo Colitti <lorenzo@google.com>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
>   include/uapi/linux/ethtool.h | 15 +++++++++++++++
>   net/ethtool/common.c         |  1 +
>   net/ethtool/ioctl.c          |  5 +++++
>   3 files changed, 21 insertions(+)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 4a0a6e703483..7b58860c3731 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -224,12 +224,27 @@ struct ethtool_value {
>   #define PFC_STORM_PREVENTION_AUTO      0xffff
>   #define PFC_STORM_PREVENTION_DISABLE   0
> 
> +/* For power/wakeup (*not* performance) related offloads */
> +enum tunable_fw_offload_disable {
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_ALL,
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_ARP,
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_ND,
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_PING,
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_PING,
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_IGMP,
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_MLD,
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_MDNS,
> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_MDNS,
> +       /* 55 bits remaining for future use */
> +};
> +
>   enum tunable_id {
>          ETHTOOL_ID_UNSPEC,
>          ETHTOOL_RX_COPYBREAK,
>          ETHTOOL_TX_COPYBREAK,
>          ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
>          ETHTOOL_TX_COPYBREAK_BUF_SIZE,
> +       ETHTOOL_FW_OFFLOAD_DISABLE, /* u64 bits numbered from LSB per tunable_fw_offload_disable */
>          /*
>           * Add your fresh new tunable attribute above and remember to update
>           * tunable_strings[] in net/ethtool/common.c
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 7257ae272296..8dc0c084fce5 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -91,6 +91,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
>          [ETHTOOL_TX_COPYBREAK]  = "tx-copybreak",
>          [ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout",
>          [ETHTOOL_TX_COPYBREAK_BUF_SIZE] = "tx-copybreak-buf-size",
> +       [ETHTOOL_FW_OFFLOAD_DISABLE] = "fw-offload-disable",
>   };
> 
>   const char
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 18cf9fa32ae3..31318ded17a7 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -2733,6 +2733,11 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
>   static int ethtool_tunable_valid(const struct ethtool_tunable *tuna)
>   {
>          switch (tuna->id) {
> +       case ETHTOOL_FW_OFFLOAD_DISABLE:
> +               if (tuna->len != sizeof(u64) ||
> +                   tuna->type_id != ETHTOOL_TUNABLE_U64)
> +                       return -EINVAL;
> +               break;
>          case ETHTOOL_RX_COPYBREAK:
>          case ETHTOOL_TX_COPYBREAK:
>          case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
> --
> 2.46.0.76.ge559c4bf1a-goog
> 
> 

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Florian Fainelli]

On 8/15/24 08:45, Jakub Kicinski wrote:
> On Wed, 14 Aug 2024 20:08:05 -0700 Florian Fainelli wrote:
>>> You gotta find an upstream driver which implements this for us to merge.
>>> If Florian doesn't have any quick uses -- I think Intel ethernet drivers
>>> have private flags for enabling/disabling an LLDP agent. That could be
>>> another way..
>>
>> Currently we have both bcmgenet and bcmasp support the WAKE_FILTER
>> Wake-on-LAN specifier. Our configuration is typically done in user-space
>> for mDNS with something like:
>>
>> ethtool -N eth0 flow-type ether dst 33:33:00:00:00:fb action
>> 0xfffffffffffffffe user-def 0x320000 m 0xffffffffff000fff
>> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb action
>> 0xfffffffffffffffe user-def 0x1e0000 m 0xffffffffff000fff
>> ethtool -s eth0 wol f
>>
>> I would offer that we wire up the tunable into bcmgenet and bcmasp and
>> we'd make sure on our side that the respective firmware implementations
>> behave accordingly, but the respective firmware implementations
>> currently look at whether any network filter have been programmed into
>> the hardware, and if so, they are using those for offload. So we do not
>> really need the tunable in a way, but if we were to add it, then we
>> would need to find a way to tell the firmware not to use the network
>> filters. We liked our design because there is no kernel <=> firmware
>> communication.
> 
> Hm, I may be lacking the practical exposure to such systems to say
> anything sensible, but when has that ever stopped me..
> IIUC you're saying that FW enables MLD offload if the wake filter is
> in place. But for ping/arp/igmp offload there's no need to wake, ever,
> so there wouldn't be a filter?

That is right, we only want to wake-up on mDNS in our case. There are 
two cases deployed, at least the first one is, the second one might have 
been more of a "to be added in the future" improvement:

- a simplistic one where we use the hardware filters to trigger a 
wake-up event, and then some piece of firmware will look at the mDNS 
query contents and figure out whether the query was for one of the 
services in the local database (typically _googlecast._tcp.local is of 
particular interest). If that is the case, we trigger a system wake-up 
and we let the Host CPU process the mDNS query and we stay awake for a 
few seconds in case a streaming operation happens

- a more sophisticated one where after the mDNS query wake-up event has 
been identified we wait until we get a 3-way TCP handshake targeting the 
_googlecast._tcp.local service before waking up the Host CPU. This is 
more reflective of an actual intent to use the device that was asleep

Hope this helps.
--
Florian

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Maciej Żenczykowski]

On Wed, Aug 14, 2024 at 5:32 PM Jakub Kicinski <kuba@kernel.org> wrote:
> Sounds sane, adding Florian, he mentioned MDNS at last netconf.
> Tho, wasn't there supposed to be a more granular API in Android
> to control such protocol offloads?

We're deprecating the 'new' but already old mdns offload logic in
favour of APFv6+ (unfortunately there was a race here, we weren't
ready with APFv6 in time...).

APF [Android Packet Filter] is a bytecode interpreter built into
[wifi] firmware, currently the bytecode is fed in via Android Wifi HAL
(we'd like to change this to ethtool or something as well) from a java
bytecode generator that knows the state of the system.

APFv2 (ingress packet filtering to prevent spurious wakeups) and APFv4
(adds drop counters) have been in use on Pixel for many many years now
(going back to Pixel 3? somewhere 2016-ish).
We require all new (launched on Android 14/U+) devices to have a
working APFv4 implementation on their wifi client/sta interface
(enforced via Android VTS).
APFv6 (only available on non publicly available hardware atm) adds
support for sending back replies (ie. arp offload & the like).  We
hope to require it for newly launched devices on (the future) Android
16/W (or whatever it ends up being) and to extend it to non-wifi (ie.
wired) as well.

The APFv6 interpreter source is here:
https://android.googlesource.com/platform/hardware/google/apf/+/refs/heads/main/v6/
for the curious.

APF is *not* eBPF simply because eBPF is *far* too complex (there's
not enough space for it in the firmware/ram of many low end wifi
chipsets).  The latest APFv6 interpreter is only ~5kB of compiled
code, and another ~2kB of bytecode (though space for 4kB is
recommended).  Some wifi chipset vendors are finding it very hard to
find room for even that.

eBPF bytecode is very space inefficient compared to APF.

> You gotta find an upstream driver which implements this for us to merge.

This came about due to a request from a major wifi chipset vendor to
notify them which offloads could/should be disabled due to being
reimplemented via APFv6 (Android Packet Filter).
So I believe (note: we're still discussing this) we likely have folks
willing to implement this for at least 1 wifi driver (not sure which
one, and if it is merged upstream or lives out-of-tree).

I am of course in a very hard position here, as I don't own any
drivers/firmware - AFAIK even Pixel's wifi/cell drivers aren't Google
owned/maintained, but rather Broadcom/Synaptics/Qualcomm/Mediatek/etc
as appropriate...

I do know there is a need for an api of this sort (not necessarily
exactly this patch though), and if we merge something (sane :-) ) into
Linux, we can then backport that (either to LTS or just to ACK), and
then we (as in Google) can require implementations (for new
hardware/drivers) in future versions of Android...

Presumably that would result in implementations in many drivers,
though not necessarily any in-tree ones (I have no idea what the
current state of in-vs-out-of-tree drivers is wrt. Android wifi/cell
hardware)

This is very much a chicken-and-egg problem though.  As long as there
is no 'public' API, the default approach is for per-vendor or even
per-chip / per-driver custom apis, hidden behind Android HALs.  For
example we have such an Android HAL api for disabling ND offload on at
least one of our devices.  Of course the HAL itself is backed by
calling into the driver - just over some driver specific netlink...

Similarly for APF bytecode installation: it all goes via Android HALs,
which then in practice immediately turn around and talk straight over
driver/vendor-specific netlink to the driver.

I'd *love* to simplify all this.  Indeed in an ideal world, we'd have
'native' Linux apis into drivers to disable offloads, load APF, and
could even teach the kernel to actually build the APF bytecode to load
in in the first place (this would fix lots of races we currently have,
where we need to fetch state out of the kernel to build the bytecode
to pass back into the kernel driver/firmware...).  Perhaps a BPF
program to build the APF bytecode during the suspend sequence --
seriously, this isn't even a joke.

That said... the problem is simply too large to bite all at once, so
I'm trying to find a way out of the logjam wrt. upstream Linux that
APF has been in for years.

If we can't settle on a 'public' Linux API for this (not saying *this*
patch is necessarily the right way) we'll likely end up implementing
this either via ethtool privflags (or something) or simply via an
Android HAL... again :-( Be aware HALs are what most of the org loves
and it's hard enough to explain why we don't want to go via a HAL....

This isn't meant to be a threat ;-) it's just an unavoidable statement
of fact... :-(

What I could offer to provide is some sort of tun/tap/veth/virtio-net
based APF implementation for testing (but that's not exactly relevant
to *this* patch, since they don't support offloads to disable in the
first place) - as I would love to have that available in cuttlefish
(Android VM) for integration testing.

> If Florian doesn't have any quick uses -- I think Intel ethernet drivers
> have private flags for enabling/disabling an LLDP agent. That could be
> another way..
> --
> pw-bot: cr

--
Maciej Żenczykowski, Kernel Networking Developer @ Google

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Maciej Żenczykowski]

On Thu, Aug 15, 2024 at 9:13 AM Nelson, Shannon <shannon.nelson@amd.com> wrote:
>
> On 8/13/2024 3:33 PM, Maciej Żenczykowski wrote:
> >
> > In order to save power (battery), most network hardware
> > designed for low power environments (ie. battery powered
> > devices) supports varying types of hardware/firmware offload
> > (filtering and/or generating replies) of incoming packets.
> >
> > The goal being to prevent device wakeups caused by ingress 'spam'.
> >
> > This is particularly true for wifi (especially phones/tablets),
> > but isn't actually wifi specific.  It can also be implemented
> > in wired nics (TV) or usb ethernet dongles.
> >
> > For examples TVs require this to keep power consumption
> > under (the EU mandated) 2 Watts while idle (display off),
> > while still being discoverable on the network.
> >
> > This may include things like: ARP/IPv6 ND, IGMP/MLD, ping, mdns,
> > but various other possibilities are also possible,
> > for example:
> >    ethertype filtering (discarding non-IP ethertypes),
> >    nat-t keepalive (discarding ingress, automating periodic egress),
> >    tcp keepalive (generation/processing/filtering),
> >    tethering (forwarding) offload
> >
> > In many ways, in its goals, it is somewhat similar to the
> > relatively standard destination mac filtering most wired nics
> > have supported for ages: reduce the amount of traffic the host
> > must handle.
> >
> > While working on Android we've discovered that there is
> > no device/driver agnostic way to disable these offloads.
> >
> > This patch is an attempt to rectify this.
> >
> > It does not add an API to configure these offloads, as usually
> > this isn't needed as the driver will just fetch the required
> > configuration information straight from the kernel.
> >
> > What it does do is add a simple API to allow disabling (masking)
> > them, either for debugging or for test purposes.
>
> I can see how this would be useful for test/debug, but it seems to me it
> is only half of a solution.  Wouldn't there also be a need to re-enable
> the offloads without having to reboot/restart the gizmo being tested?
> Or even query the current status?

Since it's a mask of things to disable, setting the mask to 0, or the
relevant bit of the mask to 0 would reenable (assuming there was
anything to enable).

>
> sln
>
> >
> > Cc: "Kory Maincent (Dent Project)" <kory.maincent@bootlin.com>
> > Cc: Ahmed Zaki <ahmed.zaki@intel.com>
> > Cc: Edward Cree <ecree.xilinx@gmail.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: Yuyang Huang <yuyanghuang@google.com>
> > Cc: Lorenzo Colitti <lorenzo@google.com>
> > Signed-off-by: Maciej Żenczykowski <maze@google.com>
> > ---
> >   include/uapi/linux/ethtool.h | 15 +++++++++++++++
> >   net/ethtool/common.c         |  1 +
> >   net/ethtool/ioctl.c          |  5 +++++
> >   3 files changed, 21 insertions(+)
> >
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index 4a0a6e703483..7b58860c3731 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -224,12 +224,27 @@ struct ethtool_value {
> >   #define PFC_STORM_PREVENTION_AUTO      0xffff
> >   #define PFC_STORM_PREVENTION_DISABLE   0
> >
> > +/* For power/wakeup (*not* performance) related offloads */
> > +enum tunable_fw_offload_disable {
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_ALL,
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_ARP,
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_ND,
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_PING,
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_PING,
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_IGMP,
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_MLD,
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_MDNS,
> > +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_MDNS,
> > +       /* 55 bits remaining for future use */
> > +};
> > +
> >   enum tunable_id {
> >          ETHTOOL_ID_UNSPEC,
> >          ETHTOOL_RX_COPYBREAK,
> >          ETHTOOL_TX_COPYBREAK,
> >          ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
> >          ETHTOOL_TX_COPYBREAK_BUF_SIZE,
> > +       ETHTOOL_FW_OFFLOAD_DISABLE, /* u64 bits numbered from LSB per tunable_fw_offload_disable */
> >          /*
> >           * Add your fresh new tunable attribute above and remember to update
> >           * tunable_strings[] in net/ethtool/common.c
> > diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> > index 7257ae272296..8dc0c084fce5 100644
> > --- a/net/ethtool/common.c
> > +++ b/net/ethtool/common.c
> > @@ -91,6 +91,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
> >          [ETHTOOL_TX_COPYBREAK]  = "tx-copybreak",
> >          [ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout",
> >          [ETHTOOL_TX_COPYBREAK_BUF_SIZE] = "tx-copybreak-buf-size",
> > +       [ETHTOOL_FW_OFFLOAD_DISABLE] = "fw-offload-disable",
> >   };
> >
> >   const char
> > diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> > index 18cf9fa32ae3..31318ded17a7 100644
> > --- a/net/ethtool/ioctl.c
> > +++ b/net/ethtool/ioctl.c
> > @@ -2733,6 +2733,11 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
> >   static int ethtool_tunable_valid(const struct ethtool_tunable *tuna)
> >   {
> >          switch (tuna->id) {
> > +       case ETHTOOL_FW_OFFLOAD_DISABLE:
> > +               if (tuna->len != sizeof(u64) ||
> > +                   tuna->type_id != ETHTOOL_TUNABLE_U64)
> > +                       return -EINVAL;
> > +               break;
> >          case ETHTOOL_RX_COPYBREAK:
> >          case ETHTOOL_TX_COPYBREAK:
> >          case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
> > --
> > 2.46.0.76.ge559c4bf1a-goog
> >
> >

--
Maciej Żenczykowski, Kernel Networking Developer @ Google

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Nelson, Shannon]

On 8/15/2024 5:55 PM, Maciej Żenczykowski wrote:
> 
> On Thu, Aug 15, 2024 at 9:13 AM Nelson, Shannon <shannon.nelson@amd.com> wrote:
>>
>> On 8/13/2024 3:33 PM, Maciej Żenczykowski wrote:
>>>
>>> In order to save power (battery), most network hardware
>>> designed for low power environments (ie. battery powered
>>> devices) supports varying types of hardware/firmware offload
>>> (filtering and/or generating replies) of incoming packets.
>>>
>>> The goal being to prevent device wakeups caused by ingress 'spam'.
>>>
>>> This is particularly true for wifi (especially phones/tablets),
>>> but isn't actually wifi specific.  It can also be implemented
>>> in wired nics (TV) or usb ethernet dongles.
>>>
>>> For examples TVs require this to keep power consumption
>>> under (the EU mandated) 2 Watts while idle (display off),
>>> while still being discoverable on the network.
>>>
>>> This may include things like: ARP/IPv6 ND, IGMP/MLD, ping, mdns,
>>> but various other possibilities are also possible,
>>> for example:
>>>     ethertype filtering (discarding non-IP ethertypes),
>>>     nat-t keepalive (discarding ingress, automating periodic egress),
>>>     tcp keepalive (generation/processing/filtering),
>>>     tethering (forwarding) offload
>>>
>>> In many ways, in its goals, it is somewhat similar to the
>>> relatively standard destination mac filtering most wired nics
>>> have supported for ages: reduce the amount of traffic the host
>>> must handle.
>>>
>>> While working on Android we've discovered that there is
>>> no device/driver agnostic way to disable these offloads.
>>>
>>> This patch is an attempt to rectify this.
>>>
>>> It does not add an API to configure these offloads, as usually
>>> this isn't needed as the driver will just fetch the required
>>> configuration information straight from the kernel.
>>>
>>> What it does do is add a simple API to allow disabling (masking)
>>> them, either for debugging or for test purposes.
>>
>> I can see how this would be useful for test/debug, but it seems to me it
>> is only half of a solution.  Wouldn't there also be a need to re-enable
>> the offloads without having to reboot/restart the gizmo being tested?
>> Or even query the current status?
> 
> Since it's a mask of things to disable, setting the mask to 0, or the
> relevant bit of the mask to 0 would reenable (assuming there was
> anything to enable).

That assumption is not clear in this patch, it only talks about things 
getting disabled.  Let's be sure it is clear that 0 will actively enable 
the features.

I think part of the issue is the use of "DISABLE" and "disable" in all 
the values - it tends to give a one-way meaning.  Maybe using "MASK" and 
"mask" instead would be a better way to imply both enable and disable.

sln

> 
>>
>> sln
>>
>>>
>>> Cc: "Kory Maincent (Dent Project)" <kory.maincent@bootlin.com>
>>> Cc: Ahmed Zaki <ahmed.zaki@intel.com>
>>> Cc: Edward Cree <ecree.xilinx@gmail.com>
>>> Cc: "David S. Miller" <davem@davemloft.net>
>>> Cc: Eric Dumazet <edumazet@google.com>
>>> Cc: Jakub Kicinski <kuba@kernel.org>
>>> Cc: Paolo Abeni <pabeni@redhat.com>
>>> Cc: Yuyang Huang <yuyanghuang@google.com>
>>> Cc: Lorenzo Colitti <lorenzo@google.com>
>>> Signed-off-by: Maciej Żenczykowski <maze@google.com>
>>> ---
>>>    include/uapi/linux/ethtool.h | 15 +++++++++++++++
>>>    net/ethtool/common.c         |  1 +
>>>    net/ethtool/ioctl.c          |  5 +++++
>>>    3 files changed, 21 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>>> index 4a0a6e703483..7b58860c3731 100644
>>> --- a/include/uapi/linux/ethtool.h
>>> +++ b/include/uapi/linux/ethtool.h
>>> @@ -224,12 +224,27 @@ struct ethtool_value {
>>>    #define PFC_STORM_PREVENTION_AUTO      0xffff
>>>    #define PFC_STORM_PREVENTION_DISABLE   0
>>>
>>> +/* For power/wakeup (*not* performance) related offloads */
>>> +enum tunable_fw_offload_disable {
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_ALL,
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_ARP,
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_ND,
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_PING,
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_PING,
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_IGMP,
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_MLD,
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV4_MDNS,
>>> +       ETHTOOL_TUNABLE_FW_OFFLOAD_DISABLE_IPV6_MDNS,
>>> +       /* 55 bits remaining for future use */
>>> +};
>>> +
>>>    enum tunable_id {
>>>           ETHTOOL_ID_UNSPEC,
>>>           ETHTOOL_RX_COPYBREAK,
>>>           ETHTOOL_TX_COPYBREAK,
>>>           ETHTOOL_PFC_PREVENTION_TOUT, /* timeout in msecs */
>>>           ETHTOOL_TX_COPYBREAK_BUF_SIZE,
>>> +       ETHTOOL_FW_OFFLOAD_DISABLE, /* u64 bits numbered from LSB per tunable_fw_offload_disable */
>>>           /*
>>>            * Add your fresh new tunable attribute above and remember to update
>>>            * tunable_strings[] in net/ethtool/common.c
>>> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
>>> index 7257ae272296..8dc0c084fce5 100644
>>> --- a/net/ethtool/common.c
>>> +++ b/net/ethtool/common.c
>>> @@ -91,6 +91,7 @@ tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = {
>>>           [ETHTOOL_TX_COPYBREAK]  = "tx-copybreak",
>>>           [ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout",
>>>           [ETHTOOL_TX_COPYBREAK_BUF_SIZE] = "tx-copybreak-buf-size",
>>> +       [ETHTOOL_FW_OFFLOAD_DISABLE] = "fw-offload-disable",
>>>    };
>>>
>>>    const char
>>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>>> index 18cf9fa32ae3..31318ded17a7 100644
>>> --- a/net/ethtool/ioctl.c
>>> +++ b/net/ethtool/ioctl.c
>>> @@ -2733,6 +2733,11 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
>>>    static int ethtool_tunable_valid(const struct ethtool_tunable *tuna)
>>>    {
>>>           switch (tuna->id) {
>>> +       case ETHTOOL_FW_OFFLOAD_DISABLE:
>>> +               if (tuna->len != sizeof(u64) ||
>>> +                   tuna->type_id != ETHTOOL_TUNABLE_U64)
>>> +                       return -EINVAL;
>>> +               break;
>>>           case ETHTOOL_RX_COPYBREAK:
>>>           case ETHTOOL_TX_COPYBREAK:
>>>           case ETHTOOL_TX_COPYBREAK_BUF_SIZE:
>>> --
>>> 2.46.0.76.ge559c4bf1a-goog
>>>
>>>
> 
> --
> Maciej Żenczykowski, Kernel Networking Developer @ Google

Re: [PATCH net-next] ethtool: add tunable api to disable various firmware offloads

[Jakub Kicinski]

On Thu, 15 Aug 2024 17:49:42 -0700 Maciej Żenczykowski wrote:
> I am of course in a very hard position here, as I don't own any
> drivers/firmware - AFAIK even Pixel's wifi/cell drivers aren't Google
> owned/maintained, but rather Broadcom/Synaptics/Qualcomm/Mediatek/etc
> as appropriate...
> 
> I do know there is a need for an api of this sort (not necessarily
> exactly this patch though), and if we merge something (sane :-) ) into
> Linux, we can then backport that (either to LTS or just to ACK), and
> then we (as in Google) can require implementations (for new
> hardware/drivers) in future versions of Android...

That's why I'm suggesting the LLDP in the Intel Ethernet driver.
Others may disagree but for me it's close enough to merge a "enable
L2 protocol agent" sort of an API. We don't need to have upstream users
for each proto. Bigger cause of sadness is that the API IIUC is a part
of a deprecation path, IOW once APF comes, it will become dead weight.
Luckily it's not a lot of code.

> Presumably that would result in implementations in many drivers,
> though not necessarily any in-tree ones (I have no idea what the
> current state of in-vs-out-of-tree drivers is wrt. Android wifi/cell
> hardware)
> 
> This is very much a chicken-and-egg problem though.  As long as there
> is no 'public' API, the default approach is for per-vendor or even
> per-chip / per-driver custom apis, hidden behind Android HALs.  For
> example we have such an Android HAL api for disabling ND offload on at
> least one of our devices.  Of course the HAL itself is backed by
> calling into the driver - just over some driver specific netlink...

I wonder if there's anything we can share between APF style offloads
and Jamal's P4 work, if it materializes.