Message ID | 20220518170756.7752-1-harini.katakam@xilinx.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 5cebb40bc9554aafcc492431181f43c6231b0459 |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net,v3] net: macb: Fix PTP one step sync support | expand |
On Wed, 18 May 2022 22:37:56 +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. Add a common mask for PTP header flag field "twoStepflag". > 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") > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com> > Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> Acked-by: Jakub Kicinski <kuba@kernel.org>
On 18.05.2022 20:07, Harini Katakam wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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. Add a common mask for PTP header flag field "twoStepflag". > 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") > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com> > Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > v3: > - Rebase on net branch > - Squash both commits on macb driver and ptp_classify.h > > v2: > - Separate fix for net branch > (Split from "Macb PTP updates" series > https://lore.kernel.org/netdev/20220517135525.GC3344@hoboy.vegasvil.org/T/) > - Fix include order > - ptp_oss -> ptp_one_step_sync > - Remove inline and add "likely" on SKB_HWTSTAMP check in ptp helper > - Dont split gem_ptp_do_tx_tstamp from if condition as order of > evaluation will take care of intent > - Remove redundant comments in macb_pad_and_fcs > - Add PTP flag to ptp_classify header for common use > (Dint add Richard's ACK as the patch changed and there's a minor addition > in ptp_classify.h as well) > > v1 Notes: > -> Added the macb pad and fcs fixes tag because strictly speaking the PTP support > patch precedes the fcs patch in timeline. > -> FYI, the error observed with setting HW TX timestamp for one step sync packets: > ptp4l[405.292]: port 1: unexpected socket error > > drivers/net/ethernet/cadence/macb_main.c | 40 +++++++++++++++++++++--- > drivers/net/ethernet/cadence/macb_ptp.c | 4 ++- > include/linux/ptp_classify.h | 3 ++ > 3 files changed, 42 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 61284baa0496..3a1b5ac48ca5 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -36,6 +36,7 @@ > #include <linux/iopoll.h> > #include <linux/phy/phy.h> > #include <linux/pm_runtime.h> > +#include <linux/ptp_classify.h> > #include <linux/reset.h> > #include "macb.h" > > @@ -1124,6 +1125,36 @@ static void macb_tx_error_task(struct work_struct *work) > spin_unlock_irqrestore(&bp->lock, flags); > } > > +static bool ptp_one_step_sync(struct sk_buff *skb) > +{ > + struct ptp_header *hdr; > + unsigned int ptp_class; > + u8 msgtype; > + > + /* No need to parse packet if PTP TS is not involved */ > + if (likely(!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))) > + goto not_oss; > + > + /* Identify and return whether PTP one step sync is being processed */ > + ptp_class = ptp_classify_raw(skb); > + if (ptp_class == PTP_CLASS_NONE) > + goto not_oss; > + > + hdr = ptp_parse_header(skb, ptp_class); > + if (!hdr) > + goto not_oss; > + > + if (hdr->flag_field[0] & PTP_FLAG_TWOSTEP) > + goto not_oss; > + > + msgtype = ptp_get_msgtype(hdr, ptp_class); > + if (msgtype == PTP_MSGTYPE_SYNC) > + return true; > + > +not_oss: > + return false; > +} > + > static void macb_tx_interrupt(struct macb_queue *queue) > { > unsigned int tail; > @@ -1168,8 +1199,8 @@ static void macb_tx_interrupt(struct macb_queue *queue) > > /* First, update TX stats if needed */ > if (skb) { > - if (unlikely(skb_shinfo(skb)->tx_flags & > - SKBTX_HW_TSTAMP) && > + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && > + !ptp_one_step_sync(skb) && > gem_ptp_do_txstamp(queue, skb, desc) == 0) { > /* skb now belongs to timestamp buffer > * and will be removed later > @@ -1999,7 +2030,8 @@ static unsigned int macb_tx_map(struct macb *bp, > ctrl |= MACB_BF(TX_LSO, lso_ctrl); > ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl); > if ((bp->dev->features & NETIF_F_HW_CSUM) && > - skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl) > + skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl && > + !ptp_one_step_sync(skb)) > ctrl |= MACB_BIT(TX_NOCRC); > } else > /* Only set MSS/MFS on payload descriptors > @@ -2097,7 +2129,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev) > > if (!(ndev->features & NETIF_F_HW_CSUM) || > !((*skb)->ip_summed != CHECKSUM_PARTIAL) || > - skb_shinfo(*skb)->gso_size) /* Not available for GSO */ > + skb_shinfo(*skb)->gso_size || ptp_one_step_sync(*skb)) > return 0; > > if (padlen <= 0) { > diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c > index fb6b27f46b15..9559c16078f9 100644 > --- a/drivers/net/ethernet/cadence/macb_ptp.c > +++ b/drivers/net/ethernet/cadence/macb_ptp.c > @@ -470,8 +470,10 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd) > case HWTSTAMP_TX_ONESTEP_SYNC: > if (gem_ptp_set_one_step_sync(bp, 1) != 0) > return -ERANGE; > - fallthrough; > + tx_bd_control = TSTAMP_ALL_FRAMES; > + break; > case HWTSTAMP_TX_ON: > + gem_ptp_set_one_step_sync(bp, 0); > tx_bd_control = TSTAMP_ALL_FRAMES; > break; > default: > diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h > index fefa7790dc46..2b6ea36ad162 100644 > --- a/include/linux/ptp_classify.h > +++ b/include/linux/ptp_classify.h > @@ -43,6 +43,9 @@ > #define OFF_PTP_SOURCE_UUID 22 /* PTPv1 only */ > #define OFF_PTP_SEQUENCE_ID 30 > > +/* PTP header flag fields */ > +#define PTP_FLAG_TWOSTEP BIT(1) > + > /* Below defines should actually be removed at some point in time. */ > #define IP6_HLEN 40 > #define UDP_HLEN 8 > -- > 2.17.1 >
On Wed, 2022-05-18 at 22:37 +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. Add a common mask for PTP header flag field "twoStepflag". > 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") > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com> > Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> I'm sorry, but I cut the -net PR to Linus too early for this, so the fix will have to wait for a little more (no need to repost!) and even more pause will be required for the net-next follow-up. Sorry for the inconvenince, Paolo
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Wed, 18 May 2022 22:37:56 +0530 you 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. Add a common mask for PTP header flag field "twoStepflag". > Also reset ptp OSS bit when one step is not selected. > > [...] Here is the summary with links: - [net,v3] net: macb: Fix PTP one step sync support https://git.kernel.org/netdev/net/c/5cebb40bc955 You are awesome, thank you!
> -----Original Message----- > From: Paolo Abeni <pabeni@redhat.com> > Sent: Thursday, May 19, 2022 11:48 PM > To: Harini Katakam <harinik@xilinx.com>; nicolas.ferre@microchip.com; > davem@davemloft.net; richardcochran@gmail.com; > claudiu.beznea@microchip.com; kuba@kernel.org; edumazet@google.com > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Michal Simek > <michals@xilinx.com>; harinikatakamlinux@gmail.com; Radhey Shyam > Pandey <radheys@xilinx.com> > Subject: Re: [PATCH net v3] net: macb: Fix PTP one step sync support > > On Wed, 2022-05-18 at 22:37 +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. Add a common mask for PTP header flag field "twoStepflag". > > 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") > > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com> > > Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com> > > I'm sorry, but I cut the -net PR to Linus too early for this, so the fix will have to > wait for a little more (no need to repost!) and even more pause will be > required for the net-next follow-up. > > Sorry for the inconvenince, Thanks Paolo. No problem, will wait and send net-next follow up. Regards, Harini
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 61284baa0496..3a1b5ac48ca5 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -36,6 +36,7 @@ #include <linux/iopoll.h> #include <linux/phy/phy.h> #include <linux/pm_runtime.h> +#include <linux/ptp_classify.h> #include <linux/reset.h> #include "macb.h" @@ -1124,6 +1125,36 @@ static void macb_tx_error_task(struct work_struct *work) spin_unlock_irqrestore(&bp->lock, flags); } +static bool ptp_one_step_sync(struct sk_buff *skb) +{ + struct ptp_header *hdr; + unsigned int ptp_class; + u8 msgtype; + + /* No need to parse packet if PTP TS is not involved */ + if (likely(!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))) + goto not_oss; + + /* Identify and return whether PTP one step sync is being processed */ + ptp_class = ptp_classify_raw(skb); + if (ptp_class == PTP_CLASS_NONE) + goto not_oss; + + hdr = ptp_parse_header(skb, ptp_class); + if (!hdr) + goto not_oss; + + if (hdr->flag_field[0] & PTP_FLAG_TWOSTEP) + goto not_oss; + + msgtype = ptp_get_msgtype(hdr, ptp_class); + if (msgtype == PTP_MSGTYPE_SYNC) + return true; + +not_oss: + return false; +} + static void macb_tx_interrupt(struct macb_queue *queue) { unsigned int tail; @@ -1168,8 +1199,8 @@ static void macb_tx_interrupt(struct macb_queue *queue) /* First, update TX stats if needed */ if (skb) { - if (unlikely(skb_shinfo(skb)->tx_flags & - SKBTX_HW_TSTAMP) && + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && + !ptp_one_step_sync(skb) && gem_ptp_do_txstamp(queue, skb, desc) == 0) { /* skb now belongs to timestamp buffer * and will be removed later @@ -1999,7 +2030,8 @@ static unsigned int macb_tx_map(struct macb *bp, ctrl |= MACB_BF(TX_LSO, lso_ctrl); ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl); if ((bp->dev->features & NETIF_F_HW_CSUM) && - skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl) + skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl && + !ptp_one_step_sync(skb)) ctrl |= MACB_BIT(TX_NOCRC); } else /* Only set MSS/MFS on payload descriptors @@ -2097,7 +2129,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev) if (!(ndev->features & NETIF_F_HW_CSUM) || !((*skb)->ip_summed != CHECKSUM_PARTIAL) || - skb_shinfo(*skb)->gso_size) /* Not available for GSO */ + skb_shinfo(*skb)->gso_size || ptp_one_step_sync(*skb)) return 0; if (padlen <= 0) { diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c index fb6b27f46b15..9559c16078f9 100644 --- a/drivers/net/ethernet/cadence/macb_ptp.c +++ b/drivers/net/ethernet/cadence/macb_ptp.c @@ -470,8 +470,10 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd) case HWTSTAMP_TX_ONESTEP_SYNC: if (gem_ptp_set_one_step_sync(bp, 1) != 0) return -ERANGE; - fallthrough; + tx_bd_control = TSTAMP_ALL_FRAMES; + break; case HWTSTAMP_TX_ON: + gem_ptp_set_one_step_sync(bp, 0); tx_bd_control = TSTAMP_ALL_FRAMES; break; default: diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h index fefa7790dc46..2b6ea36ad162 100644 --- a/include/linux/ptp_classify.h +++ b/include/linux/ptp_classify.h @@ -43,6 +43,9 @@ #define OFF_PTP_SOURCE_UUID 22 /* PTPv1 only */ #define OFF_PTP_SEQUENCE_ID 30 +/* PTP header flag fields */ +#define PTP_FLAG_TWOSTEP BIT(1) + /* Below defines should actually be removed at some point in time. */ #define IP6_HLEN 40 #define UDP_HLEN 8