linux-kernel - Re: [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW timestamp using CPTS FIFO

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: <dc45f4f104a0d0691715398d2f7efa6a0a3447b8.camel@redhat.com>
Date: Tue, 12 Mar 2024 11:50:30 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Chintan Vankar <c-vankar@...com>, Dan Carpenter
 <dan.carpenter@...aro.org>,  Siddharth Vadapalli <s-vadapalli@...com>,
 Heiner Kallweit <hkallweit1@...il.com>, Vladimir Oltean
 <vladimir.oltean@....com>, Grygorii Strashko <grygorii.strashko@...com>, 
 Andrew Lunn <andrew@...n.ch>, Roger Quadros <rogerq@...nel.org>, Richard
 Cochran <richardcochran@...il.com>,  Jakub Kicinski <kuba@...nel.org>, Eric
 Dumazet <edumazet@...gle.com>, "David S. Miller" <davem@...emloft.net>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2 1/3] net: ethernet: ti: am65-cpts: Enable PTP RX HW
 timestamp using CPTS FIFO

On Tue, 2024-03-12 at 15:32 +0530, Chintan Vankar wrote:
> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
> index c66618d91c28..6c1d571c5e0b 100644
> --- a/drivers/net/ethernet/ti/am65-cpts.c
> +++ b/drivers/net/ethernet/ti/am65-cpts.c
> @@ -859,29 +859,6 @@ static long am65_cpts_ts_work(struct ptp_clock_info *ptp)
>  	return delay;
>  }
>  
> -/**
> - * am65_cpts_rx_enable - enable rx timestamping
> - * @cpts: cpts handle
> - * @en: enable
> - *
> - * This functions enables rx packets timestamping. The CPTS can timestamp all
> - * rx packets.
> - */
> -void am65_cpts_rx_enable(struct am65_cpts *cpts, bool en)
> -{
> -	u32 val;
> -
> -	mutex_lock(&cpts->ptp_clk_lock);
> -	val = am65_cpts_read32(cpts, control);
> -	if (en)
> -		val |= AM65_CPTS_CONTROL_TSTAMP_EN;
> -	else
> -		val &= ~AM65_CPTS_CONTROL_TSTAMP_EN;
> -	am65_cpts_write32(cpts, val, control);
> -	mutex_unlock(&cpts->ptp_clk_lock);
> -}
> -EXPORT_SYMBOL_GPL(am65_cpts_rx_enable);

It looks like the above chunk will cause a transient build break, as
the function is still in use and the caller will be removed by the next
patch. I guess you should move this chunk there.

> -
>  static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
>  {
>  	unsigned int ptp_class = ptp_classify_raw(skb);
> @@ -906,6 +883,72 @@ static int am65_skb_get_mtype_seqid(struct sk_buff *skb, u32 *mtype_seqid)
>  	return 1;
>  }
>  
> +static u64 am65_cpts_find_rx_ts(struct am65_cpts *cpts, struct sk_buff *skb,
> +				int ev_type, u32 skb_mtype_seqid)
> +{
> +	struct list_head *this, *next;
> +	struct am65_cpts_event *event;
> +	unsigned long flags;
> +	u32 mtype_seqid;
> +	u64 ns = 0;
> +
> +	am65_cpts_fifo_read(cpts);
> +	spin_lock_irqsave(&cpts->lock, flags);
> +	list_for_each_safe(this, next, &cpts->events) {
> +		event = list_entry(this, struct am65_cpts_event, list);
> +		if (time_after(jiffies, event->tmo)) {
> +			list_del_init(&event->list);
> +			list_add(&event->list, &cpts->pool);
> +			continue;
> +		}
> +
> +		mtype_seqid = event->event1 &
> +			      (AM65_CPTS_EVENT_1_MESSAGE_TYPE_MASK |
> +			       AM65_CPTS_EVENT_1_SEQUENCE_ID_MASK |
> +			       AM65_CPTS_EVENT_1_EVENT_TYPE_MASK);
> +
> +		if (mtype_seqid == skb_mtype_seqid) {
> +			ns = event->timestamp;
> +			list_del_init(&event->list);
> +			list_add(&event->list, &cpts->pool);
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&cpts->lock, flags);

Ouch, you have to acquire an additional lock per packet and a lot of
cacheline dithering.

Not strictly necessary for this series, but I suggest to invest some
time reconsidering this schema, it looks bad from performance pov.

Possibly protecting the list with RCU and leaving the recycle to the
producer could help.

Additionally net-next is currently closed for the merge window, please
post the new revision (including the target tree in the subj prefix)
when net-next will re-open in ~2weeks.

Cheers,

Paolo