[dpdk-dev] [PATCH v5 2/7] net: Add common PTP structures and functions

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, November 10, 2015 11:26 AM
> To: Mrzyglod, DanielX T
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/7] net: Add common PTP structures and
> functions
> 
> 2015-11-05 15:06, Daniel Mrzyglod:
> > This patch add common functions and structures used for PTP processing.
> >
> > Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod at intel.com>
> > ---
> >  lib/librte_net/Makefile  |   2 +-
> >  lib/librte_net/rte_ptp.h | 105
> > +++++++++++++++++++++++++++++++++++++++++++++++
> 
> The library librte_net gather some structures and functions for network
> headers/layers parsing.
> These PTP functions looks really generic. Why not add them in EAL?
> Maybe they could benefit from specific implementations in the arch/
> directory.

Hi Thomas,

How about the following location and new header file?

    lib/librte_eal/common/include/rte_time.h


 
> > +/*
> > + * Structure for cyclecounter IEEE1588 functionality.
> > + */
> > +struct cyclecounter {
> > +	uint64_t (*read)(struct rte_eth_dev *dev);
> 
> This field is not used.
> Please avoid using a reference to ethdev here.

Ok. We'll try rework this to be more generic.


 
> > +	uint64_t mask;
> > +	uint32_t shift;
> > +};
> > +
> > +/*
> > + * Structure to hold and calculate Unix epoch time.
> > + */
> > +struct timecounter {
> > +	struct cyclecounter *cc;
> > +	uint64_t cycle_last;
> > +	uint64_t nsec;
> > +	uint64_t mask;
> > +	uint64_t frac;
> > +};
> 
> This structure is not used.
> 
> It is not clear what these structures are for, and what means each field.
> When adding a new field in an API, it must be commented in Doxygen.


The structure is used. We will add Doxygen comments.


 
> > +static inline uint64_t
> > +timespec_to_ns(const struct timespec *ts)
> [...]
> > +static inline struct timespec
> > +ns_to_timespec(uint64_t nsec)
> [...]
> > +static inline uint64_t
> > +cyclecounter_cycles_to_ns(const struct cyclecounter *cc,
> > +		      uint64_t cycles, uint64_t mask, uint64_t *frac)
> [...]
> > +static inline uint64_t
> > +cyclecounter_cycles_to_ns_backwards(const struct cyclecounter *cc,
> > +			       uint64_t cycles, uint64_t frac)
> 
> They must be prefixed with rte_ with full doxygen comments.


We can do that. But is the intention that these function are public? We really only need them internally. It is possible that they could be used somewhere else but probably not likely. How about if we document them but mark them as @internal in Doxygen. Then if they are require by some other libs they can be made external at a later stage.

John