Re: [dpdk-dev] [PATCH 1/8] net/bnxt: remove assert for zero data len in Tx path

[Ferruh Yigit <ferruh.yigit@xilinx.com>]

On 6/20/2022 12:09 AM, Ajit Khaparde wrote:
> On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
>>
>> On 6/15/2022 3:56 PM, Kalesh A P wrote:
>>>
>>> From: Somnath Kotur <somnath.kotur@broadcom.com>
>>>
>>> Currently the PMD tries to detect a potential 0 byte DMA by
>>> using RTE_VERIFY.
>>> But since RTE_VERIFY internally calls rte_panic() it is fatal to
>>> the application and some applications want to avoid that.
>>> So return an error from the bnxt xmit handler if such a bad pkt is
>>> encountered by logging an error message, dumping the pkt header and
>>> dump the current stack as well
>>>
>>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>> ---
>>>    drivers/net/bnxt/bnxt_txr.c | 29 ++++++++++++++++++++++++++---
>>>    1 file changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/bnxt/bnxt_txr.c b/drivers/net/bnxt/bnxt_txr.c
>>> index 7a7196a..67e0167 100644
>>> --- a/drivers/net/bnxt/bnxt_txr.c
>>> +++ b/drivers/net/bnxt/bnxt_txr.c
>>> @@ -123,6 +123,26 @@ bnxt_xmit_need_long_bd(struct rte_mbuf *tx_pkt, struct bnxt_tx_queue *txq)
>>>           return false;
>>>    }
>>>
>>> +static bool
>>> +bnxt_zero_data_len_tso_segsz(struct rte_mbuf *tx_pkt, uint8_t data_len_chk)
>>> +{
>>> +       const char *type_str = "Data len";
>>> +       uint16_t len_to_check = tx_pkt->data_len;
>>> +
>>> +       if (data_len_chk == 0) {
>>> +               type_str = "TSO Seg size";
>>> +               len_to_check = tx_pkt->tso_segsz;
>>> +       }
>>> +
>>> +       if (len_to_check == 0) {
>>> +               PMD_DRV_LOG(ERR, "Error! Tx pkt %s == 0\n", type_str);
>>> +               rte_pktmbuf_dump(stdout, tx_pkt, 64);
>>> +               rte_dump_stack();
>>> +               return true;
>>> +       }
>>> +       return false;
>>> +}
>>> +
>>>    static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>>>                                   struct bnxt_tx_queue *txq,
>>>                                   uint16_t *coal_pkts,
>>> @@ -179,7 +199,8 @@ static uint16_t bnxt_start_xmit(struct rte_mbuf *tx_pkt,
>>>           }
>>>
>>>           /* Check non zero data_len */
>>> -       RTE_VERIFY(tx_pkt->data_len);
>>> +       if (unlikely(bnxt_zero_data_len_tso_segsz(tx_pkt, 1)))
>>> +               return -EIO;
>>>
>>
>> Some PMDs does the similar verification in the 'rte_eth_tx_prepare()'
>> API (tx_pkt_prepare() dev_ops), this helps to separate the checks and Tx
>> data path code, do you want to do the same?
> 
> 
> When we originally added these checks, we were not sure how prevalent
> is the usage of tx_pkt_prepare() dev_op by various applications.
> 
> We will stick with this patch for now  and implement that
> rte_eth_tx_prepare() in the next release?
> 

'rte_eth_tx_prepare()' is not mandatory, so yes there may be 
applications that are not calling this API.

If we have a consensus to have checks in the prepare function, I think 
it helps both to PMDs and applications.