Re: [PATCH 1/3] enetc: add hardware timestamping support

[Richard Cochran]

On Thu, May 16, 2019 at 09:59:08AM +0000, Y.b. Lu wrote:

> +config FSL_ENETC_HW_TIMESTAMPING
> +	bool "ENETC hardware timestamping support"
> +	depends on FSL_ENETC || FSL_ENETC_VF
> +	help
> +	  Enable hardware timestamping support on the Ethernet packets
> +	  using the SO_TIMESTAMPING API. Because the RX BD ring dynamic
> +	  allocation hasn't been supported and it's too expensive to use

s/it's/it is/

> +	  extended RX BDs if timestamping isn't used, the option was used
> +	  to control hardware timestamping/extended RX BDs to be enabled
> +	  or not.

..., this option enables extended RX BDs in order to support hardware
timestamping.

>  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:

	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;
			}

>  		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?

>  
>  	struct enetc_bdr *tx_ring[16];
>  	struct enetc_bdr *rx_ring[16];

Thanks,
Richard