Re: [PATCH v7 5/7] test: add event timer adapter auto-test

["Carrillo, Erik G" <erik.g.carrillo@intel.com>]

Hi Pavan,

> -----Original Message-----
> From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Wednesday, March 14, 2018 8:31 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>;
> jerin.jacob@caviumnetworks.com; nipun.gupta@nxp.com;
> hemant.agrawal@nxp.com
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v7 5/7] test: add event timer adapter auto-test
> 
> Hi Gabriel,
> 
> Please make sure that the unit tests are generic, I could see that some places
> it is verifying whether event port is used or service cores are used, but
> doesn't verify if actually event port/service core are needed i.e.
> INTERNAL_PORT capability.

Good point... I'll make these updates.

Thanks for reviewing,
Gabriel

> 
> On Thu, Mar 08, 2018 at 03:54:04PM -0600, Erik Gabriel Carrillo wrote:
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > ---
> >  test/test/Makefile                   |    1 +
> >  test/test/test_event_timer_adapter.c | 1234
> > ++++++++++++++++++++++++++++++++++
> >  2 files changed, 1235 insertions(+)
> >  create mode 100644 test/test/test_event_timer_adapter.c
> >
> <snip>
> > +
> > +/* This port conf callback is used by the max adapter instance creation
> test.
> > + * Because that test may be limited by the number of ports available
> > +in the
> > + * event device, this callback allocates just one port and returns it
> > +each
> > + * time a port is requested.
> > + */
> > +static int
> > +test_port_conf_cb(uint16_t id, uint8_t event_dev_id, uint8_t
> *event_port_id,
> > +		  void *conf_arg)
> > +{
> > +	struct rte_eventdev *dev;
> > +	struct rte_event_dev_config dev_conf;
> > +	struct rte_event_port_conf *port_conf, def_port_conf = {0};
> > +	int started;
> > +	static int port_allocated;
> > +	static uint8_t port_id;
> > +	uint8_t dev_id;
> > +	int ret;
> > +
> > +	if (port_allocated) {
> > +		*event_port_id = port_id;
> > +		return 0;
> > +	}
> > +
> > +	RTE_SET_USED(id);
> > +
> > +	dev = &rte_eventdevs[event_dev_id];
> 
> I don't think this is the correct way of accessing event dev information i.e.
> accessing the global rte_eventdevs structure from a application.
> 
> > +	dev_id = dev->data->dev_id;
> > +	dev_conf = dev->data->dev_conf;
> > +
> > +	started = dev->data->dev_started;
> > +	if (started)
> > +		rte_event_dev_stop(dev_id);
> > +
> > +	port_id = dev_conf.nb_event_ports;
> > +	dev_conf.nb_event_ports += 1;
> > +	ret = rte_event_dev_configure(dev_id, &dev_conf);
> > +	if (ret < 0) {
> > +		if (started)
> > +			rte_event_dev_start(dev_id);
> Shouldn't this be !started ?. The same pattern repeats a few places.
> 
> > +
> > +		return ret;
> > +	}
> > +
> > +	if (conf_arg != NULL)
> > +		port_conf = conf_arg;
> > +	else {
> > +		port_conf = &def_port_conf;
> > +		ret = rte_event_port_default_conf_get(dev_id, port_id,
> > +						      port_conf);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	ret = rte_event_port_setup(dev_id, port_id, port_conf);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*event_port_id = port_id;
> > +
> > +	if (started)
> > +		rte_event_dev_start(dev_id);
> > +
> > +	port_allocated = 1;
> > +
> > +	return 0;
> > +}
> <snip>
> 
> Thanks,
> Pavan.