Message-ID: <CAP5jrPGzrzMYmBBT+B6U5Oh6v_Tcie1rj0KqsWOEZOBR7JBoXA@mail.gmail.com>
Date:   Thu, 6 Apr 2023 00:21:37 -0600
From:   Max Georgiev <glipus@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Vladimir Oltean <vladimir.oltean@....com>,
        kory.maincent@...tlin.com, netdev@...r.kernel.org,
        maxime.chevallier@...tlin.com, vadim.fedorenko@...ux.dev,
        richardcochran@...il.com, gerhard@...leder-embedded.com
Subject: Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

On Wed, Apr 5, 2023 at 6:00 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> 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.

I tried my best to follow the discussion, and convert it to compilable code.
Here is what I have in mind for generic_hwtstamp_get_lower():

int generic_hwtstamp_get_lower(struct net_dev *dev,
                           struct kernel_hwtstamp_config *kernel_cfg,
                           struct netlink_ext_ack *extack)
{
        const struct net_device_ops *ops = dev->netdev_ops;
        struct hwtstamp_config cfg;
        int err;

        if (!netif_device_present(dev))
                return -ENODEV;

        if (ops->ndo_hwtstamp_get)
                return ops->ndo_hwtstamp_get(dev, cfg, extack);

        if (!cfg->ifr)
                return -EOPNOTSUPP;

        err = dev_eth_ioctl(dev, cfg->ifr, SIOCGHWTSTAMP);
        if (err)
            return err;

        if (copy_from_user(&cfg, cfg->ifr->ifr_data, sizeof(cfg)))
                return -EFAULT;

        hwtstamp_config_to_kernel(kernel_cfg, &cfg);

        return 0;
}

It looks like there is a possibility that the returned hwtstamp_config structure
will be copied twice to ifr and copied once from ifr on the return path
in case if the underlying driver does not implement ndo_hwtstamp_get():
- the underlying driver calls copy_to_user() inside its ndo_eth_ioctl()
  implementation to return the data to generic_hwtstamp_get_lower();
- then generic_hwtstamp_get_lower() calls copy_from_user() to copy it
  back out of the ifr to kernel_hwtstamp_config structure;
- then dev_get_hwtstamp() calls copy_to_user() again to update
  the same ifr with the same data the ifr already contains.

Should we consider this acceptable?

Powered by blists - more mailing lists