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
Hi Dan,
> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Wednesday, April 25, 2018 6:04 PM
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; [email protected]; Greg
> Kroah-Hartman <[email protected]>; Richard Cochran
> <[email protected]>; Ruxandra Ioana Ciocoi Radulescu
> <[email protected]>
> 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