Re: [net-next v2 14/17] i40e: enable PTP

[Ben Hutchings <bhutchings@solarflare.com>]

On Fri, 2014-01-10 at 21:42 +0000, Keller, Jacob E wrote:
> On Fri, 2014-01-10 at 20:37 +0000, Ben Hutchings wrote:
> > On Fri, 2014-01-10 at 12:30 -0800, Jeff Kirsher wrote:
> > > From: Jacob Keller <jacob.e.keller@intel.com>
> > > 
> > > New feature: Enable PTP support in the i40e driver.
> > > 
> > > Change-ID: I6a8e799f582705191f9583afb1b9231a8db96cc8
> > > Cc: Richard Cochran <richardcochran@gmail.com>
> > > Cc: Ben Hutchings <bhutchings@solarflare.com>
> > > Signed-off-by: Matthew Vick <matthew.vick@intel.com>
> > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/Kconfig             |   1 +
> > >  drivers/net/ethernet/intel/i40e/Makefile       |   1 +
> > >  drivers/net/ethernet/intel/i40e/i40e.h         |  26 +
> > >  drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  33 +-
> > >  drivers/net/ethernet/intel/i40e/i40e_main.c    |  47 +-
> > >  drivers/net/ethernet/intel/i40e/i40e_ptp.c     | 662 +++++++++++++++++++++++++
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.c    |  53 ++
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.h    |   3 +
> > >  8 files changed, 824 insertions(+), 2 deletions(-)
> > >  create mode 100644 drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > > 
> > > diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
> > > index 9fb2eb8..333bb54 100644
> > > --- a/drivers/net/ethernet/intel/Kconfig
> > > +++ b/drivers/net/ethernet/intel/Kconfig
> > [...]
> > > +void i40e_ptp_init(struct i40e_pf *pf)
> > > +{
> > > +	struct i40e_hw *hw = &pf->hw;
> > > +	struct net_device *netdev = pf->vsi[pf->lan_vsi]->netdev;
> > > +
> > > +	strncpy(pf->ptp_caps.name, netdev->name, sizeof(pf->ptp_caps.name));
> > [...]
> > 
> > I recommended using the *driver* name as the clock name.  The net device
> > doesn't even have a proper name at this point.

Actually, on closer inspection it looks like the net device gets
registered before this is called, so the name will be valid.  However
the net device can (and probably will) be renamed after registration so
this name may become stale.

If the net device is registered first, this raises a different problem:
i40e_get_ts_info() can then race with PTP init and cleanup; it can see a
non-null but invalid value of pf->ptp_clock if it races with this in
i40e_ptp_init():

> +	/* Attempt to register the clock before enabling the hardware. */
> +	pf->ptp_clock = ptp_clock_register(&pf->ptp_caps, &pf->pdev->dev);
> +	if (IS_ERR(pf->ptp_clock)) {

or it can see a non-null value of ptp_clock just before the clock is
freed, then use-after-free.

You could use rtnl_lock() to serialise the registration/unregistration
with i40e_get_ts_info(), or reorder init and cleanup so the clock is
registered before the net device and unregistered after.

> > Ben.
> > 
> 
> We use netdev->name a few lines farther down.. should that be changed as
> well?

I think that's fine if it's done after the net device is registered.
However you would normally use netdev_info() to include the name at the
beginning of the log line.

Ben.

> Sorry about this one. The ixgbe driver initializes clock at open
> instead of at init, so it has a valid clock name already.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.