Messages in this thread | | | From | Harini Katakam <> | Date | Wed, 18 May 2022 09:53:29 +0530 | Subject | Re: [PATCH 1/3] net: macb: Fix PTP one step sync support |
| |
Hi Jakub,
On Wed, May 18, 2022 at 8:12 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 17 May 2022 13:02:57 +0530 Harini Katakam wrote: > > PTP one step sync packets cannot have CSUM padding and insertion in > > SW since time stamp is inserted on the fly by HW. > > In addition, ptp4l version 3.0 and above report an error when skb > > timestamps are reported for packets that not processed for TX TS > > after transmission. > > Add a helper to identify PTP one step sync and fix the above two > > errors. > > Also reset ptp OSS bit when one step is not selected. > > > > Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support") > > Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation") > > Please make sure to CC authors of the patches under fixes. > ./scripts/get_maintainer should point them out.
Thanks for the review. Rafal Ozieblo <rafalo@cadence.com> is the author of the first Fixes patch but that address hasn't worked in the last ~4 yrs. I have cced Claudiu and everyone else from the maintainers (Eric Dumazet <edumazet@google.com> also doesn't work)
<snip> > > +/* IEEE1588 PTP flag field values */ > > +#define PTP_FLAG_TWOSTEP 0x2 > > Shouldn't this go into the PTP header?
Let me add it to ptp_classify where the relevant helpers are present.
<snip> > > +static inline bool ptp_oss(struct sk_buff *skb) > > Please spell out then name more oss == open source software.
Will change to ptp_one_step_sync
> > No inline here, please, let the compiler decide if the function is > small enough. One step timestamp may be a rare use case so inlining > this twice is not necessarily the right choice.
One step is a rare case but the check happens on every PTP packet in the transmit data path and hence I wanted to explicitly inline it.
<snip> > > @@ -1158,13 +1192,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget) > > > > /* First, update TX stats if needed */ > > if (skb) { > > - if (unlikely(skb_shinfo(skb)->tx_flags & > > - SKBTX_HW_TSTAMP) && > > - gem_ptp_do_txstamp(queue, skb, desc) == 0) { > > - /* skb now belongs to timestamp buffer > > - * and will be removed later > > - */ > > - tx_skb->skb = NULL; > > + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && > > ptp_oss already checks if HW_TSTAMP is set.
The check for SKBTX_HW_TSTAMP is required here universally and not just inside ptp_oss. I will remove the redundant check in ptp_oss instead. Please see the reply below.
> > > + !ptp_oss(skb)) { > > + if (gem_ptp_do_txstamp(queue, skb, desc) == 0) { > > Why convert the gem_ptp_do_txstamp check from a && in the condition to > a separate if?
The intention is that ptp_oss should only be evaluated when SKBTX_HW_TSTAMP is set and gem_ptp_do_txstamp should only be called if ptp_oss is false. Since compiler follows the order of evaluation, I'll simplify this to:
if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && !ptp_oss(skb) && gem_ptp_do_txstamp(queue, skb, desc) == 0) { ... }
Regards, Harini
| |