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

Message-ID: <20181219042757.vhetz2zsrpsz6p3u@localhost>
Date:   Tue, 18 Dec 2018 20:27:57 -0800
From:   Richard Cochran <richardcochran@...il.com>
To:     dwesterg@...il.com
Cc:     netdev@...r.kernel.org, dinguyen@...nel.org,
        thor.thayer@...ux.intel.com, davem@...emloft.net,
        vbridger@...nsource.altera.com, robh+dt@...nel.org,
        devicetree@...r.kernel.org, hean.loong.ong@...el.com,
        Dalon Westergreen <dalon.westergreen@...el.com>
Subject: Re: [PATCH v2 net-next 08/10] net: eth: altera: add support for ptp
 and timestamping

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...

> +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