Re: [dpdk-dev] [PATCH v3 6/6] raw/octeontx2_ep: add driver self test

[Mahipal Challa <mchalla@marvell.com>]

Hi Gavin,
Please see response inline

> -----Original Message-----
> From: Gavin Hu <Gavin.Hu@arm.com>
> Sent: Tuesday, January 7, 2020 8:13 AM
> To: Mahipal Challa <mchalla@marvell.com>; dev@dpdk.org
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Narayana Prasad Raju
> Athreya <pathreya@marvell.com>; Subrahmanyam Nilla
> <snilla@marvell.com>; Venkateshwarlu Nalla <venkatn@marvell.com>; nd
> <nd@arm.com>
> Subject: [EXT] RE: [dpdk-dev] [PATCH v3 6/6] raw/octeontx2_ep: add driver
> self test
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Mahipal,
> 
> > -----Original Message-----
> > From: Mahipal Challa <mchalla@marvell.com>
> > Sent: Monday, January 6, 2020 8:07 PM
> > To: dev@dpdk.org
> > Cc: jerinj@marvell.com; pathreya@marvell.com; snilla@marvell.com;
> > venkatn@marvell.com; Gavin Hu <Gavin.Hu@arm.com>
> > Subject: [dpdk-dev] [PATCH v3 6/6] raw/octeontx2_ep: add driver self
> > test
> >
> > Add rawdev's selftest feature in SDP VF driver, which verifies the EP
> > mode functionality test.
> >
> > Signed-off-by: Mahipal Challa <mchalla@marvell.com>
> > ---
> >  doc/guides/rawdevs/octeontx2_ep.rst       |  13 +++
> >  drivers/raw/octeontx2_ep/Makefile         |   1 +
> >  drivers/raw/octeontx2_ep/meson.build      |   1 +
> >  drivers/raw/octeontx2_ep/otx2_ep_rawdev.c |   1 +
> >  drivers/raw/octeontx2_ep/otx2_ep_rawdev.h |   2 +
> >  drivers/raw/octeontx2_ep/otx2_ep_test.c   | 166
> > ++++++++++++++++++++++++++++++
> >  6 files changed, 184 insertions(+)
> >
> > diff --git a/doc/guides/rawdevs/octeontx2_ep.rst
> > b/doc/guides/rawdevs/octeontx2_ep.rst
> > index 39a7c29..bbcf530 100644
> > --- a/doc/guides/rawdevs/octeontx2_ep.rst
> > +++ b/doc/guides/rawdevs/octeontx2_ep.rst
> > @@ -74,3 +74,16 @@ Performing Data Transfer  To perform data transfer
> > using SDP VF EP rawdev devices use standard
> > ``rte_rawdev_enqueue_buffers()`` and ``rte_rawdev_dequeue_buffers()``
> > APIs.
> >
> > +Self test
> > +---------
> > +
> > +On EAL initialization, SDP VF devices will be probed and populated
> > +into the raw devices. The rawdev ID of the device can be obtained
> > +using
> > +
> > +* Invoke ``rte_rawdev_get_dev_id("SDPEP:x")`` from the test
> > +application
> > +  where x is the VF device's bus id specified in
> > +"bus:device.func"(BDF)
> > +  format. Use this index for further rawdev function calls.
> > +
> > +* The driver's selftest rawdev API can be used to verify the SDP EP
> > +mode
> > +  functional tests which can send/receive the raw data packets
> > +to/from the
> > +  EP device.
> > diff --git a/drivers/raw/octeontx2_ep/Makefile
> > b/drivers/raw/octeontx2_ep/Makefile
> > index 02853fb..44fdf89 100644
> > --- a/drivers/raw/octeontx2_ep/Makefile
> > +++ b/drivers/raw/octeontx2_ep/Makefile
> > @@ -37,6 +37,7 @@ LIBABIVER := 1
> >  #
> >  SRCS-$(CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV) +=
> otx2_ep_rawdev.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV) +=
> otx2_ep_enqdeq.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV) +=
> > otx2_ep_test.c
> >  SRCS-$(CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV) +=
> otx2_ep_vf.c
> >
> >
> > diff --git a/drivers/raw/octeontx2_ep/meson.build
> > b/drivers/raw/octeontx2_ep/meson.build
> > index 99e6c6d..0e6338f 100644
> > --- a/drivers/raw/octeontx2_ep/meson.build
> > +++ b/drivers/raw/octeontx2_ep/meson.build
> > @@ -5,4 +5,5 @@
> >  deps += ['bus_pci', 'common_octeontx2', 'rawdev']  sources =
> > files('otx2_ep_rawdev.c',
> >  		'otx2_ep_enqdeq.c',
> > +		'otx2_ep_test.c',
> >  		'otx2_ep_vf.c')
> > diff --git a/drivers/raw/octeontx2_ep/otx2_ep_rawdev.c
> > b/drivers/raw/octeontx2_ep/otx2_ep_rawdev.c
> > index 7158b97..0778603 100644
> > --- a/drivers/raw/octeontx2_ep/otx2_ep_rawdev.c
> > +++ b/drivers/raw/octeontx2_ep/otx2_ep_rawdev.c
> > @@ -253,6 +253,7 @@
> >  	.dev_close      = sdp_rawdev_close,
> >  	.enqueue_bufs   = sdp_rawdev_enqueue,
> >  	.dequeue_bufs   = sdp_rawdev_dequeue,
> > +	.dev_selftest   = sdp_rawdev_selftest,
> >  };
> >
> >  static int
> > diff --git a/drivers/raw/octeontx2_ep/otx2_ep_rawdev.h
> > b/drivers/raw/octeontx2_ep/otx2_ep_rawdev.h
> > index a77cbab..dab2fb7 100644
> > --- a/drivers/raw/octeontx2_ep/otx2_ep_rawdev.h
> > +++ b/drivers/raw/octeontx2_ep/otx2_ep_rawdev.h
> > @@ -494,4 +494,6 @@ int sdp_rawdev_enqueue(struct rte_rawdev *dev,
> > struct rte_rawdev_buf **buffers,  int sdp_rawdev_dequeue(struct
> > rte_rawdev *dev, struct rte_rawdev_buf **buffers,
> >  		       unsigned int count, rte_rawdev_obj_t context);
> >
> > +int sdp_rawdev_selftest(uint16_t dev_id);
> > +
> >  #endif /* _OTX2_EP_RAWDEV_H_ */
> > diff --git a/drivers/raw/octeontx2_ep/otx2_ep_test.c
> > b/drivers/raw/octeontx2_ep/otx2_ep_test.c
> > new file mode 100644
> > index 0000000..fc913a6
> > --- /dev/null
> > +++ b/drivers/raw/octeontx2_ep/otx2_ep_test.c
> > @@ -0,0 +1,166 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2019 Marvell International Ltd.
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +#include <rte_common.h>
> > +#include <rte_eal.h>
> > +#include <rte_lcore.h>
> > +#include <rte_mempool.h>
> > +
> > +#include <rte_rawdev.h>
> > +#include <rte_rawdev_pmd.h>
> > +
> > +#include "otx2_common.h"
> > +#include "otx2_ep_rawdev.h"
> > +
> > +#define SDP_IOQ_NUM_BUFS   (4 * 1024)
> > +#define SDP_IOQ_BUF_SIZE   (2 * 1024)
> > +
> > +#define SDP_TEST_PKT_FSZ   (0)
> > +#define SDP_TEST_PKT_SIZE  (1024)
> > +
> > +static int
> > +sdp_validate_data(struct sdp_droq_pkt *oq_pkt, uint8_t *iq_pkt,
> > +		  uint32_t pkt_len)
> > +{
> > +	if (!oq_pkt)
> > +		return -EINVAL;
> > +
> > +	if (pkt_len != oq_pkt->len) {
> > +		otx2_err("Invalid packet length");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (memcmp(oq_pkt->data, iq_pkt, pkt_len) != 0) {
> > +		otx2_err("Data validation failed");
> > +		return -EINVAL;
> > +	}
> > +	otx2_sdp_dbg("Data validation successful");
> > +
> > +	return 0;
> > +}
> > +
> > +static void
> > +sdp_ioq_buffer_fill(uint8_t *addr, uint32_t len) {
> > +	uint32_t idx;
> > +
> > +	memset(addr, 0, len);
> > +
> > +	for (idx = 0; idx < len; idx++)
> > +		addr[idx] = idx;
> > +}
> > +
> > +static struct rte_mempool*
> > +sdp_ioq_mempool_create(void)
> > +{
> > +	struct rte_mempool *mpool;
> > +
> > +	mpool = rte_mempool_create("ioqbuf_pool",
> > +				   SDP_IOQ_NUM_BUFS /*num elt*/,
> > +				   SDP_IOQ_BUF_SIZE /*elt size*/,
> > +				   0 /*cache_size*/,
> > +				   0 /*private_data_size*/,
> > +				   NULL /*mp_init*/,
> > +				   NULL /*mp_init arg*/,
> > +				   NULL /*obj_init*/,
> > +				   NULL /*obj_init arg*/,
> > +				   rte_socket_id() /*socket id*/,
> > +				   (MEMPOOL_F_SP_PUT |
> > MEMPOOL_F_SC_GET));
> > +
> > +	return mpool;
> > +}
> > +
> > +
> > +int
> > +sdp_rawdev_selftest(uint16_t dev_id)
> > +{
> > +	struct sdp_rawdev_info app_info = {0};
> > +	struct rte_rawdev_info dev_info = {0};
> > +
> > +	struct rte_rawdev_buf *d_buf[1];
> > +	struct sdp_droq_pkt oq_pkt;
> > +	struct sdp_soft_instr si;
> > +	struct sdp_device sdpvf;
> > +
> > +	uint32_t buf_size;
> > +	int ret = 0;
> > +	void *buf;
> > +
> > +	otx2_info("SDP RAWDEV Self Test: Started");
> > +
> > +	memset(&oq_pkt, 0x00, sizeof(oq_pkt));
> > +	d_buf[0] = (struct rte_rawdev_buf *)&oq_pkt;
> > +
> > +	struct rte_mempool *ioq_mpool = sdp_ioq_mempool_create();
> > +	if (!ioq_mpool) {
> > +		otx2_err("IOQ mpool creation failed");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	app_info.enqdeq_mpool = ioq_mpool;
> > +	app_info.app_conf = NULL; /* Use default conf */
> > +
> > +	dev_info.dev_private = &app_info;
> > +
> > +	ret = rte_rawdev_configure(dev_id, &dev_info);
> > +	if (ret) {
> > +		otx2_err("Unable to configure SDP_VF %d", dev_id);
> 
> The ioq_mpool was still not freed in the failure path?
[Mahipal]: Will handle it in v4, but I feel there is no harm with this as this is a test API for a minimal test application which exits end of this test, anyway will handle it.

> I see similar possible leakage in the other patches.
[Mahipal]: Assuming you are talking about the comment you gave in v2, Please see the response to the same. We have handled all the err handling, it is part of the  patch " [v3,3/6] raw/octeontx2_ep: add device uninitialization". 

If you still find anymore please feel free  to point me.

> 
> > +		return -ENOMEM;
> > +	}
> > +	otx2_info("SDP VF rawdev[%d] configured successfully", dev_id);
> > +
> > +	memset(&si, 0x00, sizeof(si));
> > +	memset(&sdpvf, 0x00, sizeof(sdpvf));
> > +
> > +	buf_size = SDP_TEST_PKT_SIZE;
> > +
> > +	si.q_no = 0;
> > +	si.reqtype = SDP_REQTYPE_NORESP;
> > +	si.rptr = NULL;
> > +
> > +	si.ih.fsz = SDP_TEST_PKT_FSZ;
> > +	si.ih.tlen = buf_size;
> > +	si.ih.gather = 0;
> > +
> > +	/* Enqueue raw pkt data */
> > +	rte_mempool_get(ioq_mpool, &buf);
> > +	if (!buf) {
> > +		otx2_err("Buffer allocation failed");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	sdp_ioq_buffer_fill(buf, buf_size);
> > +	si.dptr = (uint8_t *)buf;
> > +
> > +	rte_rawdev_enqueue_buffers(dev_id, NULL, 1, &si);
> > +	usleep(10000);
> > +
> > +	/* Dequeue raw pkt data */
> > +	ret = 0;
> > +	while (ret < 1) {
> > +		ret = rte_rawdev_dequeue_buffers(dev_id, &d_buf[0], 1,
> > &si);
> > +		rte_pause();
> > +	}
> > +
> > +	/* Validate the dequeued raw pkt data */
> > +	if (sdp_validate_data((struct sdp_droq_pkt *)d_buf[0],
> > +			      buf, buf_size) != 0) {
> > +		otx2_err("Data invalid");
> > +		return -EINVAL;
> > +	}
> > +
> > +	rte_mempool_put(ioq_mpool, ((struct sdp_droq_pkt *)d_buf[0])-
> > >data);
> > +	if (ioq_mpool)
> > +		rte_mempool_free(ioq_mpool);
> > +
> > +	otx2_info("SDP RAWDEV Self Test: Successful");
> > +
> > +	return 0;
> > +}
> > +
> > --
> > 1.8.3.1


Thanks,
Mahipal