RE: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting crypto callbacks macro

[Akhil Goyal <gakhil@marvell.com>]

> > From: Kundapura, Ganapati [mailto:ganapati.kundapura@intel.com]
> > Sent: Thursday, 30 May 2024 16.22
> >
> > Hi,
> >
> > > From: Akhil Goyal <gakhil@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.

Regards,
Akhil