[PATCH 3/3] test/event_crypto_adapter: fix crypto null dev creation

[Gujjar, Abhinandan S]

Hi Anoob,

> -----Original Message-----
> From: Anoob Joseph <anoobj at marvell.com>
> Sent: Wednesday, August 23, 2023 11:17 AM
> To: Gujjar, Abhinandan S <abhinandan.gujjar at intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Shijith Thotton
> <sthotton at marvell.com>; dev at dpdk.org; Aakash Sasidharan
> <asasidharan at marvell.com>; Akhil Goyal <gakhil at marvell.com>
> Subject: RE: [PATCH 3/3] test/event_crypto_adapter: fix crypto null dev
> creation
> 
> Hi Abhinandan,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Gujjar, Abhinandan S <abhinandan.gujjar at intel.com>
> > Sent: Wednesday, August 23, 2023 10:19 AM
> > To: Aakash Sasidharan <asasidharan at marvell.com>; Akhil Goyal
> > <gakhil at marvell.com>
> > Cc: Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Shijith Thotton
> > <sthotton at marvell.com>; Anoob Joseph <anoobj at marvell.com>;
> > dev at dpdk.org
> > Subject: [EXT] RE: [PATCH 3/3] test/event_crypto_adapter: fix crypto
> > null dev creation
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> >
> >
> > > -----Original Message-----
> > > From: Aakash Sasidharan <asasidharan at marvell.com>
> > > Sent: Tuesday, August 22, 2023 6:47 PM
> > > To: Gujjar, Abhinandan S <abhinandan.gujjar at intel.com>; Akhil Goyal
> > > <gakhil at marvell.com>
> > > Cc: jerinj at marvell.com; sthotton at marvell.com; anoobj at marvell.com;
> > > dev at dpdk.org; asasidharan at marvell.com
> > > Subject: [PATCH 3/3] test/event_crypto_adapter: fix crypto null dev
> > > creation
> > >
> > > Create crypto null device only if no other crypto devices were found.
> > >
> > > Fixes: 3c2c535ecfc0 ("test: add event crypto adapter auto-test")
> > > Cc: abhinandan.gujjar at intel.com
> > >
> > > Signed-off-by: Aakash Sasidharan <asasidharan at marvell.com>
> > > ---
> > >  app/test/test_event_crypto_adapter.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/app/test/test_event_crypto_adapter.c
> > > b/app/test/test_event_crypto_adapter.c
> > > index 00b20fa443..0c56744ba0 100644
> > > --- a/app/test/test_event_crypto_adapter.c
> > > +++ b/app/test/test_event_crypto_adapter.c
> > > @@ -1072,11 +1072,10 @@ configure_cryptodev(void)
> > >  		return TEST_FAILED;
> > >  	}
> > >
> > > -	/* Create a NULL crypto device */
> > > -	nb_devs = rte_cryptodev_device_count_by_driver(
> > > -			rte_cryptodev_driver_id_get(
> > > -			RTE_STR(CRYPTODEV_NAME_NULL_PMD)));
> > > +
> > > +	nb_devs = rte_cryptodev_count();
> > The test is intended to do on NULL cipher with NULL PMD.
> 
> [Anoob] This test is intended to verify any cryptodev with crypto adapter.
> Why do we want to restrict the usage with NULL devices only?
No such restriction. Please refer below comment.
> 
> > Right now, the test does cipher with " RTE_CRYPTO_CIPHER_NULL".
> 
> [Anoob] It has support for asymmetric cases also.
Well, I was talking about cipher algo used in the test in the context of keeping it simple to verify the crypto adapter as it is agnostic to any underlying device.
Not sym vs asym support.
> 
> > With this changes, does this test work, if you provide any other
> > cryptodev like AESNI, QAT, OpenSSL other than NULL PMD?
> 
> [Anoob] If NULL cipher is supported by those PMDs, it should be. Because,
> this being a standard test application should stick to standard semantics and
> so shouldn't care about whether the PMD under test is NULL or QAT. If NULL
> cipher is not supported by the PMD, then the case should get skipped at
> capability layer. Or are you saying that crypto adapter is only supposed to be
> used with NULL PMD?
It is not about supporting other PMDs.
This is about keeping the test case simple with minimal conf overhead so that it is easy for testing and maintaining.
>From the code, I see that few PMDs supports NULL too without much changes to the app. It should be ok.
> 
> Also, what we have seen is, if there are other cryptodevs (like cnxk), that
> would be the first cryptodev. And the test application is only testing CDEV_ID
> 0. So creating a NULL crypto PMD when other PMDs are available is pointless.
Sounds good.

Acked-by: Abhinandan Gujjar <abhinandan.gujjar at intel.com>
> 
> > >  	if (!nb_devs) {
> > > +		/* Create a NULL crypto device */
> > >  		ret = rte_vdev_init(
> > >  			RTE_STR(CRYPTODEV_NAME_NULL_PMD), NULL);
> > >
> > > --
> > > 2.25.1