Re: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware timestamping

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Apr 25, 2018 at 05:17:49PM +0800, Yangbo Lu wrote:
> @@ -275,6 +278,16 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv *priv,
>  
>  	prefetch(skb->data);
>  
> +	/* Get the timestamp value */
> +	if (priv->ts_rx_en) {
> +		struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
> +		u64 *ns = dpaa2_get_ts(vaddr, false);
> +
> +		*ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);

This will cause Sparse endianess warnings.

I don't totally understand why we're writing to *ns.  Do we access *ns
again or not?  Either way, this doesn't seem right.  In other words, why
don't we do this:

		__le64 *period = dpaa2_get_ts(vaddr, false);
		u64 ns;

		ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(period);
		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
		shhwtstamps->hwtstamp = ns_to_ktime(ns);

Then if we need to save a munged *ns then we can do this at the end:

		/* we need this because blah blah blah */
		*period = (__le64)ns;


> +		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> +		shhwtstamps->hwtstamp = ns_to_ktime(*ns);
> +	}
> +
>  	/* Check if we need to validate the L4 csum */
>  	if (likely(dpaa2_fd_get_frc(fd) & DPAA2_FD_FRC_FASV)) {
>  		status = le32_to_cpu(fas->status);

[ snip ]

> @@ -520,6 +561,19 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv,
>  		return;
>  	}
>  
> +	/* Get the timestamp value */
> +	if (priv->ts_tx_en && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> +		struct skb_shared_hwtstamps shhwtstamps;
> +		u64 *ns;
> +
> +		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +
> +		ns = dpaa2_get_ts(skbh, true);
> +		*ns = DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);
> +		shhwtstamps.hwtstamp = ns_to_ktime(*ns);
> +		skb_tstamp_tx(skb, &shhwtstamps);

Sparse issues here also.

> +	}
> +
>  	/* Free SGT buffer allocated on tx */
>  	if (fd_format != dpaa2_fd_single)
>  		skb_free_frag(skbh);
> @@ -552,6 +606,10 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev)
>  			goto err_alloc_headroom;
>  		}
>  		percpu_extras->tx_reallocs++;
> +
> +		if (skb->sk)
> +			skb_set_owner_w(ns, skb->sk);

Is this really related?  (I have not looked at this code).

> +
>  		dev_kfree_skb(skb);
>  		skb = ns;
>  	}

[ snip ]

> @@ -319,6 +351,9 @@ struct dpaa2_eth_priv {
>  	u16 bpid;
>  	struct iommu_domain *iommu_domain;
>  
> +	bool ts_tx_en; /* Tx timestamping enabled */
> +	bool ts_rx_en; /* Rx timestamping enabled */

These variable names are not great.  I wouldn't have understood "ts_"
without the comment.  "tx_" is good.  "en" is confusing until you read
the comment.  But really it should just be left out because "enable" is
assumed, generally.  Last week I asked someone to rewrite a patch that
had a _disable variable because negative variables lead to double
negatives which screw with my tiny head.

	if (blah_disable != 0) {

OH MY BLASTED WORD MY BRIAN ESPLODED!!!1!

So let's just name these "tx_timestamps" or something.


> +
>  	u16 tx_qdid;
>  	u16 rx_buf_align;
>  	struct fsl_mc_io *mc_io;

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel