Re: [PATCH V6 net-next 06/11] net: hibmcge: Implement .ndo_start_xmit function

[Paolo Abeni <pabeni@redhat.com>]

On 8/30/24 14:15, Jijie Shao wrote:
> +netdev_tx_t hbg_net_start_xmit(struct sk_buff *skb, struct net_device *net_dev)
> +{
> +	struct hbg_ring *ring = netdev_get_tx_ring(net_dev);
> +	struct hbg_priv *priv = netdev_priv(net_dev);
> +	/* This smp_load_acquire() pairs with smp_store_release() in
> +	 * hbg_tx_buffer_recycle() called in tx interrupt handle process.
> +	 */
> +	u32 ntc = smp_load_acquire(&ring->ntc);
> +	struct hbg_buffer *buffer;
> +	struct hbg_tx_desc tx_desc;
> +	u32 ntu = ring->ntu;
> +
> +	if (unlikely(!hbg_nic_is_open(priv))) {
> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}
> +
> +	if (unlikely(!skb->len ||
> +		     skb->len > hbg_spec_max_frame_len(priv, HBG_DIR_TX))) {
> +		dev_kfree_skb_any(skb);
> +		net_dev->stats.tx_errors++;
> +		return NETDEV_TX_OK;
> +	}
> +
> +	if (unlikely(hbg_queue_is_full(ntc, ntu, ring) ||
> +		     hbg_fifo_is_full(ring->priv, ring->dir))) {
> +		netif_stop_queue(net_dev);
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	buffer = &ring->queue[ntu];
> +	buffer->skb = skb;
> +	buffer->skb_len = skb->len;
> +	if (unlikely(hbg_dma_map(buffer))) {
> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}
> +
> +	buffer->state = HBG_TX_STATE_START;
> +	hbg_init_tx_desc(buffer, &tx_desc);
> +	hbg_hw_set_tx_desc(priv, &tx_desc);
> +
> +	/* This smp_store_release() pairs with smp_load_acquire() in
> +	 * hbg_tx_buffer_recycle() called in tx interrupt handle process.
> +	 */
> +	smp_store_release(&ring->ntu, hbg_queue_next_prt(ntu, ring));

Here you should probably check for netif_txq_maybe_stop()

> +	net_dev->stats.tx_bytes += skb->len;
> +	net_dev->stats.tx_packets++;

Try to avoid 'dev->stats' usage. Instead you could use per napi stats 
accounting (no contention).

Side note: 'net_dev' is quite an unusual variable name for a network device.

Cheers,

Paolo