| Message ID | 20231026224509.112353-4-florian.fainelli@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | WAKE_FILTER for Broadcom PHY (v2) | expand |
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);
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).
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?
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 --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);
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(+)