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

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

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2110 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)

Changes in v2:

- corrected sparse annotationes for h_proto (Vincent)
- added 2 spaces for commands in commit messages (Vincent)
- added hunk in patch 5 back into patch 4 (self)

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

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

[Jacob Keller]

On 10/26/2023 3:45 PM, Florian Fainelli wrote:
> 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>

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

[PATCH net-next v2 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 --]

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

[Jacob Keller]

On 10/26/2023 3:45 PM, Florian Fainelli wrote:
> 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.
> 

Makes sense to enforce this.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

[PATCH net-next v2 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 --]

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

[Jacob Keller]

On 10/26/2023 3:45 PM, Florian Fainelli wrote:
> +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 */

Any particular reason this comment is required? I don't see it enforced
above so I'm curious.

> +	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);

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

[Florian Fainelli]

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

On 10/26/23 16:20, Jacob Keller wrote:
> 
> 
> On 10/26/2023 3:45 PM, Florian Fainelli wrote:
>> +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 */
> 
> Any particular reason this comment is required? I don't see it enforced
> above so I'm curious.

The comment is not required, though I put it in there to help readers 
understand that this is purely for use by Wake-on-LAN, since unlike a 
MACs, PHYs are not capable of redirecting certain flows to certain 
queues, too low in the packet processing that there is not a notion of a 
queue at that point (FIFO yes, but not queue as in what the networking 
stack wants to use).
-- 
Florian


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

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

[Jacob Keller]

On 10/26/2023 4:32 PM, Florian Fainelli wrote:
> On 10/26/23 16:20, Jacob Keller wrote:
>>
>>
>> On 10/26/2023 3:45 PM, Florian Fainelli wrote:
>>> +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 */
>>
>> Any particular reason this comment is required? I don't see it enforced
>> above so I'm curious.
> 
> The comment is not required, though I put it in there to help readers 
> understand that this is purely for use by Wake-on-LAN, since unlike a 
> MACs, PHYs are not capable of redirecting certain flows to certain 
> queues, too low in the packet processing that there is not a notion of a 
> queue at that point (FIFO yes, but not queue as in what the networking 
> stack wants to use).

Ah, right that makes sense. Thanks for the clarification.

Perhaps we could enforce that in the interface so that if something does
try to pass a different kind of filter to the phy_rxnfc it would fail or
complain somehow? Because that seems like it would be a programming mistake.

I guess with the current implementation you effectively rely on
returning -EOPNOTSUP for non-wake filters since the driver forwards the
filter to the PHY to check whether it can support it.

Thanks,
Jake

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

[Jakub Kicinski]

On Thu, 26 Oct 2023 15:45:07 -0700 Florian Fainelli wrote:
> 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.

Should we explicitly check for WAKE? WAKE, and DISC are probably the
only values that make sense for PHY nfc?

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

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 8419 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 | 205 +++++++++++++++++++++++++++++++++-
 drivers/net/phy/bcm-phy-lib.h |   5 +
 drivers/net/phy/broadcom.c    |   2 +
 3 files changed, 211 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 876f28fd8256..5dfc1a5fde2e 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,21 @@ 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;
+
+	/* 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.
@@ -935,6 +951,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 +1016,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 +1093,182 @@ 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 = cpu_to_be16(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;
+	const u8 *da;
+	u16 h_proto;
+
+	/* 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++) {
+		u16 mask = da[i * 2] << 8 | da[i * 2 + 1];
+		ret = bcm_phy_write_exp(phydev,
+					BCM54XX_WOL_MASK(2 - i),
+					~mask);
+		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 < 0)
+		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 v2 4/5] net: phy: broadcom: Add support for WAKE_FILTER

[Jacob Keller]

On 10/26/2023 3:45 PM, 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. 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>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

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

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1326 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 ++++++++++++++++
 1 file changed, 16 insertions(+)

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


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

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

[Jacob Keller]

On 10/26/2023 3:45 PM, Florian Fainelli wrote:
> Determine whether the PHY can support waking up from the user programmed
> network filter, and if it can utilize it.
> 

Here, you're passing through to phy_ethtool_set_rxnfc, basically
allowing the lower device to program the wakeup filter if its supported. Ok.

This almost feels like it would belong generally in the higher level
ethtool code rather than in the driver?

Thanks,
Jake

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

[Florian Fainelli]

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

On 10/26/23 16:23, Jacob Keller wrote:
> 
> 
> On 10/26/2023 3:45 PM, Florian Fainelli wrote:
>> Determine whether the PHY can support waking up from the user programmed
>> network filter, and if it can utilize it.
>>
> 
> Here, you're passing through to phy_ethtool_set_rxnfc, basically
> allowing the lower device to program the wakeup filter if its supported. Ok.
> 
> This almost feels like it would belong generally in the higher level
> ethtool code rather than in the driver?

Agreed, as Doug just pointed out to me, there is still an open question 
about reconciling the PHY and the MAC RXNFC spaces into a single 
ethtool_rxnfc structure.

An ideal goal is to have zero modifications to neither the MAC or the 
PHY drivers such that they can both work in their own spaces as if they 
were alone, or combined.

I suppose that if we get the number of supported rules from the MAC 
first, and then get the supported number of rules from the PHY next, we 
could do something like this:

rule index
| 0|
| .| -> MAC rules
|15|
|16| -> PHY rule

and each of the MAC or the PHY {get,set}_rxnfc() operate within a base 
rule number which is relative to their own space. So the MAC driver 
would continue to care about its (max..first) - base (0) range, and the 
PHY would care about (max..first) - base (16).

Though then the issue is discoverability, how do you know which rule 
location is backed by which hardware block. We could create an 
intermediate and inert rule at index 16 for instance that acts as a 
delimiter?

Or we could create yet another RX_CLS_LOC_* value that is "special" and 
can denote whether of the MAC or the PHY we should be targeting 
whichever is supported, but that does not usually lend itself to being 
logically ORed with the existing RX_CLS_LOC_* values. WDYT?

pw-bot: cr
-- 
Florian


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

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

[Jacob Keller]

On 10/26/2023 4:52 PM, Florian Fainelli wrote:
> On 10/26/23 16:23, Jacob Keller wrote:
>>
>>
>> On 10/26/2023 3:45 PM, Florian Fainelli wrote:
>>> Determine whether the PHY can support waking up from the user programmed
>>> network filter, and if it can utilize it.
>>>
>>
>> Here, you're passing through to phy_ethtool_set_rxnfc, basically
>> allowing the lower device to program the wakeup filter if its supported. Ok.
>>
>> This almost feels like it would belong generally in the higher level
>> ethtool code rather than in the driver?
> 
> Agreed, as Doug just pointed out to me, there is still an open question 
> about reconciling the PHY and the MAC RXNFC spaces into a single 
> ethtool_rxnfc structure.
> 
> An ideal goal is to have zero modifications to neither the MAC or the 
> PHY drivers such that they can both work in their own spaces as if they 
> were alone, or combined.
> 
> I suppose that if we get the number of supported rules from the MAC 
> first, and then get the supported number of rules from the PHY next, we 
> could do something like this:
> 
> rule index
> | 0|
> | .| -> MAC rules
> |15|
> |16| -> PHY rule
> 
> and each of the MAC or the PHY {get,set}_rxnfc() operate within a base 
> rule number which is relative to their own space. So the MAC driver 
> would continue to care about its (max..first) - base (0) range, and the 
> PHY would care about (max..first) - base (16).
> 
> Though then the issue is discoverability, how do you know which rule 
> location is backed by which hardware block. We could create an 
> intermediate and inert rule at index 16 for instance that acts as a 
> delimiter?
> 
> Or we could create yet another RX_CLS_LOC_* value that is "special" and 
> can denote whether of the MAC or the PHY we should be targeting 
> whichever is supported, but that does not usually lend itself to being 
> logically ORed with the existing RX_CLS_LOC_* values. WDYT?
> 
> pw-bot: cr

Ah, yea there is a lot of complexity to consider here.

I'm not entirely sure what we should do here. What about extending with
another attribute entirely instead of another bit in RX_CLS_LOC?

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

[Florian Fainelli]

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

On 10/27/23 09:55, Jacob Keller wrote:
> 
> 
> On 10/26/2023 4:52 PM, Florian Fainelli wrote:
>> On 10/26/23 16:23, Jacob Keller wrote:
>>>
>>>
>>> On 10/26/2023 3:45 PM, Florian Fainelli wrote:
>>>> Determine whether the PHY can support waking up from the user programmed
>>>> network filter, and if it can utilize it.
>>>>
>>>
>>> Here, you're passing through to phy_ethtool_set_rxnfc, basically
>>> allowing the lower device to program the wakeup filter if its supported. Ok.
>>>
>>> This almost feels like it would belong generally in the higher level
>>> ethtool code rather than in the driver?
>>
>> Agreed, as Doug just pointed out to me, there is still an open question
>> about reconciling the PHY and the MAC RXNFC spaces into a single
>> ethtool_rxnfc structure.
>>
>> An ideal goal is to have zero modifications to neither the MAC or the
>> PHY drivers such that they can both work in their own spaces as if they
>> were alone, or combined.
>>
>> I suppose that if we get the number of supported rules from the MAC
>> first, and then get the supported number of rules from the PHY next, we
>> could do something like this:
>>
>> rule index
>> | 0|
>> | .| -> MAC rules
>> |15|
>> |16| -> PHY rule
>>
>> and each of the MAC or the PHY {get,set}_rxnfc() operate within a base
>> rule number which is relative to their own space. So the MAC driver
>> would continue to care about its (max..first) - base (0) range, and the
>> PHY would care about (max..first) - base (16).
>>
>> Though then the issue is discoverability, how do you know which rule
>> location is backed by which hardware block. We could create an
>> intermediate and inert rule at index 16 for instance that acts as a
>> delimiter?
>>
>> Or we could create yet another RX_CLS_LOC_* value that is "special" and
>> can denote whether of the MAC or the PHY we should be targeting
>> whichever is supported, but that does not usually lend itself to being
>> logically ORed with the existing RX_CLS_LOC_* values. WDYT?
>>
>> pw-bot: cr
> 
> Ah, yea there is a lot of complexity to consider here.

Yes this is only the tip of iceberg! Here is hopefully a better 
description of our particular system where this is being requested (the 
fact there is a single one also makes me question the entire effort, but 
anyway). We have 2 distinct system sleep modes:

- akin to ACPI S2 where the Ethernet PHY and MAC remain enabled and both 
can be used for Wake-on-LAN filtering, with the MAC being more capable 
than the PHY. System power consumption is just around 500mW at the wall. 
In that case it would make sense to leverage the MAC's capability 
because it is better and would lead to fewer false wake-ups

- akin to ACPI S3 where the Ethernet PHY only remains enabled, the MAC 
is powered off (as is most of the SoC), but we have limited Wake-on-LAN 
capability in the form of network filter as we can only match on a 
custom MAC DA + mask. System power consumption is closer to 350mW at the 
wall.

My users are not really willing to use the broad WAKE_MCAST because they 
want to match specifically on mDNS over IPv4 (or IPv6), so they prefer 
to program an exact match to limit the amount of false wake-ups. 
Arguably there will already be quite a lot in home network due to 
phones, IoT devices, and whatnot.

 From an user perspective they would know which system standby state is 
being entered so one could imagine that ahead of entry, we could 
configure either the MAC, or the PHY when targeting S2, or just the PHY 
when targeting S3. This implies that we can selectively target one 
entity, or the other.

For the current time being, and knowing the use case of my users, 
directing all of the Wake-on-LAN configuration towards the PHY would be 
enough IMHO, even if that means we stop leveraging the MAC capabilities, 
hence this patch series.

> 
> I'm not entirely sure what we should do here. What about extending with
> another attribute entirely instead of another bit in RX_CLS_LOC?

Yes possibly, or we just target different objects, right now we have 
visibility into the MACs via the net_device, it seems like we ought to 
be able to target some ethtool APIs towards PHY objects, which currently 
have no netlink representation. There is on-going work to bridge that gap:

https://lore.kernel.org/netdev/ffc6ff4a-d1af-4643-a538-fd13e6be9e06@lunn.ch/T/

but I am not sure we will reach an agreement any time soon. Maybe I can 
convince my masters to wait for that to land and use WAKE_MCAST in the 
meantime.

I would not necessary want to invent a new set of ethtool commands and 
kernel APIs such that we could do the below examples, though maybe this 
is not incompatible with the work being done by Maxime:

# Target the Ethernet MAC
ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 # 
Assumes MAC by default
ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 
target mac

# Target the Ethernet PHY, if capable
ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 
target phy

# Enable WAKE_FILTER at the MAC level
ethtool -s eth0 wol f # assumes MAC by default
ethtool -s eth0 wol f target mac

# Enable WAKE_FILTER at the PHY level, if capable
ethtool -s eth0 wol f target phy

though maybe this is the much needed addition to ethtool so we can be 
more selective.

After a bunch of candies on Tuesday I might reach a state of trance and 
figure which way to proceed :D
-- 
Florian


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

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

[Jacob Keller]

On 10/27/2023 10:15 AM, Florian Fainelli wrote:
> On 10/27/23 09:55, Jacob Keller wrote:
>>
>>
>> On 10/26/2023 4:52 PM, Florian Fainelli wrote:
>>> On 10/26/23 16:23, Jacob Keller wrote:
>>>>
>>>>
>>>> On 10/26/2023 3:45 PM, Florian Fainelli wrote:
>>>>> Determine whether the PHY can support waking up from the user programmed
>>>>> network filter, and if it can utilize it.
>>>>>
>>>>
>>>> Here, you're passing through to phy_ethtool_set_rxnfc, basically
>>>> allowing the lower device to program the wakeup filter if its supported. Ok.
>>>>
>>>> This almost feels like it would belong generally in the higher level
>>>> ethtool code rather than in the driver?
>>>
>>> Agreed, as Doug just pointed out to me, there is still an open question
>>> about reconciling the PHY and the MAC RXNFC spaces into a single
>>> ethtool_rxnfc structure.
>>>
>>> An ideal goal is to have zero modifications to neither the MAC or the
>>> PHY drivers such that they can both work in their own spaces as if they
>>> were alone, or combined.
>>>
>>> I suppose that if we get the number of supported rules from the MAC
>>> first, and then get the supported number of rules from the PHY next, we
>>> could do something like this:
>>>
>>> rule index
>>> | 0|
>>> | .| -> MAC rules
>>> |15|
>>> |16| -> PHY rule
>>>
>>> and each of the MAC or the PHY {get,set}_rxnfc() operate within a base
>>> rule number which is relative to their own space. So the MAC driver
>>> would continue to care about its (max..first) - base (0) range, and the
>>> PHY would care about (max..first) - base (16).
>>>
>>> Though then the issue is discoverability, how do you know which rule
>>> location is backed by which hardware block. We could create an
>>> intermediate and inert rule at index 16 for instance that acts as a
>>> delimiter?
>>>
>>> Or we could create yet another RX_CLS_LOC_* value that is "special" and
>>> can denote whether of the MAC or the PHY we should be targeting
>>> whichever is supported, but that does not usually lend itself to being
>>> logically ORed with the existing RX_CLS_LOC_* values. WDYT?
>>>
>>> pw-bot: cr
>>
>> Ah, yea there is a lot of complexity to consider here.
> 
> Yes this is only the tip of iceberg! Here is hopefully a better 
> description of our particular system where this is being requested (the 
> fact there is a single one also makes me question the entire effort, but 
> anyway). We have 2 distinct system sleep modes:
> 
> - akin to ACPI S2 where the Ethernet PHY and MAC remain enabled and both 
> can be used for Wake-on-LAN filtering, with the MAC being more capable 
> than the PHY. System power consumption is just around 500mW at the wall. 
> In that case it would make sense to leverage the MAC's capability 
> because it is better and would lead to fewer false wake-ups
> 
> - akin to ACPI S3 where the Ethernet PHY only remains enabled, the MAC 
> is powered off (as is most of the SoC), but we have limited Wake-on-LAN 
> capability in the form of network filter as we can only match on a 
> custom MAC DA + mask. System power consumption is closer to 350mW at the 
> wall.
> 
> My users are not really willing to use the broad WAKE_MCAST because they 
> want to match specifically on mDNS over IPv4 (or IPv6), so they prefer 
> to program an exact match to limit the amount of false wake-ups. 
> Arguably there will already be quite a lot in home network due to 
> phones, IoT devices, and whatnot.
> 
>  From an user perspective they would know which system standby state is 
> being entered so one could imagine that ahead of entry, we could 
> configure either the MAC, or the PHY when targeting S2, or just the PHY 
> when targeting S3. This implies that we can selectively target one 
> entity, or the other.
> 
> For the current time being, and knowing the use case of my users, 
> directing all of the Wake-on-LAN configuration towards the PHY would be 
> enough IMHO, even if that means we stop leveraging the MAC capabilities, 
> hence this patch series.
> 

Right.

>>
>> I'm not entirely sure what we should do here. What about extending with
>> another attribute entirely instead of another bit in RX_CLS_LOC?
> 
> Yes possibly, or we just target different objects, right now we have 
> visibility into the MACs via the net_device, it seems like we ought to 
> be able to target some ethtool APIs towards PHY objects, which currently 
> have no netlink representation. There is on-going work to bridge that gap:
> 
> https://lore.kernel.org/netdev/ffc6ff4a-d1af-4643-a538-fd13e6be9e06@lunn.ch/T/
> 
> but I am not sure we will reach an agreement any time soon. Maybe I can 
> convince my masters to wait for that to land and use WAKE_MCAST in the 
> meantime.
> 

Sure, but this obviously costs a potentially significant amount of extra
power, and it would be better to avoid that.

> I would not necessary want to invent a new set of ethtool commands and 
> kernel APIs such that we could do the below examples, though maybe this 
> is not incompatible with the work being done by Maxime:
> 
> # Target the Ethernet MAC
> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 # 
> Assumes MAC by default
> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 
> target mac
> 
> # Target the Ethernet PHY, if capable
> ethtool -N eth0 flow-type ether dst 01:00:5e:00:00:fb loc 0 action -2 
> target phy
> 
> # Enable WAKE_FILTER at the MAC level
> ethtool -s eth0 wol f # assumes MAC by default
> ethtool -s eth0 wol f target mac
> 
> # Enable WAKE_FILTER at the PHY level, if capable
> ethtool -s eth0 wol f target phy
> 
> though maybe this is the much needed addition to ethtool so we can be 
> more selective.
> 
> After a bunch of candies on Tuesday I might reach a state of trance and 
> figure which way to proceed :D

It does seem like an acceptable compromise here, and perhaps being
driver specific is ok, since this does depend a lot on the individual
device support, thus broadly applying this across all drivers could be
problematic.

I like the idea of being able to more precisely target the rules so that
its clear to userspace what is being done... but I also understand the
challenge of wanting to deliver what feels like a small win and being
asked to do something much larger.

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

[Andrew Lunn]

> It does seem like an acceptable compromise here, and perhaps being
> driver specific is ok, since this does depend a lot on the individual
> device support, thus broadly applying this across all drivers could be
> problematic.

The Marvell PHYs have a similar capability. Its actually more feature
rich. It allows upto 8 matches, each being of up to 128 bytes, and you
can enable/disable each byte within the 128 bytes. This would in fact
be better for Florian's use case, since it could match deeper into the
frame and reduce the false positive. But its a Marvell device...

My real point is, other hardware does have similar capabilities. Its
unclear if anybody else will ever actually need it, but we should try
to avoid a one device solution.

   Andrew