Messages in this thread Patch in this message | | | Date | Wed, 25 Apr 2018 13:03:45 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware timestamping |
| |
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
| |