Subject: Re: [PATCH RFC net-next v8 06/13] net: Add struct kernel_ethtool_ts_info

From: Rahul Rameshbabu <[email-masked]>
Date: Fri, 16 Feb 2024 10:27:32 -0800

On Fri, 16 Feb, 2024 16:52:24 +0100 Kory Maincent <[email-masked]> wrote:
> In prevision to add new UAPI for hwtstamp we will be limited to the struct
> ethtool_ts_info that is currently passed in fixed binary format through the
> ETHTOOL_GET_TS_INFO ethtool ioctl. It would be good if new kernel code
> already started operating on an extensible kernel variant of that
> structure, similar in concept to struct kernel_hwtstamp_config vs struct
> hwtstamp_config.
>
> Since struct ethtool_ts_info is in include/uapi/linux/ethtool.h, here
> we introduce the kernel-only structure in include/linux/ethtool.h.
> The manual copy is then made in the function called by ETHTOOL_GET_TS_INFO.
>
> Signed-off-by: Kory Maincent <[email-masked]>
> ---
>  drivers/net/bonding/bond_main.c                    |  4 ++--
>  drivers/net/can/dev/dev.c                          |  2 +-
>  drivers/net/can/peak_canfd/peak_canfd.c            |  2 +-
>  drivers/net/can/usb/gs_usb.c                       |  2 +-
>  drivers/net/can/usb/peak_usb/pcan_usb_core.c       |  2 +-
>  drivers/net/can/usb/peak_usb/pcan_usb_core.h       |  2 +-
>  drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c    |  2 +-
>  drivers/net/dsa/hirschmann/hellcreek_hwtstamp.h    |  2 +-
>  drivers/net/dsa/microchip/ksz_ptp.c                |  2 +-
>  drivers/net/dsa/microchip/ksz_ptp.h                |  2 +-
>  drivers/net/dsa/mv88e6xxx/hwtstamp.c               |  2 +-
>  drivers/net/dsa/mv88e6xxx/hwtstamp.h               |  4 ++--
>  drivers/net/dsa/ocelot/felix.c                     |  2 +-
>  drivers/net/dsa/sja1105/sja1105_ptp.c              |  2 +-
>  drivers/net/dsa/sja1105/sja1105_ptp.h              |  2 +-
>  drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c       |  2 +-
>  .../net/ethernet/aquantia/atlantic/aq_ethtool.c    |  2 +-
>  .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c    |  2 +-
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  |  2 +-
>  drivers/net/ethernet/broadcom/tg3.c                |  2 +-
>  drivers/net/ethernet/cadence/macb.h                |  2 +-
>  drivers/net/ethernet/cadence/macb_main.c           |  4 ++--
>  drivers/net/ethernet/cavium/liquidio/lio_ethtool.c |  2 +-
>  .../net/ethernet/cavium/thunder/nicvf_ethtool.c    |  2 +-
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c |  2 +-
>  drivers/net/ethernet/cisco/enic/enic_ethtool.c     |  2 +-
>  drivers/net/ethernet/engleder/tsnep_ethtool.c      |  2 +-
>  drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c |  2 +-
>  .../net/ethernet/freescale/dpaa2/dpaa2-ethtool.c   |  2 +-
>  .../net/ethernet/freescale/enetc/enetc_ethtool.c   |  2 +-
>  drivers/net/ethernet/freescale/fec_main.c          |  2 +-
>  drivers/net/ethernet/freescale/gianfar_ethtool.c   |  2 +-
>  .../net/ethernet/fungible/funeth/funeth_ethtool.c  |  2 +-
>  drivers/net/ethernet/hisilicon/hns3/hnae3.h        |  2 +-
>  drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c |  2 +-
>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c |  2 +-
>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.h |  2 +-
>  drivers/net/ethernet/intel/e1000e/ethtool.c        |  2 +-
>  drivers/net/ethernet/intel/i40e/i40e_ethtool.c     |  2 +-
>  drivers/net/ethernet/intel/ice/ice_ethtool.c       |  2 +-
>  drivers/net/ethernet/intel/igb/igb_ethtool.c       |  2 +-
>  drivers/net/ethernet/intel/igc/igc_ethtool.c       |  2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c   |  2 +-
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c    |  2 +-
>  .../ethernet/marvell/octeontx2/nic/otx2_ethtool.c  |  2 +-
>  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c    |  2 +-
>  drivers/net/ethernet/mellanox/mlx5/core/en.h       |  2 +-
>  .../net/ethernet/mellanox/mlx5/core/en_ethtool.c   |  4 ++--
>  .../ethernet/mellanox/mlx5/core/ipoib/ethtool.c    |  2 +-
>  drivers/net/ethernet/mellanox/mlxsw/spectrum.h     |  2 +-
>  .../net/ethernet/mellanox/mlxsw/spectrum_ethtool.c |  2 +-
>  drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c |  4 ++--
>  drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.h | 10 ++++-----
>  drivers/net/ethernet/microchip/lan743x_ethtool.c   |  2 +-
>  .../ethernet/microchip/lan966x/lan966x_ethtool.c   |  2 +-
>  .../net/ethernet/microchip/sparx5/sparx5_ethtool.c |  2 +-
>  drivers/net/ethernet/mscc/ocelot_net.c             |  2 +-
>  drivers/net/ethernet/mscc/ocelot_ptp.c             |  2 +-
>  .../net/ethernet/pensando/ionic/ionic_ethtool.c    |  2 +-
>  drivers/net/ethernet/qlogic/qede/qede_ethtool.c    |  2 +-
>  drivers/net/ethernet/qlogic/qede/qede_ptp.c        |  2 +-
>  drivers/net/ethernet/qlogic/qede/qede_ptp.h        |  2 +-
>  drivers/net/ethernet/renesas/ravb_main.c           |  2 +-
>  drivers/net/ethernet/renesas/rswitch.c             |  2 +-
>  drivers/net/ethernet/sfc/ethtool.c                 |  2 +-
>  drivers/net/ethernet/sfc/falcon/nic.h              |  2 +-
>  drivers/net/ethernet/sfc/ptp.c                     |  2 +-
>  drivers/net/ethernet/sfc/ptp.h                     |  5 +++--
>  drivers/net/ethernet/sfc/siena/ethtool.c           |  2 +-
>  drivers/net/ethernet/sfc/siena/ptp.c               |  2 +-
>  drivers/net/ethernet/sfc/siena/ptp.h               |  4 ++--
>  .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   |  2 +-
>  drivers/net/ethernet/ti/am65-cpsw-ethtool.c        |  2 +-
>  drivers/net/ethernet/ti/cpsw_ethtool.c             |  4 ++--
>  drivers/net/ethernet/ti/cpsw_priv.h                |  2 +-
>  drivers/net/ethernet/ti/icssg/icssg_ethtool.c      |  2 +-
>  drivers/net/ethernet/ti/netcp_ethss.c              |  4 ++--
>  drivers/net/ethernet/xscale/ixp4xx_eth.c           |  2 +-
>  drivers/net/macvlan.c                              |  2 +-
>  drivers/net/netdevsim/ethtool.c                    |  2 +-
>  drivers/net/phy/bcm-phy-ptp.c                      |  2 +-
>  drivers/net/phy/dp83640.c                          |  2 +-
>  drivers/net/phy/marvell_ptp.c                      |  2 +-
>  drivers/net/phy/micrel.c                           |  4 ++--
>  drivers/net/phy/mscc/mscc_ptp.c                    |  2 +-
>  drivers/net/phy/nxp-c45-tja11xx.c                  |  2 +-
>  drivers/ptp/ptp_ines.c                             |  2 +-
>  drivers/s390/net/qeth_ethtool.c                    |  2 +-
>  include/linux/can/dev.h                            |  2 +-
>  include/linux/ethtool.h                            | 25 +++++++++++++++++++---
>  include/linux/mii_timestamper.h                    |  2 +-
>  include/linux/phy.h                                |  2 +-
>  include/net/dsa.h                                  |  2 +-
>  include/soc/mscc/ocelot.h                          |  2 +-
>  net/8021q/vlan_dev.c                               |  2 +-
>  net/dsa/user.c                                     |  2 +-
>  net/ethtool/common.c                               |  6 +++---
>  net/ethtool/common.h                               |  2 +-
>  net/ethtool/ioctl.c                                | 12 +++++++++--
>  net/ethtool/tsinfo.c                               |  6 +++---
>  net/sched/sch_taprio.c                             |  2 +-
>  101 files changed, 150 insertions(+), 122 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index b90c33607594..f74190a8d9ee 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -18,6 +18,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/netlink.h>
>  #include <uapi/linux/ethtool.h>
> +#include <uapi/linux/net_tstamp.h>
>  
>  struct compat_ethtool_rx_flow_spec {
>  	u32		flow_type;
> @@ -644,6 +645,22 @@ struct ethtool_rxfh_param {
>  	u8	input_xfrm;
>  };
>  
> +/**
> + * struct kernel_ethtool_ts_info - kernel copy of struct ethtool_ts_info
> + * @cmd: command number = %ETHTOOL_GET_TS_INFO
> + * @so_timestamping: bit mask of the sum of the supported SO_TIMESTAMPING flags
> + * @phc_index: device index of the associated PHC, or -1 if there is none
> + * @tx_types: bit mask of the supported hwtstamp_tx_types enumeration values
> + * @rx_filters: bit mask of the supported hwtstamp_rx_filters enumeration values
> + */
> +struct kernel_ethtool_ts_info {
> +	u32 cmd;
> +	u32 so_timestamping;
> +	int phc_index;
> +	enum hwtstamp_tx_types tx_types;

I might just be terrible at reading code, but isn't this what
tx_reserved in ethtool_ts_info is for? I feel like using one of the
reserved memspaces for this enum removes the need to make
kernel_ethtool_ts_info? I might be missing something obvious however.

> +	enum hwtstamp_rx_filters rx_filters;
> +};

--
Thanks,

Rahul Rameshbabu