Hi Akhil,
> -----Original Message----- > From: Akhil Goyal [mailto:akhil.go...@nxp.com] > Sent: Monday, January 15, 2018 5:49 PM > 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/15/2018 5:21 PM, Abhinandan Gujjar wrote: > > Update rte_crypto_op to indicate private data type and offset. > > > > The application may want to store private data along with the > > rte_cryptodev that is transparent to the rte_cryptodev layer. > > For e.g., If an eventdev based application is submitting a > > rte_cryptodev_sym_session operation and wants to indicate event > > information required to construct a new event that will be enqueued to > > eventdev after completion of the rte_cryptodev_sym_session operation. > > This patch provides a mechanism for the application to associate this > > information with the rte_cryptodev_sym_session session. > > The application can set the private data using > > rte_cryptodev_sym_session_set_private_data() and retrieve it using > > rte_cryptodev_sym_session_get_private_data(). > > > > Signed-off-by: Abhinandan Gujjar <abhinandan.guj...@intel.com> > > Signed-off-by: Nikhil Rao <nikhil....@intel.com> > Subject line should be cryptodev:.... > similar comment for 2/2 Ok. > > --- > > lib/librte_cryptodev/rte_crypto.h | 19 +++++++++++++++++-- > > lib/librte_cryptodev/rte_cryptodev.h | 30 > ++++++++++++++++++++++++++++++ > > 2 files changed, 47 insertions(+), 2 deletions(-) > > > > 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? > > /** > > * 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. > > + 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 */ > > diff --git a/lib/librte_cryptodev/rte_cryptodev.h > > b/lib/librte_cryptodev/rte_cryptodev.h > > index dade554..56958a6 100644 > > --- a/lib/librte_cryptodev/rte_cryptodev.h > > +++ b/lib/librte_cryptodev/rte_cryptodev.h > > @@ -1033,6 +1033,36 @@ struct rte_cryptodev_sym_session * > > */ > > const char *rte_cryptodev_driver_name_get(uint8_t driver_id); > > > > +/** > > + * Set private data for a session. > > + * > > + * @param sess Session pointer allocated by > > + * *rte_cryptodev_sym_session_create*. > > + * @param data Pointer to the private data. > > + * @param size Size of the private data. > > + * > > + * @return > > + * - On success, zero. > > + * - On failure, a negative value. > > + */ > > +int rte_cryptodev_sym_session_set_private_data( > > + struct rte_cryptodev_sym_session *sess, > > + void *data, > > + uint16_t size); > Looks like this is an RFC patch. There is no implementation of this API and > the > formatting is also incorrect. Please correct formatting while sending the > complete patch. > Same comment for 2/2 patch Ok > > + > > +/** > > + * 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. > > #ifdef __cplusplus > > } > > #endif > > > > -Akhil Abhinandan
