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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f16855bf-ae2a-4a0c-b3e9-d25f64478900@intel.com>
Date: Wed, 17 Jul 2024 10:43:05 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Kory Maincent <kory.maincent@...tlin.com>, Florian Fainelli
	<florian.fainelli@...adcom.com>, Broadcom internal kernel review list
	<bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn <andrew@...n.ch>, Heiner
 Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, "David
 S. Miller" <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>, Richard
 Cochran <richardcochran@...il.com>, "Radu Pirea"
	<radu-nicolae.pirea@....nxp.com>, Jay Vosburgh <j.vosburgh@...il.com>, Andy
 Gospodarek <andy@...yhouse.net>, Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>, Willem de Bruijn
	<willemdebruijn.kernel@...il.com>, Jonathan Corbet <corbet@....net>, Horatiu
 Vultur <horatiu.vultur@...rochip.com>, <UNGLinuxDriver@...rochip.com>, Simon
 Horman <horms@...nel.org>, "Vladimir Oltean" <vladimir.oltean@....com>,
	<donald.hunter@...il.com>, <danieller@...dia.com>, <ecree.xilinx@...il.com>
CC: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, Maxime Chevallier
	<maxime.chevallier@...tlin.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>,
	Willem de Bruijn <willemb@...gle.com>, Shannon Nelson
	<shannon.nelson@....com>, Alexandra Winter <wintera@...ux.ibm.com>
Subject: Re: [PATCH net-next v17 13/14] net: ethtool: Add support for tsconfig
 command to get/set hwtstamp config



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@...tlin.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@...el.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.