netdev - Re: [PATCH v2 net-next 08/10] net: eth: altera: add support for ptp and timestamping

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] [day] [month] [year] [list]

Date:   Wed, 19 Dec 2018 19:27:53 +0000
From:   "Westergreen, Dalon" <dalon.westergreen@...el.com>
To:     "richardcochran@...il.com" <richardcochran@...il.com>
CC:     "Ong, Hean Loong" <hean.loong.ong@...el.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "thor.thayer@...ux.intel.com" <thor.thayer@...ux.intel.com>,
        "dinguyen@...nel.org" <dinguyen@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "vbridger@...nsource.altera.com" <vbridger@...nsource.altera.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v2 net-next 08/10] net: eth: altera: add support for ptp
 and timestamping

Richard,

On Tue, 2018-12-18 at 20:27 -0800, Richard Cochran wrote:
> On Thu, Dec 13, 2018 at 09:52:50AM -0800, dwesterg@...il.com wrote:
> > Changes from V1:
> >  -> Remove debugfs for tod manipulation
> >  -> Reorder variable declarations in functions to order
> >     by length of declaration, largest to smallest
> >  -> Rename altera_ptp to intel_fpga_tod
> >  -> Rename functions in intel_fpga_tod so that they are not
> >     generic
> >  -> Use imply instead of select in Kconfig
> >  -> Re-write adjust time function to remove while loop with
> >     64 bit divide.
> 
> Overall this is looking better.  A few more comments follow...

Thanks, all of your comments will be included in a v3 patch.

--dalon

> 
> > +static int tse_get_ts_info(struct net_device *dev,
> > +			   struct ethtool_ts_info *info)
> > +{
> > +	struct altera_tse_private *priv = netdev_priv(dev);
> > +
> > +	if (priv->ptp_enable) {
> > +		if (priv->ptp_priv.ptp_clock)
> > +			info->phc_index =
> > +				ptp_clock_index(priv->ptp_priv.ptp_clock);
> 
> else
> info->phc_index = -1;
> 


> > +		info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
> > +					SOF_TIMESTAMPING_RX_HARDWARE |
> > +					SOF_TIMESTAMPING_RAW_HARDWARE;
> > +
> > +		info->tx_types = (1 << HWTSTAMP_TX_OFF) |
> > +						 (1 << HWTSTAMP_TX_ON);
> 
> This fits on one line ^^^
> 
> > +		info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> > +						   (1 << HWTSTAMP_FILTER_ALL);
> 
> Funky indentation here.  One extra level is enough:
> 
> 		info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> 			(1 << HWTSTAMP_FILTER_ALL);
> 
> > +		return 0;
> > +	} else {
> > +		return ethtool_op_get_ts_info(dev, info);
> > +	}
> > +}
> > +
> 
> ...
> 
> > @@ -609,7 +613,14 @@ static netdev_tx_t tse_start_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> >  	if (ret)
> >  		goto out;
> >  
> > -	skb_tx_timestamp(skb);
> > +	/* Provide a hardware time stamp if requested.
> > +	 */
> > +	if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> > +		     priv->hwts_tx_en))
> > +		/* declare that device is doing timestamping */
> 
> Please drop those two comments.  They are redundant because they only
> restate what the code does.
> 
> > +		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > +	else
> > +		skb_tx_timestamp(skb);
> >  
> >  	priv->tx_prod++;
> >  	dev->stats.tx_bytes += skb->len;
> 
> Thanks,
> Richard

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (3282 bytes)