| Message ID | 20220322210722.6405-6-gerhard@engleder-embedded.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp: Support hardware clocks with additional free running time | expand |
Hi Gerhard,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a0cb83ba6e0cd73a50fa4f84736846bf0029f2b
config: microblaze-randconfig-r022-20220321 (https://download.01.org/0day-ci/archive/20220323/202203230801.KMxkRkMZ-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/754e870cb9699166113d6ea383e48b0207165c1a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
git checkout 754e870cb9699166113d6ea383e48b0207165c1a
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/spi/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/spi/spi.c:36:
>> include/linux/ptp_clock_kernel.h:418:1: error: expected identifier or '(' before '{' token
418 | { return 0; }
| ^
include/linux/ptp_clock_kernel.h:415:23: warning: 'ptp_get_timestamp' declared 'static' but never defined [-Wunused-function]
415 | static inline ktime_t ptp_get_timestamp(int index,
| ^~~~~~~~~~~~~~~~~
vim +418 include/linux/ptp_clock_kernel.h
404
405 /**
406 * ptp_convert_timestamp() - convert timestamp to a ptp vclock time
407 *
408 * @hwtstamp: timestamp
409 * @vclock_index: phc index of ptp vclock.
410 *
411 * Returns converted timestamp, or 0 on error.
412 */
413 ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
414 #else
415 static inline ktime_t ptp_get_timestamp(int index,
416 const struct skb_shared_hwtstamps *hwtstamps,
417 bool cycles);
> 418 { return 0; }
419 static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
420 { return 0; }
421 static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
422 int vclock_index)
423 { return 0; }
424
Hi Gerhard,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a0cb83ba6e0cd73a50fa4f84736846bf0029f2b
config: hexagon-buildonly-randconfig-r004-20220320 (https://download.01.org/0day-ci/archive/20220323/202203230811.oaI4Q7IP-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 902f4708fe1d03b0de7e5315ef875006a6adc319)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/754e870cb9699166113d6ea383e48b0207165c1a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
git checkout 754e870cb9699166113d6ea383e48b0207165c1a
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/spi/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/spi/spi.c:36:
>> include/linux/ptp_clock_kernel.h:418:1: error: expected identifier or '('
{ return 0; }
^
1 error generated.
vim +418 include/linux/ptp_clock_kernel.h
404
405 /**
406 * ptp_convert_timestamp() - convert timestamp to a ptp vclock time
407 *
408 * @hwtstamp: timestamp
409 * @vclock_index: phc index of ptp vclock.
410 *
411 * Returns converted timestamp, or 0 on error.
412 */
413 ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
414 #else
415 static inline ktime_t ptp_get_timestamp(int index,
416 const struct skb_shared_hwtstamps *hwtstamps,
417 bool cycles);
> 418 { return 0; }
419 static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
420 { return 0; }
421 static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
422 int vclock_index)
423 { return 0; }
424
Hi Gerhard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 4a0cb83ba6e0cd73a50fa4f84736846bf0029f2b
config: nios2-randconfig-r023-20220321 (https://download.01.org/0day-ci/archive/20220323/202203231015.YRlUe3av-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/754e870cb9699166113d6ea383e48b0207165c1a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gerhard-Engleder/ptp-Support-hardware-clocks-with-additional-free-running-time/20220323-051003
git checkout 754e870cb9699166113d6ea383e48b0207165c1a
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nios2 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from net/socket.c:108:
include/linux/ptp_clock_kernel.h:418:1: error: expected identifier or '(' before '{' token
418 | { return 0; }
| ^
net/socket.c: In function '__sys_getsockopt':
net/socket.c:2227:13: warning: variable 'max_optlen' set but not used [-Wunused-but-set-variable]
2227 | int max_optlen;
| ^~~~~~~~~~
In file included from net/socket.c:108:
net/socket.c: At top level:
>> include/linux/ptp_clock_kernel.h:415:23: warning: 'ptp_get_timestamp' used but never defined
415 | static inline ktime_t ptp_get_timestamp(int index,
| ^~~~~~~~~~~~~~~~~
vim +/ptp_get_timestamp +415 include/linux/ptp_clock_kernel.h
404
405 /**
406 * ptp_convert_timestamp() - convert timestamp to a ptp vclock time
407 *
408 * @hwtstamp: timestamp
409 * @vclock_index: phc index of ptp vclock.
410 *
411 * Returns converted timestamp, or 0 on error.
412 */
413 ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index);
414 #else
> 415 static inline ktime_t ptp_get_timestamp(int index,
416 const struct skb_shared_hwtstamps *hwtstamps,
417 bool cycles);
418 { return 0; }
419 static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index)
420 { return 0; }
421 static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp,
422 int vclock_index)
423 { return 0; }
424
On Tue, Mar 22, 2022 at 10:07:21PM +0100, Gerhard Engleder wrote: > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > index 54b9f54ac0b2..b7a8cf27c349 100644 > --- a/drivers/ptp/ptp_clock.c > +++ b/drivers/ptp/ptp_clock.c > @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp) > } > EXPORT_SYMBOL(ptp_cancel_worker_sync); > > +ktime_t ptp_get_timestamp(int index, > + const struct skb_shared_hwtstamps *hwtstamps, > + bool cycles) > +{ > + char name[PTP_CLOCK_NAME_LEN] = ""; > + struct ptp_clock *ptp; > + struct device *dev; > + ktime_t ts; > + > + snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index); > + dev = class_find_device_by_name(ptp_class, name); This seems expensive for every single Rx frame in a busy PTP network. Can't this be cached in the socket? > + if (!dev) > + return 0; > + > + ptp = dev_get_drvdata(dev); > + > + if (ptp->info->gettstamp) > + ts = ptp->info->gettstamp(ptp->info, hwtstamps, cycles); > + else > + ts = hwtstamps->hwtstamp; > + > + put_device(dev); > + > + return ts; > +} > +EXPORT_SYMBOL(ptp_get_timestamp); > + > /* module operations */ > > static void __exit ptp_exit(void) > diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h > index cc6a7b2e267d..f4f0d8a880c6 100644 > --- a/include/linux/ptp_clock_kernel.h > +++ b/include/linux/ptp_clock_kernel.h > @@ -133,6 +133,16 @@ struct ptp_system_timestamp { > * parameter cts: Contains timestamp (device,system) pair, > * where system time is realtime and monotonic. > * > + * @gettstamp: Get hardware timestamp based on normal/adjustable time or free > + * running time. If @getcycles64 or @getcyclesx64 are supported, > + * then this method is required to provide timestamps based on the > + * free running time. This method will be called if > + * SKBTX_HW_TSTAMP_PHC is set by the driver. > + * parameter hwtstamps: skb_shared_hwtstamps structure pointer. > + * parameter cycles: If false, then hardware timestamp based on > + * normal/adjustable time is requested. If true, then hardware > + * timestamp based on free running time is requested. > + * > * @enable: Request driver to enable or disable an ancillary feature. > * parameter request: Desired resource to enable or disable. > * parameter on: Caller passes one to enable or zero to disable. > @@ -185,6 +195,9 @@ struct ptp_clock_info { > struct ptp_system_timestamp *sts); > int (*getcrosscycles)(struct ptp_clock_info *ptp, > struct system_device_crosststamp *cts); > + ktime_t (*gettstamp)(struct ptp_clock_info *ptp, > + const struct skb_shared_hwtstamps *hwtstamps, > + bool cycles); > int (*enable)(struct ptp_clock_info *ptp, > struct ptp_clock_request *request, int on); > int (*verify)(struct ptp_clock_info *ptp, unsigned int pin, > @@ -364,6 +377,19 @@ static inline void ptp_cancel_worker_sync(struct ptp_clock *ptp) > * a loadable module. > */ > > +/** > + * ptp_get_timestamp() - get timestamp of ptp clock > + * > + * @index: phc index of ptp pclock. > + * @hwtstamps: skb_shared_hwtstamps structure pointer. > + * @cycles: true for timestamp based on cycles. > + * > + * Returns timestamp, or 0 on error. > + */ > +ktime_t ptp_get_timestamp(int index, > + const struct skb_shared_hwtstamps *hwtstamps, > + bool cycles); > + > /** > * ptp_get_vclocks_index() - get all vclocks index on pclock, and > * caller is responsible to free memory > @@ -386,6 +412,10 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index); > */ > ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index); > #else > +static inline ktime_t ptp_get_timestamp(int index, > + const struct skb_shared_hwtstamps *hwtstamps, > + bool cycles); > +{ return 0; } > static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index) > { return 0; } > static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index f494ddbfc826..38929c113953 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -564,7 +564,10 @@ static inline bool skb_frag_must_loop(struct page *p) > * &skb_shared_info. Use skb_hwtstamps() to get a pointer. > */ > struct skb_shared_hwtstamps { > - ktime_t hwtstamp; > + union { > + ktime_t hwtstamp; > + void *phc_data; needs kdoc update > + }; > }; > > /* Definitions for tx_flags in struct skb_shared_info */ > @@ -581,6 +584,9 @@ enum { > /* generate hardware time stamp based on cycles if supported */ > SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3, > > + /* call PHC to get actual hardware time stamp */ > + SKBTX_HW_TSTAMP_PHC = 1 << 3, > + > /* generate wifi status information (where possible) */ > SKBTX_WIFI_STATUS = 1 << 4, > > diff --git a/net/socket.c b/net/socket.c > index 2e932c058002..fe765d559086 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -804,21 +804,17 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp) > return skb->tstamp && !false_tstamp && skb_is_err_queue(skb); > } > > -static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb) > +static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb, > + int if_index) > { > struct scm_ts_pktinfo ts_pktinfo; > - struct net_device *orig_dev; > > if (!skb_mac_header_was_set(skb)) > return; > > memset(&ts_pktinfo, 0, sizeof(ts_pktinfo)); > > - rcu_read_lock(); > - orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > - if (orig_dev) > - ts_pktinfo.if_index = orig_dev->ifindex; > - rcu_read_unlock(); > + ts_pktinfo.if_index = if_index; > > ts_pktinfo.pkt_length = skb->len - skb_mac_offset(skb); > put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO, > @@ -838,6 +834,9 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > int empty = 1, false_tstamp = 0; > struct skb_shared_hwtstamps *shhwtstamps = > skb_hwtstamps(skb); > + struct net_device *orig_dev; > + int if_index = 0; > + int phc_index = -1; > ktime_t hwtstamp; > > /* Race occurred between timestamp enabling and packet > @@ -886,18 +885,32 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > if (shhwtstamps && > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > !skb_is_swtx_tstamp(skb, false_tstamp)) { > - if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) > - hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp, > - sk->sk_bind_phc); > + rcu_read_lock(); > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > + if (orig_dev) { > + if_index = orig_dev->ifindex; > + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) > + phc_index = ethtool_get_phc(orig_dev); again, this is something that can be cached, no? > + } > + rcu_read_unlock(); > + > + if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) && > + (phc_index != -1)) > + hwtstamp = ptp_get_timestamp(phc_index, shhwtstamps, > + sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC); > else > hwtstamp = shhwtstamps->hwtstamp; > > + if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) > + hwtstamp = ptp_convert_timestamp(&hwtstamp, > + sk->sk_bind_phc); > + > if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) { > empty = 0; > > if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) && > !skb_is_err_queue(skb)) > - put_ts_pktinfo(msg, skb); > + put_ts_pktinfo(msg, skb, if_index); > } > } > if (!empty) { > -- > 2.20.1 > Thanks, Richard
> > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > > index 54b9f54ac0b2..b7a8cf27c349 100644 > > --- a/drivers/ptp/ptp_clock.c > > +++ b/drivers/ptp/ptp_clock.c > > @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp) > > } > > EXPORT_SYMBOL(ptp_cancel_worker_sync); > > > > +ktime_t ptp_get_timestamp(int index, > > + const struct skb_shared_hwtstamps *hwtstamps, > > + bool cycles) > > +{ > > + char name[PTP_CLOCK_NAME_LEN] = ""; > > + struct ptp_clock *ptp; > > + struct device *dev; > > + ktime_t ts; > > + > > + snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index); > > + dev = class_find_device_by_name(ptp_class, name); > > This seems expensive for every single Rx frame in a busy PTP network. > Can't this be cached in the socket? I thought that PTP packages are rare and that bloating the socket is not welcome. I'll try to implement some caching. > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -564,7 +564,10 @@ static inline bool skb_frag_must_loop(struct page *p) > > * &skb_shared_info. Use skb_hwtstamps() to get a pointer. > > */ > > struct skb_shared_hwtstamps { > > - ktime_t hwtstamp; > > + union { > > + ktime_t hwtstamp; > > + void *phc_data; > > needs kdoc update Sorry, I totally forgot that. I'll fix it. > > @@ -886,18 +885,32 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > > if (shhwtstamps && > > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > > !skb_is_swtx_tstamp(skb, false_tstamp)) { > > - if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) > > - hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp, > > - sk->sk_bind_phc); > > + rcu_read_lock(); > > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > > + if (orig_dev) { > > + if_index = orig_dev->ifindex; > > + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) > > + phc_index = ethtool_get_phc(orig_dev); > > again, this is something that can be cached, no? I'll try to implement some caching. Thanks for your review! Gerhard
On Thu, Mar 24, 2022 at 08:52:18PM +0100, Gerhard Engleder wrote: > I thought that PTP packages are rare and that bloating the socket is > not welcome. Some PTP profiles use insanely high frame rates. like G.8275.1 with Sync and Delay Req at 16/sec each. times the number of clients. Bloating the skbuff is bad, but the `sock` is not so critical in its storage costs. Thanks, Richard
On Thu, Mar 24, 2022 at 05:04:32PM -0700, Richard Cochran wrote: > On Thu, Mar 24, 2022 at 08:52:18PM +0100, Gerhard Engleder wrote: > > > I thought that PTP packages are rare and that bloating the socket is > > not welcome. > > Some PTP profiles use insanely high frame rates. like G.8275.1 with > Sync and Delay Req at 16/sec each. times the number of clients. times the number of vclocks/Domains.
> > > > > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > > > > > index 54b9f54ac0b2..b7a8cf27c349 100644 > > > > > --- a/drivers/ptp/ptp_clock.c > > > > > +++ b/drivers/ptp/ptp_clock.c > > > > > @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp) > > > > > } > > > > > EXPORT_SYMBOL(ptp_cancel_worker_sync); > > > > > > > > > > +ktime_t ptp_get_timestamp(int index, > > > > > + const struct skb_shared_hwtstamps *hwtstamps, > > > > > + bool cycles) > > > > > +{ > > > > > + char name[PTP_CLOCK_NAME_LEN] = ""; > > > > > + struct ptp_clock *ptp; > > > > > + struct device *dev; > > > > > + ktime_t ts; > > > > > + > > > > > + snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index); > > > > > + dev = class_find_device_by_name(ptp_class, name); > > > > > > > > This seems expensive for every single Rx frame in a busy PTP network. > > > > Can't this be cached in the socket? > > > I thought that PTP packages are rare and that bloating the socket is > > > not welcome. > > > > Some PTP profiles use insanely high frame rates. like G.8275.1 with > > Sync and Delay Req at 16/sec each. times the number of clients. > > times the number of vclocks/Domains. Getting the physical clock in __sock_recv_timestamp() is the expensive path. The network device is already available __sock_recv_timestamp(). Timestamp determination based on address/cookie could be done by the network device instead of the physical clock. In my opinion, that would be a good fit, because timestamp generation is already a task of the network device and implementation would be faster/simpler. What do you think? Gerhard
Gerhard Engleder <gerhard@engleder-embedded.com> writes: > If a physical clock supports a free running time called cycles, then > timestamps shall be based on this time too. For TX it is known in > advance before the transmission if a timestamp based on cycles is > needed. For RX it is impossible to know which timestamp is needed before > the packet is received and assigned to a socket. > > Support late timestamp determination by a physical clock. Therefore, an > address/cookie is stored within the new phc_data field of struct > skb_shared_hwtstamps. This address/cookie is provided to a new physical > clock method called gettstamp(), which returns a timestamp based on the > normal/adjustable time or based on cycles. > > The previously introduced flag SKBTX_HW_TSTAMP_USE_CYCLES is reused with > an additional alias to request the late timestamp determination by the > physical clock. That is possible, because SKBTX_HW_TSTAMP_USE_CYCLES is > not used in the receive path. > > Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> > --- > drivers/ptp/ptp_clock.c | 27 ++++++++++++++++++++++++ > include/linux/ptp_clock_kernel.h | 30 +++++++++++++++++++++++++++ > include/linux/skbuff.h | 8 +++++++- > net/socket.c | 35 ++++++++++++++++++++++---------- > 4 files changed, 88 insertions(+), 12 deletions(-) > > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > index 54b9f54ac0b2..b7a8cf27c349 100644 > --- a/drivers/ptp/ptp_clock.c > +++ b/drivers/ptp/ptp_clock.c > @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp) > } > EXPORT_SYMBOL(ptp_cancel_worker_sync); > > +ktime_t ptp_get_timestamp(int index, > + const struct skb_shared_hwtstamps *hwtstamps, > + bool cycles) > +{ > + char name[PTP_CLOCK_NAME_LEN] = ""; > + struct ptp_clock *ptp; > + struct device *dev; > + ktime_t ts; > + > + snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index); > + dev = class_find_device_by_name(ptp_class, name); > + if (!dev) > + return 0; > + > + ptp = dev_get_drvdata(dev); > + > + if (ptp->info->gettstamp) > + ts = ptp->info->gettstamp(ptp->info, hwtstamps, cycles); > + else > + ts = hwtstamps->hwtstamp; > + > + put_device(dev); > + > + return ts; > +} > +EXPORT_SYMBOL(ptp_get_timestamp); > + > /* module operations */ > > static void __exit ptp_exit(void) > diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h > index cc6a7b2e267d..f4f0d8a880c6 100644 > --- a/include/linux/ptp_clock_kernel.h > +++ b/include/linux/ptp_clock_kernel.h > @@ -133,6 +133,16 @@ struct ptp_system_timestamp { > * parameter cts: Contains timestamp (device,system) pair, > * where system time is realtime and monotonic. > * > + * @gettstamp: Get hardware timestamp based on normal/adjustable time or free > + * running time. If @getcycles64 or @getcyclesx64 are supported, > + * then this method is required to provide timestamps based on the > + * free running time. This method will be called if > + * SKBTX_HW_TSTAMP_PHC is set by the driver. > + * parameter hwtstamps: skb_shared_hwtstamps structure pointer. > + * parameter cycles: If false, then hardware timestamp based on > + * normal/adjustable time is requested. If true, then hardware > + * timestamp based on free running time is requested. > + * > * @enable: Request driver to enable or disable an ancillary feature. > * parameter request: Desired resource to enable or disable. > * parameter on: Caller passes one to enable or zero to disable. > @@ -185,6 +195,9 @@ struct ptp_clock_info { > struct ptp_system_timestamp *sts); > int (*getcrosscycles)(struct ptp_clock_info *ptp, > struct system_device_crosststamp *cts); > + ktime_t (*gettstamp)(struct ptp_clock_info *ptp, > + const struct skb_shared_hwtstamps *hwtstamps, > + bool cycles); > int (*enable)(struct ptp_clock_info *ptp, > struct ptp_clock_request *request, int on); > int (*verify)(struct ptp_clock_info *ptp, unsigned int pin, > @@ -364,6 +377,19 @@ static inline void ptp_cancel_worker_sync(struct ptp_clock *ptp) > * a loadable module. > */ > > +/** > + * ptp_get_timestamp() - get timestamp of ptp clock > + * > + * @index: phc index of ptp pclock. > + * @hwtstamps: skb_shared_hwtstamps structure pointer. > + * @cycles: true for timestamp based on cycles. > + * > + * Returns timestamp, or 0 on error. > + */ > +ktime_t ptp_get_timestamp(int index, > + const struct skb_shared_hwtstamps *hwtstamps, > + bool cycles); > + > /** > * ptp_get_vclocks_index() - get all vclocks index on pclock, and > * caller is responsible to free memory > @@ -386,6 +412,10 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index); > */ > ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index); > #else > +static inline ktime_t ptp_get_timestamp(int index, > + const struct skb_shared_hwtstamps *hwtstamps, > + bool cycles); > +{ return 0; } > static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index) > { return 0; } > static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index f494ddbfc826..38929c113953 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -564,7 +564,10 @@ static inline bool skb_frag_must_loop(struct page *p) > * &skb_shared_info. Use skb_hwtstamps() to get a pointer. > */ > struct skb_shared_hwtstamps { > - ktime_t hwtstamp; > + union { > + ktime_t hwtstamp; > + void *phc_data; > + }; > }; > > /* Definitions for tx_flags in struct skb_shared_info */ > @@ -581,6 +584,9 @@ enum { > /* generate hardware time stamp based on cycles if supported */ > SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3, > > + /* call PHC to get actual hardware time stamp */ > + SKBTX_HW_TSTAMP_PHC = 1 << 3, > + > /* generate wifi status information (where possible) */ > SKBTX_WIFI_STATUS = 1 << 4, > > diff --git a/net/socket.c b/net/socket.c > index 2e932c058002..fe765d559086 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -804,21 +804,17 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp) > return skb->tstamp && !false_tstamp && skb_is_err_queue(skb); > } > > -static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb) > +static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb, > + int if_index) > { > struct scm_ts_pktinfo ts_pktinfo; > - struct net_device *orig_dev; > > if (!skb_mac_header_was_set(skb)) > return; > > memset(&ts_pktinfo, 0, sizeof(ts_pktinfo)); > > - rcu_read_lock(); > - orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > - if (orig_dev) > - ts_pktinfo.if_index = orig_dev->ifindex; > - rcu_read_unlock(); > + ts_pktinfo.if_index = if_index; > > ts_pktinfo.pkt_length = skb->len - skb_mac_offset(skb); > put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO, > @@ -838,6 +834,9 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > int empty = 1, false_tstamp = 0; > struct skb_shared_hwtstamps *shhwtstamps = > skb_hwtstamps(skb); > + struct net_device *orig_dev; > + int if_index = 0; > + int phc_index = -1; > ktime_t hwtstamp; > > /* Race occurred between timestamp enabling and packet > @@ -886,18 +885,32 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, > if (shhwtstamps && > (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && > !skb_is_swtx_tstamp(skb, false_tstamp)) { > - if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) > - hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp, > - sk->sk_bind_phc); > + rcu_read_lock(); > + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); > + if (orig_dev) { > + if_index = orig_dev->ifindex; > + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) > + phc_index = ethtool_get_phc(orig_dev); > + } > + rcu_read_unlock(); > + > + if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) && > + (phc_index != -1)) > + hwtstamp = ptp_get_timestamp(phc_index, shhwtstamps, > + sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC); > else I think that the only part that I don't like is how ethtool_get_phc() and ptp_get_timestamp() work together. Would it make sense for ethtool_get_phc() to return a 'struct ptp_clock' directly? And make ptp_get_timestamp() accept a ptp_clock? I always get worried that by using indexes it's hard to guarantee that we are using the "right" device (thinking when the user is creating and destrying devices non stop). It could be that by addressing Richard comments you will handle this concern as well. > hwtstamp = shhwtstamps->hwtstamp; > > + if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) > + hwtstamp = ptp_convert_timestamp(&hwtstamp, > + sk->sk_bind_phc); > + > if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) { > empty = 0; > > if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) && > !skb_is_err_queue(skb)) > - put_ts_pktinfo(msg, skb); > + put_ts_pktinfo(msg, skb, if_index); > } > } > if (!empty) { > -- > 2.20.1 > Cheers,
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 54b9f54ac0b2..b7a8cf27c349 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -450,6 +450,33 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp) } EXPORT_SYMBOL(ptp_cancel_worker_sync); +ktime_t ptp_get_timestamp(int index, + const struct skb_shared_hwtstamps *hwtstamps, + bool cycles) +{ + char name[PTP_CLOCK_NAME_LEN] = ""; + struct ptp_clock *ptp; + struct device *dev; + ktime_t ts; + + snprintf(name, PTP_CLOCK_NAME_LEN, "ptp%d", index); + dev = class_find_device_by_name(ptp_class, name); + if (!dev) + return 0; + + ptp = dev_get_drvdata(dev); + + if (ptp->info->gettstamp) + ts = ptp->info->gettstamp(ptp->info, hwtstamps, cycles); + else + ts = hwtstamps->hwtstamp; + + put_device(dev); + + return ts; +} +EXPORT_SYMBOL(ptp_get_timestamp); + /* module operations */ static void __exit ptp_exit(void) diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h index cc6a7b2e267d..f4f0d8a880c6 100644 --- a/include/linux/ptp_clock_kernel.h +++ b/include/linux/ptp_clock_kernel.h @@ -133,6 +133,16 @@ struct ptp_system_timestamp { * parameter cts: Contains timestamp (device,system) pair, * where system time is realtime and monotonic. * + * @gettstamp: Get hardware timestamp based on normal/adjustable time or free + * running time. If @getcycles64 or @getcyclesx64 are supported, + * then this method is required to provide timestamps based on the + * free running time. This method will be called if + * SKBTX_HW_TSTAMP_PHC is set by the driver. + * parameter hwtstamps: skb_shared_hwtstamps structure pointer. + * parameter cycles: If false, then hardware timestamp based on + * normal/adjustable time is requested. If true, then hardware + * timestamp based on free running time is requested. + * * @enable: Request driver to enable or disable an ancillary feature. * parameter request: Desired resource to enable or disable. * parameter on: Caller passes one to enable or zero to disable. @@ -185,6 +195,9 @@ struct ptp_clock_info { struct ptp_system_timestamp *sts); int (*getcrosscycles)(struct ptp_clock_info *ptp, struct system_device_crosststamp *cts); + ktime_t (*gettstamp)(struct ptp_clock_info *ptp, + const struct skb_shared_hwtstamps *hwtstamps, + bool cycles); int (*enable)(struct ptp_clock_info *ptp, struct ptp_clock_request *request, int on); int (*verify)(struct ptp_clock_info *ptp, unsigned int pin, @@ -364,6 +377,19 @@ static inline void ptp_cancel_worker_sync(struct ptp_clock *ptp) * a loadable module. */ +/** + * ptp_get_timestamp() - get timestamp of ptp clock + * + * @index: phc index of ptp pclock. + * @hwtstamps: skb_shared_hwtstamps structure pointer. + * @cycles: true for timestamp based on cycles. + * + * Returns timestamp, or 0 on error. + */ +ktime_t ptp_get_timestamp(int index, + const struct skb_shared_hwtstamps *hwtstamps, + bool cycles); + /** * ptp_get_vclocks_index() - get all vclocks index on pclock, and * caller is responsible to free memory @@ -386,6 +412,10 @@ int ptp_get_vclocks_index(int pclock_index, int **vclock_index); */ ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, int vclock_index); #else +static inline ktime_t ptp_get_timestamp(int index, + const struct skb_shared_hwtstamps *hwtstamps, + bool cycles); +{ return 0; } static inline int ptp_get_vclocks_index(int pclock_index, int **vclock_index) { return 0; } static inline ktime_t ptp_convert_timestamp(const ktime_t *hwtstamp, diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index f494ddbfc826..38929c113953 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -564,7 +564,10 @@ static inline bool skb_frag_must_loop(struct page *p) * &skb_shared_info. Use skb_hwtstamps() to get a pointer. */ struct skb_shared_hwtstamps { - ktime_t hwtstamp; + union { + ktime_t hwtstamp; + void *phc_data; + }; }; /* Definitions for tx_flags in struct skb_shared_info */ @@ -581,6 +584,9 @@ enum { /* generate hardware time stamp based on cycles if supported */ SKBTX_HW_TSTAMP_USE_CYCLES = 1 << 3, + /* call PHC to get actual hardware time stamp */ + SKBTX_HW_TSTAMP_PHC = 1 << 3, + /* generate wifi status information (where possible) */ SKBTX_WIFI_STATUS = 1 << 4, diff --git a/net/socket.c b/net/socket.c index 2e932c058002..fe765d559086 100644 --- a/net/socket.c +++ b/net/socket.c @@ -804,21 +804,17 @@ static bool skb_is_swtx_tstamp(const struct sk_buff *skb, int false_tstamp) return skb->tstamp && !false_tstamp && skb_is_err_queue(skb); } -static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb) +static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb, + int if_index) { struct scm_ts_pktinfo ts_pktinfo; - struct net_device *orig_dev; if (!skb_mac_header_was_set(skb)) return; memset(&ts_pktinfo, 0, sizeof(ts_pktinfo)); - rcu_read_lock(); - orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); - if (orig_dev) - ts_pktinfo.if_index = orig_dev->ifindex; - rcu_read_unlock(); + ts_pktinfo.if_index = if_index; ts_pktinfo.pkt_length = skb->len - skb_mac_offset(skb); put_cmsg(msg, SOL_SOCKET, SCM_TIMESTAMPING_PKTINFO, @@ -838,6 +834,9 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, int empty = 1, false_tstamp = 0; struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb); + struct net_device *orig_dev; + int if_index = 0; + int phc_index = -1; ktime_t hwtstamp; /* Race occurred between timestamp enabling and packet @@ -886,18 +885,32 @@ void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, if (shhwtstamps && (sk->sk_tsflags & SOF_TIMESTAMPING_RAW_HARDWARE) && !skb_is_swtx_tstamp(skb, false_tstamp)) { - if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) - hwtstamp = ptp_convert_timestamp(&shhwtstamps->hwtstamp, - sk->sk_bind_phc); + rcu_read_lock(); + orig_dev = dev_get_by_napi_id(skb_napi_id(skb)); + if (orig_dev) { + if_index = orig_dev->ifindex; + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) + phc_index = ethtool_get_phc(orig_dev); + } + rcu_read_unlock(); + + if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP_PHC) && + (phc_index != -1)) + hwtstamp = ptp_get_timestamp(phc_index, shhwtstamps, + sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC); else hwtstamp = shhwtstamps->hwtstamp; + if (sk->sk_tsflags & SOF_TIMESTAMPING_BIND_PHC) + hwtstamp = ptp_convert_timestamp(&hwtstamp, + sk->sk_bind_phc); + if (ktime_to_timespec64_cond(hwtstamp, tss.ts + 2)) { empty = 0; if ((sk->sk_tsflags & SOF_TIMESTAMPING_OPT_PKTINFO) && !skb_is_err_queue(skb)) - put_ts_pktinfo(msg, skb); + put_ts_pktinfo(msg, skb, if_index); } } if (!empty) {
If a physical clock supports a free running time called cycles, then timestamps shall be based on this time too. For TX it is known in advance before the transmission if a timestamp based on cycles is needed. For RX it is impossible to know which timestamp is needed before the packet is received and assigned to a socket. Support late timestamp determination by a physical clock. Therefore, an address/cookie is stored within the new phc_data field of struct skb_shared_hwtstamps. This address/cookie is provided to a new physical clock method called gettstamp(), which returns a timestamp based on the normal/adjustable time or based on cycles. The previously introduced flag SKBTX_HW_TSTAMP_USE_CYCLES is reused with an additional alias to request the late timestamp determination by the physical clock. That is possible, because SKBTX_HW_TSTAMP_USE_CYCLES is not used in the receive path. Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> --- drivers/ptp/ptp_clock.c | 27 ++++++++++++++++++++++++ include/linux/ptp_clock_kernel.h | 30 +++++++++++++++++++++++++++ include/linux/skbuff.h | 8 +++++++- net/socket.c | 35 ++++++++++++++++++++++---------- 4 files changed, 88 insertions(+), 12 deletions(-)