[dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto operation support

[De Lara Guarch, Pablo]

Hi Hemant,

> -----Original Message-----
> From: Hemant Agrawal [mailto:hemant.agrawal at nxp.com]
> Sent: Wednesday, April 19, 2017 6:48 PM
> To: De Lara Guarch, Pablo; Akhil Goyal; dev at 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.guarch at intel.com]
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of
> > > akhil.goyal at nxp.com
> > > Sent: Wednesday, April 19, 2017 4:38 PM
> > > To: dev at dpdk.org
> > > Cc: Doherty, Declan; Mcnamara, John; hemant.agrawal at nxp.com
> > > Subject: [dpdk-dev] [PATCH v8 11/13] crypto/dpaa2_sec: add crypto
> > > operation support
> > >
> > > From: Akhil Goyal <akhil.goyal at nxp.com>
> > >
> > > Signed-off-by: Akhil Goyal <akhil.goyal at nxp.com>
> > > Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
> > > ---
> > >  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