Hi Akhil, > -----Original Message----- > From: Akhil Goyal <gak...@marvell.com> > Sent: Thursday, June 13, 2024 11:34 PM > To: Kundapura, Ganapati <ganapati.kundap...@intel.com>; dev@dpdk.org; > Gujjar, Abhinandan S <abhinandan.guj...@intel.com>; Mcnamara, John > <john.mcnam...@intel.com>; Richardson, Bruce > <bruce.richard...@intel.com> > Cc: Morten Brørup <m...@smartsharesystems.com>; ferruh.yi...@amd.com; > fanzhang....@gmail.com; tho...@monjalon.net > Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting > crypto callbacks macro > > > > > From: Kundapura, Ganapati [mailto:ganapati.kundap...@intel.com] > > > > Sent: Thursday, 30 May 2024 16.22 > > > > > > > > Hi, > > > > > > > > > From: Akhil Goyal <gak...@marvell.com> > > > > > Sent: Thursday, May 30, 2024 5:17 PM > > > > > > > > > > > > > #if may not be needed in application. > > > > > > > > Test should be skipped if API is not available/supported. > > > > > > > > > > > > > > It's needed otherwise application developer has to check the > > > > > > implementation for supported/not supported or else run the > > > > > > application to get to know whether api is supported or not. > > > > > > > > > > > > > > > > Application is always required to check the return value or else > > > > > it will > > > > miss the > > > > > other errors that the API can return. > > > > Currently RTE_CRYPTO_CALLBACKS is enabled by default and test > > > > application checks the return value of the APIs. This patch fixes > > > > build issues on compiling the DPDK with unsetting > > > > RTE_CRYPTO_CALLBACKS. > > > > > > > > > > > > > > diff --git a/lib/cryptodev/rte_cryptodev.c > > > > > > > > > b/lib/cryptodev/rte_cryptodev.c index 886eb7a..2e0890f > > > > > > > > > 100644 > > > > > > > > > --- a/lib/cryptodev/rte_cryptodev.c > > > > > > > > > +++ b/lib/cryptodev/rte_cryptodev.c > > > > > > > > > @@ -628,6 +628,7 @@ > > > > > > > rte_cryptodev_asym_xform_capability_check_hash( > > > > > > > > > return ret; > > > > > > > > > } > > > > > > > > > > > > > > > > > > +#if RTE_CRYPTO_CALLBACKS > > > > > > > > > /* spinlock for crypto device enq callbacks */ static > > > > > > > > > rte_spinlock_t rte_cryptodev_callback_lock = > > > > > > > > RTE_SPINLOCK_INITIALIZER; > > > > > > > > > > > > > > > > > > @@ -744,6 +745,7 @@ cryptodev_cb_init(struct > > > > > > > > > rte_cryptodev > > *dev) > > > > > > > > > cryptodev_cb_cleanup(dev); > > > > > > > > > return -ENOMEM; > > > > > > > > > } > > > > > > > > > +#endif /* RTE_CRYPTO_CALLBACKS */ > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1485,6 +1491,7 @@ > > > > > > > > > rte_cryptodev_queue_pair_setup(uint8_t > > > > > > > dev_id, > > > > > > > > > uint16_t queue_pair_id, > > > > > > > > > socket_id); > > > > > > > > > } > > > > > > > > > > > > > > > > > > +#if RTE_CRYPTO_CALLBACKS > > > > > > > > > struct rte_cryptodev_cb * > > > > > > > > > rte_cryptodev_add_enq_callback(uint8_t dev_id, > > > > > > > > > uint16_t qp_id, @@ -1763,6 > > > > > > > > > +1770,7 @@ > > > > > rte_cryptodev_remove_deq_callback(uint8_t > > > > > > > dev_id, > > > > > > > > > rte_spinlock_unlock(&rte_cryptodev_callback_lock); > > > > > > > > > return ret; > > > > > > > > > } > > > > > > > > > +#endif /* RTE_CRYPTO_CALLBACKS */ > > > > > > > > > > > > > > > > There is an issue here. > > > > > > > > The APIs are visible in .h file and are available for > > > > > > > > application to > > > > use. > > > > > > > > But the API implementation is compiled out. > > > > > > > > Rather, you should add a return ENOTSUP from the beginning > > > > > > > > of the APIs if RTE_CRYPTO_CALLBACKS is enabled. > > > > > > > > With this approach application will not need to put #if in its > > > > > > > > code. > > > > > > API declarations wrapped under the macro changes in next patch. > > > > > > > > > > No, that is not the correct way. Application should check the return > value. > > > > > And we cannot force it to add ifdefs. > > > > Test application is indeed checking the return value. Ifdefs are > > > > added to avoid build issues on compiling with RTE_CRYPTO_CALLBACKS > > > > is turned off Which is on by default. > > > > > > The test application should be able to build and run, regardless if > > > the DPDK > > library > > > was built with RTE_CRYPTO_CALLBACKS defined or not. > > > > > > The test application should not assume that the DPDK library was > > > built with the same RTE_CRYPTO_CALLBACKS configuration (i.e. defined > > > or not) as the test application. > > > > > > > Even ethdev callbacks also doesn't return -ENOTSUP on > > > > setting/unsetting RTE_ETHDEV_RXTX_CALLBACKS config. > > > > > > That would be a bug in the ethdev library. > > > I just checked the ethdev source code > > > (/source/lib/ethdev/rte_ethdev.c), and > > all > > > the add/remove rx/tx callback functions fail with ENOTSUP if > > > RTE_ETHDEV_RXTX_CALLBACKS is not defined. > > > Please note that some ethdev callbacks are not rx/tx callbacks, and > > > thus are not gated by RTE_ETHDEV_RXTX_CALLBACKS. > > > > Hi Ganapati, > > Can you send a new version incorporating above comments and work on > > similar lines as ethdev is currently doing. > > > > I believe as Morten pointed out, use of ifdef is as per DPDK > > convention, So better move it that way. > > We can discuss later if we can incorporate these in meson options. > > > Any update on this? Working on it, will post the patch soon.
Thanks, Ganapati
