Subject: Re: [PATCH v1 1/4] ethtool: Add new hwtstamp flag

From: Jakub Kicinski <[email-masked]>
Date: Tue, 27 Sep 2022 17:11:32 -0700

On Tue, 27 Sep 2022 21:06:53 +0800 Muhammad Husaini Zulkifli wrote:
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -675,6 +675,7 @@ enum ethtool_link_ext_substate_module {
>   * @ETH_SS_MSG_CLASSES: debug message class names
>   * @ETH_SS_WOL_MODES: wake-on-lan modes
>   * @ETH_SS_SOF_TIMESTAMPING: SOF_TIMESTAMPING_* flags
> + * @ETH_SS_HWTSTAMP_FLAG: timestamping flags
>   * @ETH_SS_TS_TX_TYPES: timestamping Tx types
>   * @ETH_SS_TS_RX_FILTERS: timestamping Rx filters
>   * @ETH_SS_UDP_TUNNEL_TYPES: UDP tunnel types
> @@ -700,6 +701,7 @@ enum ethtool_stringset {
>  	ETH_SS_MSG_CLASSES,
>  	ETH_SS_WOL_MODES,
>  	ETH_SS_SOF_TIMESTAMPING,
> +	ETH_SS_HWTSTAMP_FLAG,
>  	ETH_SS_TS_TX_TYPES,
>  	ETH_SS_TS_RX_FILTERS,
>  	ETH_SS_UDP_TUNNEL_TYPES,
> @@ -1367,6 +1369,7 @@ struct ethtool_ts_info {
>  	__u32	cmd;
>  	__u32	so_timestamping;
>  	__s32	phc_index;
> +	__u32	flag;
>  	__u32	tx_types;
>  	__u32	tx_reserved[3];
>  	__u32	rx_filters;
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 408a664fad59..58d073b5a6d2 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -452,6 +452,7 @@ enum {
>  	ETHTOOL_A_TSINFO_UNSPEC,
>  	ETHTOOL_A_TSINFO_HEADER,			/* nest - _A_HEADER_* */
>  	ETHTOOL_A_TSINFO_TIMESTAMPING,			/* bitset */
> +	ETHTOOL_A_TSINFO_FLAG,				/* bitset */
>  	ETHTOOL_A_TSINFO_TX_TYPES,			/* bitset */
>  	ETHTOOL_A_TSINFO_RX_FILTERS,			/* bitset */
>  	ETHTOOL_A_TSINFO_PHC_INDEX,			/* u32 */

You can't add stuff into the middle of an enum or a struct in uAPI.
What's worse for the struct ethtool_ts_info you can't actually add
anything in, period. You can reuse reserved fields but even that
requires extra legwork. If the fields were not previously validated on
input to the kernel (ie. kernel didn't check they are zero) the ioctl
path can't use them, because some application may had been passing in
garbage.