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:     Claudiu Manoil <claudiu.manoil@nxp.com>,
        Richard Cochran <richardcochran@gmail.com>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        David Miller <davem@davemloft.net>,
        Shawn Guo <shawnguo@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/3] enetc: add hardware timestamping support
Thread-Topic: [PATCH 1/3] enetc: add hardware timestamping support
Thread-Index: AQHVC/xYQ2w4Gq0RdEuPdLhmipvA1KZzXh8A
Date:   Mon, 20 May 2019 03:25:45 +0000
Message-ID: <VI1PR0401MB22377C6B1B8C35F133E86DB4F8060@VI1PR0401MB2237.eurprd04.prod.outlook.com>
References: <20190516100028.48256-1-yangbo.lu@nxp.com>
 <20190516100028.48256-2-yangbo.lu@nxp.com>
 <20190516143251.akbt3ns6ue2jrhl5@localhost>
 <VI1PR04MB4880B9B346D29E0EFC715D28960A0@VI1PR04MB4880.eurprd04.prod.outlook.com>
In-Reply-To: <VI1PR04MB4880B9B346D29E0EFC715D28960A0@VI1PR04MB4880.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 0791374d-a918-40ba-62a0-08d6dcd2d8d6
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 May 2019 03:25:45.7504
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB2366
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,

> -----Original Message-----
> From: Claudiu Manoil
> Sent: Thursday, May 16, 2019 11:31 PM
> To: Richard Cochran <richardcochran@gmail.com>; Y.b. Lu
> <yangbo.lu@nxp.com>
> Cc: netdev@vger.kernel.org; David Miller <davem@davemloft.net>; Shawn
> Guo <shawnguo@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 1/3] enetc: add hardware timestamping support
>=20
>=20
> >-----Original Message-----
> >From: Richard Cochran <richardcochran@gmail.com>
> >Sent: Thursday, May 16, 2019 5:33 PM
> >To: Y.b. Lu <yangbo.lu@nxp.com>
> >Cc: netdev@vger.kernel.org; David Miller <davem@davemloft.net>; Claudiu
> >Manoil <claudiu.manoil@nxp.com>; Shawn Guo <shawnguo@kernel.org>;
> Rob
> >Herring <robh+dt@kernel.org>; devicetree@vger.kernel.org; linux-arm-
> >kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> >Subject: Re: [PATCH 1/3] enetc: add hardware timestamping support
> >
> >On Thu, May 16, 2019 at 09:59:08AM +0000, Y.b. Lu wrote:
> >
> [...]
> >
> >>  static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int
> >> napi_budget)  {
> >>  	struct net_device *ndev =3D tx_ring->ndev;
> >> +	struct enetc_ndev_priv *priv =3D netdev_priv(ndev);
> >>  	int tx_frm_cnt =3D 0, tx_byte_cnt =3D 0;
> >>  	struct enetc_tx_swbd *tx_swbd;
> >> +	union enetc_tx_bd *txbd;
> >> +	bool do_tstamp;
> >>  	int i, bds_to_clean;
> >> +	u64 tstamp =3D 0;
> >
> >Please keep in reverse Christmas tree order as much as possible:
>=20
> For the xmass tree part, Yangbo, better move the priv and txbd declaratio=
ns
> inside the scope of the if() {} block where they are actually used, i.e.:
>=20
> 		if (unlikely(tx_swbd->check_wb)) {
> 			struct enetc_ndev_priv *priv =3D netdev_priv(ndev);
> 			union enetc_tx_bd *txbd;
> 			[...]
> 		}
>=20

[Y.b. Lu] Will do that.

> >
> >	union enetc_tx_bd *txbd;
> >	int i, bds_to_clean;
> >	bool do_tstamp;
> >	u64 tstamp =3D 0;
> >
> >>  	i =3D tx_ring->next_to_clean;
> >>  	tx_swbd =3D &tx_ring->tx_swbd[i];
> >>  	bds_to_clean =3D enetc_bd_ready_count(tx_ring, i);
> >>
> >> +	do_tstamp =3D false;
> >> +
> >>  	while (bds_to_clean && tx_frm_cnt < ENETC_DEFAULT_TX_WORK) {
> >>  		bool is_eof =3D !!tx_swbd->skb;
> >>
> >> +		if (unlikely(tx_swbd->check_wb)) {
> >> +			txbd =3D ENETC_TXBD(*tx_ring, i);
> >> +
> >> +			if (!(txbd->flags & ENETC_TXBD_FLAGS_W))
> >> +				goto no_wb;
> >> +
> >> +			if (tx_swbd->do_tstamp) {
> >> +				enetc_get_tx_tstamp(&priv->si->hw, txbd,
> >> +						    &tstamp);
> >> +				do_tstamp =3D true;
> >> +			}
> >> +		}
> >> +no_wb:
> >
> >This goto seems strange and unnecessary.  How about this instead?
> >
> >			if (txbd->flags & ENETC_TXBD_FLAGS_W &&
> >			    tx_swbd->do_tstamp) {
> >				enetc_get_tx_tstamp(&priv->si->hw, txbd, &tstamp);
> >				do_tstamp =3D true;
> >			}
> >
>=20
> Absolutely, somehow I missed this.  I guess the intention was to be able =
to
> support multiple
> if() blocks for the writeback case (W flag set) but the code is much bett=
er off
> without the goto.

[Y.b. Lu] Will use this to support current single tstamp writeback case.

>=20
> >>  		enetc_unmap_tx_buff(tx_ring, tx_swbd);
> >>  		if (is_eof) {
> >> +			if (unlikely(do_tstamp)) {
> >> +				enetc_tstamp_tx(tx_swbd->skb, tstamp);
> >> +				do_tstamp =3D false;
> >> +			}
> >>  			napi_consume_skb(tx_swbd->skb, napi_budget);
> >>  			tx_swbd->skb =3D NULL;
> >>  		}
> >> @@ -167,6 +169,11 @@ struct enetc_cls_rule {
> >>
> >>  #define ENETC_MAX_BDR_INT	2 /* fixed to max # of available cpus */
> >>
> >> +enum enetc_hw_features {
> >
> >This is a poor choice of name.  It sounds like it describes HW
> >capabilities, but you use it to track whether a feature is requested at
> >run time.
> >
> >> +	ENETC_F_RX_TSTAMP	=3D BIT(0),
> >> +	ENETC_F_TX_TSTAMP	=3D BIT(1),
> >> +};
> >> +
> >>  struct enetc_ndev_priv {
> >>  	struct net_device *ndev;
> >>  	struct device *dev; /* dma-mapping device */ @@ -178,6 +185,7 @@
> >> struct enetc_ndev_priv {
> >>  	u16 rx_bd_count, tx_bd_count;
> >>
> >>  	u16 msg_enable;
> >> +	int hw_features;
> >
> >This is also poorly named.  How about "tstamp_request" instead?
> >
>=20
> This ndev_priv variable was intended to gather flags for all the active h=
/w
> related features, i.e. keeping count of what h/w offloads are enabled for=
 the
> current device (at least for those that don't have already a netdev_featu=
res_t
> flag).
> I wouldn't waste an int for 2 timestamp flags, I'd rather have a more gen=
eric
> name.
> Maybe active_offloads then?
>=20
> Anyway, the name can be changed later too, when other offloads will be
> added.

[Y.b. Lu] How about using active_offloads, and add TODO comments in enum en=
etc_active_offloads?

>=20
> Thanks,
> Claudiu