LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Claudiu Manoil <claudiu.manoil@nxp.com>
To: Richard Cochran <richardcochran@gmail.com>,
	"Y.b. Lu" <yangbo.lu@nxp.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
Date: Thu, 16 May 2019 15:30:51 +0000	[thread overview]
Message-ID: <VI1PR04MB4880B9B346D29E0EFC715D28960A0@VI1PR04MB4880.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20190516143251.akbt3ns6ue2jrhl5@localhost>


>-----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 = tx_ring->ndev;
>> +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
>>  	int tx_frm_cnt = 0, tx_byte_cnt = 0;
>>  	struct enetc_tx_swbd *tx_swbd;
>> +	union enetc_tx_bd *txbd;
>> +	bool do_tstamp;
>>  	int i, bds_to_clean;
>> +	u64 tstamp = 0;
>
>Please keep in reverse Christmas tree order as much as possible:

For the xmass tree part, Yangbo, better move the priv and txbd declarations
inside the scope of the if() {} block where they are actually used, i.e.:

		if (unlikely(tx_swbd->check_wb)) {
			struct enetc_ndev_priv *priv = netdev_priv(ndev);
			union enetc_tx_bd *txbd;
			[...]
		}

>
>	union enetc_tx_bd *txbd;
>	int i, bds_to_clean;
>	bool do_tstamp;
>	u64 tstamp = 0;
>
>>  	i = tx_ring->next_to_clean;
>>  	tx_swbd = &tx_ring->tx_swbd[i];
>>  	bds_to_clean = enetc_bd_ready_count(tx_ring, i);
>>
>> +	do_tstamp = false;
>> +
>>  	while (bds_to_clean && tx_frm_cnt < ENETC_DEFAULT_TX_WORK) {
>>  		bool is_eof = !!tx_swbd->skb;
>>
>> +		if (unlikely(tx_swbd->check_wb)) {
>> +			txbd = 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 = 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 = true;
>			}
>

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 better off without the goto.

>>  		enetc_unmap_tx_buff(tx_ring, tx_swbd);
>>  		if (is_eof) {
>> +			if (unlikely(do_tstamp)) {
>> +				enetc_tstamp_tx(tx_swbd->skb, tstamp);
>> +				do_tstamp = false;
>> +			}
>>  			napi_consume_skb(tx_swbd->skb, napi_budget);
>>  			tx_swbd->skb = 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	= BIT(0),
>> +	ENETC_F_TX_TSTAMP	= 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?
>

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_features_t flag).
I wouldn't waste an int for 2 timestamp flags, I'd rather have a more generic name.
Maybe active_offloads then?

Anyway, the name can be changed later too, when other offloads will be added.

Thanks,
Claudiu

  reply	other threads:[~2019-05-16 15:30 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-16  9:59 [PATCH 0/3] ENETC: support hardware timestamping Y.b. Lu
2019-05-16  9:59 ` [PATCH 1/3] enetc: add hardware timestamping support Y.b. Lu
2019-05-16 13:31   ` Claudiu Manoil
2019-05-20  2:55     ` Y.b. Lu
2019-05-16 14:32   ` Richard Cochran
2019-05-16 15:30     ` Claudiu Manoil [this message]
2019-05-20  3:25       ` Y.b. Lu
2019-05-20  3:20     ` [EXT] " Y.b. Lu
2019-05-20  4:41       ` Richard Cochran
2019-05-16  9:59 ` [PATCH 2/3] enetc: add get_ts_info interface for ethtool Y.b. Lu
2019-05-16  9:59 ` [PATCH 3/3] arm64: dts: fsl: ls1028a: add ENETC 1588 timer node Y.b. Lu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR04MB4880B9B346D29E0EFC715D28960A0@VI1PR04MB4880.eurprd04.prod.outlook.com \
    --to=claudiu.manoil@nxp.com \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=richardcochran@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=yangbo.lu@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).