Re: [PATCH net-next v25 06/13] rtase: Implement .ndo_start_xmit function

[Heiner Kallweit]

On 30.07.2024 11:27, Justin Lai wrote:
>> On Mon, 29 Jul 2024 14:21:14 +0800 Justin Lai wrote:
>>> +     stop_queue = !netif_subqueue_maybe_stop(dev, ring->index,
>>> +                                             rtase_tx_avail(ring),
>>> +
>> RTASE_TX_STOP_THRS,
>>> +
>> RTASE_TX_START_THRS);
>>> +
>>> +     if (door_bell || stop_queue)
>>> +             rtase_w8(tp, RTASE_TPPOLL, BIT(ring->index));
>>> +
>>> +     return NETDEV_TX_OK;
>>> +
>>> +err_dma_1:
>>> +     ring->skbuff[entry] = NULL;
>>> +     rtase_tx_clear_range(ring, ring->cur_idx + 1, frags);
>>> +
>>> +err_dma_0:
>>> +     tp->stats.tx_dropped++;
>>> +     dev_kfree_skb_any(skb);
>>> +     return NETDEV_TX_OK;
>>> +
>>> +err_stop:
>>> +     netif_stop_queue(dev);
>>> +     tp->stats.tx_dropped++;
>>> +     return NETDEV_TX_BUSY;
>>
>> If you're dropping a packet you should somehow check that the previous xmit
>> didn't enqueue a packet to the ring and skip the doorbell because door_bell
>> was false. If that's the case you have to ring the doorbell now.
>>
I briefly checked and this may be a common issue, e.g. stmmac_xmit() behaves
the same as r8169 here.
Hitting this error path should be a rare event, so we could ring the doorbell
always, w/o any further checks?

>> Also you shouldn't increment dropped if you return TX_BUSY.
> 
I'll take care of this change for r8169.

> Thank you for your response. I will modify it.
>>
>> Please fix these issues in the driver you're copying from, too.
> 
> I will inform the person responsible for the R8169 of the issues we've
> identified and discuss what modifications need to be made.
> 
> Thanks
> Justin
>