[PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations.

[PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations.

[Richard Cochran]

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>
---
 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();
-	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);
+
 const struct ethtool_phy_ops *ethtool_phy_ops;
 
 void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops)
-- 
2.20.1

Re: [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations.

[Vladimir Oltean]

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
>

Re: [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations.

[Hangbin Liu]

On Thu, Jan 20, 2022 at 04:13:29PM +0000, Vladimir Oltean wrote:
> > 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.

Hi Vladimir,

Yes, it should be enough to use rtnl_dereference() since ->get_ts_info is
protected by the rtnl_lock. I just thought there is an existing get active
slave function and rcu read should be OK to be used here. So I just used the
existing one.

Hi Jay,

Do you think if there is a need to add a rtnl version of
bond_option_active_slave_get()?

Thanks
Hangbin

Re: [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations.

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Thu, Jan 20, 2022 at 04:13:29PM +0000, Vladimir Oltean wrote:
>> > 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.
>
>Hi Vladimir,
>
>Yes, it should be enough to use rtnl_dereference() since ->get_ts_info is
>protected by the rtnl_lock. I just thought there is an existing get active
>slave function and rcu read should be OK to be used here. So I just used the
>existing one.
>
>Hi Jay,
>
>Do you think if there is a need to add a rtnl version of
>bond_option_active_slave_get()?

	I think the RCU primitives used should match the calling
context, so, yes, there should be an "rtnl" flavor in this call path as
that's really what's being relied upon.

	You could add a "...active_slave_get_rtnl" variant, or there's
rcu_deference_rtnl() that goes both ways (rcu_read_lock or RTNL).  That
could be used in bond_option_active_slave_get_rcu() in place of the
current rcu_dereference().  I don't have a strong preference one way or
the other.

	Either way, as mentioned, I agree that the rcu_read_lock/unlock
in bond_ethtool_get_ts_info() is superfluous, since the whole call is
under RTNL.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com