Message ID | 20200706042758.168819-4-f.fainelli@gmail.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | net: ethtool: Untangle PHYLIB dependency | expand |
On Sun, 5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote: > + ops = ethtool_phy_ops; > + if (!ops || !ops->start_cable_test) { nit: don't think member-by-member checking is necessary. We don't expect there to be any alternative versions of the ops, right? > + ret = -EOPNOTSUPP; > + goto out_rtnl; > + } > + > ret = ethnl_ops_begin(dev); > if (ret < 0) > goto out_rtnl; > > - ret = phy_start_cable_test(dev->phydev, info->extack); > + ret = ops->start_cable_test(dev->phydev, info->extack); nit: my personal preference would be to hide checking the ops and calling the member in a static inline helper. Note that we should be able to remove this from phy.h now: #if IS_ENABLED(CONFIG_PHYLIB) int phy_start_cable_test(struct phy_device *phydev, struct netlink_ext_ack *extack); int phy_start_cable_test_tdr(struct phy_device *phydev, struct netlink_ext_ack *extack, const struct phy_tdr_config *config); #else static inline int phy_start_cable_test(struct phy_device *phydev, struct netlink_ext_ack *extack) { NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support"); return -EOPNOTSUPP; } static inline int phy_start_cable_test_tdr(struct phy_device *phydev, struct netlink_ext_ack *extack, const struct phy_tdr_config *config) { NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support"); return -EOPNOTSUPP; } #endif We could even risk a direct call: #if IS_REACHABLE(CONFIG_PHYLIB) static inline int do_x() { return __do_x(); } #else static inline int do_x() { if (!ops) return -EOPNOTSUPP; return ops->do_x(); } #endif But that's perhaps doing too much...
On 7/6/2020 11:40 AM, Jakub Kicinski wrote: > On Sun, 5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote: >> + ops = ethtool_phy_ops; >> + if (!ops || !ops->start_cable_test) { > > nit: don't think member-by-member checking is necessary. We don't > expect there to be any alternative versions of the ops, right? There could be, a network device driver not using PHYLIB could register its own operations and only implement a subset of these operations. > >> + ret = -EOPNOTSUPP; >> + goto out_rtnl; >> + } >> + >> ret = ethnl_ops_begin(dev); >> if (ret < 0) >> goto out_rtnl; >> >> - ret = phy_start_cable_test(dev->phydev, info->extack); >> + ret = ops->start_cable_test(dev->phydev, info->extack); > > nit: my personal preference would be to hide checking the ops and > calling the member in a static inline helper. > > Note that we should be able to remove this from phy.h now: I would prefer to keep thsose around in case a network device driver cannot punt entirely onto PHYLIB and instead needs to wrap those calls around. > > #if IS_ENABLED(CONFIG_PHYLIB) > int phy_start_cable_test(struct phy_device *phydev, > struct netlink_ext_ack *extack); > int phy_start_cable_test_tdr(struct phy_device *phydev, > struct netlink_ext_ack *extack, > const struct phy_tdr_config *config); > #else > static inline > int phy_start_cable_test(struct phy_device *phydev, > struct netlink_ext_ack *extack) > { > NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support"); > return -EOPNOTSUPP; > } > static inline > int phy_start_cable_test_tdr(struct phy_device *phydev, > struct netlink_ext_ack *extack, > const struct phy_tdr_config *config) > { > NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support"); > return -EOPNOTSUPP; > } > #endif > > > We could even risk a direct call: > > #if IS_REACHABLE(CONFIG_PHYLIB) > static inline int do_x() > { > return __do_x(); > } > #else > static inline int do_x() > { > if (!ops) > return -EOPNOTSUPP; > return ops->do_x(); > } > #endif > > But that's perhaps doing too much... Fine either way with me, let us see what Michal and Andrew think about that.
On Mon, 6 Jul 2020 11:45:38 -0700 Florian Fainelli wrote: > On 7/6/2020 11:40 AM, Jakub Kicinski wrote: > > On Sun, 5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote: > >> + ops = ethtool_phy_ops; > >> + if (!ops || !ops->start_cable_test) { > > > > nit: don't think member-by-member checking is necessary. We don't > > expect there to be any alternative versions of the ops, right? > > There could be, a network device driver not using PHYLIB could register > its own operations and only implement a subset of these operations. I'd strongly prefer drivers did not insert themselves into subsys-to-subsys glue :S > > We could even risk a direct call: > > > > #if IS_REACHABLE(CONFIG_PHYLIB) > > static inline int do_x() > > { > > return __do_x(); > > } > > #else > > static inline int do_x() > > { > > if (!ops) > > return -EOPNOTSUPP; > > return ops->do_x(); > > } > > #endif > > > > But that's perhaps doing too much... > > Fine either way with me, let us see what Michal and Andrew think about that. Ack, let's hear it :)
On Mon, Jul 06, 2020 at 11:40:00AM -0700, Jakub Kicinski wrote: > On Sun, 5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote: > > + ops = ethtool_phy_ops; > > + if (!ops || !ops->start_cable_test) { > > nit: don't think member-by-member checking is necessary. We don't > expect there to be any alternative versions of the ops, right? I would not like to see anything else registering an ops. So i think taking an Opps would be a good indication somebody is doing something wrong and needs fixing. > We could even risk a direct call: > > #if IS_REACHABLE(CONFIG_PHYLIB) > static inline int do_x() > { > return __do_x(); > } > #else > static inline int do_x() > { > if (!ops) > return -EOPNOTSUPP; > return ops->do_x(); > } > #endif > > But that's perhaps doing too much... I would say it is too far. Two ways of doing the same thing requires twice as much testing. And these are not hot paths where we want to eliminate as many instructions and trampolines as possible. Andrew
On Mon, Jul 06, 2020 at 09:56:03PM +0200, Andrew Lunn wrote: > On Mon, Jul 06, 2020 at 11:40:00AM -0700, Jakub Kicinski wrote: > > On Sun, 5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote: > > > + ops = ethtool_phy_ops; > > > + if (!ops || !ops->start_cable_test) { > > > > nit: don't think member-by-member checking is necessary. We don't > > expect there to be any alternative versions of the ops, right? > > I would not like to see anything else registering an ops. So i think > taking an Opps would be a good indication somebody is doing something > wrong and needs fixing. > > > We could even risk a direct call: > > > > #if IS_REACHABLE(CONFIG_PHYLIB) > > static inline int do_x() > > { > > return __do_x(); > > } > > #else > > static inline int do_x() > > { > > if (!ops) > > return -EOPNOTSUPP; > > return ops->do_x(); > > } > > #endif > > > > But that's perhaps doing too much... > > I would say it is too far. Two ways of doing the same thing requires > twice as much testing. And these are not hot paths where we want to > eliminate as many instructions and trampolines as possible. Agreed, it seems a bit over the top. Michal
diff --git a/net/Kconfig b/net/Kconfig index d1672280d6a4..3831206977a1 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -455,7 +455,6 @@ config FAILOVER config ETHTOOL_NETLINK bool "Netlink interface for ethtool" default y - depends on PHYLIB=y || PHYLIB=n help An alternative userspace interface for ethtool based on generic netlink. It provides better extensibility and some new features, diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c index 7194956aa09e..4f9fbdf7610c 100644 --- a/net/ethtool/cabletest.c +++ b/net/ethtool/cabletest.c @@ -58,6 +58,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) { struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1]; struct ethnl_req_info req_info = {}; + const struct ethtool_phy_ops *ops; struct net_device *dev; int ret; @@ -81,11 +82,17 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) } rtnl_lock(); + ops = ethtool_phy_ops; + if (!ops || !ops->start_cable_test) { + ret = -EOPNOTSUPP; + goto out_rtnl; + } + ret = ethnl_ops_begin(dev); if (ret < 0) goto out_rtnl; - ret = phy_start_cable_test(dev->phydev, info->extack); + ret = ops->start_cable_test(dev->phydev, info->extack); ethnl_ops_complete(dev); @@ -308,6 +315,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) { struct nlattr *tb[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1]; struct ethnl_req_info req_info = {}; + const struct ethtool_phy_ops *ops; struct phy_tdr_config cfg; struct net_device *dev; int ret; @@ -337,11 +345,17 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) goto out_dev_put; rtnl_lock(); + ops = ethtool_phy_ops; + if (!ops || !ops->start_cable_test_tdr) { + ret = -EOPNOTSUPP; + goto out_rtnl; + } + ret = ethnl_ops_begin(dev); if (ret < 0) goto out_rtnl; - ret = phy_start_cable_test_tdr(dev->phydev, info->extack, &cfg); + ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg); ethnl_ops_complete(dev);
Now that we have introduced ethtool_phy_ops and the PHY library dynamically registers its operations with that function pointer, we can remove the direct PHYLIB dependency in favor of using dynamic operations. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- net/Kconfig | 1 - net/ethtool/cabletest.c | 18 ++++++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-)