Messages in this thread | | | From | Vladimir Oltean <> | Subject | Re: [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations. | Date | Thu, 20 Jan 2022 16:13:29 +0000 |
| |
On Mon, Jan 03, 2022 at 03:25:52PM -0800, Richard Cochran wrote: > Both the vlan and the bonding drivers call their "real" device driver > in order to report the time stamping capabilities. Provide a core > ethtool helper function to avoid copy/paste in the stack. > > Signed-off-by: Richard Cochran <richardcochran@gmail.com> > ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> drivers/net/bonding/bond_main.c | 14 ++------------ > include/linux/ethtool.h | 8 ++++++++ > net/8021q/vlan_dev.c | 15 +-------------- > net/ethtool/common.c | 6 ++++++ > 4 files changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index b60e22f6394a..f28b88b67b9e 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -5353,23 +5353,13 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev, > struct ethtool_ts_info *info) > { > struct bonding *bond = netdev_priv(bond_dev); > - const struct ethtool_ops *ops; > struct net_device *real_dev; > - struct phy_device *phydev; > > rcu_read_lock(); > real_dev = bond_option_active_slave_get_rcu(bond); > rcu_read_unlock();
Side note: I'm a bit confused about this rcu_read_lock() -> rcu_dereference_protected() -> rcu_read_unlock() pattern, and use of the real_dev outside the RCU critical section. Isn't ->get_ts_info() protected by the rtnl_mutex? Shouldn't there be a bond_option_active_slave_get() which uses rtnl_dereference()? I see the code has been recently added by Hangbin Liu.
> - if (real_dev) { > - ops = real_dev->ethtool_ops; > - phydev = real_dev->phydev; > - > - if (phy_has_tsinfo(phydev)) { > - return phy_ts_info(phydev, info); > - } else if (ops->get_ts_info) { > - return ops->get_ts_info(real_dev, info); > - } > - } > + if (real_dev) > + return ethtool_get_ts_info_by_layer(real_dev, info); > > info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | > SOF_TIMESTAMPING_SOFTWARE; > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index a26f37a27167..1d72344493bb 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -824,6 +824,14 @@ ethtool_params_from_link_mode(struct ethtool_link_ksettings *link_ksettings, > */ > int ethtool_get_phc_vclocks(struct net_device *dev, int **vclock_index); > > +/** > + * ethtool_get_ts_info_by_layer - Obtains time stamping capabilities from the MAC or PHY layer. > + * @dev: pointer to net_device structure > + * @info: buffer to hold the result > + * Returns zero on sauces, non-zero otherwise. > + */ > +int ethtool_get_ts_info_by_layer(struct net_device *dev, struct ethtool_ts_info *info); > + > /** > * ethtool_sprintf - Write formatted string to ethtool string data > * @data: Pointer to start of string to update > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > index 26d031a43cc1..c645d7c46d78 100644 > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -679,20 +679,7 @@ static int vlan_ethtool_get_ts_info(struct net_device *dev, > struct ethtool_ts_info *info) > { > const struct vlan_dev_priv *vlan = vlan_dev_priv(dev); > - const struct ethtool_ops *ops = vlan->real_dev->ethtool_ops; > - struct phy_device *phydev = vlan->real_dev->phydev; > - > - if (phy_has_tsinfo(phydev)) { > - return phy_ts_info(phydev, info); > - } else if (ops->get_ts_info) { > - return ops->get_ts_info(vlan->real_dev, info); > - } else { > - info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | > - SOF_TIMESTAMPING_SOFTWARE; > - info->phc_index = -1; > - } > - > - return 0; > + return ethtool_get_ts_info_by_layer(vlan->real_dev, info); > } > > static void vlan_dev_get_stats64(struct net_device *dev, > diff --git a/net/ethtool/common.c b/net/ethtool/common.c > index 0c5210015911..651d18eef589 100644 > --- a/net/ethtool/common.c > +++ b/net/ethtool/common.c > @@ -569,6 +569,12 @@ int ethtool_get_phc_vclocks(struct net_device *dev, int **vclock_index) > } > EXPORT_SYMBOL(ethtool_get_phc_vclocks); > > +int ethtool_get_ts_info_by_layer(struct net_device *dev, struct ethtool_ts_info *info) > +{ > + return __ethtool_get_ts_info(dev, info); > +} > +EXPORT_SYMBOL(ethtool_get_ts_info_by_layer);
I would probably replace all __ethtool_get_ts_info() function name occurrences with ethtool_get_ts_info_by_layer() (since it isn't a bad name) to make it absolutely clear that it's recursive for VLAN and bond interfaces. But maybe that's just me.
> + > const struct ethtool_phy_ops *ethtool_phy_ops; > > void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops) > -- > 2.20.1 >
| |