Re: [dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto operation support
Hi Hemant,
> -Original Message-
> From: Hemant Agrawal [mailto:hemant.agra...@nxp.com]
> Sent: Wednesday, April 19, 2017 6:48 PM
> To: De Lara Guarch, Pablo; Akhil Goyal; dev@dpdk.org
> Cc: Doherty, Declan; Mcnamara, John
> Subject: RE: [dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto
> operation support
>
> Hi Pablo,
>
> > -Original Message-
> > From: De Lara Guarch, Pablo [mailto:pablo.de.lara.gua...@intel.com]
> > > -Original Message-
> > > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of
> > > akhil.go...@nxp.com
> > > Sent: Wednesday, April 19, 2017 4:38 PM
> > > To: dev@dpdk.org
> > > Cc: Doherty, Declan; Mcnamara, John; hemant.agra...@nxp.com
> > > Subject: [dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto
> > > operation support
> > >
> > > From: Akhil Goyal
> > >
> > > Signed-off-by: Akhil Goyal
> > > Signed-off-by: Hemant Agrawal
> > > ---
> > > drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 1236
> > > +++
> > > drivers/crypto/dpaa2_sec/dpaa2_sec_priv.h | 143
> > > 2 files changed, 1379 insertions(+)
> > >
> > > diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> > > b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> > > index e0e8cfb..7c497c0 100644
> > > --- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> > > +++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> >
> > ...
> >
> > > +/** Clear the memory of session so it doesn't leave key material
> > > +behind */ static void dpaa2_sec_session_clear(struct rte_cryptodev
> > > +*dev __rte_unused, void
> > > *sess)
> > > +{
> > > + PMD_INIT_FUNC_TRACE();
> > > + dpaa2_sec_session *s = (dpaa2_sec_session *)sess;
> > > +
> > > + if (s) {
> > > + if (s->ctxt)
> > > + rte_free(s->ctxt);
> > > + if (&s->cipher_key)
> > > + rte_free(s->cipher_key.data);
> > > + if (&s->auth_key)
> > > + rte_free(s->auth_key.data);
> >
> > No need for these checks, rte_free can handle NULL pointers (assuming
> that the
> > structure is initialized to all 0s when created, which looks like it is
> happening
> > below).
> >
> > Unless there are other changes required (I am currently reviewing the
> patchset),
> > I can make this and the change from the other email myself, when
> applying the
> > patchset.
>
> [Hemant] No, we are not expecting other changes.
>
> If you want, I can send the new patchset or you can make the changes -
> either way is fine with us.
> (2nd is preferred 😊)
There are other issues with this patchset.
1 - There are two functions that are not being used:
/root/dpdk-next-crypto-nxp/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:77:1:
error: unused function 'print_fd' [-Werror,-Wunused-function]
print_fd(const struct qbman_fd *fd)
^
/root/dpdk-next-crypto-nxp/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:91:1:
error: unused function 'print_fle' [-Werror,-Wunused-function]
print_fle(const struct qbman_fle *fle)
^
2 - When enabling CONFIG_RTE_LIBRTE_DPAA2_SEC_DEBUG_RX, I see the following
errors
/root/dpdk-next-crypto-nxp/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:554:6:
error: 'bpid_info' undeclared (first use in this function)
bpid_info[DPAA2_GET_FD_BPID(fd)].meta_data_size,
^
/root/dpdk-next-crypto-nxp/build/include/rte_log.h:334:32: note: in definition
of macro 'RTE_LOG'
RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
^~~
/root/dpdk-next-crypto-nxp/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c:551:2:
note: in expansion of macro 'PMD_RX_LOG'
PMD_RX_LOG(DEBUG, "fdaddr =%p bpid =%d meta =%d off =%d, len =%d",
^~
So, I think these errors deserve a v9, sorry I just spotted them.
Pablo
> >
> > Thanks,
> > Pablo
Re: [dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto operation support
Hi Pablo,
> -Original Message-
> From: De Lara Guarch, Pablo [mailto:pablo.de.lara.gua...@intel.com]
> > -Original Message-
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of
> > akhil.go...@nxp.com
> > Sent: Wednesday, April 19, 2017 4:38 PM
> > To: dev@dpdk.org
> > Cc: Doherty, Declan; Mcnamara, John; hemant.agra...@nxp.com
> > Subject: [dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto
> > operation support
> >
> > From: Akhil Goyal
> >
> > Signed-off-by: Akhil Goyal
> > Signed-off-by: Hemant Agrawal
> > ---
> > drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 1236
> > +++
> > drivers/crypto/dpaa2_sec/dpaa2_sec_priv.h | 143
> > 2 files changed, 1379 insertions(+)
> >
> > diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> > b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> > index e0e8cfb..7c497c0 100644
> > --- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> > +++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
>
> ...
>
> > +/** Clear the memory of session so it doesn't leave key material
> > +behind */ static void dpaa2_sec_session_clear(struct rte_cryptodev
> > +*dev __rte_unused, void
> > *sess)
> > +{
> > + PMD_INIT_FUNC_TRACE();
> > + dpaa2_sec_session *s = (dpaa2_sec_session *)sess;
> > +
> > + if (s) {
> > + if (s->ctxt)
> > + rte_free(s->ctxt);
> > + if (&s->cipher_key)
> > + rte_free(s->cipher_key.data);
> > + if (&s->auth_key)
> > + rte_free(s->auth_key.data);
>
> No need for these checks, rte_free can handle NULL pointers (assuming that the
> structure is initialized to all 0s when created, which looks like it is
> happening
> below).
>
> Unless there are other changes required (I am currently reviewing the
> patchset),
> I can make this and the change from the other email myself, when applying the
> patchset.
[Hemant] No, we are not expecting other changes.
If you want, I can send the new patchset or you can make the changes - either
way is fine with us.
(2nd is preferred 😊)
>
> Thanks,
> Pablo
Re: [dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto operation support
> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of
> akhil.go...@nxp.com
> Sent: Wednesday, April 19, 2017 4:38 PM
> To: dev@dpdk.org
> Cc: Doherty, Declan; Mcnamara, John; hemant.agra...@nxp.com
> Subject: [dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto
> operation support
>
> From: Akhil Goyal
>
> Signed-off-by: Akhil Goyal
> Signed-off-by: Hemant Agrawal
> ---
> drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 1236
> +++
> drivers/crypto/dpaa2_sec/dpaa2_sec_priv.h | 143
> 2 files changed, 1379 insertions(+)
>
> diff --git a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> index e0e8cfb..7c497c0 100644
> --- a/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
> +++ b/drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c
...
> +/** Clear the memory of session so it doesn't leave key material behind */
> +static void
> +dpaa2_sec_session_clear(struct rte_cryptodev *dev __rte_unused, void
> *sess)
> +{
> + PMD_INIT_FUNC_TRACE();
> + dpaa2_sec_session *s = (dpaa2_sec_session *)sess;
> +
> + if (s) {
> + if (s->ctxt)
> + rte_free(s->ctxt);
> + if (&s->cipher_key)
> + rte_free(s->cipher_key.data);
> + if (&s->auth_key)
> + rte_free(s->auth_key.data);
No need for these checks, rte_free can handle NULL pointers
(assuming that the structure is initialized to all 0s when created, which looks
like it is happening below).
Unless there are other changes required (I am currently reviewing the
patchset), I can make this and
the change from the other email myself, when applying the patchset.
Thanks,
Pablo
