| Message ID | 20230502043150.17097-3-glipus@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | New NDO methods ndo_hwtstamp_get/set | expand |
On Mon, 1 May 2023 22:31:47 -0600 Maxim Georgiev wrote: > + err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP); > + if (!err) { > + kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru; > + kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT; > + } > + return err; nit: I think we should stick to the normal flow even if it costs a few extra lines: err = dev_eth_ioctl(.. if (err) return err; kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru; kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT; return 0; Other than that patches LGTM :)
On Wed, May 3, 2023 at 9:05 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 1 May 2023 22:31:47 -0600 Maxim Georgiev wrote: > > + err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP); > > + if (!err) { > > + kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru; > > + kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT; > > + } > > + return err; > > nit: I think we should stick to the normal flow even if it costs > a few extra lines: > > err = dev_eth_ioctl(.. > if (err) > return err; > > kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru; > kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT; > > return 0; > > > Other than that patches LGTM :) Got it, wil update both generic_hwtstamp_get_lower() and generic_hwtstamp_set_lower(). What would be the best practice with updating a single patch in a stack (or a couple of patches in a stack)? Should I resend only the updated patch(es), or should I increment the patch stack revision and resend all the parches?
On Thu, 4 May 2023 09:21:42 -0600 Max Georgiev wrote: > Got it, wil update both generic_hwtstamp_get_lower() and > generic_hwtstamp_set_lower(). > > What would be the best practice with updating a single patch in a > stack (or a couple of > patches in a stack)? Should I resend only the updated patch(es), or > should I increment the > patch stack revision and resend all the parches? You'll need to resend all, but this is minor enough that, unless there's more comments, I'd just wait until Monday and send non-RFC at that point (with "a" driver conversion included).
On Mon, May 01, 2023 at 10:31:47PM -0600, Maxim Georgiev wrote: > Considering the stackable nature of drivers there will be situations > where a driver implementing ndo_hwtstamp_get/set functions will have > to translate requests back to SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs > to pass them to lower level drivers that do not provide > ndo_hwtstamp_get/set callbacks. To simplify request translation in > such scenarios let's include a pointer to the original struct ifreq > to kernel_hwtstamp_config structure. > > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Maxim Georgiev <glipus@gmail.com> > > Notes: > Changes in v6: > - Patch title was updated. No code changes. > Changes in v5: > - kernel_hwtstamp_config kdoc is updated with the new field > descriptions. > Changes in V4: > - Introducing KERNEL_HWTSTAMP_FLAG_IFR_RESULT flag indicating that > the operation results are returned in the ifr referred by > struct kernel_hwtstamp_config instead of kernel_hwtstamp_config > glags/tx_type/rx_filter fields. > - Implementing generic_hwtstamp_set/set_lower() functions > which will be used by vlan, maxvlan, bond and potentially > other drivers translating ndo_hwtstamp_set/set calls to > lower level drivers. Usually the notes go below the "---" line so that they are stripped once the patch gets applied, but are otherwise visible to reviewers. It's good having them nonetheless. > --- > include/linux/net_tstamp.h | 9 +++++ > include/linux/netdevice.h | 6 +++ > net/core/dev_ioctl.c | 80 +++++++++++++++++++++++++++++++++++--- > 3 files changed, 89 insertions(+), 6 deletions(-) > > diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h > index 7c59824f43f5..91d2738bf0a0 100644 > --- a/include/linux/net_tstamp.h > +++ b/include/linux/net_tstamp.h > @@ -11,6 +11,8 @@ > * @flags: see struct hwtstamp_config > * @tx_type: see struct hwtstamp_config > * @rx_filter: see struct hwtstamp_config > + * @ifr: pointer to ifreq structure from the original IOCTL request > + * @kernel_flags: possible flags defined by kernel_hwtstamp_flags below > * > * Prefer using this structure for in-kernel processing of hardware > * timestamping configuration, over the inextensible struct hwtstamp_config > @@ -20,6 +22,13 @@ struct kernel_hwtstamp_config { > int flags; > int tx_type; > int rx_filter; > + struct ifreq *ifr; > + int kernel_flags; > +}; > + > +/* possible values for kernel_hwtstamp_config->kernel_flags */ > +enum kernel_hwtstamp_flags { > + KERNEL_HWTSTAMP_FLAG_IFR_RESULT = BIT(0), > }; I think "kernel_flags" and KERNEL_HWTSTAMP_FLAG_IFR_RESULT are slightly overengineered and poorly named (I couldn't understand yesterday what they do, tried again today, finally did). I believe that a single "bool copied_to_user" would do the trick just fine and also be more self-explanatory? > > static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kernel_cfg, > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 7160135ca540..42e96b12fd21 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -3942,6 +3942,12 @@ int put_user_ifreq(struct ifreq *ifr, void __user *arg); > int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr, > void __user *data, bool *need_copyout); > int dev_ifconf(struct net *net, struct ifconf __user *ifc); > +int generic_hwtstamp_set_lower(struct net_device *dev, > + struct kernel_hwtstamp_config *kernel_cfg, > + struct netlink_ext_ack *extack); > +int generic_hwtstamp_get_lower(struct net_device *dev, > + struct kernel_hwtstamp_config *kernel_cfg, > + struct netlink_ext_ack *extack); > int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *userdata); > unsigned int dev_get_flags(const struct net_device *); > int __dev_change_flags(struct net_device *dev, unsigned int flags, > diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c > index a157b9ab5237..da1d2391822f 100644 > --- a/net/core/dev_ioctl.c > +++ b/net/core/dev_ioctl.c > @@ -265,14 +265,17 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr) > if (!netif_device_present(dev)) > return -ENODEV; > > + kernel_cfg.ifr = ifr; > err = ops->ndo_hwtstamp_get(dev, &kernel_cfg, NULL); > if (err) > return err; > > - hwtstamp_config_from_kernel(&config, &kernel_cfg); > + if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) { > + hwtstamp_config_from_kernel(&config, &kernel_cfg); > > - if (copy_to_user(ifr->ifr_data, &config, sizeof(config))) > - return -EFAULT; > + if (copy_to_user(ifr->ifr_data, &config, sizeof(config))) > + return -EFAULT; > + } > > return 0; > } > @@ -289,6 +292,7 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) > return -EFAULT; > > hwtstamp_config_to_kernel(&kernel_cfg, &cfg); > + kernel_cfg.ifr = ifr; > > err = net_hwtstamp_validate(&kernel_cfg); > if (err) > @@ -311,14 +315,78 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) > if (err) > return err; > > - hwtstamp_config_from_kernel(&cfg, &kernel_cfg); > + if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) { > + hwtstamp_config_from_kernel(&cfg, &kernel_cfg); > > - if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg))) > - return -EFAULT; > + if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg))) > + return -EFAULT; > + } > > return 0; > } > > +int generic_hwtstamp_set_lower(struct net_device *dev, > + struct kernel_hwtstamp_config *kernel_cfg, > + struct netlink_ext_ack *extack) > +{ > + const struct net_device_ops *ops = dev->netdev_ops; > + struct ifreq ifrr; > + int err; > + > + if (!netif_device_present(dev)) > + return -ENODEV; > + > + if (ops->ndo_hwtstamp_set) { > + kernel_cfg->kernel_flags &= ~KERNEL_HWTSTAMP_FLAG_IFR_RESULT; I don't think you ever need to unset this flag, just set it? The structure is initialized in dev_get_hwtstamp() and dev_set_hwtstamp() as: struct kernel_hwtstamp_config kernel_cfg = {}; which zero-fills it. > + err = ops->ndo_hwtstamp_set(dev, kernel_cfg, extack); > + return err; return ops->ndo_hwtstamp_set(...) i.e. skip the "err" assignment > + } > + > + if (!kernel_cfg->ifr) > + return -EOPNOTSUPP; > + > + strscpy_pad(ifrr.ifr_name, dev->name, IFNAMSIZ); > + ifrr.ifr_ifru = kernel_cfg->ifr->ifr_ifru; > + err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP); > + if (!err) { > + kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru; > + kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT; > + } > + return err; > +} > +EXPORT_SYMBOL(generic_hwtstamp_set_lower); EXPORT_SYMBOL_GPL()? We expect this to be used by core network stack (vlan, macvlan, bonding), not by vendor drivers. Same comments for "set".
diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h index 7c59824f43f5..91d2738bf0a0 100644 --- a/include/linux/net_tstamp.h +++ b/include/linux/net_tstamp.h @@ -11,6 +11,8 @@ * @flags: see struct hwtstamp_config * @tx_type: see struct hwtstamp_config * @rx_filter: see struct hwtstamp_config + * @ifr: pointer to ifreq structure from the original IOCTL request + * @kernel_flags: possible flags defined by kernel_hwtstamp_flags below * * Prefer using this structure for in-kernel processing of hardware * timestamping configuration, over the inextensible struct hwtstamp_config @@ -20,6 +22,13 @@ struct kernel_hwtstamp_config { int flags; int tx_type; int rx_filter; + struct ifreq *ifr; + int kernel_flags; +}; + +/* possible values for kernel_hwtstamp_config->kernel_flags */ +enum kernel_hwtstamp_flags { + KERNEL_HWTSTAMP_FLAG_IFR_RESULT = BIT(0), }; static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kernel_cfg, diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 7160135ca540..42e96b12fd21 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3942,6 +3942,12 @@ int put_user_ifreq(struct ifreq *ifr, void __user *arg); int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr, void __user *data, bool *need_copyout); int dev_ifconf(struct net *net, struct ifconf __user *ifc); +int generic_hwtstamp_set_lower(struct net_device *dev, + struct kernel_hwtstamp_config *kernel_cfg, + struct netlink_ext_ack *extack); +int generic_hwtstamp_get_lower(struct net_device *dev, + struct kernel_hwtstamp_config *kernel_cfg, + struct netlink_ext_ack *extack); int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *userdata); unsigned int dev_get_flags(const struct net_device *); int __dev_change_flags(struct net_device *dev, unsigned int flags, diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c index a157b9ab5237..da1d2391822f 100644 --- a/net/core/dev_ioctl.c +++ b/net/core/dev_ioctl.c @@ -265,14 +265,17 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr) if (!netif_device_present(dev)) return -ENODEV; + kernel_cfg.ifr = ifr; err = ops->ndo_hwtstamp_get(dev, &kernel_cfg, NULL); if (err) return err; - hwtstamp_config_from_kernel(&config, &kernel_cfg); + if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) { + hwtstamp_config_from_kernel(&config, &kernel_cfg); - if (copy_to_user(ifr->ifr_data, &config, sizeof(config))) - return -EFAULT; + if (copy_to_user(ifr->ifr_data, &config, sizeof(config))) + return -EFAULT; + } return 0; } @@ -289,6 +292,7 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) return -EFAULT; hwtstamp_config_to_kernel(&kernel_cfg, &cfg); + kernel_cfg.ifr = ifr; err = net_hwtstamp_validate(&kernel_cfg); if (err) @@ -311,14 +315,78 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr) if (err) return err; - hwtstamp_config_from_kernel(&cfg, &kernel_cfg); + if (!(kernel_cfg.kernel_flags & KERNEL_HWTSTAMP_FLAG_IFR_RESULT)) { + hwtstamp_config_from_kernel(&cfg, &kernel_cfg); - if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg))) - return -EFAULT; + if (copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg))) + return -EFAULT; + } return 0; } +int generic_hwtstamp_set_lower(struct net_device *dev, + struct kernel_hwtstamp_config *kernel_cfg, + struct netlink_ext_ack *extack) +{ + const struct net_device_ops *ops = dev->netdev_ops; + struct ifreq ifrr; + int err; + + if (!netif_device_present(dev)) + return -ENODEV; + + if (ops->ndo_hwtstamp_set) { + kernel_cfg->kernel_flags &= ~KERNEL_HWTSTAMP_FLAG_IFR_RESULT; + err = ops->ndo_hwtstamp_set(dev, kernel_cfg, extack); + return err; + } + + if (!kernel_cfg->ifr) + return -EOPNOTSUPP; + + strscpy_pad(ifrr.ifr_name, dev->name, IFNAMSIZ); + ifrr.ifr_ifru = kernel_cfg->ifr->ifr_ifru; + err = dev_eth_ioctl(dev, &ifrr, SIOCSHWTSTAMP); + if (!err) { + kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru; + kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT; + } + return err; +} +EXPORT_SYMBOL(generic_hwtstamp_set_lower); + +int generic_hwtstamp_get_lower(struct net_device *dev, + struct kernel_hwtstamp_config *kernel_cfg, + struct netlink_ext_ack *extack) +{ + const struct net_device_ops *ops = dev->netdev_ops; + struct ifreq ifrr; + int err; + + if (!netif_device_present(dev)) + return -ENODEV; + + if (ops->ndo_hwtstamp_get) { + kernel_cfg->kernel_flags &= ~KERNEL_HWTSTAMP_FLAG_IFR_RESULT; + err = ops->ndo_hwtstamp_get(dev, kernel_cfg, extack); + return err; + } + + if (!kernel_cfg->ifr) + return -EOPNOTSUPP; + + strscpy_pad(ifrr.ifr_name, dev->name, IFNAMSIZ); + ifrr.ifr_ifru = kernel_cfg->ifr->ifr_ifru; + err = dev_eth_ioctl(dev, &ifrr, SIOCGHWTSTAMP); + if (!err) { + kernel_cfg->ifr->ifr_ifru = ifrr.ifr_ifru; + kernel_cfg->kernel_flags |= KERNEL_HWTSTAMP_FLAG_IFR_RESULT; + } + return err; +} +EXPORT_SYMBOL(generic_hwtstamp_get_lower); + static int dev_siocbond(struct net_device *dev, struct ifreq *ifr, unsigned int cmd) {
Considering the stackable nature of drivers there will be situations where a driver implementing ndo_hwtstamp_get/set functions will have to translate requests back to SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs to pass them to lower level drivers that do not provide ndo_hwtstamp_get/set callbacks. To simplify request translation in such scenarios let's include a pointer to the original struct ifreq to kernel_hwtstamp_config structure. Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Maxim Georgiev <glipus@gmail.com> Notes: Changes in v6: - Patch title was updated. No code changes. Changes in v5: - kernel_hwtstamp_config kdoc is updated with the new field descriptions. Changes in V4: - Introducing KERNEL_HWTSTAMP_FLAG_IFR_RESULT flag indicating that the operation results are returned in the ifr referred by struct kernel_hwtstamp_config instead of kernel_hwtstamp_config glags/tx_type/rx_filter fields. - Implementing generic_hwtstamp_set/set_lower() functions which will be used by vlan, maxvlan, bond and potentially other drivers translating ndo_hwtstamp_set/set calls to lower level drivers. --- include/linux/net_tstamp.h | 9 +++++ include/linux/netdevice.h | 6 +++ net/core/dev_ioctl.c | 80 +++++++++++++++++++++++++++++++++++--- 3 files changed, 89 insertions(+), 6 deletions(-)