netdev - Re: [RFC v2 PATCH 06/10] 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] [day] [month] [year] [list]

Message-ID: <20250131103352.GH24105@kernel.org>
Date: Fri, 31 Jan 2025 10:33:52 +0000
From: Simon Horman <horms@...nel.org>
To: Basharath Hussain Khaja <basharath@...thit.com>
Cc: 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, nm@...com, ssantosh@...nel.org,
	tony@...mide.com, richardcochran@...il.com, parvathi@...thit.com,
	schnelle@...ux.ibm.com, rdunlap@...radead.org,
	diogo.ivo@...mens.com, m-karicheri2@...com,
	jacob.e.keller@...el.com, m-malladi@...com,
	javier.carrasco.cruz@...il.com, afd@...com, s-anna@...com,
	linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-omap@...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: [RFC v2 PATCH 06/10] net: ti: prueth: Adds HW timestamping
 support for PTP using PRU-ICSS IEP module

On Fri, Jan 24, 2025 at 07:10:52PM +0530, Basharath Hussain Khaja 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: Parvathi Pudi <parvathi@...thit.com>
> Signed-off-by: Basharath Hussain Khaja <basharath@...thit.com>

...

> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c b/drivers/net/ethernet/ti/icssm/icssm_prueth.c

...

> @@ -682,9 +899,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 *)roundup((uintptr_t)src_addr,
> +					   ICSS_BLOCK_SIZE);

Can PTR_ALIGN() be used here?

> +		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);
>  		local_bh_disable();

The code preceding the hunk below is:

static int icssm_emac_request_irqs(struct prueth_emac *emac)
{
	struct net_device *ndev = emac->ndev;
	int ret;

	ret = request_threaded_irq(emac->rx_irq, NULL, icssm_emac_rx_thread,
				   IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
				   ndev->name, ndev);
	if (ret) {
		netdev_err(ndev, "unable to request RX IRQ\n");
		return ret;
	}

> @@ -855,9 +1085,64 @@ static int icssm_emac_request_irqs(struct prueth_emac *emac)
>  		return ret;
>  	}
>  
> +	if (emac->emac_ptp_tx_irq) {
> +		ret = request_threaded_irq(emac->emac_ptp_tx_irq,
> +					   icssm_prueth_ptp_tx_irq_handle,
> +					   icssm_prueth_ptp_tx_irq_work,
> +					   IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +					   ndev->name, ndev);
> +		if (ret) {
> +			netdev_err(ndev, "unable to request PTP TX IRQ\n");
> +			free_irq(emac->rx_irq, ndev);
> +			free_irq(emac->tx_irq, ndev);

This seems somewhat asymmetric. This function does request emac->rx_irq
but not emac->tx_irq. So I don't think it is appropriate to free emac->tx_irq
here.

Also, I would suggest using a goto label for unwind here.

> +		}
> +	}
> +
>  	return ret;
>  }
>  

...