| Message ID | E1tKefn-006SMv-Lg@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add phylink managed EEE support | expand |
> +/** > + * mac_validate_tx_lpi() - validate the LPI parameters in EEE > + * @config: a pointer to a &struct phylink_config. > + * @e: EEE configuration to be validated. > + * > + * Validate the LPI configuration parameters in @e, returning an appropriate > + * error. This will be called prior to any changes being made, and must not > + * make any changes to existing configuration. Returns zero on success. Maybe suggest -EOPNOTSUPP? We might then get more uniform error codes from MAC drivers? Andrew
On Tue, Dec 10, 2024 at 04:21:51AM +0100, Andrew Lunn wrote: > > +/** > > + * mac_validate_tx_lpi() - validate the LPI parameters in EEE > > + * @config: a pointer to a &struct phylink_config. > > + * @e: EEE configuration to be validated. > > + * > > + * Validate the LPI configuration parameters in @e, returning an appropriate > > + * error. This will be called prior to any changes being made, and must not > > + * make any changes to existing configuration. Returns zero on success. > > Maybe suggest -EOPNOTSUPP? We might then get more uniform error codes > from MAC drivers? -EOPNOTSUPP would be appropriate if the driver doesn't support EEE at all. Other errors are more appropriate when validating the value of the parameters (e.g. tx_lpi_timer.) However, we probably need to have a discussion about the best way to handle tx_lpi_timer values that the hardware can't deal with. Should we handle it with a hrtimer? If so, we probably need mac_enable_tx_lpi() to return an error code, so that the core knows that the hardware can't cope with the value at a particular speed, and needs to switch to a software timer (I'm not currently sure how complex that would be to implement, but I think stmmac takes this approach.) That could make mac_validate_tx_lpi() redundant, but then I have a stack of DSA patches that could use this callback to indicate that EEE is not supported. I don't have all the answers for this, so I was hoping to kick off a discussion in the RFC patchset before submitting something that could be merged, but that didn't happen.
On Tue, Dec 10, 2024 at 09:58:40AM +0000, Russell King (Oracle) wrote: > That could make mac_validate_tx_lpi() redundant, but then I have a > stack of DSA patches that could use this callback to indicate that > EEE is not supported. Scratch that idea - DSA also needs the ethtool get_eee() method to return -EOPNOTSUPP, which mac_validate_tx_lpi() wouldn't cover.
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 750356b6a2e9..995bfbbd18e9 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1570,6 +1570,14 @@ static const char *phylink_pause_to_str(int pause) } } +static int phylink_validate_tx_lpi(struct phylink *pl, struct ethtool_keee *eee) +{ + if (!pl->mac_ops->mac_validate_tx_lpi) + return 0; + + return pl->mac_ops->mac_validate_tx_lpi(pl->config, eee); +} + static void phylink_deactivate_lpi(struct phylink *pl) { if (pl->mac_enable_tx_lpi) { @@ -3170,7 +3178,7 @@ EXPORT_SYMBOL_GPL(phylink_ethtool_get_eee); int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_keee *eee) { bool mac_eee = pl->mac_supports_eee; - int ret = -EOPNOTSUPP; + int ret; ASSERT_RTNL(); @@ -3187,6 +3195,12 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_keee *eee) eee->tx_lpi_timer); } + ret = phylink_validate_tx_lpi(pl, eee); + if (ret) + return ret; + + ret = -EOPNOTSUPP; + if (pl->phydev) { ret = phy_ethtool_set_eee(pl->phydev, eee); if (ret == 0) diff --git a/include/linux/phylink.h b/include/linux/phylink.h index f72f0a1feb89..03e790579203 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -187,6 +187,7 @@ void phylink_limit_mac_speed(struct phylink_config *config, u32 max_speed); * @mac_finish: finish a major reconfiguration of the interface. * @mac_link_down: take the link down. * @mac_link_up: allow the link to come up. + * @mac_validate_tx_lpi: validate LPI configuration. * @mac_disable_tx_lpi: disable LPI. * @mac_enable_tx_lpi: enable and configure LPI. * @@ -209,6 +210,8 @@ struct phylink_mac_ops { struct phy_device *phy, unsigned int mode, phy_interface_t interface, int speed, int duplex, bool tx_pause, bool rx_pause); + int (*mac_validate_tx_lpi)(struct phylink_config *config, + struct ethtool_keee *e); void (*mac_disable_tx_lpi)(struct phylink_config *config); void (*mac_enable_tx_lpi)(struct phylink_config *config, u32 timer, bool tx_clk_stop); @@ -407,6 +410,18 @@ void mac_link_up(struct phylink_config *config, struct phy_device *phy, unsigned int mode, phy_interface_t interface, int speed, int duplex, bool tx_pause, bool rx_pause); +/** + * mac_validate_tx_lpi() - validate the LPI parameters in EEE + * @config: a pointer to a &struct phylink_config. + * @e: EEE configuration to be validated. + * + * Validate the LPI configuration parameters in @e, returning an appropriate + * error. This will be called prior to any changes being made, and must not + * make any changes to existing configuration. Returns zero on success. + */ +int (*mac_validate_tx_lpi)(struct phylink_config *config, + struct ethtool_keee *e); + /** * mac_disable_tx_lpi() - disable LPI generation at the MAC * @config: a pointer to a &struct phylink_config.
Allow the MAC driver to validate EEE parameters before accepting the set_eee() call. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phylink.c | 16 +++++++++++++++- include/linux/phylink.h | 15 +++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-)