[PATCH net-next 0/3] WAKE_FILTER for Broadcom PHY

[PATCH net-next 0/3] WAKE_FILTER for Broadcom PHY

[Florian Fainelli]

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

This patch series adds support for WAKE_FILTER to the Broadcom PHY with
the narrow use case of being able to program a custom Ethernet MAC DA to
be waking up from.

This is currently useful for Set-top-box applications where we might
want to wake-up from select multicast MAC DA pertaining to mDNS for
instance (Wake-on-Cast typically).

The approach taken here is the same as what has been pioneered and
proposed before for the GENET and SYSTEMPORT drivers.

Thanks!

Florian Fainelli (3):
  net: phy: Add pluming for ethtool_{get,set}_rxnfc
  net: phy: broadcom: Add support for WAKE_FILTER
  net: bcmgenet: Interrogate PHY for WAKE_FILTER programming

 .../net/ethernet/broadcom/genet/bcmgenet.c    |  12 ++
 drivers/net/phy/bcm-phy-lib.c                 | 147 +++++++++++++++++-
 drivers/net/phy/bcm-phy-lib.h                 |   6 +
 drivers/net/phy/broadcom.c                    |  15 ++
 drivers/net/phy/phy.c                         |  19 +++
 include/linux/phy.h                           |   8 +
 6 files changed, 206 insertions(+), 1 deletion(-)

-- 
2.34.1


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

[PATCH net-next 1/3] net: phy: Add pluming for ethtool_{get,set}_rxnfc

[Florian Fainelli]

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

Ethernet MAC drivers supporting Wake-on-LAN using programmable filters
(WAKE_FILTER) typically configure such programmable filters using the
ethtool::set_rxnfc API and with a sepcial RX_CLS_FLOW_WAKE to indicate
the filter is also wake-up capable.

In order to offer the same functionality for capable Ethernet PHY
drivers, wire-up the ethtool::{get,set}_rxnfc APIs within the PHY
library.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/phy/phy.c | 19 +++++++++++++++++++
 include/linux/phy.h   |  8 ++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0c0df38cd1ab..15c03fb5aab4 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1683,3 +1683,22 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
 	return ret;
 }
 EXPORT_SYMBOL(phy_ethtool_nway_reset);
+
+int phy_ethtool_get_rxnfc(struct phy_device *phydev,
+			  struct ethtool_rxnfc *nfc, u32 *rule_locs)
+{
+	if (phydev->drv && phydev->drv->get_rxnfc)
+		return phydev->drv->get_rxnfc(phydev, nfc, rule_locs);
+
+	return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL(phy_ethtool_get_rxnfc);
+
+int phy_ethtool_set_rxnfc(struct phy_device *phydev, struct ethtool_rxnfc *nfc)
+{
+	if (phydev->drv && phydev->drv->set_rxnfc)
+		return phydev->drv->set_rxnfc(phydev, nfc);
+
+	return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL(phy_ethtool_set_rxnfc);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e0df8b3c2bdb..3de9ac620088 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1069,6 +1069,10 @@ struct phy_driver {
 	int (*get_sqi)(struct phy_device *dev);
 	/** @get_sqi_max: Get the maximum signal quality indication */
 	int (*get_sqi_max)(struct phy_device *dev);
+	/* Used for WAKE_FILTER programming only */
+	int (*get_rxnfc)(struct phy_device *dev,
+			 struct ethtool_rxnfc *nfc, u32 *rule_locs);
+	int (*set_rxnfc)(struct phy_device *dev, struct ethtool_rxnfc *nfc);
 
 	/* PLCA RS interface */
 	/** @get_plca_cfg: Return the current PLCA configuration */
@@ -1920,6 +1924,10 @@ int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
 			     struct netlink_ext_ack *extack);
 int phy_ethtool_get_plca_status(struct phy_device *phydev,
 				struct phy_plca_status *plca_st);
+int phy_ethtool_get_rxnfc(struct phy_device *phydev,
+			  struct ethtool_rxnfc *nfc, u32 *rule_locs);
+int phy_ethtool_set_rxnfc(struct phy_device *phydev,
+			  struct ethtool_rxnfc *nfc);
 
 static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
 {
-- 
2.34.1


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

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

[Florian Fainelli]

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

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. For instance, in
order to wake-up from the Ethernet MAC address corresponding to the IPv4
multicast IP address of 224.0.0.1, one could do:

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

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/phy/bcm-phy-lib.c | 147 +++++++++++++++++++++++++++++++++-
 drivers/net/phy/bcm-phy-lib.h |   6 ++
 drivers/net/phy/broadcom.c    |  15 ++++
 3 files changed, 167 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 5603d0a9ce96..546c21ce9775 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -822,7 +822,8 @@ EXPORT_SYMBOL_GPL(bcm_phy_cable_test_get_status_rdb);
 					 WAKE_MCAST | \
 					 WAKE_BCAST | \
 					 WAKE_MAGIC | \
-					 WAKE_MAGICSECURE)
+					 WAKE_MAGICSECURE | \
+					 WAKE_FILTER)
 
 int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
@@ -876,6 +877,12 @@ int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 	ctl &= ~BCM54XX_WOL_DIR_PKT_EN;
 	ctl &= ~(BCM54XX_WOL_SECKEY_OPT_MASK << BCM54XX_WOL_SECKEY_OPT_SHIFT);
 
+	/* For WAKE_FILTER, we have already programmed the desired MAC DA
+	 * and associated mask by the time we get there.
+	 */
+	if (wol->wolopts & WAKE_FILTER)
+		goto program_ctl;
+
 	/* When using WAKE_MAGIC, we program the magic pattern filter to match
 	 * the device's MAC address and we accept any MAC DA in the Ethernet
 	 * frame.
@@ -930,6 +937,7 @@ int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 			return ret;
 	}
 
+program_ctl:
 	if (wol->wolopts & WAKE_MAGICSECURE) {
 		ctl |= BCM54XX_WOL_SECKEY_OPT_6B <<
 		       BCM54XX_WOL_SECKEY_OPT_SHIFT;
@@ -1034,6 +1042,143 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_wol_isr);
 
+static int bcm_phy_get_rule(struct phy_device *phydev,
+			    struct ethtool_rxnfc *nfc,
+			    int loc)
+{
+	u8 da[ETH_ALEN];
+	unsigned int i;
+	int ret;
+
+	if (loc != 0)
+		return -EINVAL;
+
+	memset(nfc, 0, sizeof(*nfc));
+	nfc->flow_type = ETHER_FLOW;
+	nfc->fs.flow_type = ETHER_FLOW;
+
+	for (i = 0; i < sizeof(da) / 2; i++) {
+		ret = bcm_phy_read_exp(phydev,
+				       BCM54XX_WOL_MPD_DATA2(2 - i));
+		if (ret < 0)
+			return ret;
+
+		da[i * 2] = ret >> 8;
+		da[i * 2 + 1] = ret & 0xff;
+	}
+	ether_addr_copy(nfc->fs.h_u.ether_spec.h_dest, da);
+
+	for (i = 0; i < sizeof(da) / 2; i++) {
+		ret = bcm_phy_read_exp(phydev,
+				       BCM54XX_WOL_MASK(2 - i));
+		if (ret < 0)
+			return ret;
+
+		da[i * 2] = ret >> 8;
+		da[i * 2 + 1] = ret & 0xff;
+	}
+	ether_addr_copy(nfc->fs.m_u.ether_spec.h_dest, da);
+
+	nfc->fs.ring_cookie = RX_CLS_FLOW_WAKE;
+	nfc->fs.location = 0;
+
+	return 0;
+}
+
+static int bcm_phy_set_rule(struct phy_device *phydev,
+			    struct ethtool_rxnfc *nfc)
+{
+	int ret = -EOPNOTSUPP;
+	unsigned int i;
+	const u8 *da;
+
+	/* We support only matching on the MAC DA, reject anything else */
+	if (nfc->fs.ring_cookie != RX_CLS_FLOW_WAKE ||
+	    nfc->fs.location != 0 ||
+	    nfc->fs.flow_type != ETHER_FLOW ||
+	    nfc->fs.h_u.ether_spec.h_proto ||
+	    !is_zero_ether_addr(nfc->fs.h_u.ether_spec.h_source) ||
+	    nfc->fs.m_u.ether_spec.h_proto ||
+	    !is_zero_ether_addr(nfc->fs.m_u.ether_spec.h_source))
+		return ret;
+
+	da = nfc->fs.h_u.ether_spec.h_dest;
+	for (i = 0; i < ETH_ALEN / 2; i++) {
+		ret = bcm_phy_write_exp(phydev,
+					BCM54XX_WOL_MPD_DATA2(2 - i),
+					da[i * 2] << 8 | da[i * 2 + 1]);
+		if (ret < 0)
+			return ret;
+	}
+
+	da = nfc->fs.m_u.ether_spec.h_dest;
+	for (i = 0; i < ETH_ALEN / 2; i++) {
+		ret = bcm_phy_write_exp(phydev,
+					BCM54XX_WOL_MASK(2 - i),
+					da[i * 2] << 8 | da[i * 2 + 1]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+int bcm_phy_get_rxnfc(struct phy_device *phydev,
+		      struct ethtool_rxnfc *cmd, u32 *rule_locs)
+{
+	int err = 0;
+
+	switch (cmd->cmd) {
+	case ETHTOOL_GRXCLSRLCNT:
+		cmd->rule_cnt = 1;
+		cmd->data = 1 | RX_CLS_LOC_SPECIAL;
+		break;
+	case ETHTOOL_GRXCLSRULE:
+		err = bcm_phy_get_rule(phydev, cmd, cmd->fs.location);
+		break;
+	case ETHTOOL_GRXCLSRLALL:
+		rule_locs[0] = 0;
+		cmd->rule_cnt = 1;
+		cmd->data = 1;
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_get_rxnfc);
+
+int bcm_phy_set_rxnfc(struct phy_device *phydev,
+		      struct ethtool_rxnfc *cmd,
+		      bool *installed)
+{
+	int err = 0;
+
+	switch (cmd->cmd) {
+	case ETHTOOL_SRXCLSRLINS:
+		err = bcm_phy_set_rule(phydev, cmd);
+		if (err)
+			return err;
+
+		*installed = true;
+		break;
+	case ETHTOOL_SRXCLSRLDEL:
+		if (cmd->fs.location != 0)
+			return err;
+
+		*installed = false;
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_set_rxnfc);
+
 MODULE_DESCRIPTION("Broadcom PHY Library");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Broadcom Corporation");
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 2f30ce0cab0e..4881ea34e99c 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -118,4 +118,10 @@ int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol);
 void bcm_phy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol);
 irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
 
+int bcm_phy_get_rxnfc(struct phy_device *phydev,
+		      struct ethtool_rxnfc *nfc, u32 *rule_locs);
+int bcm_phy_set_rxnfc(struct phy_device *phydev,
+		      struct ethtool_rxnfc *nfc,
+		      bool *installed);
+
 #endif /* _LINUX_BCM_PHY_LIB_H */
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 822c8b01dc53..b4a8aba7d5ef 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -36,6 +36,7 @@ struct bcm54xx_phy_priv {
 	struct bcm_ptp_private *ptp;
 	int	wake_irq;
 	bool	wake_irq_enabled;
+	bool	wake_filter_installed;
 };
 
 static bool bcm54xx_phy_can_wakeup(struct phy_device *phydev)
@@ -860,6 +861,8 @@ static int brcm_fet_suspend(struct phy_device *phydev)
 static void bcm54xx_phy_get_wol(struct phy_device *phydev,
 				struct ethtool_wolinfo *wol)
 {
+	struct bcm54xx_phy_priv *priv = phydev->priv;
+
 	/* We cannot wake-up if we do not have a dedicated PHY interrupt line
 	 * or an out of band GPIO descriptor for wake-up. Zeroing
 	 * wol->supported allows the caller (MAC driver) to play through and
@@ -871,6 +874,8 @@ static void bcm54xx_phy_get_wol(struct phy_device *phydev,
 	}
 
 	bcm_phy_get_wol(phydev, wol);
+	if (priv->wake_filter_installed)
+		wol->wolopts |= WAKE_FILTER;
 }
 
 static int bcm54xx_phy_set_wol(struct phy_device *phydev,
@@ -893,6 +898,14 @@ static int bcm54xx_phy_set_wol(struct phy_device *phydev,
 	return 0;
 }
 
+static int bcm54xx_phy_set_rxnfc(struct phy_device *phydev,
+				 struct ethtool_rxnfc *cmd)
+{
+	struct bcm54xx_phy_priv *priv = phydev->priv;
+
+	return bcm_phy_set_rxnfc(phydev, cmd, &priv->wake_filter_installed);
+}
+
 static int bcm54xx_phy_probe(struct phy_device *phydev)
 {
 	struct bcm54xx_phy_priv *priv;
@@ -1031,6 +1044,8 @@ static struct phy_driver broadcom_drivers[] = {
 	.resume		= bcm54xx_resume,
 	.get_wol	= bcm54xx_phy_get_wol,
 	.set_wol	= bcm54xx_phy_set_wol,
+	.get_rxnfc	= bcm_phy_get_rxnfc,
+	.set_rxnfc	= bcm54xx_phy_set_rxnfc,
 }, {
 	.phy_id		= PHY_ID_BCM5461,
 	.phy_id_mask	= 0xfffffff0,
-- 
2.34.1


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

[PATCH net-next 3/3] net: bcmgenet: Interrogate PHY for WAKE_FILTER programming

[Florian Fainelli]

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

Determine whether the PHY can support waking up from the user programmed
network filter, and if it can utilize it.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index f28ffc31df22..bbd9d4b73402 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1527,6 +1527,12 @@ static int bcmgenet_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	int err = 0;
 
+	if (dev->phydev) {
+		err = phy_ethtool_set_rxnfc(dev->phydev, cmd);
+		if (err != -EOPNOTSUPP)
+			return err;
+	}
+
 	switch (cmd->cmd) {
 	case ETHTOOL_SRXCLSRLINS:
 		err = bcmgenet_insert_flow(dev, cmd);
@@ -1582,6 +1588,12 @@ static int bcmgenet_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
 	int err = 0;
 	int i = 0;
 
+	if (dev->phydev) {
+		err = phy_ethtool_get_rxnfc(dev->phydev, cmd, rule_locs);
+		if (err != -EOPNOTSUPP)
+			return err;
+	}
+
 	switch (cmd->cmd) {
 	case ETHTOOL_GRXRINGS:
 		cmd->data = priv->hw_params->rx_queues ?: 1;
-- 
2.34.1


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

Re: [PATCH net-next 0/3] WAKE_FILTER for Broadcom PHY

[Simon Horman]

On Tue, May 16, 2023 at 04:17:10PM -0700, Florian Fainelli wrote:
> This patch series adds support for WAKE_FILTER to the Broadcom PHY with
> the narrow use case of being able to program a custom Ethernet MAC DA to
> be waking up from.
> 
> This is currently useful for Set-top-box applications where we might
> want to wake-up from select multicast MAC DA pertaining to mDNS for
> instance (Wake-on-Cast typically).
> 
> The approach taken here is the same as what has been pioneered and
> proposed before for the GENET and SYSTEMPORT drivers.
> 
> Thanks!

Hi Florian,

I hate to be a pain.
But this series doesn't apply on net-next.

--
pw-bot: cr

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

[Andrew Lunn]

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

Re: [PATCH net-next 0/3] WAKE_FILTER for Broadcom PHY

[Florian Fainelli]

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



On 5/17/2023 2:24 AM, Simon Horman wrote:
> On Tue, May 16, 2023 at 04:17:10PM -0700, Florian Fainelli wrote:
>> This patch series adds support for WAKE_FILTER to the Broadcom PHY with
>> the narrow use case of being able to program a custom Ethernet MAC DA to
>> be waking up from.
>>
>> This is currently useful for Set-top-box applications where we might
>> want to wake-up from select multicast MAC DA pertaining to mDNS for
>> instance (Wake-on-Cast typically).
>>
>> The approach taken here is the same as what has been pioneered and
>> proposed before for the GENET and SYSTEMPORT drivers.
>>
>> Thanks!
> 
> Hi Florian,
> 
> I hate to be a pain.
> But this series doesn't apply on net-next.

Right, that's because it depends upon "[PATCH net-next] net: phy: 
broadcom: Register dummy IRQ handler". I did not make that clear in the 
cover letter but definitively should have.
-- 
Florian

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

Re: [PATCH net-next 0/3] WAKE_FILTER for Broadcom PHY

[Simon Horman]

On Wed, May 17, 2023 at 08:18:25AM -0700, Florian Fainelli wrote:
> 
> 
> On 5/17/2023 2:24 AM, Simon Horman wrote:
> > On Tue, May 16, 2023 at 04:17:10PM -0700, Florian Fainelli wrote:
> > > This patch series adds support for WAKE_FILTER to the Broadcom PHY with
> > > the narrow use case of being able to program a custom Ethernet MAC DA to
> > > be waking up from.
> > > 
> > > This is currently useful for Set-top-box applications where we might
> > > want to wake-up from select multicast MAC DA pertaining to mDNS for
> > > instance (Wake-on-Cast typically).
> > > 
> > > The approach taken here is the same as what has been pioneered and
> > > proposed before for the GENET and SYSTEMPORT drivers.
> > > 
> > > Thanks!
> > 
> > Hi Florian,
> > 
> > I hate to be a pain.
> > But this series doesn't apply on net-next.
> 
> Right, that's because it depends upon "[PATCH net-next] net: phy: broadcom:
> Register dummy IRQ handler". I did not make that clear in the cover letter
> but definitively should have.

Thanks Florian, got it.

Of course review can occur within that context.
But perhaps it is best to repost once those patches are in,
so the CI can run.

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

[Florian Fainelli]

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

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

[Andrew Lunn]

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