Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 5 Apr 2023 21:01:21 +0300 Vladimir Oltean wrote:
> - bonding is also DSA master when it has a DSA master as lower, so the
>   DSA master restriction has already run once - on the bonding device
>   itself

Huh, didn't know that.

> > The latter could be used for the first descend as well I'd presume.
> > And it can be exported for the use of more complex drivers like
> > bonding which want to walk the lowers themselves.
> >   
> > > - it requires cfg.flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX to be set in
> > >   SET requests
> > > 
> > > - it sets cfg.flags | HWTSTAMP_FLAG_BONDED_PHC_INDEX in GET responses  
> > 
> > IIRC that was to indicate to user space that the real PHC may change
> > for this netdev so it needs to pay attention to netlink notifications.
> > Shouldn't apply to *vlans?  
> 
> No, this shouldn't apply to *vlans, but I didn't suggest that it should.

Good, so if we just target *vlans we don't have to worry.

> I don't think my proposal was clear enough, so here's some code
> (untested, written in email client).
> 
> static int macvlan_hwtstamp_get(struct net_device *dev,
> 				struct kernel_hwtstamp_config *cfg,
> 				struct netlink_ext_ack *extack)
> {
> 	struct net_device *real_dev = macvlan_dev_real_dev(dev);
> 
> 	return generic_hwtstamp_get_lower(real_dev, cfg, extack);
> }
> 
> static int macvlan_hwtstamp_set(struct net_device *dev,
> 				struct kernel_hwtstamp_config *cfg,
> 				struct netlink_ext_ack *extack)
> {
> 	struct net_device *real_dev = macvlan_dev_real_dev(dev);
> 
> 	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> }
> 
> static int vlan_hwtstamp_get(struct net_device *dev,
> 			     struct kernel_hwtstamp_config *cfg,
> 			     struct netlink_ext_ack *extack)
> {
> 	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
> 
> 	return generic_hwtstamp_get_lower(real_dev, cfg, extack);
> }
> 
> static int vlan_hwtstamp_set(struct net_device *dev,
> 			     struct kernel_hwtstamp_config *cfg,
> 			     struct netlink_ext_ack *extack)
> {
> 	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
> 
> 	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> }

I got that, but why wouldn't this not be better, as it avoids 
the 3 driver stubs? (also written in the MUA)

int net_lower_hwtstamp_set(struct net_device *dev,
			   struct kernel_hwtstamp_config *cfg,
			   struct netlink_ext_ack *extack)
{
	struct list_head *iter = dev->adj_list.lower.next;
	struct net_device *lower;
	
	lower = netdev_lower_get_next(dev, &iter);
	return generic_hwtstamp_set_lower(lower, cfg, extack);
}

> static int bond_hwtstamp_get(struct net_device *bond_dev,
> 			     struct kernel_hwtstamp_config *cfg,
> 			     struct netlink_ext_ack *extack)
> {
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct net_device *real_dev = bond_option_active_slave_get_rcu(bond);
> 	int err;
> 
> 	if (!real_dev)
> 		return -EOPNOTSUPP;
> 
> 	err = generic_hwtstamp_get_lower(real_dev, cfg, extack);
> 	if (err)
> 		return err;
> 
> 	/* Set the BOND_PHC_INDEX flag to notify user space */
> 	cfg->flags |= HWTSTAMP_FLAG_BONDED_PHC_INDEX;
> 
> 	return 0;
> }
> 
> static int bond_hwtstamp_set(struct net_device *bond_dev,
> 			     struct kernel_hwtstamp_config *cfg,
> 			     struct netlink_ext_ack *extack)
> {
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct net_device *real_dev = bond_option_active_slave_get_rcu(bond);
> 	int err;
> 
> 	if (!real_dev)
> 		return -EOPNOTSUPP;
> 
> 	if (!(cfg->flags & HWTSTAMP_FLAG_BONDED_PHC_INDEX))
> 		return -EOPNOTSUPP;
> 
> 	return generic_hwtstamp_set_lower(real_dev, cfg, extack);
> }
> 
> Doesn't seem in any way necessary to complicate things with the netdev
> adjacence lists?

What is the complication? We can add a "get first" helper maybe to hide
the oddities of the linking.

> > Yes, user space must be involved anyway, because the entire clock will
> > change. IMHO implementing the pass thru for timestamping requests on
> > bonding is checkbox engineering, kernel can't make it work
> > transparently. But nobody else spoke up when it was proposed so...  
> 
> ok, but that's a bit beside the point here.

You cut off the quote it was responding to so IDK if it is.