[PATCH net-next 0/5] WAKE_FILTER for Broadcom PHY (v2)

[PATCH net-next 0/5] WAKE_FILTER for Broadcom PHY (v2)

[Florian Fainelli]

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

This is a re-submission of the series that was submitted before:

https://lore.kernel.org/all/20230516231713.2882879-1-florian.fainelli@broadcom.com/

A number of things have changed:

- there is proper fallback between the PHY and the MAC now, whereas
  before we would abort right away due to -EOPNOTSUPP

- added support for programming the Ethernet type for matching

- fixed reporting of masks via ethtool

- added proper validation of RX_CLS_LOC_ANY/RX_CLS_LOC_FIRST and
  location

- addressed feedback given by Andrew by validating that we have at least
  one RX_CLS_FLOW_WAKE filter programmed to allow enabling WAKE_FILTER
  via ethtool, otherwise we will not wake-up from WAKE_FILTER

- use a PHY register as a scratchpad to accurately report whether a
  custom filter was installed, before that we would falsely report
  WAKE_UCAST/BCAST/MCAST but not WAKE_FILTER upon cold boot

- corrected commit message to be accurate (MAC address did not match the
  IPv4 address in the example)

Florian Fainelli (5):
  net: ethtool: Make RXNFC walking code accept a callback
  net: ethtool: Add validation for WAKE_FILTER
  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    |  16 ++
 drivers/net/phy/bcm-phy-lib.c                 | 205 +++++++++++++++++-
 drivers/net/phy/bcm-phy-lib.h                 |   5 +
 drivers/net/phy/broadcom.c                    |   2 +
 drivers/net/phy/phy.c                         |  19 ++
 include/linux/phy.h                           |   8 +
 net/ethtool/common.c                          |  96 +++++++-
 net/ethtool/common.h                          |   3 +
 net/ethtool/ioctl.c                           |   3 +
 net/ethtool/wol.c                             |   3 +
 10 files changed, 350 insertions(+), 10 deletions(-)

-- 
2.34.1



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

[PATCH net-next 1/5] net: ethtool: Make RXNFC walking code accept a callback

[Florian Fainelli]

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

In preparation for iterating over RXNFC rules for a different purpose,
factor the generic code that already does that by allowing a callback to
be specified. The body of ethtool_get_max_rxnfc_channel() now accepts a
callback as an argument and is renamed to __ethtool_for_each_rxnfc().

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 net/ethtool/common.c | 54 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index b4419fb6df6a..143dae872fb2 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -536,12 +536,24 @@ static int ethtool_get_rxnfc_rule_count(struct net_device *dev)
 	return info.rule_cnt;
 }
 
-int ethtool_get_max_rxnfc_channel(struct net_device *dev, u64 *max)
+/**
+ * __ethtool_for_each_rxnfc: Iterate over each RXNFC rule installed
+ * @dev: network device
+ * @cb: callback to analyze an %ethtool_rxnfc rule
+ * @priv: private pointer passed to the callback
+ *
+ * @cb is supposed to return the following:
+ *   < 0 on error
+ *   == 0 to continue
+ *   > 0 to stop iterating
+ */
+static int __ethtool_for_each_rxnfc(struct net_device *dev,
+				    int (*cb)(struct ethtool_rxnfc *info,
+					      void *priv), void *priv)
 {
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	struct ethtool_rxnfc *info;
 	int err, i, rule_cnt;
-	u64 max_ring = 0;
 
 	if (!ops->get_rxnfc)
 		return -EOPNOTSUPP;
@@ -570,16 +582,14 @@ int ethtool_get_max_rxnfc_channel(struct net_device *dev, u64 *max)
 		if (err)
 			goto err_free_info;
 
-		if (rule_info.fs.ring_cookie != RX_CLS_FLOW_DISC &&
-		    rule_info.fs.ring_cookie != RX_CLS_FLOW_WAKE &&
-		    !(rule_info.flow_type & FLOW_RSS) &&
-		    !ethtool_get_flow_spec_ring_vf(rule_info.fs.ring_cookie))
-			max_ring =
-				max_t(u64, max_ring, rule_info.fs.ring_cookie);
+		err = cb(&rule_info, priv);
+		if (err < 0)
+			goto err_free_info;
+		if (err > 0)
+			break;
 	}
 
 	kvfree(info);
-	*max = max_ring;
 	return 0;
 
 err_free_info:
@@ -587,6 +597,32 @@ int ethtool_get_max_rxnfc_channel(struct net_device *dev, u64 *max)
 	return err;
 }
 
+static int __ethtool_get_max_rxnfc_channel(struct ethtool_rxnfc *rule_info,
+					   void *priv)
+{
+	u64 *max_ring = priv;
+
+	if (rule_info->fs.ring_cookie != RX_CLS_FLOW_DISC &&
+	    rule_info->fs.ring_cookie != RX_CLS_FLOW_WAKE &&
+	    !(rule_info->flow_type & FLOW_RSS) &&
+	    !ethtool_get_flow_spec_ring_vf(rule_info->fs.ring_cookie))
+		*max_ring =
+			max_t(u64, *max_ring, rule_info->fs.ring_cookie);
+
+	return 0;
+}
+
+int ethtool_get_max_rxnfc_channel(struct net_device *dev, u64 *max)
+{
+	u64 max_ring = 0;
+	int ret;
+
+	ret = __ethtool_for_each_rxnfc(dev, __ethtool_get_max_rxnfc_channel,
+				       &max_ring);
+	*max = max_ring;
+	return ret;
+}
+
 int ethtool_get_max_rxfh_channel(struct net_device *dev, u32 *max)
 {
 	u32 dev_size, current_max = 0;
-- 
2.34.1


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

[PATCH net-next 2/5] net: ethtool: Add validation for WAKE_FILTER

[Florian Fainelli]

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

A driver implementing WAKE_FILTER must first install at least one rule
with RX_CLS_FLOW_WAKE for WAKE_FILTER to be effective. Iterate over
RXNFC rules to validate that condition while trying to enable
WAKE_FILTER.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 net/ethtool/common.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 net/ethtool/common.h |  3 +++
 net/ethtool/ioctl.c  |  3 +++
 net/ethtool/wol.c    |  3 +++
 4 files changed, 51 insertions(+)

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 143dae872fb2..bab901b35731 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -742,3 +742,45 @@ ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps, u32 size)
 	}
 }
 EXPORT_SYMBOL_GPL(ethtool_forced_speed_maps_init);
+
+static int __ethtool_check_rxnfc_wake_filter(struct ethtool_rxnfc *rule_info,
+					     void *priv)
+{
+	bool *verdict = priv;
+
+	if (rule_info->fs.ring_cookie == RX_CLS_FLOW_WAKE) {
+		*verdict = true;
+		return 1;
+	}
+
+	return 0;
+}
+
+/**
+ * ethtool_dev_check_wake_filter: Tests if a network device can use the
+ * WAKE_FILTER Wake-on-LAN option.
+ * @dev: network device to test
+ * @wol: %ethtool_wolinfo structure with Wake-on-LAN configuration
+ *
+ * Returns true if there is no support for %WAKE_FILTER, no support
+ * for RXNFC ethtool operations, or if there is at least one WAKE_FILTER
+ * installed.
+ */
+bool ethtool_dev_check_wake_filter(struct net_device *dev,
+				   const struct ethtool_wolinfo *wol)
+{
+	bool verdict = false;
+	int ret;
+
+	if (!(wol->wolopts & WAKE_FILTER))
+		return true;
+
+	if (!dev->ethtool_ops->get_rxnfc ||
+	    !dev->ethtool_ops->set_rxnfc)
+		return true;
+
+	ret = __ethtool_for_each_rxnfc(dev, __ethtool_check_rxnfc_wake_filter,
+				       &verdict);
+
+	return ret < 0 ? false : verdict;
+}
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 28b8aaaf9bcb..6cd3286d5038 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -56,4 +56,7 @@ int ethtool_get_module_eeprom_call(struct net_device *dev,
 
 bool __ethtool_dev_mm_supported(struct net_device *dev);
 
+bool ethtool_dev_check_wake_filter(struct net_device *dev,
+				   const struct ethtool_wolinfo *wol);
+
 #endif /* _ETHTOOL_COMMON_H */
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0b0ce4f81c01..954446185158 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1457,6 +1457,9 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
 	    !memcmp(wol.sopass, cur_wol.sopass, sizeof(wol.sopass)))
 		return 0;
 
+	if (!ethtool_dev_check_wake_filter(dev, &wol))
+		return -EOPNOTSUPP;
+
 	ret = dev->ethtool_ops->set_wol(dev, &wol);
 	if (ret)
 		return ret;
diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
index 0ed56c9ac1bc..65fbe743a070 100644
--- a/net/ethtool/wol.c
+++ b/net/ethtool/wol.c
@@ -132,6 +132,9 @@ ethnl_set_wol(struct ethnl_req_info *req_info, struct genl_info *info)
 				    tb[ETHTOOL_A_WOL_SOPASS], &mod);
 	}
 
+	if (!ethtool_dev_check_wake_filter(dev, &wol))
+		return -EOPNOTSUPP;
+
 	if (!mod)
 		return 0;
 	ret = dev->ethtool_ops->set_wol(dev, &wol);
-- 
2.34.1


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

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

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2557 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 a5fa077650e8..e2f2cc38ff31 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1740,3 +1740,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 3cc52826f18e..03e7c6352aef 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1077,6 +1077,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 */
@@ -1989,6 +1993,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);
 
 int __phy_hwtstamp_get(struct phy_device *phydev,
 		       struct kernel_hwtstamp_config *config);
-- 
2.34.1


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

[PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 8124 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.251 (e.g.: multicast DNS), 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 | 195 +++++++++++++++++++++++++++++++++-
 drivers/net/phy/bcm-phy-lib.h |   5 +
 drivers/net/phy/broadcom.c    |   2 +
 3 files changed, 201 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 876f28fd8256..cfbeedc5ee81 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -827,7 +827,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)
 {
@@ -881,6 +882,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.
@@ -935,6 +942,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;
@@ -999,6 +1007,16 @@ void bcm_phy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 	if (!(ctl & BCM54XX_WOL_EN))
 		return;
 
+	ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
+	if (ret < 0)
+		return;
+
+	/* Mutualy exclusive with other modes */
+	if (ret) {
+		wol->wolopts |= WAKE_FILTER;
+		return;
+	}
+
 	for (i = 0; i < sizeof(da) / 2; i++) {
 		ret = bcm_phy_read_exp(phydev,
 				       BCM54XX_WOL_MPD_DATA2(2 - i));
@@ -1066,6 +1084,181 @@ int bcm_phy_led_brightness_set(struct phy_device *phydev,
 }
 EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set);
 
+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);
+
+	ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_INNER_PROTO);
+	if (ret < 0)
+		return ret;
+
+	nfc->fs.h_u.ether_spec.h_proto = be16_to_cpu(ret);
+
+	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;
+	__be16 h_proto;
+	const u8 *da;
+
+	/* We support only matching on the MAC DA with a custom mask and
+	 * optionally with a specific Ethernet type, reject anything else.
+	 */
+	if (nfc->fs.ring_cookie != RX_CLS_FLOW_WAKE ||
+	    (nfc->fs.location != 0 &&
+	     nfc->fs.location != RX_CLS_LOC_ANY &&
+	     nfc->fs.location != RX_CLS_LOC_FIRST) ||
+	    nfc->fs.flow_type != ETHER_FLOW ||
+	    !is_zero_ether_addr(nfc->fs.h_u.ether_spec.h_source) ||
+	    !is_zero_ether_addr(nfc->fs.m_u.ether_spec.h_source))
+		return ret;
+
+	ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
+	if (ret < 0)
+		return ret;
+
+	if (ret)
+		return -EBUSY;
+
+	if (nfc->fs.location == RX_CLS_LOC_ANY ||
+	    nfc->fs.location == RX_CLS_LOC_FIRST)
+		nfc->fs.location = 0;
+
+	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;
+	}
+
+	/* Restore default inner protocol field unless overridden by the flow
+	 * specification.
+	 */
+	h_proto = be16_to_cpu(nfc->fs.h_u.ether_spec.h_proto);
+	if (!h_proto)
+		h_proto = ETH_P_8021Q;
+
+	ret = bcm_phy_write_exp(phydev, BCM54XX_WOL_INNER_PROTO,
+				h_proto);
+	if (ret)
+		return ret;
+
+	/* Use BCM54XX_WOL_SEC_KEY_8B as a scratch register to record
+	 * that we installed a filter rule.
+	 */
+	return bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 1);
+}
+
+int bcm_phy_get_rxnfc(struct phy_device *phydev,
+		      struct ethtool_rxnfc *cmd, u32 *rule_locs)
+{
+	int err = 0, rule_cnt = 0;
+
+	err = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
+	if (err < 0)
+		return err;
+
+	rule_cnt = err;
+	err = 0;
+
+	switch (cmd->cmd) {
+	case ETHTOOL_GRXCLSRLCNT:
+		cmd->rule_cnt = rule_cnt;
+		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:
+		if (rule_cnt)
+			rule_locs[0] = 0;
+		cmd->rule_cnt = rule_cnt;
+		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)
+{
+	int err = 0;
+
+	switch (cmd->cmd) {
+	case ETHTOOL_SRXCLSRLINS:
+		err = bcm_phy_set_rule(phydev, cmd);
+		break;
+	case ETHTOOL_SRXCLSRLDEL:
+		if (cmd->fs.location != 0)
+			return err;
+
+		err = bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 0);
+		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 b52189e45a84..7081edcec06b 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -121,4 +121,9 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
 int bcm_phy_led_brightness_set(struct phy_device *phydev,
 			       u8 index, enum led_brightness value);
 
+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);
+
 #endif /* _LINUX_BCM_PHY_LIB_H */
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 3a627105675a..6c2212bd2779 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -1107,6 +1107,8 @@ static struct phy_driver broadcom_drivers[] = {
 	.get_wol	= bcm54xx_phy_get_wol,
 	.set_wol	= bcm54xx_phy_set_wol,
 	.led_brightness_set	= bcm_phy_led_brightness_set,
+	.get_rxnfc	= bcm_phy_get_rxnfc,
+	.set_rxnfc	= bcm_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 --]

Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER

[Vincent MAILHOL]

Hi Florian,

On Thu. 26 Oct. 2023 at 02:32, Florian Fainelli
<florian.fainelli@broadcom.com> 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. For instance, in
> order to wake-up from the Ethernet MAC address corresponding to the IPv4
> multicast IP address of 224.0.0.251 (e.g.: multicast DNS), 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

Nit: indent the commands and their output with two spaces.

> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/phy/bcm-phy-lib.c | 195 +++++++++++++++++++++++++++++++++-
>  drivers/net/phy/bcm-phy-lib.h |   5 +
>  drivers/net/phy/broadcom.c    |   2 +
>  3 files changed, 201 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> index 876f28fd8256..cfbeedc5ee81 100644
> --- a/drivers/net/phy/bcm-phy-lib.c
> +++ b/drivers/net/phy/bcm-phy-lib.c
> @@ -827,7 +827,8 @@ EXPORT_SYMBOL_GPL(bcm_phy_cable_test_get_status_rdb);
>                                          WAKE_MCAST | \
>                                          WAKE_BCAST | \
>                                          WAKE_MAGIC | \
> -                                        WAKE_MAGICSECURE)
> +                                        WAKE_MAGICSECURE | \
> +                                        WAKE_FILTER)

Nit: you may want to have the closing bracket on a newline to have a
cleaner diff for new future additions.

>  int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>  {
> @@ -881,6 +882,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.
> @@ -935,6 +942,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;
> @@ -999,6 +1007,16 @@ void bcm_phy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>         if (!(ctl & BCM54XX_WOL_EN))
>                 return;
>
> +       ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
> +       if (ret < 0)
> +               return;
> +
> +       /* Mutualy exclusive with other modes */
> +       if (ret) {
> +               wol->wolopts |= WAKE_FILTER;
> +               return;
> +       }
> +
>         for (i = 0; i < sizeof(da) / 2; i++) {
>                 ret = bcm_phy_read_exp(phydev,
>                                        BCM54XX_WOL_MPD_DATA2(2 - i));
> @@ -1066,6 +1084,181 @@ int bcm_phy_led_brightness_set(struct phy_device *phydev,
>  }
>  EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set);
>
> +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;

This looks like an endianness conversion (I can not tell if this is
big to little or the opposite)...

> +       }
> +       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);
> +
> +       ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_INNER_PROTO);
> +       if (ret < 0)
> +               return ret;
> +
> +       nfc->fs.h_u.ether_spec.h_proto = be16_to_cpu(ret);

... but here it is big endian to cpu endian? It does not look coherent.

Also, did you run parse to check your endianness conversions?

  https://www.kernel.org/doc/html/latest/dev-tools/sparse.html

For example, I would have expected htons() (a.k.a. cpu_to_be16())
instead of be16_to_cpu().

> +       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;
> +       __be16 h_proto;
> +       const u8 *da;
> +
> +       /* We support only matching on the MAC DA with a custom mask and
> +        * optionally with a specific Ethernet type, reject anything else.
> +        */
> +       if (nfc->fs.ring_cookie != RX_CLS_FLOW_WAKE ||
> +           (nfc->fs.location != 0 &&
> +            nfc->fs.location != RX_CLS_LOC_ANY &&
> +            nfc->fs.location != RX_CLS_LOC_FIRST) ||
> +           nfc->fs.flow_type != ETHER_FLOW ||
> +           !is_zero_ether_addr(nfc->fs.h_u.ether_spec.h_source) ||
> +           !is_zero_ether_addr(nfc->fs.m_u.ether_spec.h_source))
> +               return ret;
> +
> +       ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (ret)
> +               return -EBUSY;
> +
> +       if (nfc->fs.location == RX_CLS_LOC_ANY ||
> +           nfc->fs.location == RX_CLS_LOC_FIRST)
> +               nfc->fs.location = 0;
> +
> +       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;
> +       }
> +
> +       /* Restore default inner protocol field unless overridden by the flow
> +        * specification.
> +        */
> +       h_proto = be16_to_cpu(nfc->fs.h_u.ether_spec.h_proto);
> +       if (!h_proto)
> +               h_proto = ETH_P_8021Q;
> +
> +       ret = bcm_phy_write_exp(phydev, BCM54XX_WOL_INNER_PROTO,
> +                               h_proto);
> +       if (ret)
> +               return ret;
> +
> +       /* Use BCM54XX_WOL_SEC_KEY_8B as a scratch register to record
> +        * that we installed a filter rule.
> +        */
> +       return bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 1);
> +}
> +
> +int bcm_phy_get_rxnfc(struct phy_device *phydev,
> +                     struct ethtool_rxnfc *cmd, u32 *rule_locs)
> +{
> +       int err = 0, rule_cnt = 0;
> +
> +       err = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
> +       if (err < 0)
> +               return err;
> +
> +       rule_cnt = err;
> +       err = 0;
> +
> +       switch (cmd->cmd) {
> +       case ETHTOOL_GRXCLSRLCNT:
> +               cmd->rule_cnt = rule_cnt;
> +               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:
> +               if (rule_cnt)
> +                       rule_locs[0] = 0;
> +               cmd->rule_cnt = rule_cnt;
> +               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)
> +{
> +       int err = 0;
> +
> +       switch (cmd->cmd) {
> +       case ETHTOOL_SRXCLSRLINS:
> +               err = bcm_phy_set_rule(phydev, cmd);
> +               break;
> +       case ETHTOOL_SRXCLSRLDEL:
> +               if (cmd->fs.location != 0)
> +                       return err;
> +
> +               err = bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 0);
> +               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 b52189e45a84..7081edcec06b 100644
> --- a/drivers/net/phy/bcm-phy-lib.h
> +++ b/drivers/net/phy/bcm-phy-lib.h
> @@ -121,4 +121,9 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
>  int bcm_phy_led_brightness_set(struct phy_device *phydev,
>                                u8 index, enum led_brightness value);
>
> +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);
> +
>  #endif /* _LINUX_BCM_PHY_LIB_H */
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 3a627105675a..6c2212bd2779 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -1107,6 +1107,8 @@ static struct phy_driver broadcom_drivers[] = {
>         .get_wol        = bcm54xx_phy_get_wol,
>         .set_wol        = bcm54xx_phy_set_wol,
>         .led_brightness_set     = bcm_phy_led_brightness_set,
> +       .get_rxnfc      = bcm_phy_get_rxnfc,
> +       .set_rxnfc      = bcm_phy_set_rxnfc,
>  }, {
>         .phy_id         = PHY_ID_BCM5461,
>         .phy_id_mask    = 0xfffffff0,
> --
> 2.34.1
>

Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER

[Vincent MAILHOL]

On Thu. 26 Oct. 2023 at 10:10, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> Hi Florian,
>
> On Thu. 26 Oct. 2023 at 02:32, Florian Fainelli
> <florian.fainelli@broadcom.com> 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. For instance, in
> > order to wake-up from the Ethernet MAC address corresponding to the IPv4
> > multicast IP address of 224.0.0.251 (e.g.: multicast DNS), 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
>
> Nit: indent the commands and their output with two spaces.
>
> > Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > ---
> >  drivers/net/phy/bcm-phy-lib.c | 195 +++++++++++++++++++++++++++++++++-
> >  drivers/net/phy/bcm-phy-lib.h |   5 +
> >  drivers/net/phy/broadcom.c    |   2 +
> >  3 files changed, 201 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> > index 876f28fd8256..cfbeedc5ee81 100644
> > --- a/drivers/net/phy/bcm-phy-lib.c
> > +++ b/drivers/net/phy/bcm-phy-lib.c
> > @@ -827,7 +827,8 @@ EXPORT_SYMBOL_GPL(bcm_phy_cable_test_get_status_rdb);
> >                                          WAKE_MCAST | \
> >                                          WAKE_BCAST | \
> >                                          WAKE_MAGIC | \
> > -                                        WAKE_MAGICSECURE)
> > +                                        WAKE_MAGICSECURE | \
> > +                                        WAKE_FILTER)
>
> Nit: you may want to have the closing bracket on a newline to have a
> cleaner diff for new future additions.
>
> >  int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> >  {
> > @@ -881,6 +882,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.
> > @@ -935,6 +942,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;
> > @@ -999,6 +1007,16 @@ void bcm_phy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> >         if (!(ctl & BCM54XX_WOL_EN))
> >                 return;
> >
> > +       ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
> > +       if (ret < 0)
> > +               return;
> > +
> > +       /* Mutualy exclusive with other modes */
> > +       if (ret) {
> > +               wol->wolopts |= WAKE_FILTER;
> > +               return;
> > +       }
> > +
> >         for (i = 0; i < sizeof(da) / 2; i++) {
> >                 ret = bcm_phy_read_exp(phydev,
> >                                        BCM54XX_WOL_MPD_DATA2(2 - i));
> > @@ -1066,6 +1084,181 @@ int bcm_phy_led_brightness_set(struct phy_device *phydev,
> >  }
> >  EXPORT_SYMBOL_GPL(bcm_phy_led_brightness_set);
> >
> > +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;
>
> This looks like an endianness conversion (I can not tell if this is
> big to little or the opposite)...

Oopsy! On second look, this is an open coded cpu to big endian
conversion. So the question I should have asked is:

  why not use the put_unaligned_be16() helper here?

Below comments still remain.

> > +       }
> > +       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);
> > +
> > +       ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_INNER_PROTO);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       nfc->fs.h_u.ether_spec.h_proto = be16_to_cpu(ret);
>
> ... but here it is big endian to cpu endian? It does not look coherent.
>
> Also, did you run parse to check your endianness conversions?
>
>   https://www.kernel.org/doc/html/latest/dev-tools/sparse.html
>
> For example, I would have expected htons() (a.k.a. cpu_to_be16())
> instead of be16_to_cpu().
>
> > +       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;
> > +       __be16 h_proto;
> > +       const u8 *da;
> > +
> > +       /* We support only matching on the MAC DA with a custom mask and
> > +        * optionally with a specific Ethernet type, reject anything else.
> > +        */
> > +       if (nfc->fs.ring_cookie != RX_CLS_FLOW_WAKE ||
> > +           (nfc->fs.location != 0 &&
> > +            nfc->fs.location != RX_CLS_LOC_ANY &&
> > +            nfc->fs.location != RX_CLS_LOC_FIRST) ||
> > +           nfc->fs.flow_type != ETHER_FLOW ||
> > +           !is_zero_ether_addr(nfc->fs.h_u.ether_spec.h_source) ||
> > +           !is_zero_ether_addr(nfc->fs.m_u.ether_spec.h_source))
> > +               return ret;
> > +
> > +       ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (ret)
> > +               return -EBUSY;
> > +
> > +       if (nfc->fs.location == RX_CLS_LOC_ANY ||
> > +           nfc->fs.location == RX_CLS_LOC_FIRST)
> > +               nfc->fs.location = 0;
> > +
> > +       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;
> > +       }
> > +
> > +       /* Restore default inner protocol field unless overridden by the flow
> > +        * specification.
> > +        */
> > +       h_proto = be16_to_cpu(nfc->fs.h_u.ether_spec.h_proto);
> > +       if (!h_proto)
> > +               h_proto = ETH_P_8021Q;
> > +
> > +       ret = bcm_phy_write_exp(phydev, BCM54XX_WOL_INNER_PROTO,
> > +                               h_proto);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Use BCM54XX_WOL_SEC_KEY_8B as a scratch register to record
> > +        * that we installed a filter rule.
> > +        */
> > +       return bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 1);
> > +}
> > +
> > +int bcm_phy_get_rxnfc(struct phy_device *phydev,
> > +                     struct ethtool_rxnfc *cmd, u32 *rule_locs)
> > +{
> > +       int err = 0, rule_cnt = 0;
> > +
> > +       err = bcm_phy_read_exp(phydev, BCM54XX_WOL_SEC_KEY_8B);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       rule_cnt = err;
> > +       err = 0;
> > +
> > +       switch (cmd->cmd) {
> > +       case ETHTOOL_GRXCLSRLCNT:
> > +               cmd->rule_cnt = rule_cnt;
> > +               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:
> > +               if (rule_cnt)
> > +                       rule_locs[0] = 0;
> > +               cmd->rule_cnt = rule_cnt;
> > +               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)
> > +{
> > +       int err = 0;
> > +
> > +       switch (cmd->cmd) {
> > +       case ETHTOOL_SRXCLSRLINS:
> > +               err = bcm_phy_set_rule(phydev, cmd);
> > +               break;
> > +       case ETHTOOL_SRXCLSRLDEL:
> > +               if (cmd->fs.location != 0)
> > +                       return err;
> > +
> > +               err = bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 0);
> > +               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 b52189e45a84..7081edcec06b 100644
> > --- a/drivers/net/phy/bcm-phy-lib.h
> > +++ b/drivers/net/phy/bcm-phy-lib.h
> > @@ -121,4 +121,9 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
> >  int bcm_phy_led_brightness_set(struct phy_device *phydev,
> >                                u8 index, enum led_brightness value);
> >
> > +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);
> > +
> >  #endif /* _LINUX_BCM_PHY_LIB_H */
> > diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> > index 3a627105675a..6c2212bd2779 100644
> > --- a/drivers/net/phy/broadcom.c
> > +++ b/drivers/net/phy/broadcom.c
> > @@ -1107,6 +1107,8 @@ static struct phy_driver broadcom_drivers[] = {
> >         .get_wol        = bcm54xx_phy_get_wol,
> >         .set_wol        = bcm54xx_phy_set_wol,
> >         .led_brightness_set     = bcm_phy_led_brightness_set,
> > +       .get_rxnfc      = bcm_phy_get_rxnfc,
> > +       .set_rxnfc      = bcm_phy_set_rxnfc,
> >  }, {
> >         .phy_id         = PHY_ID_BCM5461,
> >         .phy_id_mask    = 0xfffffff0,
> > --
> > 2.34.1
> >

Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER

[Florian Fainelli]

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

Hi Vincent,

On 10/25/23 19:13, Vincent MAILHOL wrote:
[snip]
>>
>> This looks like an endianness conversion (I can not tell if this is
>> big to little or the opposite)...
> 
> Oopsy! On second look, this is an open coded cpu to big endian
> conversion. So the question I should have asked is:
> 
>    why not use the put_unaligned_be16() helper here?

Because this is consistent with the existing code, though I will keep 
that suggestion in mind as a subsequent patch. I personally find it 
clearer expressed that way, but can update.

> 
> Below comments still remain.
> 
>>> +       }
>>> +       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);
>>> +
>>> +       ret = bcm_phy_read_exp(phydev, BCM54XX_WOL_INNER_PROTO);
>>> +       if (ret < 0)
>>> +               return ret;
>>> +
>>> +       nfc->fs.h_u.ether_spec.h_proto = be16_to_cpu(ret);
>>
>> ... but here it is big endian to cpu endian? It does not look coherent.

You are right, it was not consistent.

>>
>> Also, did you run parse to check your endianness conversions?

I did, though nothing came out with C=1 or C=2, I might have to check 
that separately.

>>
>>    https://www.kernel.org/doc/html/latest/dev-tools/sparse.html
>>
>> For example, I would have expected htons() (a.k.a. cpu_to_be16())
>> instead of be16_to_cpu().

Thanks for having taken a look.
-- 
Florian


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

Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER

[Jakub Kicinski]

On Thu, 26 Oct 2023 10:55:10 -0700 Florian Fainelli wrote:
> >> Also, did you run parse to check your endianness conversions?  
> 
> I did, though nothing came out with C=1 or C=2, I might have to check 
> that separately.

FWIW

drivers/net/phy/bcm-phy-lib.c:1128:42: warning: cast to restricted __be16
drivers/net/phy/bcm-phy-lib.c:1128:40: warning: incorrect type in assignment (different base types)
drivers/net/phy/bcm-phy-lib.c:1128:40:    expected restricted __be16 [usertype] h_proto
drivers/net/phy/bcm-phy-lib.c:1128:40:    got unsigned short [usertype]
drivers/net/phy/bcm-phy-lib.c:1188:17: warning: incorrect type in assignment (different base types)
drivers/net/phy/bcm-phy-lib.c:1188:17:    expected restricted __be16 [usertype] h_proto
drivers/net/phy/bcm-phy-lib.c:1188:17:    got unsigned short [usertype]
drivers/net/phy/bcm-phy-lib.c:1190:25: warning: incorrect type in assignment (different base types)
drivers/net/phy/bcm-phy-lib.c:1190:25:    expected restricted __be16 [usertype] h_proto
drivers/net/phy/bcm-phy-lib.c:1190:25:    got int
drivers/net/phy/bcm-phy-lib.c:1193:33: warning: incorrect type in argument 3 (different base types)
drivers/net/phy/bcm-phy-lib.c:1193:33:    expected unsigned short [usertype] val
drivers/net/phy/bcm-phy-lib.c:1193:33:    got restricted __be16 [usertype] h_proto
-- 
pw-bot: cr

Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER

[Florian Fainelli]

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

On 10/26/23 14:56, Jakub Kicinski wrote:
> On Thu, 26 Oct 2023 10:55:10 -0700 Florian Fainelli wrote:
>>>> Also, did you run parse to check your endianness conversions?
>>
>> I did, though nothing came out with C=1 or C=2, I might have to check
>> that separately.
> 
> FWIW
> 
> drivers/net/phy/bcm-phy-lib.c:1128:42: warning: cast to restricted __be16
> drivers/net/phy/bcm-phy-lib.c:1128:40: warning: incorrect type in assignment (different base types)
> drivers/net/phy/bcm-phy-lib.c:1128:40:    expected restricted __be16 [usertype] h_proto
> drivers/net/phy/bcm-phy-lib.c:1128:40:    got unsigned short [usertype]
> drivers/net/phy/bcm-phy-lib.c:1188:17: warning: incorrect type in assignment (different base types)
> drivers/net/phy/bcm-phy-lib.c:1188:17:    expected restricted __be16 [usertype] h_proto
> drivers/net/phy/bcm-phy-lib.c:1188:17:    got unsigned short [usertype]
> drivers/net/phy/bcm-phy-lib.c:1190:25: warning: incorrect type in assignment (different base types)
> drivers/net/phy/bcm-phy-lib.c:1190:25:    expected restricted __be16 [usertype] h_proto
> drivers/net/phy/bcm-phy-lib.c:1190:25:    got int
> drivers/net/phy/bcm-phy-lib.c:1193:33: warning: incorrect type in argument 3 (different base types)
> drivers/net/phy/bcm-phy-lib.c:1193:33:    expected unsigned short [usertype] val
> drivers/net/phy/bcm-phy-lib.c:1193:33:    got restricted __be16 [usertype] h_proto

Yep, finally able to see that with an x86 build, not sure yet why sparse 
did not work with my aarch64 build, now fixed, thanks!
-- 
Florian


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

Re: [PATCH net-next 4/5] net: phy: broadcom: Add support for WAKE_FILTER

[Vincent MAILHOL]

On Fri. 27 Oct. 2023 at 02:55, Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
> Hi Vincent,
>
> On 10/25/23 19:13, Vincent MAILHOL wrote:
> [snip]
> >>
> >> This looks like an endianness conversion (I can not tell if this is
> >> big to little or the opposite)...
> >
> > Oopsy! On second look, this is an open coded cpu to big endian
> > conversion. So the question I should have asked is:
> >
> >    why not use the put_unaligned_be16() helper here?
>
> Because this is consistent with the existing code, though I will keep
> that suggestion in mind as a subsequent patch. I personally find it
> clearer expressed that way, but can update.

Fair enough. I agree that this is not something to be fixed in this series.

For your future consideration, I would have done it as:

        __be16 da[ETH_ALEN / sizeof(__be16)];
        /* ... */
        da[i] = cpu_to_be16(~ret);

da[] can eventually be casted back to u8 * once populated.

(...)

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

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2516 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 | 16 ++++++++++++++++
 drivers/net/phy/bcm-phy-lib.c                  | 12 +++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 9282403d1bf6..9d01c13552eb 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1524,6 +1524,14 @@ 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;
+
+		err = 0;
+	}
+
 	switch (cmd->cmd) {
 	case ETHTOOL_SRXCLSRLINS:
 		err = bcmgenet_insert_flow(dev, cmd);
@@ -1579,6 +1587,14 @@ 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;
+
+		err = 0;
+	}
+
 	switch (cmd->cmd) {
 	case ETHTOOL_GRXRINGS:
 		cmd->data = priv->hw_params->rx_queues ?: 1;
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index cfbeedc5ee81..569aeab68f97 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -888,6 +888,15 @@ int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 	if (wol->wolopts & WAKE_FILTER)
 		goto program_ctl;
 
+	/* Enabling options other than WAKE_FILTER nullifies the one and only
+	 * network rule that we support.
+	 */
+	if (!(wol->wolopts & WAKE_FILTER)) {
+		ret = bcm_phy_write_exp(phydev, BCM54XX_WOL_SEC_KEY_8B, 0);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* 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.
@@ -1175,9 +1184,10 @@ static int bcm_phy_set_rule(struct phy_device *phydev,
 
 	da = nfc->fs.m_u.ether_spec.h_dest;
 	for (i = 0; i < ETH_ALEN / 2; i++) {
+		u16 mask = da[i * 2] << 8 | da[i * 2 + 1];
 		ret = bcm_phy_write_exp(phydev,
 					BCM54XX_WOL_MASK(2 - i),
-					da[i * 2] << 8 | da[i * 2 + 1]);
+					~mask);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.34.1


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

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

[Florian Fainelli]

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

On 10/25/23 10:33, Florian Fainelli wrote:
> 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>>
> ---

[snip]

> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> index cfbeedc5ee81..569aeab68f97 100644
> --- a/drivers/net/phy/bcm-phy-lib.c
> +++ b/drivers/net/phy/bcm-phy-lib.c

This hunk belongs in the previous patch, I will send a v2 after 
collecting feedback.
-- 
Florian


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