Re: [PATCH net-next v2 3/3] net: ethtool: Remove PHYLIB direct dependency

On 7/6/2020 11:40 AM, Jakub Kicinski wrote:

On Sun,  5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote:

quoted

+	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.

quoted

+		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.
-- 
Florian