| Message ID | 20210521043619.44694-8-yangbo.lu@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | ptp: support virtual clocks for multiple | expand |
hi Yangbo, On 21.05.2021 07:36, Yangbo Lu wrote: > Support timestamp conversion to specified PTP domain in PTP packet. > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> > --- > Changes for v2: > - Fixed build waring. > - Updated copyright. > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 39 ++++++++++++++++++-- > 1 file changed, 35 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 3ca93adb9662..cd0429c73999 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) > -/* Copyright 2017-2019 NXP */ > +/* Copyright 2017-2021 NXP */ > > #include "enetc.h" > #include <linux/bpf_trace.h> > @@ -7,6 +7,7 @@ > #include <linux/udp.h> > #include <linux/vmalloc.h> > #include <linux/ptp_classify.h> > +#include <linux/ptp_clock_kernel.h> > #include <net/pkt_sched.h> > > static int enetc_num_stack_tx_queues(struct enetc_ndev_priv *priv) > @@ -472,13 +473,36 @@ static void enetc_get_tx_tstamp(struct enetc_hw *hw, union enetc_tx_bd *txbd, > *tstamp = (u64)hi << 32 | tstamp_lo; > } > > -static void enetc_tstamp_tx(struct sk_buff *skb, u64 tstamp) > +static int enetc_ptp_parse_domain(struct sk_buff *skb, u8 *domain) > +{ > + unsigned int ptp_class; > + struct ptp_header *hdr; > + > + ptp_class = ptp_classify_raw(skb); > + if (ptp_class == PTP_CLASS_NONE) > + return -EINVAL; > + > + hdr = ptp_parse_header(skb, ptp_class); > + if (!hdr) > + return -EINVAL; > + > + *domain = hdr->domain_number; > + return 0; > +} > + Why is this function defined inside the enetc driver? I don't see anything enetc specific to it, but it looks like another generic ptp procedure, similar to ptp_parse_header() or ptp_clock_domain_tstamp().
On Fri, May 21, 2021 at 12:36:19PM +0800, Yangbo Lu wrote: > @@ -472,13 +473,36 @@ static void enetc_get_tx_tstamp(struct enetc_hw *hw, union enetc_tx_bd *txbd, > *tstamp = (u64)hi << 32 | tstamp_lo; > } > > -static void enetc_tstamp_tx(struct sk_buff *skb, u64 tstamp) > +static int enetc_ptp_parse_domain(struct sk_buff *skb, u8 *domain) > +{ > + unsigned int ptp_class; > + struct ptp_header *hdr; > + > + ptp_class = ptp_classify_raw(skb); > + if (ptp_class == PTP_CLASS_NONE) > + return -EINVAL; > + > + hdr = ptp_parse_header(skb, ptp_class); > + if (!hdr) > + return -EINVAL; > + > + *domain = hdr->domain_number; This is really clunky. We do NOT want to have drivers starting to handle the PTP. That is the job of the user space stack. Instead, the conversion from raw time stamp to vclock time stamp should happen in the core infrastructure. That way, no driver hacks will be needed, and it will "just work" everywhere. We need a way to associate a given socket with a particular vclock. Perhaps we can extend the SO_TIMESTAMPING API to allow that. > + return 0; > +} Thanks, Richard
On Tue, May 25, 2021 at 05:37:11AM -0700, Richard Cochran wrote: > Instead, the conversion from raw time stamp to vclock time stamp > should happen in the core infrastructure. That way, no driver hacks > will be needed, and it will "just work" everywhere. For transmit time stamps, we have skb_complete_tx_timestamp(). For receive, most drivers use the following cliche: shwt = skb_hwtstamps(skb); memset(shwt, 0, sizeof(*shwt)); shwt->hwtstamp = ns_to_ktime(ns); So the first step will be to introduce a helper function for that, and then re-factor the drivers to use the helper. Thanks, Richard
Hi Claudiu and Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: 2021年5月25日 20:37 > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; Claudiu > Manoil <claudiu.manoil@nxp.com>; Jakub Kicinski <kuba@kernel.org> > Subject: Re: [net-next, v2, 7/7] enetc: support PTP domain timestamp > conversion > > On Fri, May 21, 2021 at 12:36:19PM +0800, Yangbo Lu wrote: > > > @@ -472,13 +473,36 @@ static void enetc_get_tx_tstamp(struct enetc_hw > *hw, union enetc_tx_bd *txbd, > > *tstamp = (u64)hi << 32 | tstamp_lo; } > > > > -static void enetc_tstamp_tx(struct sk_buff *skb, u64 tstamp) > > +static int enetc_ptp_parse_domain(struct sk_buff *skb, u8 *domain) { > > + unsigned int ptp_class; > > + struct ptp_header *hdr; > > + > > + ptp_class = ptp_classify_raw(skb); > > + if (ptp_class == PTP_CLASS_NONE) > > + return -EINVAL; > > + > > + hdr = ptp_parse_header(skb, ptp_class); > > + if (!hdr) > > + return -EINVAL; > > + > > + *domain = hdr->domain_number; > > This is really clunky. We do NOT want to have drivers starting to handle the > PTP. That is the job of the user space stack. > > Instead, the conversion from raw time stamp to vclock time stamp should > happen in the core infrastructure. That way, no driver hacks will be needed, > and it will "just work" everywhere. That's perfect way. > > We need a way to associate a given socket with a particular vclock. > Perhaps we can extend the SO_TIMESTAMPING API to allow that. How about adding a flag SOF_TIMESTAMPING_BIND_PHC, and redefining the data passing by setsockopt like, struct timestamping { int flags; u8 hwtstamp_phc; /*phc index */ }; The sock could have a new member sk_hwtstamp_phc to record it. > > > + return 0; > > +} > > Thanks, > Richard
> -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: 2021年5月25日 20:49 > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; Claudiu > Manoil <claudiu.manoil@nxp.com>; Jakub Kicinski <kuba@kernel.org> > Subject: Re: [net-next, v2, 7/7] enetc: support PTP domain timestamp > conversion > > On Tue, May 25, 2021 at 05:37:11AM -0700, Richard Cochran wrote: > > Instead, the conversion from raw time stamp to vclock time stamp > > should happen in the core infrastructure. That way, no driver hacks > > will be needed, and it will "just work" everywhere. > > For transmit time stamps, we have skb_complete_tx_timestamp(). > > For receive, most drivers use the following cliche: > > shwt = skb_hwtstamps(skb); > memset(shwt, 0, sizeof(*shwt)); > shwt->hwtstamp = ns_to_ktime(ns); > > So the first step will be to introduce a helper function for that, and then > re-factor the drivers to use the helper. So, the timestamp conversion could be in skbuff.c. That's good to do this. But there are quite a lot of drivers using timestamping. Should we convert all drivers to use the helper, or let others do this when they need? Thanks. > > Thanks, > Richard > >
> -----Original Message----- > From: Y.b. Lu <yangbo.lu@nxp.com> > Sent: 2021年5月31日 18:52 > To: Richard Cochran <richardcochran@gmail.com>; Claudiu Manoil > <claudiu.manoil@nxp.com> > Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; Jakub > Kicinski <kuba@kernel.org> > Subject: RE: [net-next, v2, 7/7] enetc: support PTP domain timestamp > conversion > > Hi Claudiu and Richard, > > > -----Original Message----- > > From: Richard Cochran <richardcochran@gmail.com> > > Sent: 2021年5月25日 20:37 > > To: Y.b. Lu <yangbo.lu@nxp.com> > > Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; > > Claudiu Manoil <claudiu.manoil@nxp.com>; Jakub Kicinski > > <kuba@kernel.org> > > Subject: Re: [net-next, v2, 7/7] enetc: support PTP domain timestamp > > conversion > > > > On Fri, May 21, 2021 at 12:36:19PM +0800, Yangbo Lu wrote: > > > > > @@ -472,13 +473,36 @@ static void enetc_get_tx_tstamp(struct > > > enetc_hw > > *hw, union enetc_tx_bd *txbd, > > > *tstamp = (u64)hi << 32 | tstamp_lo; } > > > > > > -static void enetc_tstamp_tx(struct sk_buff *skb, u64 tstamp) > > > +static int enetc_ptp_parse_domain(struct sk_buff *skb, u8 *domain) { > > > + unsigned int ptp_class; > > > + struct ptp_header *hdr; > > > + > > > + ptp_class = ptp_classify_raw(skb); > > > + if (ptp_class == PTP_CLASS_NONE) > > > + return -EINVAL; > > > + > > > + hdr = ptp_parse_header(skb, ptp_class); > > > + if (!hdr) > > > + return -EINVAL; > > > + > > > + *domain = hdr->domain_number; > > > > This is really clunky. We do NOT want to have drivers starting to > > handle the PTP. That is the job of the user space stack. > > > > Instead, the conversion from raw time stamp to vclock time stamp > > should happen in the core infrastructure. That way, no driver hacks > > will be needed, and it will "just work" everywhere. > > That's perfect way. > > > > > We need a way to associate a given socket with a particular vclock. > > Perhaps we can extend the SO_TIMESTAMPING API to allow that. > > How about adding a flag SOF_TIMESTAMPING_BIND_PHC, and redefining the > data passing by setsockopt like, > > struct timestamping { > int flags; > u8 hwtstamp_phc; /*phc index */ > }; > > The sock could have a new member sk_hwtstamp_phc to record it. But one problem is how to check the phc availability for current network interface. If user can make sure it's using right phc device for current network interface, that's no problem. Otherwise, timestamp will be token on wrong phc... > > > > > > + return 0; > > > +} > > > > Thanks, > > Richard
On Mon, May 31, 2021 at 11:26:18AM +0000, Y.b. Lu wrote: > So, the timestamp conversion could be in skbuff.c. > That's good to do this. But there are quite a lot of drivers using timestamping. > Should we convert all drivers to use the helper, or let others do this when they need? I think we should convert them all. Yes, it is work, but I will help. I really like the vclock idea, especially because it will work with every clock. Also, adding the helper will be a nice refactoring all by itself. Thanks, Richard
> But one problem is how to check the phc availability for current network interface. > If user can make sure it's using right phc device for current network interface, that's no problem. > Otherwise, timestamp will be token on wrong phc... Right, when user space requests the option, the kernel needs to check: sock -> dev -> physical phc_index -> vclocks. Today we have ethtool get_ts_info. We could add get_vclock_info for example. Thanks, Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: 2021年5月31日 21:49 > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; Claudiu > Manoil <claudiu.manoil@nxp.com>; Jakub Kicinski <kuba@kernel.org> > Subject: Re: [net-next, v2, 7/7] enetc: support PTP domain timestamp > conversion > > On Mon, May 31, 2021 at 11:26:18AM +0000, Y.b. Lu wrote: > > So, the timestamp conversion could be in skbuff.c. > > That's good to do this. But there are quite a lot of drivers using > timestamping. > > Should we convert all drivers to use the helper, or let others do this when > they need? > > I think we should convert them all. Yes, it is work, but I will help. > I really like the vclock idea, especially because it will work with every clock. > Also, adding the helper will be a nice refactoring all by itself. It seems socket.c __sock_recv_timestamp may be the better place. HW timestamps are handled in it before going to user space. I sent out a v3 patch for your reviewing. Maybe we don’t have to convert all drivers to a helper. Thank you very much. > > Thanks, > Richard
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 3ca93adb9662..cd0429c73999 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) -/* Copyright 2017-2019 NXP */ +/* Copyright 2017-2021 NXP */ #include "enetc.h" #include <linux/bpf_trace.h> @@ -7,6 +7,7 @@ #include <linux/udp.h> #include <linux/vmalloc.h> #include <linux/ptp_classify.h> +#include <linux/ptp_clock_kernel.h> #include <net/pkt_sched.h> static int enetc_num_stack_tx_queues(struct enetc_ndev_priv *priv) @@ -472,13 +473,36 @@ static void enetc_get_tx_tstamp(struct enetc_hw *hw, union enetc_tx_bd *txbd, *tstamp = (u64)hi << 32 | tstamp_lo; } -static void enetc_tstamp_tx(struct sk_buff *skb, u64 tstamp) +static int enetc_ptp_parse_domain(struct sk_buff *skb, u8 *domain) +{ + unsigned int ptp_class; + struct ptp_header *hdr; + + ptp_class = ptp_classify_raw(skb); + if (ptp_class == PTP_CLASS_NONE) + return -EINVAL; + + hdr = ptp_parse_header(skb, ptp_class); + if (!hdr) + return -EINVAL; + + *domain = hdr->domain_number; + return 0; +} + +static void enetc_tstamp_tx(struct enetc_ndev_priv *priv, struct sk_buff *skb, + u64 tstamp) { struct skb_shared_hwtstamps shhwtstamps; + u64 ts = tstamp; + u8 domain; if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) { + if (!enetc_ptp_parse_domain(skb, &domain)) + ptp_clock_domain_tstamp(priv->ptp_dev, &ts, domain); + memset(&shhwtstamps, 0, sizeof(shhwtstamps)); - shhwtstamps.hwtstamp = ns_to_ktime(tstamp); + shhwtstamps.hwtstamp = ns_to_ktime(ts); skb_txtime_consumed(skb); skb_tstamp_tx(skb, &shhwtstamps); } @@ -575,7 +599,7 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget) */ schedule_work(&priv->tx_onestep_tstamp); } else if (unlikely(do_twostep_tstamp)) { - enetc_tstamp_tx(skb, tstamp); + enetc_tstamp_tx(priv, skb, tstamp); do_twostep_tstamp = false; } napi_consume_skb(skb, napi_budget); @@ -698,6 +722,7 @@ static void enetc_get_rx_tstamp(struct net_device *ndev, struct enetc_hw *hw = &priv->si->hw; u32 lo, hi, tstamp_lo; u64 tstamp; + u8 domain; if (le16_to_cpu(rxbd->r.flags) & ENETC_RXBD_FLAG_TSTMP) { lo = enetc_rd_reg_hot(hw->reg + ENETC_SICTR0); @@ -708,6 +733,12 @@ static void enetc_get_rx_tstamp(struct net_device *ndev, hi -= 1; tstamp = (u64)hi << 32 | tstamp_lo; + + skb_reset_mac_header(skb); + + if (!enetc_ptp_parse_domain(skb, &domain)) + ptp_clock_domain_tstamp(priv->ptp_dev, &tstamp, domain); + memset(shhwtstamps, 0, sizeof(*shhwtstamps)); shhwtstamps->hwtstamp = ns_to_ktime(tstamp); }
Support timestamp conversion to specified PTP domain in PTP packet. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> --- Changes for v2: - Fixed build waring. - Updated copyright. --- drivers/net/ethernet/freescale/enetc/enetc.c | 39 ++++++++++++++++++-- 1 file changed, 35 insertions(+), 4 deletions(-)