Re: [dpdk-dev] [PATCH 1/8] cryptodev: separate out internal structures

[Akhil Goyal <gakhil@marvell.com>]

Hi Anoob,
Please see inline.

> >
> > +#include <rte_cryptodev_core.h>
> 
> [Anoob] Is this intentional? Shouldn't we keep includes together in the top of
> the file?
> 
It is intentional, and will be removed in a later patch, when the new framework is ready.
> > +/**
> > + *
> 
> [Anoob] Is there an extra blank line?
> 
> > + * Dequeue a burst of processed crypto operations from a queue on the
> > +crypto
> > + * device. The dequeued operation are stored in *rte_crypto_op*
> > +structures
> > + * whose pointers are supplied in the *ops* array.
> > + *
> > + * The rte_cryptodev_dequeue_burst() function returns the number of
> ops
> > + * actually dequeued, which is the number of *rte_crypto_op* data
> > +structures
> > + * effectively supplied into the *ops* array.
> > + *
> > + * A return value equal to *nb_ops* indicates that the queue contained
> > + * at least *nb_ops* operations, and this is likely to signify that
> > +other
> > + * processed operations remain in the devices output queue.
> > +Applications
> > + * implementing a "retrieve as many processed operations as possible"
> > +policy
> > + * can check this specific case and keep invoking the
> > + * rte_cryptodev_dequeue_burst() function until a value less than
> > + * *nb_ops* is returned.
> > + *
> > + * The rte_cryptodev_dequeue_burst() function does not provide any
> > +error
> > + * notification to avoid the corresponding overhead.
> > + *
> > + * @param	dev_id		The symmetric crypto device identifier
> 
> [Anoob] I do realize this is copied from existing code, but "symmetric" should
> not be there in the above string, right? The API is equally applicable for all
> cryptodev operations, right?
Agreed, I did not realize it was like this till now. This code will be removed
In a later patch of the series, but will check the final thing.

> 
> > +
> > +
> 
> [Anoob] Couple of extra blank lines can be removed?
This code is removed in a later patch.
> 
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/cryptodev/rte_cryptodev_core.h
> > b/lib/cryptodev/rte_cryptodev_core.h
> > new file mode 100644
> > index 0000000000..1633e55889
> > --- /dev/null
> > +++ b/lib/cryptodev/rte_cryptodev_core.h
> > @@ -0,0 +1,100 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2021 Marvell.
> 
> [Anoob] Since the code is mostly a copy from rte_cryptodev.h shouldn't we
> retain original licenses also?

This is an intermediate patch. At the end of this patchset, the new file will be 
A very small file with stuff only related to the new framework and will not have
Anything copied from other files, hence did not mention previous licenses.

> 
> > + */
> > +
> > +#ifndef _RTE_CRYPTODEV_CORE_H_
> > +#define _RTE_CRYPTODEV_CORE_H_
> 
> [Anoob] We are not including any of the dependent headers in this file. So
> the caller would be required to include all the dependent headers. Might be
> better to include atleast basic required ones (for rte_crypto_op etc).

This file is not required to be included directly as mentioned at the top of the file.
Hence no need to include other header files.
> 


> > +/** @internal The data structure associated with each crypto device. */
> > +struct rte_cryptodev {
> > +	dequeue_pkt_burst_t dequeue_burst;
> > +	/**< Pointer to PMD receive function. */
> > +	enqueue_pkt_burst_t enqueue_burst;
> > +	/**< Pointer to PMD transmit function. */
> 
> [Anoob] Transmit & receive are more ethdev specific terminology, right? Why
> not enqueue/dequeue itself?
This is kind of a legacy code, I just copied. We need not correct here, as this code
Will be removed in a later patch.