Re: [Patch net-next v2 07/13] net: dsa: microchip: ptp: add packet reception timestamping

[<Arun.Ramadoss@microchip.com>]

Hi Vladimir,
Thanks for the review comment.
On Tue, 2022-12-06 at 14:53 +0200, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Dec 06, 2022 at 02:44:22PM +0530, Arun Ramadoss wrote:
> > +static void ksz_rcv_timestamp(struct sk_buff *skb, u8 *tag,
> > +                           struct net_device *dev, unsigned int
> > port)
> > +{
> > +     struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
> > +     struct dsa_switch *ds = dev->dsa_ptr->ds;
> > +     u8 *tstamp_raw = tag - KSZ_PTP_TAG_LEN;
> > +     struct ksz_tagger_data *tagger_data;
> > +     struct ptp_header *ptp_hdr;
> > +     unsigned int ptp_type;
> > +     u8 ptp_msg_type;
> > +     ktime_t tstamp;
> > +     s64 correction;
> > +
> > +     tagger_data = ksz_tagger_data(ds);
> > +     if (!tagger_data->meta_tstamp_handler)
> > +             return;
> 
> The meta_tstamp_handler doesn't seem to be needed.
> 
> Just save the partial timestamp in KSZ_SKB_CB(), and reconstruct that
> timestamp with the full PTP time in the ds->ops->port_rxtstamp()
> method.
> 
> Biggest advantage is that ptp_classify_raw() won't be called twice in
> the RX path for the same packet, as will currently happen with your
> code.
> 

I looked into the sja1105 and hellcreek rxtstamp() implementation.
Here, SKB is queued in rxtstamp() and ptp_schedule_worker is started.
In the work queue, skb is dequeued and current ptp hardware clock is
read. Using the partial time stamp and phc clock, absolute time stamp
is calculated and posted.
In this KSZ implementation, ptp_schedule_worker is used for maintaining
the ptp software clock which read value from hardware clock every
second for faster access of clock value.

Based on the above observation, I have doubt on how to implement. Below
are the algorithm. Kindly suggest which one to proceed.
1. Remove the existing ptp software clock mainpulation using
ptp_schedule_worker. Instead in the ptp_schedule_worker, dequeue the
skb and timestamp the rx packets by directly reading from the ptp
hardware clock.
2. Keep the existing implementation, add the rxtstamp() where it will
not queue skb instead just process the timestamping with using software
clock and KSZ_SKB_CB()->tstamp.


Thanks
Arun