diff mbox series

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

Message ID 20231026224509.112353-4-florian.fainelli@broadcom.com
State New
Headers
Series WAKE_FILTER for Broadcom PHY (v2) |

Commit Message

Florian Fainelli Oct. 26, 2023, 10:45 p.m. UTC 
  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(+)

Comments

Jacob Keller Oct. 26, 2023, 11:20 p.m. UTC | #1 
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);
Florian Fainelli Oct. 26, 2023, 11:32 p.m. UTC | #2 
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).
Jakub Kicinski Oct. 27, 2023, 2:53 a.m. UTC | #3 
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?
Jacob Keller Oct. 27, 2023, 4:57 p.m. UTC | #4 
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
diff mbox series

Patch

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