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