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

[Jakub Kicinski]

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