On Mon, Jun 20, 2022 at 3:55 AM Ferruh Yigit <ferruh.yi...@xilinx.com> wrote:
>
> On 6/20/2022 12:09 AM, Ajit Khaparde wrote:
> > On Thu, Jun 16, 2022 at 10:03 AM Ferruh Yigit <ferruh.yi...@xilinx.com>
> > wrote:
> >>
> >> On 6/15/2022 3:56 PM, Kalesh A P wrote:
> >>>
> >>> From: Somnath Kotur <somnath.ko...@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.ko...@broadcom.com>
> >>> Reviewed-by: Ajit Khaparde <ajit.khapa...@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.
I agree.
Once the changes for tx_pkt_prepare() are ready,
we will remove the checks from the Tx burst handler.
BNXT PMD will carry the current checks till then.
smime.p7s
Description: S/MIME Cryptographic Signature
