Hi Dan, > -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Wednesday, April 25, 2018 6:04 PM > To: Y.b. Lu <yangbo.lu@xxxxxxx> > Cc: devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Greg > Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Richard Cochran > <richardcochran@xxxxxxxxx>; Ruxandra Ioana Ciocoi Radulescu > <ruxandra.radulescu@xxxxxxx> > 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; > [Y.b. Lu] You're right. I will modify the code according to your suggestion. > > > + 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. [Y.b. Lu] Will modify the code according to your suggestion. > > > + } > > + > > /* 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). [Y.b. Lu] Yes. The skb_tstamp_tx() function will check that. > > > + > > 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. [Y.b. Lu] Ok. Let me use tx_tstamp/rx_tstamp instead. The tstamp is common used in driver. > > > > + > > u16 tx_qdid; > > u16 rx_buf_align; > > struct fsl_mc_io *mc_io; > > regards, > dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
