Re: [PATCH net-next 2/3] net: phy: broadcom: Add support for WAKE_FILTER

[Florian Fainelli <florian.fainelli@broadcom.com>]

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



On 5/17/2023 5:49 AM, Andrew Lunn wrote:
> On Tue, May 16, 2023 at 04:17:12PM -0700, Florian Fainelli wrote:
>> Since the PHY is capable of matching any arbitrary Ethernet MAC
>> destination as a programmable wake-up pattern, add support for doing
>> that using the WAKE_FILTER and ethtool::rxnfc API.
> 
> Are there other actions the PHY can perform?

Not really, it can match on a custom Ethernet MAC DA and that is pretty 
much it, unless you use the WAKE_MAGIC or WAKE_MAGICSECURE where it can 
match either or.

> 
> For a MAC based filter, i expect there are other actions, like drop,
> queue selection, etc. So using the generic RXNFC API makes some sense.
> 
>> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2
>> ethtool -n eth0
>> Total 1 rules
>>
>> Filter: 0
>>          Flow Type: Raw Ethernet
>>          Src MAC addr: 00:00:00:00:00:00 mask: FF:FF:FF:FF:FF:FF
>>          Dest MAC addr: 01:00:5E:00:00:FB mask: 00:00:00:00:00:00
>>          Ethertype: 0x0 mask: 0xFFFF
>>          Action: Wake-on-LAN
>> ethtool -s eth0 wol f
> 
> What i don't particularly like about this is its not vary
> discoverable, since it is not part of:
> 
>            wol p|u|m|b|a|g|s|f|d...
>                    Sets Wake-on-LAN options.  Not all devices support
>                    this.  The argument to this option is a string of
>                    characters specifying which options to enable.
> 
>                    p   Wake on PHY activity
>                    u   Wake on unicast messages
>                    m   Wake on multicast messages
>                    b   Wake on broadcast messages
>                    a   Wake on ARP
>                    g   Wake on MagicPacket™
>                    s   Enable SecureOn™ password for MagicPacket™
>                    f   Wake on filter(s)
>                    d   Disable (wake on  nothing).   This  option
>                        clears all previous options.
> 
> If the PHY hardware is not generic, it only has one action, WoL, it
> might be better to have this use the standard wol commands. Can it be
> made to work under the 'f' option?

You actually need both, if you only configure the filter with 
RX_CLS_FLOW_WAKE but forget to set the 'f' bit in wolopts, then the 
wake-up will not occur because the PHY will not have been configured 
with the correct matching mode.

This is how I originally designed it for SYSTEMPORT and this was copied 
over to GENET and now to this PHY driver.

> 
> The API to the PHY driver could then be made much more narrow, and you
> would not need the comment this is supposed to only be used for WoL.

I was initially considering that the 'sopass' field could become an 
union since it is exactly the size of a MAC address (6 bytes) and you 
could do something like:

ethtool -s eth0 wol f mac 01:00:5E:00:00:FB

but then we have some intersection with the 'u', 'm' and 'b' options 
too, which are just short hand for specific MAC DAs. This felt a bit 
like bending the framework for one specific PHY that supports that use 
case, so I felt like RXNFC, although we only use a very narrow space 
could be a better fit in case a more capable PHY came along in the future.
-- 
Florian

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