Message-ID: <011706c2-f0fb-42d2-81a9-7e5e4fbd784d@broadcom.com>
Date: Wed, 17 May 2023 08:54:46 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, Doug Berger <opendmb@...il.com>,
 Florian Fainelli <f.fainelli@...il.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 2/3] net: phy: broadcom: Add support for
 WAKE_FILTER



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

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

Powered by blists - more mailing lists