Re: [PATCH net-next v17 13/14] net: ethtool: Add support for tsconfig command to get/set hwtstamp config

[Jacob Keller <jacob.e.keller@intel.com>]

On 7/9/2024 6:53 AM, Kory Maincent wrote:
> Introduce support for ETHTOOL_MSG_TSCONFIG_GET/SET ethtool netlink socket
> to read and configure hwtstamp configuration of a PHC provider. Note that
> simultaneous hwtstamp isn't supported; configuring a new one disables the
> previous setting.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Changes in v16:
> - Add a new patch to separate tsinfo into a new tsconfig command to get
>   and set the hwtstamp config.
> 
> Changes in v17:
> - Fix a doc misalignment.
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> +The legacy configuration is the use of the ioctl(SIOCSHWTSTAMP) with a pointer
> +to a struct ifreq whose ifr_data points to a struct hwtstamp_config.
> +The tx_type and rx_filter are hints to the driver what it is expected to do.
> +If the requested fine-grained filtering for incoming packets is not
>  supported, the driver may time stamp more than just the requested types
>  of packets.
>  

Does the core automatically handle SIOCSHWTSTAMP and SIOCGHWTSTAMP in
terms of the new API? I'm guessing yes because of the new
.ndo_set_hwtstamp ops?

> @@ -531,9 +536,12 @@ Only a processes with admin rights may change the configuration. User
>  space is responsible to ensure that multiple processes don't interfere
>  with each other and that the settings are reset.
>  
> -Any process can read the actual configuration by passing this
> -structure to ioctl(SIOCGHWTSTAMP) in the same way.  However, this has
> -not been implemented in all drivers.
> +Any process can read the actual configuration by requesting tsconfig netlink
> +socket ETHTOOL_MSG_TSCONFIG_GET.
> +
> +The legacy usage is to pass this structure to ioctl(SIOCGHWTSTAMP) in the
> +same way as the ioctl(SIOCSHWTSTAMP).  However, this has not been implemented
> +in all drivers.
>  
>  ::
>  
> @@ -578,9 +586,10 @@ not been implemented in all drivers.
>  --------------------------------------------------------
>  
>  A driver which supports hardware time stamping must support the
> -SIOCSHWTSTAMP ioctl and update the supplied struct hwtstamp_config with
> -the actual values as described in the section on SIOCSHWTSTAMP.  It
> -should also support SIOCGHWTSTAMP.
> +ndo_hwtstamp_set NDO or the legacy SIOCSHWTSTAMP ioctl and update the
> +supplied struct hwtstamp_config with the actual values as described in
> +the section on SIOCSHWTSTAMP. It should also support ndo_hwtstamp_get or
> +the legacy SIOCGHWTSTAMP.

Can we simply drop the mention of implementing the legacy implementation
on the kernel side? I guess not all existing drivers have converted yet...?

I have a similar thought about the other legacy PTP hooks.. it is good
to completely remove the legacy/deprecated implementations as it means
drivers can't be published which don't update to new APIs. That
ultimately just wastes reviewer/maintainer time to point out that it
must be updated to new APIs.

Obviously this will require some effort to make sure all existing
drivers get refactored.