RE: [EXT] [PATCH v2 4/4] crypto: modify return value for asym session create

[Anoob Joseph <anoobj@marvell.com>]

Hi Ciara,

Minor nits. Please see inline.

With the fixes,
Acked-by: Anoob Joseph <anoobj@marvell.com>

Thanks,
Anoob

> -----Original Message-----
> From: Ciara Power <ciara.power@intel.com>
> Sent: Monday, January 24, 2022 8:34 PM
> To: dev@dpdk.org
> Cc: roy.fan.zhang@intel.com; Akhil Goyal <gakhil@marvell.com>; Anoob Joseph
> <anoobj@marvell.com>; mdr@ashroe.eu; Ciara Power
> <ciara.power@intel.com>; Declan Doherty <declan.doherty@intel.com>
> Subject: [EXT] [PATCH v2 4/4] crypto: modify return value for asym session
> create
> 
> External Email
> 
> ----------------------------------------------------------------------
> Rather than the asym session create function returning a session on success, and
> a NULL value on error, it is modified to now return int values - 0 on success or -
> EINVAL/-ENOTSUP/-ENOMEM on failure.
> The session to be used is passed as input.
> 
> This adds clarity on the failure of the create function, which enables treating the
> -ENOTSUP return as TEST_SKIPPED in test apps.
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---

[snip]

> @@ -744,11 +746,13 @@ cperf_create_session(struct rte_mempool *sess_mp,
>  		xform.modex.exponent.data = perf_mod_e;
>  		xform.modex.exponent.length = sizeof(perf_mod_e);
> 
> -		sess = (void *)rte_cryptodev_asym_session_create(sess_mp,
> dev_id, &xform);
> -		if (sess == NULL)
> +		ret = rte_cryptodev_asym_session_create(&asym_sess,
> +				sess_mp, dev_id, &xform);
> +		if (ret < 0) {
> +			RTE_LOG(ERR, USER1, "Asym session create failed");

[Anoob] Don't we need \n at the end?

[snip]
 
> @@ -644,9 +645,9 @@ test_rsa_sign_verify(void)
>  	struct crypto_testsuite_params_asym *ts_params = &testsuite_params;
>  	struct rte_mempool *sess_mpool = ts_params->session_mpool;
>  	uint8_t dev_id = ts_params->valid_devs[0];
> -	void *sess;
> +	void *sess = NULL;
>  	struct rte_cryptodev_info dev_info;
> -	int status = TEST_SUCCESS;
> +	int status = TEST_SUCCESS, ret;

[Anoob] May be move status to the end? Here and in other places.
https://doc.dpdk.org/guides/contributing/coding_style.html#local-variables
 
> 
>  	/* Test case supports op with exponent key only,
>  	 * Check in PMD feature flag for RSA exponent key type support.
> @@ -659,12 +660,12 @@ test_rsa_sign_verify(void)
>  		return TEST_SKIPPED;
>  	}
> 
> -	sess = rte_cryptodev_asym_session_create(sess_mpool, dev_id,
> &rsa_xform);
> -
> -	if (!sess) {
> +	ret = rte_cryptodev_asym_session_create(&sess, sess_mpool,
> +			dev_id, &rsa_xform);
> +	if (ret < 0) {
>  		RTE_LOG(ERR, USER1, "Session creation failed for "
>  			"sign_verify\n");
> -		status = TEST_FAILED;
> +		status = (ret == -ENOTSUP) ? TEST_SKIPPED : TEST_FAILED;
>  		goto error_exit;
>  	}
> 

[snip]

> diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h index
> 6a4d6d9934..89739def91 100644
> --- a/lib/cryptodev/rte_cryptodev.h
> +++ b/lib/cryptodev/rte_cryptodev.h
> @@ -990,18 +990,21 @@ rte_cryptodev_sym_session_create(struct
> rte_mempool *mempool);
>  /**
>   * Create asymmetric crypto session header (generic with no private data)
>   *
> + * @param   session    void ** for session to be used
>   * @param   mempool    mempool to allocate asymmetric session
>   *                     objects from
>   * @param   dev_id   ID of device that we want the session to be used on
>   * @param   xforms   Asymmetric crypto transform operations to apply on flow
>   *                   processed with this session
>   * @return
> - *  - On success return pointer to asym-session
> - *  - On failure returns NULL
> + *  - 0 on success.
> + *  - -EINVAL on invalid device ID, or invalid mempool.

[Anoob] PMD can also return an -EINVAL if some invalid configuration is requested. May be better to leave this open, like,

"- EINVAL Invalid arguments"

> + *  - -ENOMEM on memory error for session allocation.
> + *  - -ENOTSUP if device doesn't support session configuration.
>   */
>  __rte_experimental
> -void *
> -rte_cryptodev_asym_session_create(struct rte_mempool *mempool,
> +int
> +rte_cryptodev_asym_session_create(void **session, struct rte_mempool
> +*mempool,
>  		uint8_t dev_id, struct rte_crypto_asym_xform *xforms);
> 
>  /**
> --
> 2.25.1