linux-kernel - Re: [PATCH net-next v8 06/11] net: ti: prueth: Adds HW timestamping support for PTP using PRU-ICSS IEP module

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <cdcd54ff-ff67-4ad8-8aa7-baa711928242@linux.dev>
Date: Wed, 11 Jun 2025 11:29:46 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Parvathi Pudi <parvathi@...thit.com>, danishanwar@...com,
 rogerq@...nel.org, andrew+netdev@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, ssantosh@...nel.org,
 richardcochran@...il.com, s.hauer@...gutronix.de, m-karicheri2@...com,
 glaroque@...libre.com, afd@...com, saikrishnag@...vell.com,
 m-malladi@...com, jacob.e.keller@...el.com, diogo.ivo@...mens.com,
 javier.carrasco.cruz@...il.com, horms@...nel.org, s-anna@...com,
 basharath@...thit.com
Cc: linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, pratheesh@...com,
 prajith@...com, vigneshr@...com, praneeth@...com, srk@...com, rogerq@...com,
 krishna@...thit.com, pmohan@...thit.com, mohan@...thit.com
Subject: Re: [PATCH net-next v8 06/11] net: ti: prueth: Adds HW timestamping
 support for PTP using PRU-ICSS IEP module

On 10/06/2025 13:32, Parvathi Pudi wrote:
> From: Roger Quadros <rogerq@...com>
> 
> PRU-ICSS IEP module, which is capable of timestamping RX and
> TX packets at HW level, is used for time synchronization by PTP4L.
> 
> This change includes interaction between firmware and user space
> application (ptp4l) with required packet timestamps. The driver
> initializes the PRU firmware with appropriate mode and configuration
> flags. Firmware updates local registers with the flags set by driver
> and uses for further operation. RX SOF timestamp comes along with
> packet and firmware will rise interrupt with TX SOF timestamp after
> pushing the packet on to the wire.
> 
> IEP driver is available in upstream and we are reusing for hardware
> configuration for ICSSM as well. On top of that we have extended it
> with the changes for AM57xx SoC.
> 
> Extended ethtool for reading HW timestamping capability of the PRU
> interfaces.
> 
> Currently ordinary clock (OC) configuration has been validated with
> Linux ptp4l.
> 
> Signed-off-by: Roger Quadros <rogerq@...com>
> Signed-off-by: Andrew F. Davis <afd@...com>
> Signed-off-by: Basharath Hussain Khaja <basharath@...thit.com>
> Signed-off-by: Parvathi Pudi <parvathi@...thit.com>
> ---
>   drivers/net/ethernet/ti/icssg/icss_iep.c      |  42 ++
>   drivers/net/ethernet/ti/icssm/icssm_ethtool.c |  23 +
>   drivers/net/ethernet/ti/icssm/icssm_prueth.c  | 443 +++++++++++++++++-
>   drivers/net/ethernet/ti/icssm/icssm_prueth.h  |  11 +
>   .../net/ethernet/ti/icssm/icssm_prueth_ptp.h  |  85 ++++
>   5 files changed, 602 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/net/ethernet/ti/icssm/icssm_prueth_ptp.h

[...]

> @@ -732,9 +949,22 @@ int icssm_emac_rx_packet(struct prueth_emac *emac, u16 *bd_rd_ptr,
>   		src_addr += actual_pkt_len;
>   	}
>   
> +	if (pkt_info->timestamp) {
> +		src_addr = (void *)PTR_ALIGN((uintptr_t)src_addr,
> +					   ICSS_BLOCK_SIZE);
> +		dst_addr = &ts;
> +		memcpy(dst_addr, src_addr, sizeof(ts));
> +	}
> +
>   	if (!pkt_info->sv_frame) {
>   		skb_put(skb, actual_pkt_len);
>   
> +		if (icssm_prueth_ptp_rx_ts_is_enabled(emac) &&
> +		    pkt_info->timestamp) {
> +			ssh = skb_hwtstamps(skb);
> +			memset(ssh, 0, sizeof(*ssh));
> +			ssh->hwtstamp = ns_to_ktime(ts);
> +		}
>   		/* send packet up the stack */
>   		skb->protocol = eth_type_trans(skb, ndev);
>   		netif_receive_skb(skb);

Could you please explain why do you need to copy timestamp to a
temporary variable if you won't use it in some cases? I believe these
2 blocks should be placed under the last if condition and simplified a
bit, like

+		if (icssm_prueth_ptp_rx_ts_is_enabled(emac) &&
+		    pkt_info->timestamp) {
+			src_addr = (void*)PTR_ALIGN((uintptr_t)src_addr,
+					   ICSS_BLOCK_SIZE);
+			memcpy(&ts, src_addr, sizeof(ts));
+			ssh = skb_hwtstamps(skb);
+			ssh->hwtstamp = ns_to_ktime(ts);
+		}

This will avoid useless copy when the packet will be dropped anyway, WDYT?