Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 17 May 2023 14:49:49 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Florian Fainelli <florian.fainelli@broadcom.com>
Cc:     netdev@vger.kernel.org, Doug Berger <opendmb@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next 2/3] net: phy: broadcom: Add support for
 WAKE_FILTER
Message-ID: <a47d27e0-a8ef-4df0-aa45-623dda9e6412@lunn.ch>
References: <20230516231713.2882879-1-florian.fainelli@broadcom.com>
 <20230516231713.2882879-3-florian.fainelli@broadcom.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230516231713.2882879-3-florian.fainelli@broadcom.com>
Precedence: bulk

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