diff mbox series

[v2,1/2] crypto: fix build issues on unsetting crypto callbacks macro

Message ID 20240529144025.4089318-1-ganapati.kundapura@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: akhil goyal
Headers
Series [v2,1/2] crypto: fix build issues on unsetting crypto callbacks macro |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Kundapura, Ganapati May 29, 2024, 2:40 p.m. UTC 
Crypto callbacks macro is defined with value 1 and being used with ifdef,
on config value is changed to 0 to disable, crypto callback changes
still being compiled.

Used #if instead of #ifdef and also wrapped crypto callback changes
under RTE_CRYPTO_CALLBACKS macro to fix build issues when macro is
unset.

Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")
Fixes: 5523a75af539 ("test/crypto: add case for enqueue/dequeue callbacks")

Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
---
v2:
* Used #if instead of #ifdef and restored macro definition in config
* Split callback registration check in a seperate patch

Comments

Akhil Goyal May 30, 2024, 8:12 a.m. UTC | #1 
> -----Original Message-----
> From: Ganapati Kundapura <ganapati.kundapura@intel.com>
> Sent: Wednesday, May 29, 2024 8:10 PM
> To: dev@dpdk.org; Akhil Goyal <gakhil@marvell.com>;
> abhinandan.gujjar@intel.com; ferruh.yigit@amd.com; thomas@monjalon.net;
> bruce.richardson@intel.com; fanzhang.oss@gmail.com; ciara.power@intel.com
> Subject: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting crypto
> callbacks macro
> 
> Prioritize security for external emails: Confirm sender and content safety before
> clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> Crypto callbacks macro is defined with value 1 and being used with ifdef,
> on config value is changed to 0 to disable, crypto callback changes
> still being compiled.
> 
> Used #if instead of #ifdef and also wrapped crypto callback changes
> under RTE_CRYPTO_CALLBACKS macro to fix build issues when macro is
> unset.
> 
> Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")
> Fixes: 5523a75af539 ("test/crypto: add case for enqueue/dequeue callbacks")
> 
> Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> ---
> v2:
> * Used #if instead of #ifdef and restored macro definition in config
> * Split callback registration check in a seperate patch
> 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> index 1703ebc..72cf77f 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -14547,6 +14547,7 @@ test_null_burst_operation(void)
>  	return TEST_SUCCESS;
>  }
> 
> +#if RTE_CRYPTO_CALLBACKS
>  static uint16_t
>  test_enq_callback(uint16_t dev_id, uint16_t qp_id, struct rte_crypto_op **ops,
>  		  uint16_t nb_ops, void *user_param)
> @@ -14784,6 +14785,7 @@ test_deq_callback_setup(void)
> 
>  	return TEST_SUCCESS;
>  }
> +#endif /* RTE_CRYPTO_CALLBACKS */
> 
>  static void
>  generate_gmac_large_plaintext(uint8_t *data)
> @@ -18069,8 +18071,10 @@ static struct unit_test_suite
> cryptodev_gen_testsuite  = {
>  		TEST_CASE_ST(ut_setup, ut_teardown,
>  				test_device_configure_invalid_queue_pair_ids),
>  		TEST_CASE_ST(ut_setup, ut_teardown, test_stats),
> +#if RTE_CRYPTO_CALLBACKS
>  		TEST_CASE_ST(ut_setup, ut_teardown,
> test_enq_callback_setup),
>  		TEST_CASE_ST(ut_setup, ut_teardown,
> test_deq_callback_setup),
> +#endif
>  		TEST_CASES_END() /**< NULL terminate unit test array */
>  	}
>  };

#if may not be needed in application.
Test should be skipped if API is not available/supported.

> 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.
Akhil Goyal May 30, 2024, 11:01 a.m. UTC | #2 
++ Morten for comment on #if vs #ifdef

> > Subject: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting crypto
> > callbacks macro
> >
> > Crypto callbacks macro is defined with value 1 and being used with ifdef,
> > on config value is changed to 0 to disable, crypto callback changes
> > still being compiled.
> >
> > Used #if instead of #ifdef and also wrapped crypto callback changes
> > under RTE_CRYPTO_CALLBACKS macro to fix build issues when macro is
> > unset.
> >
> > Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")
> > Fixes: 5523a75af539 ("test/crypto: add case for enqueue/dequeue callbacks")
> >
> > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> > ---
> > v2:
> > * Used #if instead of #ifdef and restored macro definition in config
> > * Split callback registration check in a seperate patch
> >
> > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > index 1703ebc..72cf77f 100644
> > --- a/app/test/test_cryptodev.c
> > +++ b/app/test/test_cryptodev.c
> > @@ -14547,6 +14547,7 @@ test_null_burst_operation(void)
> >  	return TEST_SUCCESS;
> >  }
> >
> > +#if RTE_CRYPTO_CALLBACKS
> >  static uint16_t
> >  test_enq_callback(uint16_t dev_id, uint16_t qp_id, struct rte_crypto_op **ops,
> >  		  uint16_t nb_ops, void *user_param)
> > @@ -14784,6 +14785,7 @@ test_deq_callback_setup(void)
> >
> >  	return TEST_SUCCESS;
> >  }
> > +#endif /* RTE_CRYPTO_CALLBACKS */
> >
> >  static void
> >  generate_gmac_large_plaintext(uint8_t *data)
> > @@ -18069,8 +18071,10 @@ static struct unit_test_suite
> > cryptodev_gen_testsuite  = {
> >  		TEST_CASE_ST(ut_setup, ut_teardown,
> >  				test_device_configure_invalid_queue_pair_ids),
> >  		TEST_CASE_ST(ut_setup, ut_teardown, test_stats),
> > +#if RTE_CRYPTO_CALLBACKS
> >  		TEST_CASE_ST(ut_setup, ut_teardown,
> > test_enq_callback_setup),
> >  		TEST_CASE_ST(ut_setup, ut_teardown,
> > test_deq_callback_setup),
> > +#endif
> >  		TEST_CASES_END() /**< NULL terminate unit test array */
> >  	}
> >  };
> 
> #if may not be needed in application.
> Test should be skipped if API is not available/supported.
> 
> > 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.
Morten Brørup May 30, 2024, 11:13 a.m. UTC | #3 
> From: Akhil Goyal [mailto:gakhil@marvell.com]
> Sent: Thursday, 30 May 2024 13.02
> 
> ++ Morten for comment on #if vs #ifdef
> 
> > > Subject: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting
> crypto
> > > callbacks macro
> > >
> > > Crypto callbacks macro is defined with value 1 and being used with ifdef,
> > > on config value is changed to 0 to disable, crypto callback changes
> > > still being compiled.
> > >
> > > Used #if instead of #ifdef and also wrapped crypto callback changes
> > > under RTE_CRYPTO_CALLBACKS macro to fix build issues when macro is
> > > unset.
> > >
> > > Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")
> > > Fixes: 5523a75af539 ("test/crypto: add case for enqueue/dequeue
> callbacks")
> > >
> > > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> > > ---
> > > v2:
> > > * Used #if instead of #ifdef and restored macro definition in config
> > > * Split callback registration check in a seperate patch

The DPDK convention is #ifdef, not #if.
Ethdev also uses #ifdef, e.g.:
https://elixir.bootlin.com/dpdk/v24.03/source/lib/ethdev/rte_ethdev.h#L6112

PS: Personally, I prefer #if too. But we should set personal preferences aside, and follow the existing convention in DPDK and use #ifdef.
Kundapura, Ganapati May 30, 2024, 11:40 a.m. UTC | #4 
Hi

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, May 30, 2024 4:32 PM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>; dev@dpdk.org;
> Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; ferruh.yigit@amd.com;
> thomas@monjalon.net; Richardson, Bruce <bruce.richardson@intel.com>;
> fanzhang.oss@gmail.com; ciara.power@intel.com; Morten Brørup
> <mb@smartsharesystems.com>
> Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting
> crypto callbacks macro
> 
> ++ Morten for comment on #if vs #ifdef
> 
> > > Subject: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on
> > > unsetting crypto callbacks macro
> > >
> > > Crypto callbacks macro is defined with value 1 and being used with
> > > ifdef, on config value is changed to 0 to disable, crypto callback
> > > changes still being compiled.
> > >
> > > Used #if instead of #ifdef and also wrapped crypto callback changes
> > > under RTE_CRYPTO_CALLBACKS macro to fix build issues when macro is
> > > unset.
> > >
> > > Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")
> > > Fixes: 5523a75af539 ("test/crypto: add case for enqueue/dequeue
> > > callbacks")
> > >
> > > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> > > ---
> > > v2:
> > > * Used #if instead of #ifdef and restored macro definition in config
> > > * Split callback registration check in a seperate patch
> > >
> > > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > > index 1703ebc..72cf77f 100644
> > > --- a/app/test/test_cryptodev.c
> > > +++ b/app/test/test_cryptodev.c
> > > @@ -14547,6 +14547,7 @@ test_null_burst_operation(void)
> > >  	return TEST_SUCCESS;
> > >  }
> > >
> > > +#if RTE_CRYPTO_CALLBACKS
> > >  static uint16_t
> > >  test_enq_callback(uint16_t dev_id, uint16_t qp_id, struct rte_crypto_op
> **ops,
> > >  		  uint16_t nb_ops, void *user_param) @@ -14784,6
> +14785,7 @@
> > > test_deq_callback_setup(void)
> > >
> > >  	return TEST_SUCCESS;
> > >  }
> > > +#endif /* RTE_CRYPTO_CALLBACKS */
> > >
> > >  static void
> > >  generate_gmac_large_plaintext(uint8_t *data) @@ -18069,8 +18071,10
> > > @@ static struct unit_test_suite cryptodev_gen_testsuite  = {
> > >  		TEST_CASE_ST(ut_setup, ut_teardown,
> > >
> 	test_device_configure_invalid_queue_pair_ids),
> > >  		TEST_CASE_ST(ut_setup, ut_teardown, test_stats),
> > > +#if RTE_CRYPTO_CALLBACKS
> > >  		TEST_CASE_ST(ut_setup, ut_teardown,
> test_enq_callback_setup),
> > >  		TEST_CASE_ST(ut_setup, ut_teardown,
> test_deq_callback_setup),
> > > +#endif
> > >  		TEST_CASES_END() /**< NULL terminate unit test array */
> > >  	}
> > >  };
> >
> > #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.

> > > 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.
Kundapura, Ganapati May 30, 2024, 11:41 a.m. UTC | #5 
Hi,

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, May 30, 2024 4:44 PM
> To: Akhil Goyal <gakhil@marvell.com>; Kundapura, Ganapati
> <ganapati.kundapura@intel.com>; dev@dpdk.org; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>; ferruh.yigit@amd.com;
> thomas@monjalon.net; Richardson, Bruce <bruce.richardson@intel.com>;
> fanzhang.oss@gmail.com; ciara.power@intel.com
> Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting
> crypto callbacks macro
> 
> > From: Akhil Goyal [mailto:gakhil@marvell.com]
> > Sent: Thursday, 30 May 2024 13.02
> >
> > ++ Morten for comment on #if vs #ifdef
> >
> > > > Subject: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on
> > > > unsetting
> > crypto
> > > > callbacks macro
> > > >
> > > > Crypto callbacks macro is defined with value 1 and being used with
> > > > ifdef, on config value is changed to 0 to disable, crypto callback
> > > > changes still being compiled.
> > > >
> > > > Used #if instead of #ifdef and also wrapped crypto callback
> > > > changes under RTE_CRYPTO_CALLBACKS macro to fix build issues when
> > > > macro is unset.
> > > >
> > > > Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue
> > > > callbacks")
> > > > Fixes: 5523a75af539 ("test/crypto: add case for enqueue/dequeue
> > callbacks")
> > > >
> > > > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> > > > ---
> > > > v2:
> > > > * Used #if instead of #ifdef and restored macro definition in
> > > > config
> > > > * Split callback registration check in a seperate patch
> 
> The DPDK convention is #ifdef, not #if.
> Ethdev also uses #ifdef, e.g.:
> https://elixir.bootlin.com/dpdk/v24.03/source/lib/ethdev/rte_ethdev.h#L61
> 12
> 
> PS: Personally, I prefer #if too. But we should set personal preferences aside,
> and follow the existing convention in DPDK and use #ifdef.
Irrespective of RTE_CRYPTO_CALLBACKS value set to 0/1,
#ifdef RTE_CRYPTO_CALLBACKS returns true always.
Morten Brørup May 30, 2024, 11:45 a.m. UTC | #6 
> From: Kundapura, Ganapati [mailto:ganapati.kundapura@intel.com]
> Sent: Thursday, 30 May 2024 13.42
> 
> Hi,
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Thursday, May 30, 2024 4:44 PM
> >
> > > From: Akhil Goyal [mailto:gakhil@marvell.com]
> > > Sent: Thursday, 30 May 2024 13.02
> > >
> > > ++ Morten for comment on #if vs #ifdef
> > >
> > > > > Subject: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on
> > > > > unsetting
> > > crypto
> > > > > callbacks macro
> > > > >
> > > > > Crypto callbacks macro is defined with value 1 and being used with
> > > > > ifdef, on config value is changed to 0 to disable, crypto callback
> > > > > changes still being compiled.
> > > > >
> > > > > Used #if instead of #ifdef and also wrapped crypto callback
> > > > > changes under RTE_CRYPTO_CALLBACKS macro to fix build issues when
> > > > > macro is unset.
> > > > >
> > > > > Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue
> > > > > callbacks")
> > > > > Fixes: 5523a75af539 ("test/crypto: add case for enqueue/dequeue
> > > callbacks")
> > > > >
> > > > > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> > > > > ---
> > > > > v2:
> > > > > * Used #if instead of #ifdef and restored macro definition in
> > > > > config
> > > > > * Split callback registration check in a seperate patch
> >
> > The DPDK convention is #ifdef, not #if.
> > Ethdev also uses #ifdef, e.g.:
> > https://elixir.bootlin.com/dpdk/v24.03/source/lib/ethdev/rte_ethdev.h#L61
> > 12
> >
> > PS: Personally, I prefer #if too. But we should set personal preferences
> aside,
> > and follow the existing convention in DPDK and use #ifdef.
> Irrespective of RTE_CRYPTO_CALLBACKS value set to 0/1,
> #ifdef RTE_CRYPTO_CALLBACKS returns true always.

Yes, because it is defined in both cases.

So according to the #ifdef convention, rte_config.h must contain either:
#define RTE_CRYPTO_CALLBACKS 1
Or:
/* RTE_CRYPTO_CALLBACKS is not set */
Akhil Goyal May 30, 2024, 11:46 a.m. UTC | #7 
> > > #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.

> > > > 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.
Morten Brørup May 30, 2024, 11:52 a.m. UTC | #8 
> From: Akhil Goyal [mailto:gakhil@marvell.com]
> Sent: Thursday, 30 May 2024 13.47
> 
> > > > #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.
> 
> > > > > 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.

Agree; the function must always be present in DPDK, but fail if built without callbacks.

The ethdev function to add a callback does this [1]:

const struct rte_eth_rxtx_callback *
rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
		rte_rx_callback_fn fn, void *user_param)
{
#ifndef RTE_ETHDEV_RXTX_CALLBACKS
	rte_errno = ENOTSUP;
	return NULL;
#endif
	struct rte_eth_dev *dev;
	[...]


[1]: https://elixir.bootlin.com/dpdk/v24.03/source/lib/ethdev/rte_ethdev.c#L5729
Kundapura, Ganapati May 30, 2024, 2:22 p.m. UTC | #9 
Hi,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, May 30, 2024 5:17 PM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>; dev@dpdk.org;
> Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; ferruh.yigit@amd.com;
> thomas@monjalon.net; Richardson, Bruce <bruce.richardson@intel.com>;
> fanzhang.oss@gmail.com; ciara.power@intel.com; Morten Brørup
> <mb@smartsharesystems.com>
> Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting
> crypto callbacks macro
> 
> > > > #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. Even ethdev callbacks also doesn't return -ENOTSUP 
on setting/unsetting RTE_ETHDEV_RXTX_CALLBACKS config.
Morten Brørup May 30, 2024, 2:49 p.m. UTC | #10 
> 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.
Akhil Goyal June 6, 2024, 9:44 a.m. UTC | #11 
> > 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
Akhil Goyal June 13, 2024, 6:03 p.m. UTC | #12 
> > > 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.
> 
Any update on this?
Kundapura, Ganapati June 20, 2024, 2:34 p.m. UTC | #13 
Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, June 13, 2024 11:34 PM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>; dev@dpdk.org;
> Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: Morten Brørup <mb@smartsharesystems.com>; ferruh.yigit@amd.com;
> fanzhang.oss@gmail.com; thomas@monjalon.net
> Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting
> crypto callbacks macro
> 
> > > > 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.
> >
> Any update on this?
Working on it, will post the patch soon.

Thanks,
Ganapati
Akhil Goyal June 26, 2024, 6:02 a.m. UTC | #14 
> > > 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.
> 
Any update? or else this is going for next release.
Kundapura, Ganapati June 26, 2024, 10:04 p.m. UTC | #15 
Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, June 26, 2024 11:33 AM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>; dev@dpdk.org;
> Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: Morten Brørup <mb@smartsharesystems.com>; ferruh.yigit@amd.com;
> fanzhang.oss@gmail.com; thomas@monjalon.net
> Subject: RE: [EXTERNAL] [PATCH v2 1/2] crypto: fix build issues on unsetting
> crypto callbacks macro
> 
> > > > 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.
> >
> Any update? or else this is going for next release.
Posted v3

With crypto callbacks macro enabled
+ Test Suite : Crypto General Unit Test Suite
 + ------------------------------------------------------- +
CRYPTODEV: rte_cryptodev_configure() line 1148: Invalid dev_id=7
CRYPTODEV: rte_cryptodev_configure() line 1148: Invalid dev_id=255
CRYPTODEV: rte_cryptodev_stop() line 1241: Device with dev_id=0 already stopped
 + TestCase [ 0] : test_device_configure_invalid_dev_id succeeded
CRYPTODEV: rte_cryptodev_queue_pair_setup() line 1364: Invalid queue_pair_id=1
CRYPTODEV: rte_cryptodev_queue_pair_setup() line 1364: Invalid queue_pair_id=65535
CRYPTODEV: rte_cryptodev_stop() line 1241: Device with dev_id=0 already stopped
 + TestCase [ 1] : test_queue_pair_descriptor_setup succeeded
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1086: invalid param: dev 0x7fe4a91a8940, nb_queues 0
CRYPTODEV: rte_cryptodev_configure() line 1171: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1101: Invalid num queue_pairs (65535) for dev 0
CRYPTODEV: rte_cryptodev_configure() line 1171: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1101: Invalid num queue_pairs (2) for dev 0
CRYPTODEV: rte_cryptodev_configure() line 1171: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_stop() line 1241: Device with dev_id=0 already stopped
 + TestCase [ 2] : test_device_configure_invalid_queue_pair_ids succeeded
CRYPTODEV: rte_cryptodev_stats_get() line 1701: Invalid dev_id=88
CRYPTODEV: rte_cryptodev_stats_get() line 1706: Invalid stats ptr
CRYPTODEV: rte_cryptodev_stats_reset() line 1729: Invalid dev_id=44
 + TestCase [ 3] : test_stats succeeded
CRYPTODEV: rte_cryptodev_add_enq_callback() line 1425: Invalid dev_id=64
CRYPTODEV: rte_cryptodev_add_enq_callback() line 1432: Invalid queue_pair_id=2
CRYPTODEV: rte_cryptodev_add_enq_callback() line 1419: Callback is NULL on dev_id=0
CRYPTODEV: rte_cryptodev_remove_enq_callback() line 1495: Invalid dev_id=64
CRYPTODEV: rte_cryptodev_remove_enq_callback() line 1503: Invalid queue_pair_id=2
CRYPTODEV: rte_cryptodev_remove_enq_callback() line 1490: Callback is NULL
 + TestCase [ 4] : test_enq_callback_setup succeeded
CRYPTODEV: rte_cryptodev_add_deq_callback() line 1570: Invalid dev_id=64
CRYPTODEV: rte_cryptodev_add_deq_callback() line 1577: Invalid queue_pair_id=2
CRYPTODEV: rte_cryptodev_add_deq_callback() line 1564: Callback is NULL on dev_id=0
CRYPTODEV: rte_cryptodev_remove_enq_callback() line 1495: Invalid dev_id=64
CRYPTODEV: rte_cryptodev_remove_deq_callback() line 1649: Invalid queue_pair_id=2
CRYPTODEV: rte_cryptodev_remove_deq_callback() line 1636: Callback is NULL
 + TestCase [ 5] : test_deq_callback_setup succeeded
 + ------------------------------------------------------- +
 + Test Suite Summary : Crypto General Unit Test Suite
 + ------------------------------------------------------- +
 + Tests Total :        6
 + Tests Skipped :      0
 + Tests Executed :     6
 + Tests Unsupported:   0
 + Tests Passed :       6
 + Tests Failed :       0

With crypto_callbacks macro disabled

+ Test Suite : Crypto General Unit Test Suite
 + ------------------------------------------------------- +
CRYPTODEV: rte_cryptodev_configure() line 1148: Invalid dev_id=7
CRYPTODEV: rte_cryptodev_configure() line 1148: Invalid dev_id=255
CRYPTODEV: rte_cryptodev_stop() line 1241: Device with dev_id=0 already stopped
 + TestCase [ 0] : test_device_configure_invalid_dev_id succeeded
CRYPTODEV: rte_cryptodev_queue_pair_setup() line 1364: Invalid queue_pair_id=1
CRYPTODEV: rte_cryptodev_queue_pair_setup() line 1364: Invalid queue_pair_id=65535
CRYPTODEV: rte_cryptodev_stop() line 1241: Device with dev_id=0 already stopped
 + TestCase [ 1] : test_queue_pair_descriptor_setup succeeded
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1086: invalid param: dev 0x7f2eac8cb940, nb_queues 0
CRYPTODEV: rte_cryptodev_configure() line 1171: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1101: Invalid num queue_pairs (65535) for dev 0
CRYPTODEV: rte_cryptodev_configure() line 1171: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1101: Invalid num queue_pairs (2) for dev 0
CRYPTODEV: rte_cryptodev_configure() line 1171: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_stop() line 1241: Device with dev_id=0 already stopped
 + TestCase [ 2] : test_device_configure_invalid_queue_pair_ids succeeded
CRYPTODEV: rte_cryptodev_stats_get() line 1701: Invalid dev_id=88
CRYPTODEV: rte_cryptodev_stats_get() line 1706: Invalid stats ptr
CRYPTODEV: rte_cryptodev_stats_reset() line 1729: Invalid dev_id=44
 + TestCase [ 3] : test_stats succeeded
CRYPTODEV: test_enq_callback_setup line 13003: Not supported, skipped
CRYPTODEV: rte_cryptodev_stop() line 1241: Device with dev_id=0 already stopped
 + TestCase [ 4] : test_enq_callback_setup skipped
CRYPTODEV: test_deq_callback_setup line 13116: Not supported, skipped
CRYPTODEV: rte_cryptodev_stop() line 1241: Device with dev_id=0 already stopped
 + TestCase [ 5] : test_deq_callback_setup skipped
 + ------------------------------------------------------- +
 + Test Suite Summary : Crypto General Unit Test Suite
 + ------------------------------------------------------- +
 + Tests Total :        6
 + Tests Skipped :      2
 + Tests Executed :     6
 + Tests Unsupported:   0
 + Tests Passed :       4
 + Tests Failed :       0
 + ------------------------------------------------------- +
 + -------------------------------------------------------
diff mbox series

Patch

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 1703ebc..72cf77f 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -14547,6 +14547,7 @@  test_null_burst_operation(void)
 	return TEST_SUCCESS;
 }
 
+#if RTE_CRYPTO_CALLBACKS
 static uint16_t
 test_enq_callback(uint16_t dev_id, uint16_t qp_id, struct rte_crypto_op **ops,
 		  uint16_t nb_ops, void *user_param)
@@ -14784,6 +14785,7 @@  test_deq_callback_setup(void)
 
 	return TEST_SUCCESS;
 }
+#endif /* RTE_CRYPTO_CALLBACKS */
 
 static void
 generate_gmac_large_plaintext(uint8_t *data)
@@ -18069,8 +18071,10 @@  static struct unit_test_suite cryptodev_gen_testsuite  = {
 		TEST_CASE_ST(ut_setup, ut_teardown,
 				test_device_configure_invalid_queue_pair_ids),
 		TEST_CASE_ST(ut_setup, ut_teardown, test_stats),
+#if RTE_CRYPTO_CALLBACKS
 		TEST_CASE_ST(ut_setup, ut_teardown, test_enq_callback_setup),
 		TEST_CASE_ST(ut_setup, ut_teardown, test_deq_callback_setup),
+#endif
 		TEST_CASES_END() /**< NULL terminate unit test array */
 	}
 };
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 */
 
 const char *
 rte_cryptodev_get_feature_name(uint64_t flag)
@@ -1244,9 +1246,11 @@  rte_cryptodev_configure(uint8_t dev_id, struct rte_cryptodev_config *config)
 	if (*dev->dev_ops->dev_configure == NULL)
 		return -ENOTSUP;
 
+#if RTE_CRYPTO_CALLBACKS
 	rte_spinlock_lock(&rte_cryptodev_callback_lock);
 	cryptodev_cb_cleanup(dev);
 	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
+#endif
 
 	/* Setup new number of queue pairs and reconfigure device. */
 	diag = rte_cryptodev_queue_pairs_config(dev, config->nb_queue_pairs,
@@ -1257,6 +1261,7 @@  rte_cryptodev_configure(uint8_t dev_id, struct rte_cryptodev_config *config)
 		return diag;
 	}
 
+#if RTE_CRYPTO_CALLBACKS
 	rte_spinlock_lock(&rte_cryptodev_callback_lock);
 	diag = cryptodev_cb_init(dev);
 	rte_spinlock_unlock(&rte_cryptodev_callback_lock);
@@ -1264,6 +1269,7 @@  rte_cryptodev_configure(uint8_t dev_id, struct rte_cryptodev_config *config)
 		CDEV_LOG_ERR("Callback init failed for dev_id=%d", dev_id);
 		return diag;
 	}
+#endif
 
 	rte_cryptodev_trace_configure(dev_id, config);
 	return (*dev->dev_ops->dev_configure)(dev, config);
@@ -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 */
 
 int
 rte_cryptodev_stats_get(uint8_t dev_id, struct rte_cryptodev_stats *stats)
diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index 00ba6a2..357d4bc 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -1909,7 +1909,7 @@  rte_cryptodev_dequeue_burst(uint8_t dev_id, uint16_t qp_id,
 
 	nb_ops = fp_ops->dequeue_burst(qp, ops, nb_ops);
 
-#ifdef RTE_CRYPTO_CALLBACKS
+#if RTE_CRYPTO_CALLBACKS
 	if (unlikely(fp_ops->qp.deq_cb != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
@@ -1976,7 +1976,7 @@  rte_cryptodev_enqueue_burst(uint8_t dev_id, uint16_t qp_id,
 
 	fp_ops = &rte_crypto_fp_ops[dev_id];
 	qp = fp_ops->qp.data[qp_id];
-#ifdef RTE_CRYPTO_CALLBACKS
+#if RTE_CRYPTO_CALLBACKS
 	if (unlikely(fp_ops->qp.enq_cb != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;