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

[Jijie Shao <shaojijie@huawei.com>]

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