[PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Ganapati Kundapura]

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.

Defined crypto callbacks macro without value, undef to disable

Wrapped crypto callback changes with RTE_CRYPTO_CALLBACKS macro
to fix build issues when macro is undefined.

As callback head nodes have valid pointer, this patch checks the next
node from the head if callbacks registered.

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

Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 1703ebccf1..1a592f2302 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;
 }
 
+#ifdef 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),
+#ifdef 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/config/rte_config.h b/config/rte_config.h
index dd7bb0d35b..b647a69ba8 100644
--- a/config/rte_config.h
+++ b/config/rte_config.h
@@ -72,7 +72,7 @@
 /* cryptodev defines */
 #define RTE_CRYPTO_MAX_DEVS 64
 #define RTE_CRYPTODEV_NAME_LEN 64
-#define RTE_CRYPTO_CALLBACKS 1
+#define RTE_CRYPTO_CALLBACKS	/* No Value, undef/comment out to disable */
 
 /* compressdev defines */
 #define RTE_COMPRESS_MAX_DEVS 64
diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
index 886eb7adc4..5f3b17a517 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;
 }
 
+#ifdef 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;
 
+#ifdef 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;
 	}
 
+#ifdef 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);
 }
 
+#ifdef 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 00ba6a234a..b811b458d5 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -1910,7 +1910,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 (unlikely(fp_ops->qp.deq_cb != NULL)) {
+	if (unlikely(fp_ops->qp.deq_cb[qp_id].next != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
 
@@ -1977,7 +1977,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 (unlikely(fp_ops->qp.enq_cb != NULL)) {
+	if (unlikely(fp_ops->qp.enq_cb[qp_id].next != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
 
-- 
2.23.0

RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Gujjar, Abhinandan S]

> -----Original Message-----
> From: Kundapura, Ganapati <ganapati.kundapura@intel.com>
> Sent: Tuesday, April 16, 2024 1:42 PM
> To: dev@dpdk.org
> Cc: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Power, Ciara
> <ciara.power@intel.com>; gakhil@marvell.com; fanzhang.oss@gmail.com
> Subject: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined
> 
> 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.
> 
> Defined crypto callbacks macro without value, undef to disable
> 
> Wrapped crypto callback changes with RTE_CRYPTO_CALLBACKS macro to fix
> build issues when macro is undefined.
> 
> As callback head nodes have valid pointer, this patch checks the next node from
> the head if callbacks registered.
> 
> Fixes: 1c3ffb9 ("cryptodev: add enqueue and dequeue callbacks")
> Fixes: 5523a75 ("test/crypto: add case for enqueue/dequeue callbacks")
> 
> Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
>
Acked-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
> 1703ebccf1..1a592f2302 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;
>  }
> 
> +#ifdef 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),
> +#ifdef 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/config/rte_config.h b/config/rte_config.h index
> dd7bb0d35b..b647a69ba8 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -72,7 +72,7 @@
>  /* cryptodev defines */
>  #define RTE_CRYPTO_MAX_DEVS 64
>  #define RTE_CRYPTODEV_NAME_LEN 64
> -#define RTE_CRYPTO_CALLBACKS 1
> +#define RTE_CRYPTO_CALLBACKS	/* No Value, undef/comment out to
> disable */
> 
>  /* compressdev defines */
>  #define RTE_COMPRESS_MAX_DEVS 64
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c index
> 886eb7adc4..5f3b17a517 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;
>  }
> 
> +#ifdef 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;
> 
> +#ifdef 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;
>  	}
> 
> +#ifdef 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);
>  }
> 
> +#ifdef 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
> 00ba6a234a..b811b458d5 100644
> --- a/lib/cryptodev/rte_cryptodev.h
> +++ b/lib/cryptodev/rte_cryptodev.h
> @@ -1910,7 +1910,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 (unlikely(fp_ops->qp.deq_cb != NULL)) {
> +	if (unlikely(fp_ops->qp.deq_cb[qp_id].next != NULL)) {
>  		struct rte_cryptodev_cb_rcu *list;
>  		struct rte_cryptodev_cb *cb;
> 
> @@ -1977,7 +1977,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 (unlikely(fp_ops->qp.enq_cb != NULL)) {
> +	if (unlikely(fp_ops->qp.enq_cb[qp_id].next != NULL)) {
>  		struct rte_cryptodev_cb_rcu *list;
>  		struct rte_cryptodev_cb *cb;
> 
> --
> 2.23.0

RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Power, Ciara]

> -----Original Message-----
> From: Kundapura, Ganapati <ganapati.kundapura@intel.com>
> Sent: Tuesday, April 16, 2024 9:12 AM
> To: dev@dpdk.org
> Cc: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Power, Ciara
> <ciara.power@intel.com>; gakhil@marvell.com; fanzhang.oss@gmail.com
> Subject: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined
> 
> 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.
> 
> Defined crypto callbacks macro without value, undef to disable
> 
> Wrapped crypto callback changes with RTE_CRYPTO_CALLBACKS macro to fix
> build issues when macro is undefined.
> 
> As callback head nodes have valid pointer, this patch checks the next node from
> the head if callbacks registered.
> 
> Fixes: 1c3ffb9 ("cryptodev: add enqueue and dequeue callbacks")
> Fixes: 5523a75 ("test/crypto: add case for enqueue/dequeue callbacks")
> 
> Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> 

Acked-by: Ciara Power <ciara.power@intel.com>

RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Kundapura, Ganapati]

Hi Akhil,
2 acked for this patch but it's still not yet accepted.
Anything more required for this patch?

Thanks,
Ganapati


> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Wednesday, April 17, 2024 5:11 PM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>; dev@dpdk.org
> Cc: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
> gakhil@marvell.com; fanzhang.oss@gmail.com
> Subject: RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro
> undefined
> 
> 
> 
> > -----Original Message-----
> > From: Kundapura, Ganapati <ganapati.kundapura@intel.com>
> > Sent: Tuesday, April 16, 2024 9:12 AM
> > To: dev@dpdk.org
> > Cc: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>; Power, Ciara
> > <ciara.power@intel.com>; gakhil@marvell.com; fanzhang.oss@gmail.com
> > Subject: [PATCH v1] crypto: fix build issues on crypto callbacks macro
> > undefined
> >
> > 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.
> >
> > Defined crypto callbacks macro without value, undef to disable
> >
> > Wrapped crypto callback changes with RTE_CRYPTO_CALLBACKS macro to fix
> > build issues when macro is undefined.
> >
> > As callback head nodes have valid pointer, this patch checks the next
> > node from the head if callbacks registered.
> >
> > Fixes: 1c3ffb9 ("cryptodev: add enqueue and dequeue callbacks")
> > Fixes: 5523a75 ("test/crypto: add case for enqueue/dequeue callbacks")
> >
> > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> >
> 
> Acked-by: Ciara Power <ciara.power@intel.com>

RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Akhil Goyal]

> Subject: [EXTERNAL] RE: [PATCH v1] crypto: fix build issues on crypto callbacks
> macro undefined
> 
> Hi Akhil,
> 2 acked for this patch but it's still not yet accepted.
> Anything more required for this patch?
> 
Hi Ganapati,

I haven't started picking patches on crypto tree yet. Will start from next week.
However, for this patch, I see some other issues as well in crypto callbacks test in dpdk-test.
Will comment by tomorrow.

Regards,
Akhil

RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Akhil Goyal]

> Subject: RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro
> undefined
> 
> > Subject: [EXTERNAL] RE: [PATCH v1] crypto: fix build issues on crypto callbacks
> > macro undefined
> >
> > Hi Akhil,
> > 2 acked for this patch but it's still not yet accepted.
> > Anything more required for this patch?
> >
> Hi Ganapati,
> 
> I haven't started picking patches on crypto tree yet. Will start from next week.
> However, for this patch, I see some other issues as well in crypto callbacks test in
> dpdk-test.
> Will comment by tomorrow.
> 
Please check this patch also. This patch will fix the runtime issues for the case.
https://patches.dpdk.org/project/dpdk/patch/20240522134835.3453044-1-gakhil@marvell.com/

RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Kundapura, Ganapati]

Hi Akhil,
   My patch addresses build issue on setting RTE_CRYPTO_CALLBACKS to 0 and
Checks for callback registered or not from the next node instead of head node as callback head node is always valid pointer.

Thanks,
Ganapati

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, May 22, 2024 7:21 PM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>; dev@dpdk.org
> Cc: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
> fanzhang.oss@gmail.com; 'Power, Ciara' <ciara.power@intel.com>
> Subject: RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro
> undefined
> 
> > Subject: RE: [PATCH v1] crypto: fix build issues on crypto callbacks
> > macro undefined
> >
> > > Subject: [EXTERNAL] RE: [PATCH v1] crypto: fix build issues on
> > > crypto callbacks macro undefined
> > >
> > > Hi Akhil,
> > > 2 acked for this patch but it's still not yet accepted.
> > > Anything more required for this patch?
> > >
> > Hi Ganapati,
> >
> > I haven't started picking patches on crypto tree yet. Will start from next
> week.
> > However, for this patch, I see some other issues as well in crypto
> > callbacks test in dpdk-test.
> > Will comment by tomorrow.
> >
> Please check this patch also. This patch will fix the runtime issues for the case.
> https://patches.dpdk.org/project/dpdk/patch/20240522134835.3453044-
> 1-gakhil@marvell.com/

RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Akhil Goyal]

> Hi Akhil,
>    My patch addresses build issue on setting RTE_CRYPTO_CALLBACKS to 0 and
> Checks for callback registered or not from the next node instead of head node as
> callback head node is always valid pointer.
> 
Agreed but your patch cannot be verified without the fix as the callbacks are not getting called if using PMD other than NULL.

RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Kundapura, Ganapati]

Hi Akhil
   Agreed that callbacks are not getting called if not NULL PMD as it is designed to be run for NULL PMD.
Tested your patch with qat pmd and callbacks are getting called. Both the patches can go in.

+ ------------------------------------------------------- +
 + Test Suite : Crypto General Unit Test Suite
 + ------------------------------------------------------- +
CRYPTODEV: rte_cryptodev_configure() line 1150: Invalid dev_id=4
CRYPTODEV: rte_cryptodev_configure() line 1150: Invalid dev_id=255
CRYPTODEV: rte_cryptodev_stop() line 1247: Device with dev_id=0 already stopped
 + TestCase [ 0] : test_device_configure_invalid_dev_id succeeded
CRYPTODEV: rte_cryptodev_queue_pair_setup() line 1370: Invalid queue_pair_id=1
CRYPTODEV: rte_cryptodev_queue_pair_setup() line 1370: Invalid queue_pair_id=65535
CRYPTODEV: rte_cryptodev_stop() line 1247: Device with dev_id=0 already stopped
 + TestCase [ 1] : test_queue_pair_descriptor_setup succeeded
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1088: invalid param: dev 0x7fb24d547940, nb_queues 0
CRYPTODEV: rte_cryptodev_configure() line 1175: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1103: Invalid num queue_pairs (65535) for dev 0
CRYPTODEV: rte_cryptodev_configure() line 1175: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_queue_pairs_config() line 1103: Invalid num queue_pairs (2) for dev 0
CRYPTODEV: rte_cryptodev_configure() line 1175: dev0 rte_crypto_dev_queue_pairs_config = -22
CRYPTODEV: rte_cryptodev_stop() line 1247: Device with dev_id=0 already stopped
 + TestCase [ 2] : test_device_configure_invalid_queue_pair_ids succeeded
CRYPTODEV: rte_cryptodev_stats_get() line 1695: Invalid dev_id=88
CRYPTODEV: rte_cryptodev_stats_get() line 1700: Invalid stats ptr
CRYPTODEV: rte_cryptodev_stats_reset() line 1723: Invalid dev_id=44
 + TestCase [ 3] : test_stats succeeded
CRYPTODEV: rte_cryptodev_add_enq_callback() line 1428: Invalid dev_id=64
CRYPTODEV: rte_cryptodev_add_enq_callback() line 1435: Invalid queue_pair_id=2
CRYPTODEV: rte_cryptodev_add_enq_callback() line 1422: Callback is NULL on dev_id=0
crypto enqueue callback called
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 1566: Invalid dev_id=64
CRYPTODEV: rte_cryptodev_add_deq_callback() line 1573: Invalid queue_pair_id=2
CRYPTODEV: rte_cryptodev_add_deq_callback() line 1560: Callback is NULL on dev_id=0
crypto dequeue callback called
crypto dequeue callback called
crypto dequeue callback called
CRYPTODEV: rte_cryptodev_remove_deq_callback() line 1634: Invalid dev_id=64
CRYPTODEV: rte_cryptodev_remove_deq_callback() line 1642: Invalid queue_pair_id=2
CRYPTODEV: rte_cryptodev_remove_deq_callback() line 1629: 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
 + ------------------------------------------------------- +
 + -------------------------------------------------------

Thanks,
Ganapati

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, May 23, 2024 12:06 AM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>; dev@dpdk.org
> Cc: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
> fanzhang.oss@gmail.com; 'Power, Ciara' <ciara.power@intel.com>
> Subject: RE: [PATCH v1] crypto: fix build issues on crypto callbacks macro
> undefined
> 
> > Hi Akhil,
> >    My patch addresses build issue on setting RTE_CRYPTO_CALLBACKS to 0
> > and Checks for callback registered or not from the next node instead
> > of head node as callback head node is always valid pointer.
> >
> Agreed but your patch cannot be verified without the fix as the callbacks are
> not getting called if using PMD other than NULL.

RE: [EXTERNAL] [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Akhil Goyal]

> 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.

I believe we can use #if instead of ifdefs.
It seems convenient to enable/disable in my opinion.
We can use both, but whatever we use should be same as that for ethdev callbacks.

The same issue would be for ethdev callbacks too.
Ferruh, can you check?

> 
> Defined crypto callbacks macro without value, undef to disable
> 
> Wrapped crypto callback changes with RTE_CRYPTO_CALLBACKS macro
> to fix build issues when macro is undefined.
> 
> As callback head nodes have valid pointer, this patch checks the next
> node from the head if callbacks registered.
> 
> Fixes: 1c3ffb9 ("cryptodev: add enqueue and dequeue callbacks")
> Fixes: 5523a75 ("test/crypto: add case for enqueue/dequeue callbacks")
> 
> Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>


> diff --git a/config/rte_config.h b/config/rte_config.h
> index dd7bb0d35b..b647a69ba8 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -72,7 +72,7 @@
>  /* cryptodev defines */
>  #define RTE_CRYPTO_MAX_DEVS 64
>  #define RTE_CRYPTODEV_NAME_LEN 64
> -#define RTE_CRYPTO_CALLBACKS 1
> +#define RTE_CRYPTO_CALLBACKS	/* No Value, undef/comment out to
> disable */
> 
>  /* compressdev defines */
>  #define RTE_COMPRESS_MAX_DEVS 64


> diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
> index 00ba6a234a..b811b458d5 100644
> --- a/lib/cryptodev/rte_cryptodev.h
> +++ b/lib/cryptodev/rte_cryptodev.h
> @@ -1910,7 +1910,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 (unlikely(fp_ops->qp.deq_cb != NULL)) {
> +	if (unlikely(fp_ops->qp.deq_cb[qp_id].next != NULL)) {
>  		struct rte_cryptodev_cb_rcu *list;
>  		struct rte_cryptodev_cb *cb;
> 
> @@ -1977,7 +1977,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 (unlikely(fp_ops->qp.enq_cb != NULL)) {
> +	if (unlikely(fp_ops->qp.enq_cb[qp_id].next != NULL)) {
>  		struct rte_cryptodev_cb_rcu *list;
>  		struct rte_cryptodev_cb *cb;
> 
This is a separate issue. Please create a separate patch.

RE: [EXTERNAL] [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Kundapura, Ganapati]

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, May 29, 2024 2:05 PM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>; dev@dpdk.org;
> Ferruh Yigit <ferruh.yigit@amd.com>; thomas@monjalon.net; Richardson,
> Bruce <bruce.richardson@intel.com>
> Cc: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
> ciara.power@intel.com; fanzhang.oss@gmail.com
> Subject: RE: [EXTERNAL] [PATCH v1] crypto: fix build issues on crypto callbacks
> macro undefined
> 
> > 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.
> 
> I believe we can use #if instead of ifdefs.
> It seems convenient to enable/disable in my opinion.
> We can use both, but whatever we use should be same as that for ethdev
> callbacks.
> 
Using #if requires check for equality like
#if RTE_CRYPTO_CALLBACKS == 1 for a macro defined with value 1

> The same issue would be for ethdev callbacks too.
> Ferruh, can you check?
> 
> >
> > Defined crypto callbacks macro without value, undef to disable
> >
> > Wrapped crypto callback changes with RTE_CRYPTO_CALLBACKS macro to fix
> > build issues when macro is undefined.
> >
> > As callback head nodes have valid pointer, this patch checks the next
> > node from the head if callbacks registered.
> >
> > Fixes: 1c3ffb9 ("cryptodev: add enqueue and dequeue callbacks")
> > Fixes: 5523a75 ("test/crypto: add case for enqueue/dequeue callbacks")
> >
> > Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> 
> 
> > diff --git a/config/rte_config.h b/config/rte_config.h index
> > dd7bb0d35b..b647a69ba8 100644
> > --- a/config/rte_config.h
> > +++ b/config/rte_config.h
> > @@ -72,7 +72,7 @@
> >  /* cryptodev defines */
> >  #define RTE_CRYPTO_MAX_DEVS 64
> >  #define RTE_CRYPTODEV_NAME_LEN 64
> > -#define RTE_CRYPTO_CALLBACKS 1
> > +#define RTE_CRYPTO_CALLBACKS	/* No Value, undef/comment out to
> > disable */
> >
> >  /* compressdev defines */
> >  #define RTE_COMPRESS_MAX_DEVS 64
> 
> 
> > diff --git a/lib/cryptodev/rte_cryptodev.h
> > b/lib/cryptodev/rte_cryptodev.h index 00ba6a234a..b811b458d5 100644
> > --- a/lib/cryptodev/rte_cryptodev.h
> > +++ b/lib/cryptodev/rte_cryptodev.h
> > @@ -1910,7 +1910,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 (unlikely(fp_ops->qp.deq_cb != NULL)) {
> > +	if (unlikely(fp_ops->qp.deq_cb[qp_id].next != NULL)) {
> >  		struct rte_cryptodev_cb_rcu *list;
> >  		struct rte_cryptodev_cb *cb;
> >
> > @@ -1977,7 +1977,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 (unlikely(fp_ops->qp.enq_cb != NULL)) {
> > +	if (unlikely(fp_ops->qp.enq_cb[qp_id].next != NULL)) {
> >  		struct rte_cryptodev_cb_rcu *list;
> >  		struct rte_cryptodev_cb *cb;
> >
> This is a separate issue. Please create a separate patch.
ok

RE: [EXTERNAL] [PATCH v1] crypto: fix build issues on crypto callbacks macro undefined

[Akhil Goyal]

> > Subject: RE: [EXTERNAL] [PATCH v1] crypto: fix build issues on crypto callbacks
> > macro undefined
> >
> > > 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.
> >
> > I believe we can use #if instead of ifdefs.
> > It seems convenient to enable/disable in my opinion.
> > We can use both, but whatever we use should be same as that for ethdev
> > callbacks.
> >
> Using #if requires check for equality like
> #if RTE_CRYPTO_CALLBACKS == 1 for a macro defined with value 1

No

If it is set as 0, it will be disabled, else for any other value it will be enabled.
No need for check. Right?

> 
> > The same issue would be for ethdev callbacks too.
> > Ferruh, can you check?

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

[Ganapati Kundapura]

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 */
 	}
 };
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;
-- 
2.6.4

[PATCH v2 2/2] crypto: validate crypto callbacks from next node

[Ganapati Kundapura]

Crypto callbacks are invoked on checking from head node
which is always valid pointer.

This patch checks next node from the head node if callbacks
registered before invoking callbacks.

Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")

Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
---
v2:
* Seperated this patch from the combined patch

diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index 357d4bc..ce3ea36 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -1910,7 +1910,7 @@ rte_cryptodev_dequeue_burst(uint8_t dev_id, uint16_t qp_id,
 	nb_ops = fp_ops->dequeue_burst(qp, ops, nb_ops);
 
 #if RTE_CRYPTO_CALLBACKS
-	if (unlikely(fp_ops->qp.deq_cb != NULL)) {
+	if (unlikely(fp_ops->qp.deq_cb[qp_id].next != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
 
@@ -1977,7 +1977,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];
 #if RTE_CRYPTO_CALLBACKS
-	if (unlikely(fp_ops->qp.enq_cb != NULL)) {
+	if (unlikely(fp_ops->qp.enq_cb[qp_id].next != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
 
-- 
2.6.4

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

[Akhil Goyal]

> -----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.

RE: [EXTERNAL] [PATCH v2 2/2] crypto: validate crypto callbacks from next node

[Akhil Goyal]

> Crypto callbacks are invoked on checking from head node
> which is always valid pointer.
> 
> This patch checks next node from the head node if callbacks
> registered before invoking callbacks.
> 
> Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")
> 
> Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
> ---
Acked-by: Akhil Goyal <gakhil@marvell.com>

Cc: stable@dpdk.org

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

[Akhil Goyal]

++ 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.

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

[Morten Brørup]

> 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.

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

[Kundapura, Ganapati]

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.

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

[Kundapura, Ganapati]

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.

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

[Morten Brørup]

> 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 */

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

[Akhil Goyal]

> > > #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.

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

[Morten Brørup]

> 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

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

[Kundapura, Ganapati]

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.

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

[Morten Brørup]

> 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.

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

[Akhil Goyal]

> > 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

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

[Akhil Goyal]

> > > 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?

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

[Kundapura, Ganapati]

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

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

[Akhil Goyal]

> > > 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.

[PATCH v3 1/2] cryptodev: fix crypto callbacks on unsetting callbacks macro

[Ganapati Kundapura]

Crypto callbacks APIs are available in header files but when
the macro RTE_CRYPTO_CALLBACKS unset, test application need to
put #ifdef in its code.

The test application should be able to build and run, regardless
DPDK library is built with RTE_CRYPTO_CALLBACKS defined or not.

Added ENOTSUP from the beginning of the APIs implementation
if RTE_CRYPTO_CALLBACKS macro is unset/undefined.

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>

---
v3:
* Added NOTSUP from the beginning of the APIs

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 94438c5..7dca2c9 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -118,6 +118,18 @@ struct crypto_unittest_params {
 	for (j = index; j < index + num_blk_types; j++)				\
 		free_blockcipher_test_suite(parent_ts.unit_test_suites[j])
 
+#define TEST_SKIP_LOG(cond, msg, ...) do {				\
+	if ((cond)) {							\
+		RTE_LOG(ERR, CRYPTODEV, "%s line %d: "			\
+			msg "\n", __func__, __LINE__, ##__VA_ARGS__);	\
+		RTE_TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);	\
+		return TEST_SKIPPED;					\
+	}								\
+} while (0)
+
+#define TEST_SKIP(a, b, msg, ...) \
+	TEST_SKIP_LOG(a == b, msg, ##__VA_ARGS__)
+
 /*
  * Forward declarations.
  */
@@ -14754,7 +14766,7 @@ test_enq_callback_setup(void)
 
 	struct rte_cryptodev_cb *cb;
 	uint16_t qp_id = 0;
-	int j = 0;
+	int j = 0, ret;
 
 	/* Verify the crypto capabilities for which enqueue/dequeue is done. */
 	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH;
@@ -14794,6 +14806,7 @@ test_enq_callback_setup(void)
 	/* Test with invalid crypto device */
 	cb = rte_cryptodev_add_enq_callback(RTE_CRYPTO_MAX_DEVS,
 			qp_id, test_enq_callback, NULL);
+	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
 			"cryptodev %u did not fail",
 			qp_id, RTE_CRYPTO_MAX_DEVS);
@@ -14802,6 +14815,7 @@ test_enq_callback_setup(void)
 	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
 			dev_info.max_nb_queue_pairs + 1,
 			test_enq_callback, NULL);
+	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
 			"cryptodev %u did not fail",
 			dev_info.max_nb_queue_pairs + 1,
@@ -14810,6 +14824,7 @@ test_enq_callback_setup(void)
 	/* Test with NULL callback */
 	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
 			qp_id, NULL, NULL);
+	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
 			"cryptodev %u did not fail",
 			qp_id, ts_params->valid_devs[0]);
@@ -14817,6 +14832,7 @@ test_enq_callback_setup(void)
 	/* Test with valid configuration */
 	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
 			qp_id, test_enq_callback, NULL);
+	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_NOT_NULL(cb, "Failed test to add callback on "
 			"qp %u on cryptodev %u",
 			qp_id, ts_params->valid_devs[0]);
@@ -14830,24 +14846,35 @@ test_enq_callback_setup(void)
 		rte_delay_ms(10);
 
 	/* Test with invalid crypto device */
-	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
-			RTE_CRYPTO_MAX_DEVS, qp_id, cb),
+	ret = rte_cryptodev_remove_enq_callback(RTE_CRYPTO_MAX_DEVS,
+					qp_id,
+					cb);
+	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
+	TEST_ASSERT_FAIL(ret,
 			"Expected call to fail as crypto device is invalid");
 
 	/* Test with invalid queue pair */
-	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
-			ts_params->valid_devs[0],
-			dev_info.max_nb_queue_pairs + 1, cb),
+	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
+					dev_info.max_nb_queue_pairs + 1,
+					cb);
+	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
+	TEST_ASSERT_FAIL(ret,
 			"Expected call to fail as queue pair is invalid");
 
 	/* Test with NULL callback */
-	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
-			ts_params->valid_devs[0], qp_id, NULL),
+	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
+					qp_id,
+					NULL);
+	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
+	TEST_ASSERT_FAIL(ret,
 			"Expected call to fail as callback is NULL");
 
 	/* Test with valid configuration */
-	TEST_ASSERT_SUCCESS(rte_cryptodev_remove_enq_callback(
-			ts_params->valid_devs[0], qp_id, cb),
+	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
+					qp_id,
+					cb);
+	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
+	TEST_ASSERT_SUCCESS(ret,
 			"Failed test to remove callback on "
 			"qp %u on cryptodev %u",
 			qp_id, ts_params->valid_devs[0]);
@@ -14869,7 +14896,7 @@ test_deq_callback_setup(void)
 
 	struct rte_cryptodev_cb *cb;
 	uint16_t qp_id = 0;
-	int j = 0;
+	int j = 0, ret;
 
 	/* Verify the crypto capabilities for which enqueue/dequeue is done. */
 	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH;
@@ -14909,6 +14936,7 @@ test_deq_callback_setup(void)
 	/* Test with invalid crypto device */
 	cb = rte_cryptodev_add_deq_callback(RTE_CRYPTO_MAX_DEVS,
 			qp_id, test_deq_callback, NULL);
+	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
 			"cryptodev %u did not fail",
 			qp_id, RTE_CRYPTO_MAX_DEVS);
@@ -14917,6 +14945,7 @@ test_deq_callback_setup(void)
 	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
 			dev_info.max_nb_queue_pairs + 1,
 			test_deq_callback, NULL);
+	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
 			"cryptodev %u did not fail",
 			dev_info.max_nb_queue_pairs + 1,
@@ -14925,6 +14954,7 @@ test_deq_callback_setup(void)
 	/* Test with NULL callback */
 	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
 			qp_id, NULL, NULL);
+	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
 			"cryptodev %u did not fail",
 			qp_id, ts_params->valid_devs[0]);
@@ -14932,6 +14962,7 @@ test_deq_callback_setup(void)
 	/* Test with valid configuration */
 	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
 			qp_id, test_deq_callback, NULL);
+	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_NOT_NULL(cb, "Failed test to add callback on "
 			"qp %u on cryptodev %u",
 			qp_id, ts_params->valid_devs[0]);
@@ -14945,24 +14976,36 @@ test_deq_callback_setup(void)
 		rte_delay_ms(10);
 
 	/* Test with invalid crypto device */
+	ret = rte_cryptodev_remove_deq_callback(RTE_CRYPTO_MAX_DEVS,
+					qp_id,
+					cb);
+	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
 	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
 			RTE_CRYPTO_MAX_DEVS, qp_id, cb),
 			"Expected call to fail as crypto device is invalid");
 
 	/* Test with invalid queue pair */
-	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
-			ts_params->valid_devs[0],
-			dev_info.max_nb_queue_pairs + 1, cb),
+	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
+					dev_info.max_nb_queue_pairs + 1,
+					cb);
+	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
+	TEST_ASSERT_FAIL(ret,
 			"Expected call to fail as queue pair is invalid");
 
 	/* Test with NULL callback */
-	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
-			ts_params->valid_devs[0], qp_id, NULL),
+	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
+					qp_id,
+					NULL);
+	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
+	TEST_ASSERT_FAIL(ret,
 			"Expected call to fail as callback is NULL");
 
 	/* Test with valid configuration */
-	TEST_ASSERT_SUCCESS(rte_cryptodev_remove_deq_callback(
-			ts_params->valid_devs[0], qp_id, cb),
+	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
+					qp_id,
+					cb);
+	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
+	TEST_ASSERT_SUCCESS(ret,
 			"Failed test to remove callback on "
 			"qp %u on cryptodev %u",
 			qp_id, ts_params->valid_devs[0]);
diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
index 886eb7a..8a45ad7 100644
--- a/lib/cryptodev/rte_cryptodev.c
+++ b/lib/cryptodev/rte_cryptodev.c
@@ -1491,6 +1491,10 @@ rte_cryptodev_add_enq_callback(uint8_t dev_id,
 			       rte_cryptodev_callback_fn cb_fn,
 			       void *cb_arg)
 {
+#ifdef RTE_CRYPTO_CALLBACKS
+	rte_errno = ENOTSUP;
+	return NULL;
+#endif
 	struct rte_cryptodev *dev;
 	struct rte_cryptodev_cb_rcu *list;
 	struct rte_cryptodev_cb *cb, *tail;
@@ -1556,6 +1560,9 @@ rte_cryptodev_remove_enq_callback(uint8_t dev_id,
 				  uint16_t qp_id,
 				  struct rte_cryptodev_cb *cb)
 {
+#ifdef RTE_CRYPTO_CALLBACKS
+	return -ENOTSUP;
+#endif
 	struct rte_cryptodev *dev;
 	RTE_ATOMIC(struct rte_cryptodev_cb *) *prev_cb;
 	struct rte_cryptodev_cb *curr_cb;
@@ -1630,6 +1637,10 @@ rte_cryptodev_add_deq_callback(uint8_t dev_id,
 			       rte_cryptodev_callback_fn cb_fn,
 			       void *cb_arg)
 {
+#ifdef RTE_CRYPTO_CALLBACKS
+	rte_errno = ENOTSUP;
+	return NULL;
+#endif
 	struct rte_cryptodev *dev;
 	struct rte_cryptodev_cb_rcu *list;
 	struct rte_cryptodev_cb *cb, *tail;
@@ -1696,6 +1707,9 @@ rte_cryptodev_remove_deq_callback(uint8_t dev_id,
 				  uint16_t qp_id,
 				  struct rte_cryptodev_cb *cb)
 {
+#ifdef RTE_CRYPTO_CALLBACKS
+	return -ENOTSUP;
+#endif
 	struct rte_cryptodev *dev;
 	RTE_ATOMIC(struct rte_cryptodev_cb *) *prev_cb;
 	struct rte_cryptodev_cb *curr_cb;
-- 
2.6.4

[PATCH v3 2/2] cryptodev: validate crypto callbacks from next node

[Ganapati Kundapura]

Crypto callbacks are invoked on checking from head node
which is always valid pointer.

This patch checks next node from the head node if callbacks
registered before invoking callbacks.

Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")

Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>

---
v3: retained ifdef

diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index c946f74..bec947f 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -1910,7 +1910,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 (unlikely(fp_ops->qp.deq_cb != NULL)) {
+	if (unlikely(fp_ops->qp.deq_cb[qp_id].next != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
 
@@ -1977,7 +1977,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 (unlikely(fp_ops->qp.enq_cb != NULL)) {
+	if (unlikely(fp_ops->qp.enq_cb[qp_id].next != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
 
-- 
2.6.4

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

[Kundapura, Ganapati]

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
 + ------------------------------------------------------- +
 + -------------------------------------------------------

RE: [EXTERNAL] [PATCH v3 1/2] cryptodev: fix crypto callbacks on unsetting callbacks macro

[Akhil Goyal]

> Crypto callbacks APIs are available in header files but when
> the macro RTE_CRYPTO_CALLBACKS unset, test application need to
> put #ifdef in its code.
> 
> The test application should be able to build and run, regardless
> DPDK library is built with RTE_CRYPTO_CALLBACKS defined or not.
> 
> Added ENOTSUP from the beginning of the APIs implementation
> if RTE_CRYPTO_CALLBACKS macro is unset/undefined.
> 
> 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>
> 
> ---
> v3:
> * Added NOTSUP from the beginning of the APIs
> 
> diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> index 94438c5..7dca2c9 100644
> --- a/app/test/test_cryptodev.c
> +++ b/app/test/test_cryptodev.c
> @@ -118,6 +118,18 @@ struct crypto_unittest_params {
>  	for (j = index; j < index + num_blk_types; j++)
> 	\
>  		free_blockcipher_test_suite(parent_ts.unit_test_suites[j])
> 
> +#define TEST_SKIP_LOG(cond, msg, ...) do {				\
> +	if ((cond)) {							\
> +		RTE_LOG(ERR, CRYPTODEV, "%s line %d: "
> 	\
> +			msg "\n", __func__, __LINE__, ##__VA_ARGS__);
> 	\
> +		RTE_TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);
> 	\
> +		return TEST_SKIPPED;					\
> +	}								\
> +} while (0)
> +
> +#define TEST_SKIP(a, b, msg, ...) \
> +	TEST_SKIP_LOG(a == b, msg, ##__VA_ARGS__)
> +

This can be moved to app/test/test.h
Also can we rename it to TEST_ASSERT_SKIP?

>  /*
>   * Forward declarations.
>   */
> @@ -14754,7 +14766,7 @@ test_enq_callback_setup(void)
> 
>  	struct rte_cryptodev_cb *cb;
>  	uint16_t qp_id = 0;
> -	int j = 0;
> +	int j = 0, ret;
> 
>  	/* Verify the crypto capabilities for which enqueue/dequeue is done.
> */
>  	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH;
> @@ -14794,6 +14806,7 @@ test_enq_callback_setup(void)
>  	/* Test with invalid crypto device */
>  	cb = rte_cryptodev_add_enq_callback(RTE_CRYPTO_MAX_DEVS,
>  			qp_id, test_enq_callback, NULL);
> +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
>  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
>  			"cryptodev %u did not fail",
>  			qp_id, RTE_CRYPTO_MAX_DEVS);
> @@ -14802,6 +14815,7 @@ test_enq_callback_setup(void)
>  	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
>  			dev_info.max_nb_queue_pairs + 1,
>  			test_enq_callback, NULL);
> +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");

Why do we need to check for ENOTSUP at multiple places in the same function?
This can be checked at the first usage only.

>  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
>  			"cryptodev %u did not fail",
>  			dev_info.max_nb_queue_pairs + 1,
> @@ -14810,6 +14824,7 @@ test_enq_callback_setup(void)
>  	/* Test with NULL callback */
>  	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
>  			qp_id, NULL, NULL);
> +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
>  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
>  			"cryptodev %u did not fail",
>  			qp_id, ts_params->valid_devs[0]);
> @@ -14817,6 +14832,7 @@ test_enq_callback_setup(void)
>  	/* Test with valid configuration */
>  	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
>  			qp_id, test_enq_callback, NULL);
> +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
>  	TEST_ASSERT_NOT_NULL(cb, "Failed test to add callback on "
>  			"qp %u on cryptodev %u",
>  			qp_id, ts_params->valid_devs[0]);
> @@ -14830,24 +14846,35 @@ test_enq_callback_setup(void)
>  		rte_delay_ms(10);
> 
>  	/* Test with invalid crypto device */
> -	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
> -			RTE_CRYPTO_MAX_DEVS, qp_id, cb),
> +	ret = rte_cryptodev_remove_enq_callback(RTE_CRYPTO_MAX_DEVS,
> +					qp_id,
> +					cb);
> +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> +	TEST_ASSERT_FAIL(ret,
>  			"Expected call to fail as crypto device is invalid");
> 
>  	/* Test with invalid queue pair */
> -	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
> -			ts_params->valid_devs[0],
> -			dev_info.max_nb_queue_pairs + 1, cb),
> +	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
> +					dev_info.max_nb_queue_pairs + 1,
> +					cb);
> +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> +	TEST_ASSERT_FAIL(ret,
>  			"Expected call to fail as queue pair is invalid");
> 
>  	/* Test with NULL callback */
> -	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
> -			ts_params->valid_devs[0], qp_id, NULL),
> +	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
> +					qp_id,
> +					NULL);
> +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> +	TEST_ASSERT_FAIL(ret,
>  			"Expected call to fail as callback is NULL");
> 
>  	/* Test with valid configuration */
> -	TEST_ASSERT_SUCCESS(rte_cryptodev_remove_enq_callback(
> -			ts_params->valid_devs[0], qp_id, cb),
> +	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
> +					qp_id,
> +					cb);
> +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> +	TEST_ASSERT_SUCCESS(ret,
>  			"Failed test to remove callback on "
>  			"qp %u on cryptodev %u",
>  			qp_id, ts_params->valid_devs[0]);
> @@ -14869,7 +14896,7 @@ test_deq_callback_setup(void)
> 
>  	struct rte_cryptodev_cb *cb;
>  	uint16_t qp_id = 0;
> -	int j = 0;
> +	int j = 0, ret;
> 
>  	/* Verify the crypto capabilities for which enqueue/dequeue is done.
> */
>  	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH;
> @@ -14909,6 +14936,7 @@ test_deq_callback_setup(void)
>  	/* Test with invalid crypto device */
>  	cb = rte_cryptodev_add_deq_callback(RTE_CRYPTO_MAX_DEVS,
>  			qp_id, test_deq_callback, NULL);
> +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
>  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
>  			"cryptodev %u did not fail",
>  			qp_id, RTE_CRYPTO_MAX_DEVS);
> @@ -14917,6 +14945,7 @@ test_deq_callback_setup(void)
>  	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
>  			dev_info.max_nb_queue_pairs + 1,
>  			test_deq_callback, NULL);
> +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
>  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
>  			"cryptodev %u did not fail",
>  			dev_info.max_nb_queue_pairs + 1,
> @@ -14925,6 +14954,7 @@ test_deq_callback_setup(void)
>  	/* Test with NULL callback */
>  	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
>  			qp_id, NULL, NULL);
> +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
>  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
>  			"cryptodev %u did not fail",
>  			qp_id, ts_params->valid_devs[0]);
> @@ -14932,6 +14962,7 @@ test_deq_callback_setup(void)
>  	/* Test with valid configuration */
>  	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
>  			qp_id, test_deq_callback, NULL);
> +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
>  	TEST_ASSERT_NOT_NULL(cb, "Failed test to add callback on "
>  			"qp %u on cryptodev %u",
>  			qp_id, ts_params->valid_devs[0]);
> @@ -14945,24 +14976,36 @@ test_deq_callback_setup(void)
>  		rte_delay_ms(10);
> 
>  	/* Test with invalid crypto device */
> +	ret = rte_cryptodev_remove_deq_callback(RTE_CRYPTO_MAX_DEVS,
> +					qp_id,
> +					cb);
> +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
>  	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
>  			RTE_CRYPTO_MAX_DEVS, qp_id, cb),
>  			"Expected call to fail as crypto device is invalid");
> 
>  	/* Test with invalid queue pair */
> -	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
> -			ts_params->valid_devs[0],
> -			dev_info.max_nb_queue_pairs + 1, cb),
> +	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
> +					dev_info.max_nb_queue_pairs + 1,
> +					cb);
> +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> +	TEST_ASSERT_FAIL(ret,
>  			"Expected call to fail as queue pair is invalid");
> 
>  	/* Test with NULL callback */
> -	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
> -			ts_params->valid_devs[0], qp_id, NULL),
> +	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
> +					qp_id,
> +					NULL);
> +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> +	TEST_ASSERT_FAIL(ret,
>  			"Expected call to fail as callback is NULL");
> 
>  	/* Test with valid configuration */
> -	TEST_ASSERT_SUCCESS(rte_cryptodev_remove_deq_callback(
> -			ts_params->valid_devs[0], qp_id, cb),
> +	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
> +					qp_id,
> +					cb);
> +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> +	TEST_ASSERT_SUCCESS(ret,
>  			"Failed test to remove callback on "
>  			"qp %u on cryptodev %u",
>  			qp_id, ts_params->valid_devs[0]);
> diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
> index 886eb7a..8a45ad7 100644
> --- a/lib/cryptodev/rte_cryptodev.c
> +++ b/lib/cryptodev/rte_cryptodev.c
> @@ -1491,6 +1491,10 @@ rte_cryptodev_add_enq_callback(uint8_t dev_id,
>  			       rte_cryptodev_callback_fn cb_fn,
>  			       void *cb_arg)
>  {
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	rte_errno = ENOTSUP;
> +	return NULL;
> +#endif

This should be ifndef at all places.
Did you test this?


>  	struct rte_cryptodev *dev;
>  	struct rte_cryptodev_cb_rcu *list;
>  	struct rte_cryptodev_cb *cb, *tail;
> @@ -1556,6 +1560,9 @@ rte_cryptodev_remove_enq_callback(uint8_t
> dev_id,
>  				  uint16_t qp_id,
>  				  struct rte_cryptodev_cb *cb)
>  {
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	return -ENOTSUP;
> +#endif
>  	struct rte_cryptodev *dev;
>  	RTE_ATOMIC(struct rte_cryptodev_cb *) *prev_cb;
>  	struct rte_cryptodev_cb *curr_cb;
> @@ -1630,6 +1637,10 @@ rte_cryptodev_add_deq_callback(uint8_t dev_id,
>  			       rte_cryptodev_callback_fn cb_fn,
>  			       void *cb_arg)
>  {
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	rte_errno = ENOTSUP;
> +	return NULL;
> +#endif
>  	struct rte_cryptodev *dev;
>  	struct rte_cryptodev_cb_rcu *list;
>  	struct rte_cryptodev_cb *cb, *tail;
> @@ -1696,6 +1707,9 @@ rte_cryptodev_remove_deq_callback(uint8_t
> dev_id,
>  				  uint16_t qp_id,
>  				  struct rte_cryptodev_cb *cb)
>  {
> +#ifdef RTE_CRYPTO_CALLBACKS
> +	return -ENOTSUP;
> +#endif
>  	struct rte_cryptodev *dev;
>  	RTE_ATOMIC(struct rte_cryptodev_cb *) *prev_cb;
>  	struct rte_cryptodev_cb *curr_cb;
> --
> 2.6.4

[PATCH v4 1/2] cryptodev: fix crypto callbacks on unsetting callbacks macro

[Ganapati Kundapura]

Crypto callbacks APIs are available in header files but when
the macro RTE_CRYPTO_CALLBACKS unset, test application need to
put #ifdef in its code.

The test application should be able to build and run, regardless
DPDK library is built with RTE_CRYPTO_CALLBACKS defined or not.

Added ENOTSUP from the beginning of the APIs implementation
if RTE_CRYPTO_CALLBACKS macro is unset/undefined.

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>

---
v4:
* replaced ifdef with ifndef in APIs implementation
* Removed TEST_SKIP macro
* checked for ENOTSUP for first usage only

v3:
* Added NOTSUP from the beginning of the APIs

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 94438c5..6d57ea1 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -14794,6 +14794,12 @@ test_enq_callback_setup(void)
 	/* Test with invalid crypto device */
 	cb = rte_cryptodev_add_enq_callback(RTE_CRYPTO_MAX_DEVS,
 			qp_id, test_enq_callback, NULL);
+	if (rte_errno == ENOTSUP) {
+		RTE_LOG(ERR, USER1, "%s line %d: "
+			"rte_cryptodev_add_enq_callback() "
+			"Not supported, skipped\n", __func__, __LINE__);
+		return TEST_SKIPPED;
+	}
 	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
 			"cryptodev %u did not fail",
 			qp_id, RTE_CRYPTO_MAX_DEVS);
@@ -14909,6 +14915,12 @@ test_deq_callback_setup(void)
 	/* Test with invalid crypto device */
 	cb = rte_cryptodev_add_deq_callback(RTE_CRYPTO_MAX_DEVS,
 			qp_id, test_deq_callback, NULL);
+	if (rte_errno == ENOTSUP) {
+		RTE_LOG(ERR, USER1, "%s line %d: "
+			"rte_cryptodev_add_deq_callback() "
+			"Not supported, skipped\n", __func__, __LINE__);
+		return TEST_SKIPPED;
+	}
 	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
 			"cryptodev %u did not fail",
 			qp_id, RTE_CRYPTO_MAX_DEVS);
diff --git a/lib/cryptodev/rte_cryptodev.c b/lib/cryptodev/rte_cryptodev.c
index 886eb7a..682c9f4 100644
--- a/lib/cryptodev/rte_cryptodev.c
+++ b/lib/cryptodev/rte_cryptodev.c
@@ -1491,6 +1491,10 @@ rte_cryptodev_add_enq_callback(uint8_t dev_id,
 			       rte_cryptodev_callback_fn cb_fn,
 			       void *cb_arg)
 {
+#ifndef RTE_CRYPTO_CALLBACKS
+	rte_errno = ENOTSUP;
+	return NULL;
+#endif
 	struct rte_cryptodev *dev;
 	struct rte_cryptodev_cb_rcu *list;
 	struct rte_cryptodev_cb *cb, *tail;
@@ -1556,6 +1560,9 @@ rte_cryptodev_remove_enq_callback(uint8_t dev_id,
 				  uint16_t qp_id,
 				  struct rte_cryptodev_cb *cb)
 {
+#ifndef RTE_CRYPTO_CALLBACKS
+	return -ENOTSUP;
+#endif
 	struct rte_cryptodev *dev;
 	RTE_ATOMIC(struct rte_cryptodev_cb *) *prev_cb;
 	struct rte_cryptodev_cb *curr_cb;
@@ -1630,6 +1637,10 @@ rte_cryptodev_add_deq_callback(uint8_t dev_id,
 			       rte_cryptodev_callback_fn cb_fn,
 			       void *cb_arg)
 {
+#ifndef RTE_CRYPTO_CALLBACKS
+	rte_errno = ENOTSUP;
+	return NULL;
+#endif
 	struct rte_cryptodev *dev;
 	struct rte_cryptodev_cb_rcu *list;
 	struct rte_cryptodev_cb *cb, *tail;
@@ -1696,6 +1707,9 @@ rte_cryptodev_remove_deq_callback(uint8_t dev_id,
 				  uint16_t qp_id,
 				  struct rte_cryptodev_cb *cb)
 {
+#ifndef RTE_CRYPTO_CALLBACKS
+	return -ENOTSUP;
+#endif
 	struct rte_cryptodev *dev;
 	RTE_ATOMIC(struct rte_cryptodev_cb *) *prev_cb;
 	struct rte_cryptodev_cb *curr_cb;
-- 
2.6.4

[PATCH v3 2/2] cryptodev: validate crypto callbacks from next node

[Ganapati Kundapura]

Crypto callbacks are invoked on checking from head node
which is always valid pointer.

This patch checks next node from the head node if callbacks
registered before invoking callbacks.

Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")

Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>

---
v3: retained ifdef

diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index c946f74..bec947f 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -1910,7 +1910,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 (unlikely(fp_ops->qp.deq_cb != NULL)) {
+	if (unlikely(fp_ops->qp.deq_cb[qp_id].next != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
 
@@ -1977,7 +1977,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 (unlikely(fp_ops->qp.enq_cb != NULL)) {
+	if (unlikely(fp_ops->qp.enq_cb[qp_id].next != NULL)) {
 		struct rte_cryptodev_cb_rcu *list;
 		struct rte_cryptodev_cb *cb;
 
-- 
2.6.4

RE: [EXTERNAL] [PATCH v3 1/2] cryptodev: fix crypto callbacks on unsetting callbacks macro

[Kundapura, Ganapati]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, June 27, 2024 11:50 AM
> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>;
> ferruh.yigit@amd.com; Richardson, Bruce <bruce.richardson@intel.com>;
> mb@smartsharesystems.com; thomas@monjalon.net
> Cc: fanzhang.oss@gmail.com; Senthil, Bala <bala.senthil@intel.com>; Gujjar,
> Abhinandan S <abhinandan.gujjar@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; dev@dpdk.org
> Subject: RE: [EXTERNAL] [PATCH v3 1/2] cryptodev: fix crypto callbacks on
> unsetting callbacks macro
> 
> > Crypto callbacks APIs are available in header files but when the macro
> > RTE_CRYPTO_CALLBACKS unset, test application need to put #ifdef in its
> > code.
> >
> > The test application should be able to build and run, regardless DPDK
> > library is built with RTE_CRYPTO_CALLBACKS defined or not.
> >
> > Added ENOTSUP from the beginning of the APIs implementation if
> > RTE_CRYPTO_CALLBACKS macro is unset/undefined.
> >
> > 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>
> >
> > ---
> > v3:
> > * Added NOTSUP from the beginning of the APIs
> >
> > diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
> > index 94438c5..7dca2c9 100644
> > --- a/app/test/test_cryptodev.c
> > +++ b/app/test/test_cryptodev.c
> > @@ -118,6 +118,18 @@ struct crypto_unittest_params {
> >  	for (j = index; j < index + num_blk_types; j++)
> > 	\
> >  		free_blockcipher_test_suite(parent_ts.unit_test_suites[j])
> >
> > +#define TEST_SKIP_LOG(cond, msg, ...) do {				\
> > +	if ((cond)) {							\
> > +		RTE_LOG(ERR, CRYPTODEV, "%s line %d: "
> > 	\
> > +			msg "\n", __func__, __LINE__, ##__VA_ARGS__);
> > 	\
> > +		RTE_TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);
> > 	\
> > +		return TEST_SKIPPED;					\
> > +	}								\
> > +} while (0)
> > +
> > +#define TEST_SKIP(a, b, msg, ...) \
> > +	TEST_SKIP_LOG(a == b, msg, ##__VA_ARGS__)
> > +
> 
> This can be moved to app/test/test.h
> Also can we rename it to TEST_ASSERT_SKIP?
Removed this macro
> 
> >  /*
> >   * Forward declarations.
> >   */
> > @@ -14754,7 +14766,7 @@ test_enq_callback_setup(void)
> >
> >  	struct rte_cryptodev_cb *cb;
> >  	uint16_t qp_id = 0;
> > -	int j = 0;
> > +	int j = 0, ret;
> >
> >  	/* Verify the crypto capabilities for which enqueue/dequeue is done.
> > */
> >  	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH; @@ -14794,6
> +14806,7 @@
> > test_enq_callback_setup(void)
> >  	/* Test with invalid crypto device */
> >  	cb = rte_cryptodev_add_enq_callback(RTE_CRYPTO_MAX_DEVS,
> >  			qp_id, test_enq_callback, NULL);
> > +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
> >  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
> >  			"cryptodev %u did not fail",
> >  			qp_id, RTE_CRYPTO_MAX_DEVS);
> > @@ -14802,6 +14815,7 @@ test_enq_callback_setup(void)
> >  	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
> >  			dev_info.max_nb_queue_pairs + 1,
> >  			test_enq_callback, NULL);
> > +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
> 
> Why do we need to check for ENOTSUP at multiple places in the same
> function?
> This can be checked at the first usage only.
Checked for ENOTSUP only for first usage
> 
> >  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
> >  			"cryptodev %u did not fail",
> >  			dev_info.max_nb_queue_pairs + 1,
> > @@ -14810,6 +14824,7 @@ test_enq_callback_setup(void)
> >  	/* Test with NULL callback */
> >  	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
> >  			qp_id, NULL, NULL);
> > +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
> >  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
> >  			"cryptodev %u did not fail",
> >  			qp_id, ts_params->valid_devs[0]);
> > @@ -14817,6 +14832,7 @@ test_enq_callback_setup(void)
> >  	/* Test with valid configuration */
> >  	cb = rte_cryptodev_add_enq_callback(ts_params->valid_devs[0],
> >  			qp_id, test_enq_callback, NULL);
> > +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
> >  	TEST_ASSERT_NOT_NULL(cb, "Failed test to add callback on "
> >  			"qp %u on cryptodev %u",
> >  			qp_id, ts_params->valid_devs[0]);
> > @@ -14830,24 +14846,35 @@ test_enq_callback_setup(void)
> >  		rte_delay_ms(10);
> >
> >  	/* Test with invalid crypto device */
> > -	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
> > -			RTE_CRYPTO_MAX_DEVS, qp_id, cb),
> > +	ret = rte_cryptodev_remove_enq_callback(RTE_CRYPTO_MAX_DEVS,
> > +					qp_id,
> > +					cb);
> > +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> > +	TEST_ASSERT_FAIL(ret,
> >  			"Expected call to fail as crypto device is invalid");
> >
> >  	/* Test with invalid queue pair */
> > -	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
> > -			ts_params->valid_devs[0],
> > -			dev_info.max_nb_queue_pairs + 1, cb),
> > +	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
> > +					dev_info.max_nb_queue_pairs + 1,
> > +					cb);
> > +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> > +	TEST_ASSERT_FAIL(ret,
> >  			"Expected call to fail as queue pair is invalid");
> >
> >  	/* Test with NULL callback */
> > -	TEST_ASSERT_FAIL(rte_cryptodev_remove_enq_callback(
> > -			ts_params->valid_devs[0], qp_id, NULL),
> > +	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
> > +					qp_id,
> > +					NULL);
> > +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> > +	TEST_ASSERT_FAIL(ret,
> >  			"Expected call to fail as callback is NULL");
> >
> >  	/* Test with valid configuration */
> > -	TEST_ASSERT_SUCCESS(rte_cryptodev_remove_enq_callback(
> > -			ts_params->valid_devs[0], qp_id, cb),
> > +	ret = rte_cryptodev_remove_enq_callback(ts_params->valid_devs[0],
> > +					qp_id,
> > +					cb);
> > +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> > +	TEST_ASSERT_SUCCESS(ret,
> >  			"Failed test to remove callback on "
> >  			"qp %u on cryptodev %u",
> >  			qp_id, ts_params->valid_devs[0]);
> > @@ -14869,7 +14896,7 @@ test_deq_callback_setup(void)
> >
> >  	struct rte_cryptodev_cb *cb;
> >  	uint16_t qp_id = 0;
> > -	int j = 0;
> > +	int j = 0, ret;
> >
> >  	/* Verify the crypto capabilities for which enqueue/dequeue is done.
> > */
> >  	cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH; @@ -14909,6
> +14936,7 @@
> > test_deq_callback_setup(void)
> >  	/* Test with invalid crypto device */
> >  	cb = rte_cryptodev_add_deq_callback(RTE_CRYPTO_MAX_DEVS,
> >  			qp_id, test_deq_callback, NULL);
> > +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
> >  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
> >  			"cryptodev %u did not fail",
> >  			qp_id, RTE_CRYPTO_MAX_DEVS);
> > @@ -14917,6 +14945,7 @@ test_deq_callback_setup(void)
> >  	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
> >  			dev_info.max_nb_queue_pairs + 1,
> >  			test_deq_callback, NULL);
> > +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
> >  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
> >  			"cryptodev %u did not fail",
> >  			dev_info.max_nb_queue_pairs + 1,
> > @@ -14925,6 +14954,7 @@ test_deq_callback_setup(void)
> >  	/* Test with NULL callback */
> >  	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
> >  			qp_id, NULL, NULL);
> > +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
> >  	TEST_ASSERT_NULL(cb, "Add callback on qp %u on "
> >  			"cryptodev %u did not fail",
> >  			qp_id, ts_params->valid_devs[0]);
> > @@ -14932,6 +14962,7 @@ test_deq_callback_setup(void)
> >  	/* Test with valid configuration */
> >  	cb = rte_cryptodev_add_deq_callback(ts_params->valid_devs[0],
> >  			qp_id, test_deq_callback, NULL);
> > +	TEST_SKIP(rte_errno, ENOTSUP, "Not supported, skipped");
> >  	TEST_ASSERT_NOT_NULL(cb, "Failed test to add callback on "
> >  			"qp %u on cryptodev %u",
> >  			qp_id, ts_params->valid_devs[0]);
> > @@ -14945,24 +14976,36 @@ test_deq_callback_setup(void)
> >  		rte_delay_ms(10);
> >
> >  	/* Test with invalid crypto device */
> > +	ret = rte_cryptodev_remove_deq_callback(RTE_CRYPTO_MAX_DEVS,
> > +					qp_id,
> > +					cb);
> > +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> >  	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
> >  			RTE_CRYPTO_MAX_DEVS, qp_id, cb),
> >  			"Expected call to fail as crypto device is invalid");
> >
> >  	/* Test with invalid queue pair */
> > -	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
> > -			ts_params->valid_devs[0],
> > -			dev_info.max_nb_queue_pairs + 1, cb),
> > +	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
> > +					dev_info.max_nb_queue_pairs + 1,
> > +					cb);
> > +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> > +	TEST_ASSERT_FAIL(ret,
> >  			"Expected call to fail as queue pair is invalid");
> >
> >  	/* Test with NULL callback */
> > -	TEST_ASSERT_FAIL(rte_cryptodev_remove_deq_callback(
> > -			ts_params->valid_devs[0], qp_id, NULL),
> > +	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
> > +					qp_id,
> > +					NULL);
> > +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> > +	TEST_ASSERT_FAIL(ret,
> >  			"Expected call to fail as callback is NULL");
> >
> >  	/* Test with valid configuration */
> > -	TEST_ASSERT_SUCCESS(rte_cryptodev_remove_deq_callback(
> > -			ts_params->valid_devs[0], qp_id, cb),
> > +	ret = rte_cryptodev_remove_deq_callback(ts_params->valid_devs[0],
> > +					qp_id,
> > +					cb);
> > +	TEST_SKIP(ret, -ENOTSUP, "Not supported, skipped");
> > +	TEST_ASSERT_SUCCESS(ret,
> >  			"Failed test to remove callback on "
> >  			"qp %u on cryptodev %u",
> >  			qp_id, ts_params->valid_devs[0]);
> > diff --git a/lib/cryptodev/rte_cryptodev.c
> > b/lib/cryptodev/rte_cryptodev.c index 886eb7a..8a45ad7 100644
> > --- a/lib/cryptodev/rte_cryptodev.c
> > +++ b/lib/cryptodev/rte_cryptodev.c
> > @@ -1491,6 +1491,10 @@ rte_cryptodev_add_enq_callback(uint8_t
> dev_id,
> >  			       rte_cryptodev_callback_fn cb_fn,
> >  			       void *cb_arg)
> >  {
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	rte_errno = ENOTSUP;
> > +	return NULL;
> > +#endif
> 
> This should be ifndef at all places.
> Did you test this?
> 
Updated that for testing but missed to restore it while posting.
Yes, tested and retested with the latest patch(v4)

+ 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 0x7ffbd873e940, 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
USER1: test_enq_callback_setup line 12993: rte_cryptodev_add_enq_callback() Not supported, skipped
+ TestCase [ 4] : test_enq_callback_setup skipped
USER1: test_deq_callback_setup line 13098: rte_cryptodev_add_deq_callback()  Not supported, skipped
+ 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
 + ------------------------------------------------------- +
> 
> >  	struct rte_cryptodev *dev;
> >  	struct rte_cryptodev_cb_rcu *list;
> >  	struct rte_cryptodev_cb *cb, *tail;
> > @@ -1556,6 +1560,9 @@ rte_cryptodev_remove_enq_callback(uint8_t
> > dev_id,
> >  				  uint16_t qp_id,
> >  				  struct rte_cryptodev_cb *cb)
> >  {
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	return -ENOTSUP;
> > +#endif
> >  	struct rte_cryptodev *dev;
> >  	RTE_ATOMIC(struct rte_cryptodev_cb *) *prev_cb;
> >  	struct rte_cryptodev_cb *curr_cb;
> > @@ -1630,6 +1637,10 @@ rte_cryptodev_add_deq_callback(uint8_t
> dev_id,
> >  			       rte_cryptodev_callback_fn cb_fn,
> >  			       void *cb_arg)
> >  {
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	rte_errno = ENOTSUP;
> > +	return NULL;
> > +#endif
> >  	struct rte_cryptodev *dev;
> >  	struct rte_cryptodev_cb_rcu *list;
> >  	struct rte_cryptodev_cb *cb, *tail;
> > @@ -1696,6 +1707,9 @@ rte_cryptodev_remove_deq_callback(uint8_t
> > dev_id,
> >  				  uint16_t qp_id,
> >  				  struct rte_cryptodev_cb *cb)
> >  {
> > +#ifdef RTE_CRYPTO_CALLBACKS
> > +	return -ENOTSUP;
> > +#endif
> >  	struct rte_cryptodev *dev;
> >  	RTE_ATOMIC(struct rte_cryptodev_cb *) *prev_cb;
> >  	struct rte_cryptodev_cb *curr_cb;
> > --
> > 2.6.4

RE: [EXTERNAL] [PATCH v4 1/2] cryptodev: fix crypto callbacks on unsetting callbacks macro

[Akhil Goyal]

> Crypto callbacks APIs are available in header files but when
> the macro RTE_CRYPTO_CALLBACKS unset, test application need to
> put #ifdef in its code.
> 
> The test application should be able to build and run, regardless
> DPDK library is built with RTE_CRYPTO_CALLBACKS defined or not.
> 
> Added ENOTSUP from the beginning of the APIs implementation
> if RTE_CRYPTO_CALLBACKS macro is unset/undefined.
> 
> Fixes: 1c3ffb95595e ("cryptodev: add enqueue and dequeue callbacks")
> Fixes: 5523a75af539 ("test/crypto: add case for enqueue/dequeue callbacks")

Cc: stable@dpdk.org
> 
> Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
Acked-by: Akhil Goyal <gakhil@marvell.com>

Applied to dpdk-next-crypto