netdev - Re: [net-next 14/16] i40e: enable PTP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1389372527.8039.5.camel@jekeller-desk1.amr.corp.intel.com>
Date:	Fri, 10 Jan 2014 16:48:47 +0000
From:	"Keller, Jacob E" <jacob.e.keller@...el.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>,
	Richard Cochran <richardcochran@...il.com>,
	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>
Subject: Re: [net-next 14/16] i40e: enable PTP

On Fri, 2014-01-10 at 15:55 +0000, Ben Hutchings wrote:
> On Fri, 2014-01-10 at 00:58 -0800, Jeff Kirsher wrote:
> > From: Jacob Keller <jacob.e.keller@...el.com>
> > 
> > New feature: Enable PTP support in the i40e driver.
> > 
> > Change-ID: I6a8e799f582705191f9583afb1b9231a8db96cc8
> > Cc: Richard Cochran <richardcochran@...il.com>
> > Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> > Tested-by: Kavindya Deegala <kavindya.s.deegala@...el.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > ---
> >  drivers/net/ethernet/intel/i40e/Makefile       |   2 +
> >  drivers/net/ethernet/intel/i40e/i40e.h         |  24 +
> >  drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  33 +-
> >  drivers/net/ethernet/intel/i40e/i40e_main.c    |  45 +-
> >  drivers/net/ethernet/intel/i40e/i40e_ptp.c     | 640 +++++++++++++++++++++++++
> >  drivers/net/ethernet/intel/i40e/i40e_txrx.c    |  53 ++
> >  drivers/net/ethernet/intel/i40e/i40e_txrx.h    |   3 +
> >  7 files changed, 798 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/net/ethernet/intel/i40e/i40e_ptp.c
> 
> This is missing a Kconfig update to select PTP_1588_CLOCK.
> 
> [...]
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -1698,6 +1698,25 @@ static int i40e_change_mtu(struct net_device *netdev, int new_mtu)
> >  }
> >  
> >  /**
> > + * i40e_ioctl - Access the hwtstamp interface
> > + * @netdev: network interface device structure
> > + * @ifr: interface request data
> > + * @cmd: ioctl command
> > + **/
> > +int i40e_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
> > +{
> > +	struct i40e_netdev_priv *np = netdev_priv(netdev);
> > +	struct i40e_pf *pf = np->vsi->back;
> > +
> > +	switch (cmd) {
> > +	case SIOCSHWTSTAMP:
> > +		return i40e_ptp_hwtstamp_ioctl(pf, ifr, cmd);
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> 
> Please implement SIOCGHWTSTAMP as well.

Yep. I still need to implement this for other Intel drivers as well.

> 
> [...]
> > --- /dev/null
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> [...]
> > +void i40e_ptp_init(struct i40e_pf *pf)
> > +{
> > +	struct i40e_hw *hw = &pf->hw;
> > +	struct net_device *netdev = pf->vsi[pf->lan_vsi]->netdev;
> > +
> > +	snprintf(pf->ptp_caps.name, 16, "%pm", netdev->dev_addr);
> [...]
> 
> Use sizeof(), not a magic number.  Also the clock name should perhaps be
> the driver name, not the MAC address.

Some other drivers probably need to be updated to not use magic number
as well..

> 
> Is the clock shared between multiple PFs or are there independent clocks
> for each PF?

Each PF gets its own register set, and while the clock is physically
driven by the same source, there is no mechanism to ensure registers are
in sync, so effectively each PF has its own independent clock. (ie:
can't support boundary clock properly)

Thanks,
Jake

> 
> Ben.
>