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

[Chintan Vankar]

On 12/03/24 16:20, Paolo Abeni wrote:
> 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.

Thank you for your suggestion. I will update the patch with this
change in the next version.

> > -
> >   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.

Thank you for pointing out this. I will consider your point and spend
some time on it.

> 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.
Thank you for information. I will update the patch and post its next
version with above mentioned changes.

> Cheers,
>
> Paolo