Message-ID: <0271cea4-f2ab-4d8c-aa0a-9dd65a1318db@broadcom.com>
Date: Thu, 12 Oct 2023 11:32:28 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>
Cc: Michal Kubecek <mkubecek@...e.cz>, netdev@...r.kernel.org,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Jonathan Corbet <corbet@....net>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>,
 Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, opendmb@...il.com, justin.chen@...adcom.com
Subject: Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA

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


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)

Powered by blists - more mailing lists