Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   "Y.b. Lu" <yangbo.lu@nxp.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
CC:     "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Richard Cochran <richardcochran@gmail.com>,
        Ruxandra Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>
Subject: RE: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware
 timestamping
Thread-Topic: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware
 timestamping
Thread-Index: AQHT3HZv/Y/PznkfA0CVZRkLjjVOL6QRQHeAgAGUuKA=
Date:   Thu, 26 Apr 2018 10:17:38 +0000
Message-ID: <DB6PR0401MB25361EE5633F24AA34F0063CF88E0@DB6PR0401MB2536.eurprd04.prod.outlook.com>
References: <20180425091749.46841-1-yangbo.lu@nxp.com>
 <20180425100345.b5oxgdjyud7t6kzc@mwanda>
In-Reply-To: <20180425100345.b5oxgdjyud7t6kzc@mwanda>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 507f77ac-32bc-47be-0e4c-08d5ab5ef044
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Apr 2018 10:17:38.8123
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0401MB2501
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Dan,

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Wednesday, April 25, 2018 6:04 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; Richard Cochran
> <richardcochran@gmail.com>; Ruxandra Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>
> Subject: Re: [PATCH] staging: fsl-dpaa2/eth: Add support for hardware
> timestamping
>=20
> 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 =3D skb_hwtstamps(skb);
> > +		u64 *ns =3D dpaa2_get_ts(vaddr, false);
> > +
> > +		*ns =3D DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);
>=20
> This will cause Sparse endianess warnings.
>=20
> I don't totally understand why we're writing to *ns.  Do we access *ns ag=
ain
> or not?  Either way, this doesn't seem right.  In other words, why don't =
we
> do this:
>=20
> 		__le64 *period =3D dpaa2_get_ts(vaddr, false);
> 		u64 ns;
>=20
> 		ns =3D DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS *
> le64_to_cpup(period);
> 		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> 		shhwtstamps->hwtstamp =3D ns_to_ktime(ns);
>=20
> Then if we need to save a munged *ns then we can do this at the end:
>=20
> 		/* we need this because blah blah blah */
> 		*period =3D (__le64)ns;
>=20

[Y.b. Lu] You're right. I will modify the code according to your suggestion=
.

>=20
> > +		memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> > +		shhwtstamps->hwtstamp =3D 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 =3D le32_to_cpu(fas->status);
>=20
> [ snip ]
>=20
> > @@ -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 =3D dpaa2_get_ts(skbh, true);
> > +		*ns =3D DPAA2_PTP_NOMINAL_FREQ_PERIOD_NS * le64_to_cpup(ns);
> > +		shhwtstamps.hwtstamp =3D ns_to_ktime(*ns);
> > +		skb_tstamp_tx(skb, &shhwtstamps);
>=20
> Sparse issues here also.

[Y.b. Lu] Will modify the code according to your suggestion.

>=20
> > +	}
> > +
> >  	/* Free SGT buffer allocated on tx */
> >  	if (fd_format !=3D 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);
>=20
> Is this really related?  (I have not looked at this code).

[Y.b. Lu] Yes. The skb_tstamp_tx() function will check that.

>=20
> > +
> >  		dev_kfree_skb(skb);
> >  		skb =3D ns;
> >  	}
>=20
> [ snip ]
>=20
> > @@ -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 */
>=20
> These variable names are not great.  I wouldn't have understood "ts_"
> without the comment.  "tx_" is good.  "en" is confusing until you read th=
e
> comment.  But really it should just be left out because "enable" is assum=
ed,
> generally.  Last week I asked someone to rewrite a patch that had a _disa=
ble
> variable because negative variables lead to double negatives which screw =
with
> my tiny head.
>=20
> 	if (blah_disable !=3D 0) {
>=20
> OH MY BLASTED WORD MY BRIAN ESPLODED!!!1!
>=20
> 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.

>=20
>=20
> > +
> >  	u16 tx_qdid;
> >  	u16 rx_buf_align;
> >  	struct fsl_mc_io *mc_io;
>=20
> regards,
> dan carpenter