Hi Akhil,
> -----Original Message----- > From: Akhil Goyal [mailto:akhil.go...@nxp.com] > Sent: Tuesday, January 16, 2018 11:55 AM > To: Gujjar, Abhinandan S <abhinandan.guj...@intel.com>; Doherty, Declan > <declan.dohe...@intel.com> > Cc: dev@dpdk.org; Vangati, Narender <narender.vang...@intel.com>; Rao, > Nikhil <nikhil....@intel.com> > Subject: Re: [PATCH 1/2] lib/cryptodev: add support to set session private > data > > Hi Abhinandan, > On 1/16/2018 11:39 AM, Gujjar, Abhinandan S wrote: > >>> diff --git a/lib/librte_cryptodev/rte_crypto.h > >>> b/lib/librte_cryptodev/rte_crypto.h > >>> index bbc510d..3a98cbf 100644 > >>> --- a/lib/librte_cryptodev/rte_crypto.h > >>> +++ b/lib/librte_cryptodev/rte_crypto.h > >>> @@ -62,6 +62,18 @@ enum rte_crypto_op_sess_type { > >>> RTE_CRYPTO_OP_SECURITY_SESSION /**< Security session crypto > >> operation */ > >>> }; > >>> > >>> +/** Private data types for cryptographic operation > >>> + * @see rte_crypto_op::private_data_type */ enum > >>> +rte_crypto_op_private_data_type { > >>> + RTE_CRYPTO_OP_PRIVATE_DATA_NONE, > >>> + /**< No private data */ > >>> + RTE_CRYPTO_OP_PRIVATE_DATA_OP, > >>> + /**< Private data is part of rte_crypto_op and indicated by > >>> + * private_data_offset */ > >>> + RTE_CRYPTO_OP_PRIVATE_DATA_SESSION > >>> + /**< Private data is available at session */ }; > >>> + > >> We may get away with this enum. If private_data_offset is "0", then > >> we can check with the session if it has priv data or not. > > Right now, Application uses 'rte_crypto_op_private_data_type' to > > indicate rte_cryptodev_sym_session_set_private_data() > > was called to set the private data. Otherwise, how do you indicate there is > > a > private data associated with the session? > > Any suggestions? > For session based flows, the first choice to store the private data should be > in > the session. So RTE_CRYPTO_OP_WITH_SESSION or > RTE_CRYPTO_OP_SECURITY_SESSION can be used to call > rte_cryptodev_sym_session_set_private_data or > rte_security_session_set_private_data. Case 1: private_data_offset is "0" and sess_type = RTE_CRYPTO_OP_WITH_SESSION -> usual case Case 2: private_data_offset is "0" and sess_type = RTE_CRYPTO_OP_WITH_SESSION + event case (access private data) Differentiating between case 1 & 2 will be done by checking rte_crypto_op_private_data_type == RTE_CRYPTO_OP_PRIVATE_DATA_SESSION. > >>> /** > >>> * Cryptographic Operation. > >>> * > >>> @@ -84,8 +96,11 @@ struct rte_crypto_op { > >>> */ > >>> uint8_t sess_type; > >>> /**< operation session type */ > >>> - > >>> - uint8_t reserved[5]; > >>> + uint8_t private_data_type; > >>> + /**< Private data type. @see enum rte_crypto_op_private_data_type > >> */ > >>> + uint16_t private_data_offset; > >>> + /**< Offset to indicate start of private data */ > >> It is better to add some more information in the comments just like > description. > >> While viewing the code, it is not explicit that who is the intended > >> user of this private data. > > The propose APIs are generic, that’s that reason eventdev was not > > mentioned in the comments of this patch & mentioned only in the description. > it may be written as, "Offset to indicate start of private data (if any). The > private > data may be used by the application to store information which should remain > untouched in the library/driver" Ok > >>> + uint8_t reserved[3]; > >>> /**< Reserved bytes to fill 64 bits for future additions */ > >>> struct rte_mempool *mempool; > >>> /**< crypto operation mempool which operation is allocated from > >>> */ > > <snip> > >>> + > >>> +/** > >>> + * Get private data of a session. > >>> + * > >>> + * @param sess Session pointer allocated by > >>> + * *rte_cryptodev_sym_session_create*. > >>> + * > >>> + * @return > >>> + * - On success return pointer to private data. > >>> + * - On failure returns NULL. > >>> + */ > >>> +void *rte_cryptodev_sym_session_get_private_data( > >>> + const struct rte_cryptodev_sym_session *sess); > >>> + > >> same here. > > This doesn’t fit into a single line or in the next line aligned with > > bracket. > void * should be in a separate line. ok > > > -Akhil Abhinandan
