2023-05-17 13:08:29

by Andrew Lunn

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

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?

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?

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.

Andrew

2023-05-17 16:00:35

by Florian Fainelli

[permalink] [raw]
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


Attachments:
smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2023-05-17 21:50:54

by Andrew Lunn

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

> > > 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.

Ah. Please could you extend the man page for ethtool. Maybe make flow
type action -2 reference wol, and wol f reference flow-type?

> 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

Yes, i was thinking something like that.

> but then we have some intersection with the 'u', 'm' and 'b' options too,
> which are just short hand for specific MAC DAs.

The man page for ethtool say:

sopass xx:yy:zz:aa:bb:cc
Sets the SecureOn™ password. The argument to this option
must be 6 bytes in Ethernet MAC hex format
(xx:yy:zz:aa:bb:cc).

So i don't think it is too much of an API bendage to pass a MAC
address in a union.

I had a quick look at some Marvell switches. They allow an arbitrary
Unicast MAC address to be used to wake a port. So such an extension
could be used for it as well.

And it looks like the Marcell Alaska PHY could implement it as
well. So it would not be limited to just the Broadcom PHYs.

Andrew