From: Akhil Goyal
> > From: Akhil Goyal
> > > > > > > > +static void
> > > > > > > > +mlx5_crypto_sym_session_clear(struct rte_cryptodev *dev,
> > > > > > > > + struct rte_cryptodev_sym_session
> > > > > > > > *sess) {
> > > > > > > > + struct mlx5_crypto_priv *priv = dev->data->dev_private;
> > > > > > > > + struct mlx5_crypto_session *sess_private_data =
> > > > > > > > + get_sym_session_private_data(sess,
> > > > > > > > +dev->driver_id);
> > > > > > > > +
> > > > > > > > + if (unlikely(sess_private_data == NULL)) {
> > > > > > > > + DRV_LOG(ERR, "Failed to get session %p private
> > > > > > > > data.",
> > > > > > > > + sess_private_data);
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > + mlx5_crypto_dek_destroy(priv, sess_private_data->dek);
> > > > > > > > + DRV_LOG(DEBUG, "Session %p was cleared.",
> > > > > > > > + sess_private_data);
> > > > }
> > > > > > >
> > > > > > > Memory leakage, mempool is not freed.
> > > > > >
> > > > > > Yes, good catch, this part was missed.
> > > > > >
> > > > > > > IMO, this driver is not properly tested with the unit test app.
> > > > > >
> > > > > > The only app we tested until now is l2fwd_crypto and it works fine!
> > > > > > We can add it to doc.
> > > > > >
> > > > > > > Please add a note in the documentation that it is tested
> > > > > > > with
> > autotest.
> > > > > >
> > > > > >
> > > > > > The next app we want to test with, is test-crypto-perf.
> > > > > >
> > > > > I would recommend to run the test app first.
> > > > > It will catch most of your basic bugs like the one above.
> > > >
> > > > It is too late for this, will add the test adjustment later.
> > >
> > > Can we postpone the PMD to next release.
> >
> > We can, but this is not our plan.
> > We met all the DPDK rules to push it on time.
>
> On time!! Really? Incomplete v1 was submitted before RC1, The complete
> PMD was submitted only during RC2.
Yes, on time, you got draft in RC1 -- most of your critical comments are
relevant for that version.
> > > I believe test application makes
> > > The PMD look robust as per the DPDK crypto PMD API usage.
> >
> > Every test will add robustness to the PMD.
> >
> > > I haven't seen a PMD getting merged without test app.
> >
> > compress/mlx5, vdpa/mlx5, regex/mlx5, net/mlx5, vdev_netvsc....
> >
> All these are 'not' crypto PMDs.
> For crypto, UT is a must. I would like to add a statement in the guidelines
> if it
> is not there.
>
> > > And I apologize I did not mentioned it earlier, but it is kind of
> > > obvious thing
> > to
> > > run test app before sending it to upstream.
> >
> > In fact, no, I added more than one PMD, no one require specific test.
> >
> > > L2fwd-crypto is not doing data validation hence you cannot be sure
> > > that it
> > is
> > > working fine as per other standard stacks like Linux.
> >
> > It is not do data validation, but we check that the packet payload
> > return back has the expected encrypted\decrypted data using open-ssl.
> > Also for us it was mandatory requirement to check it.
> >
> > For us, the current validation is enough, we don't support a lot of
> > things in the crypto PMD currently, only one algorithm in the most basic
> modes.
> >
> > For sure we will continue to add tests and to increase robustness.
> > Also adding more features is in plan for future.
> >
> > If you postpone it, it yours, we don't agree with it.
> >
> Please fix the review comments by today, if you want it to be merged in RC3.
> IMO, the driver is not ready to be merged.
> I want to postpone it as it is not feasible to fix all the comments by RC3
> timeline.
Yes, it is too late.
Let's continue in next release.
Thanks for your time.
> I believe all my comments are valid and need to be addressed.
> Can you get somebody else(outside Nvidia) to ack your patch and counter
> my comments?
> I will merge it without review based on that Ack.
> Regards,
> Akhil
