Re: [PATCHv3 net-next 1/2] net_tstamp: add new flag HWTSTAMP_FLAG_BONDED_PHC_INDEX

[Hangbin Liu <liuhangbin@gmail.com>]

Hi Jakub,

When implement the user space support for this feature. I realized that
we can't use the new flag directly as the user space tool needs to have
backward compatibility. Because run the new tool with this flag enabled
on old kernel will get -EINVAL error. And we also could not use #ifdef
directly as HWTSTAMP_FLAG_BONDED_PHC_INDEX is a enum.

Do you think if we could add a #define in linux/net_tstamp.h like

#define HWTSTAMP_FLAGS_SUPPORT 1

So that the user space tool could use it like

#ifdef HWTSTAMP_FLAGS_SUPPORT
       cfg->flags = HWTSTAMP_FLAG_BONDED_PHC_INDEX;
#endif

Thanks
Hangbin

On Fri, Dec 10, 2021 at 04:59:58PM +0800, Hangbin Liu wrote:
> Since commit 94dd016ae538 ("bond: pass get_ts_info and SIOC[SG]HWTSTAMP
> ioctl to active device") the user could get bond active interface's
> PHC index directly. But when there is a failover, the bond active
> interface will change, thus the PHC index is also changed. This may
> break the user's program if they did not update the PHC timely.
> 
> This patch adds a new hwtstamp_config flag HWTSTAMP_FLAG_BONDED_PHC_INDEX.
> When the user wants to get the bond active interface's PHC, they need to
> add this flag and be aware the PHC index may be changed.
> 
> With the new flag. All flag checks in current drivers are removed. Only
> the checking in net_hwtstamp_validate() is kept.
> 
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> ---
> v3: Use bitwise test to check the flags validation
> v2: Keep the flag validation check in net_hwtstamp_validate()
>     Rename the flag to HWTSTAMP_FLAG_BONDED_PHC_INDEX

[...]

> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
> index fcc61c73a666..e258e52cfd1f 100644
> --- a/include/uapi/linux/net_tstamp.h
> +++ b/include/uapi/linux/net_tstamp.h
> @@ -62,7 +62,7 @@ struct so_timestamping {
>  /**
>   * struct hwtstamp_config - %SIOCGHWTSTAMP and %SIOCSHWTSTAMP parameter
>   *
> - * @flags:	no flags defined right now, must be zero for %SIOCSHWTSTAMP
> + * @flags:	one of HWTSTAMP_FLAG_*
>   * @tx_type:	one of HWTSTAMP_TX_*
>   * @rx_filter:	one of HWTSTAMP_FILTER_*
>   *
> @@ -78,6 +78,20 @@ struct hwtstamp_config {
>  	int rx_filter;
>  };
>  
> +/* possible values for hwtstamp_config->flags */
> +enum hwtstamp_flags {
> +	/*
> +	 * With this flag, the user could get bond active interface's
> +	 * PHC index. Note this PHC index is not stable as when there
> +	 * is a failover, the bond active interface will be changed, so
> +	 * will be the PHC index.
> +	 */
> +	HWTSTAMP_FLAG_BONDED_PHC_INDEX = (1<<0),
> +
> +	HWTSTAMP_FLAG_LAST = HWTSTAMP_FLAG_BONDED_PHC_INDEX,
> +	HWTSTAMP_FLAG_MASK = (HWTSTAMP_FLAG_LAST - 1) | HWTSTAMP_FLAG_LAST
> +};
> +
>  /* possible values for hwtstamp_config->tx_type */
>  enum hwtstamp_tx_types {
>  	/*
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 1d309a666932..1b807d119da5 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -192,7 +192,7 @@ static int net_hwtstamp_validate(struct ifreq *ifr)
>  	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
>  		return -EFAULT;
>  
> -	if (cfg.flags) /* reserved for future extensions */
> +	if (cfg.flags & ~HWTSTAMP_FLAG_MASK)
>  		return -EINVAL;
>  
>  	tx_type = cfg.tx_type;
> -- 
> 2.31.1
>