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

[Jijie Shao]

on 2024/9/3 21:31, Paolo Abeni wrote:
> 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).

use dev_sw_netstats_tx_add() or dev_sw_netstats_rx_add() ?

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

sure, 'dev' is ok.
	
	Thanks,
	Jijie Shao