Re: [PATCH v4 2/6] net/intel: add read clock feature in ICE

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Jun 12, 2025 at 11:14:30AM +0100, Loftus, Ciara wrote:
> > Subject: [PATCH v4 2/6] net/intel: add read clock feature in ICE
> 
> Since the change is specific to the ice driver only, I think the title should change to "net/ice" and you can drop the mention of "in ICE" in the message that follows the colon.
> 
> > 
> > Adding eth_ice_read_clock() feature to get current time
> > for scheduling Packets based on Tx time.
> > 
> > Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> > ---
> >  drivers/net/intel/ice/ice_ethdev.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/net/intel/ice/ice_ethdev.c
> > b/drivers/net/intel/ice/ice_ethdev.c
> > index 7cc083ca32..9478ba92df 100644
> > --- a/drivers/net/intel/ice/ice_ethdev.c
> > +++ b/drivers/net/intel/ice/ice_ethdev.c
> > @@ -187,6 +187,7 @@ static int ice_timesync_read_time(struct rte_eth_dev
> > *dev,
> >  static int ice_timesync_write_time(struct rte_eth_dev *dev,
> >  				   const struct timespec *timestamp);
> >  static int ice_timesync_disable(struct rte_eth_dev *dev);
> > +static int eth_ice_read_clock(struct rte_eth_dev *dev, uint64_t *clock);
> >  static int ice_fec_get_capability(struct rte_eth_dev *dev, struct
> > rte_eth_fec_capa *speed_fec_capa,
> >  			   unsigned int num);
> >  static int ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
> > @@ -317,6 +318,7 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
> >  	.timesync_read_time           = ice_timesync_read_time,
> >  	.timesync_write_time          = ice_timesync_write_time,
> >  	.timesync_disable             = ice_timesync_disable,
> > +	.read_clock					  = eth_ice_read_clock,
> 
> I suggest following the naming convention of the rest of the ops here. ice_read_clock instead of eth_ice_read_clock.
> Check the indentation when you are reworking this. It looks like tabs were used instead of spaces ahead of the "=".
> 
> >  	.tm_ops_get                   = ice_tm_ops_get,
> >  	.fec_get_capability           = ice_fec_get_capability,
> >  	.fec_get                      = ice_fec_get,
> > @@ -6935,6 +6937,17 @@ ice_timesync_disable(struct rte_eth_dev *dev)
> >  	return 0;
> >  }
> > 
> > +static int
> > +eth_ice_read_clock(__rte_unused struct rte_eth_dev *dev, uint64_t *clock)
> > +{
> > +	struct timespec system_time;
> > +
> > +	clock_gettime(CLOCK_REALTIME, &system_time);
> > +	*clock = system_time.tv_sec * NSEC_PER_SEC + system_time.tv_nsec;
> > +
> > +	return 0;
> > +}
> > +

It's not clear to me why this patch needs to be in the set, since if an app
wants the current CPU time in nanosecond, the app can do so directly rather
than getting it from the NIC.

Also, is CLOCK_REALTIME the correct clock to be querying here. I would
think that CLOCK_MONOTONIC_RAW is a better choice to use, since it's
unaffected by system time changes.

/Bruce