Re: [RFC PATCH v3 1/5] Add NDOs for hardware timestamp get/set

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Wed, Apr 05, 2023 at 12:31:44AM -0600, Maxim Georgiev wrote:
> Current NIC driver API demands drivers supporting hardware timestamping
> to implement handling logic for SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTLs.
> Handling these IOCTLs requires dirivers to implement request parameter
> structure translation between user and kernel address spaces, handling
> possible translation failures, etc. This translation code is pretty much
> identical across most of the NIC drivers that support SIOCGHWTSTAMP/
> SIOCSHWTSTAMP.
> This patch extends NDO functiuon set with ndo_hwtstamp_get/set
> functions, implements SIOCGHWTSTAMP/SIOCSHWTSTAMP IOCTL translation
> to ndo_hwtstamp_get/set function calls including parameter structure
> translation and translation error handling.
> 
> This patch is sent out as RFC.
> It still pending on basic testing.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Maxim Georgiev <glipus@gmail.com>
> ---
> Changes in v3:
> - Moved individual driver conversions to separate patches
> 
> Changes in v2:
> - Introduced kernel_hwtstamp_config structure
> - Added netlink_ext_ack* and kernel_hwtstamp_config* as NDO hw timestamp
>   function parameters
> - Reodered function variable declarations in dev_hwtstamp()
> - Refactored error handling logic in dev_hwtstamp()
> - Split dev_hwtstamp() into GET and SET versions
> - Changed net_hwtstamp_validate() to accept struct hwtstamp_config *
>   as a parameter
> ---
>  include/linux/net_tstamp.h |  8 ++++++++
>  include/linux/netdevice.h  | 16 ++++++++++++++++
>  net/core/dev_ioctl.c       | 36 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
> index fd67f3cc0c4b..063260475e77 100644
> --- a/include/linux/net_tstamp.h
> +++ b/include/linux/net_tstamp.h
> @@ -30,4 +30,12 @@ static inline void hwtstamp_config_to_kernel(struct kernel_hwtstamp_config *kern
>  	kernel_cfg->rx_filter = cfg->rx_filter;
>  }
>  
> +static inline void hwtstamp_kernel_to_config(struct hwtstamp_config *cfg,
> +					     const struct kernel_hwtstamp_config *kernel_cfg)

The reason why I suggested the name "hwtstamp_config_from_kernel()" was
to not break apart "hwtstamp" and "config", which together form the name
of one structure (hwtstamp_config).

> +{
> +	cfg->flags = kernel_cfg->flags;
> +	cfg->tx_type = kernel_cfg->tx_type;
> +	cfg->rx_filter = kernel_cfg->rx_filter;
> +}
> +
>  #endif /* _LINUX_NET_TIMESTAMPING_H_ */
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a740be3bb911..8356002d0ac0 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -57,6 +57,7 @@
>  struct netpoll_info;
>  struct device;
>  struct ethtool_ops;
> +struct kernel_hwtstamp_config;
>  struct phy_device;
>  struct dsa_port;
>  struct ip_tunnel_parm;
> @@ -1412,6 +1413,15 @@ struct netdev_net_notifier {
>   *	Get hardware timestamp based on normal/adjustable time or free running
>   *	cycle counter. This function is required if physical clock supports a
>   *	free running cycle counter.
> + *	int (*ndo_hwtstamp_get)(struct net_device *dev,
> + *				struct kernel_hwtstamp_config *kernel_config,
> + *				struct netlink_ext_ack *extack);
> + *	Get hardware timestamping parameters currently configured for NIC
> + *	device.
> + *	int (*ndo_hwtstamp_set)(struct net_device *dev,
> + *				struct kernel_hwtstamp_config *kernel_config,
> + *				struct netlink_ext_ack *extack);
> + *	Set hardware timestamping parameters for NIC device.
>   */
>  struct net_device_ops {
>  	int			(*ndo_init)(struct net_device *dev);
> @@ -1646,6 +1656,12 @@ struct net_device_ops {
>  	ktime_t			(*ndo_get_tstamp)(struct net_device *dev,
>  						  const struct skb_shared_hwtstamps *hwtstamps,
>  						  bool cycles);
> +	int			(*ndo_hwtstamp_get)(struct net_device *dev,
> +						    struct kernel_hwtstamp_config *kernel_config,
> +						    struct netlink_ext_ack *extack);
> +	int			(*ndo_hwtstamp_set)(struct net_device *dev,
> +						    struct kernel_hwtstamp_config *kernel_config,
> +						    struct netlink_ext_ack *extack);
>  };
>  
>  struct xdp_metadata_ops {
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 6d772837eb3f..736f310a0661 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -254,11 +254,30 @@ static int dev_eth_ioctl(struct net_device *dev,
>  
>  static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  {
> -	return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
> +	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct kernel_hwtstamp_config kernel_cfg;

Should we zero-initialize kernel_cfg (" = {}"), in case the driver does
not bother to populate, say, "flags"?

> +	struct hwtstamp_config config;
> +	int err;
> +
> +	if (!ops->ndo_hwtstamp_get)
> +		return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP);
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	err = ops->ndo_hwtstamp_get(dev, &kernel_cfg, NULL);
> +	if (err)
> +		return err;
> +
> +	hwtstamp_kernel_to_config(&config, &kernel_cfg);
> +	if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> +		return -EFAULT;
> +	return 0;
>  }
>  
>  static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  {
> +	const struct net_device_ops *ops = dev->netdev_ops;
>  	struct netdev_notifier_hwtstamp_info info = {
>  		.info.dev = dev,
>  	};
> @@ -288,7 +307,20 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
>  		return err;
>  	}
>  
> -	return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
> +	if (!ops->ndo_hwtstamp_set)
> +		return dev_eth_ioctl(dev, ifr, SIOCSHWTSTAMP);
> +
> +	if (!netif_device_present(dev))
> +		return -ENODEV;
> +
> +	err = ops->ndo_hwtstamp_set(dev, &kernel_cfg, NULL);
> +	if (err)
> +		return err;
> +
> +	hwtstamp_kernel_to_config(&cfg, &kernel_cfg);

Cosmetic blank line here

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

and here?

(and in equivalent positions in dev_get_hwtstamp())

> +	return 0;
>  }
>  
>  static int dev_siocbond(struct net_device *dev,
> -- 
> 2.39.2
>