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

[Ajit Khaparde <ajit.khaparde@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2927 bytes --]

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:
> > CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> >
> >
> > 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?

Thanks

>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]