[PATCH v2 02/22] lib: add pdcp protocol

[Anoob Joseph]

Hi Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil at marvell.com>
> Sent: Thursday, May 18, 2023 2:16 PM
> To: Anoob Joseph <anoobj at marvell.com>; Thomas Monjalon
> <thomas at monjalon.net>; Jerin Jacob Kollanukkaran <jerinj at marvell.com>;
> Konstantin Ananyev <konstantin.v.ananyev at yandex.ru>; Bernard
> Iremonger <bernard.iremonger at intel.com>
> Cc: Hemant Agrawal <hemant.agrawal at nxp.com>; Mattias Rönnblom
> <mattias.ronnblom at ericsson.com>; Kiran Kumar Kokkilagadda
> <kirankumark at marvell.com>; Volodymyr Fialko <vfialko at marvell.com>;
> dev at dpdk.org; Olivier Matz <olivier.matz at 6wind.com>
> Subject: RE: [PATCH v2 02/22] lib: add pdcp protocol
> 
> > > > > > diff --git a/lib/pdcp/rte_pdcp.c b/lib/pdcp/rte_pdcp.c new
> > > > > > file mode
> > > > > > 100644 index 0000000000..8914548dbd
> > > > > > --- /dev/null
> > > > > > +++ b/lib/pdcp/rte_pdcp.c
> > > > > > @@ -0,0 +1,138 @@
> > > > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > > > + * Copyright(C) 2023 Marvell.
> > > > > > + */
> > > > > > +
> > > > > > +#include <rte_errno.h>
> > > > > > +#include <rte_pdcp.h>
> > > > > > +#include <rte_malloc.h>
> > > > > > +
> > > > > > +#include "pdcp_crypto.h"
> > > > > > +#include "pdcp_entity.h"
> > > > > > +#include "pdcp_process.h"
> > > > > > +
> > > > > > +static int
> > > > > > +pdcp_entity_size_get(const struct rte_pdcp_entity_conf *conf) {
> > > > > > +	int size;
> > > > > > +
> > > > > > +	size = sizeof(struct rte_pdcp_entity) + sizeof(struct
> > > > > > +entity_priv);
> > > > > > +
> > > > > > +	if (conf->pdcp_xfrm.pkt_dir ==
> > RTE_SECURITY_PDCP_DOWNLINK)
> > > > > > +		size += sizeof(struct entity_priv_dl_part);
> > > > > > +	else if (conf->pdcp_xfrm.pkt_dir ==
> > RTE_SECURITY_PDCP_UPLINK)
> > > > > > +		size += sizeof(struct entity_priv_ul_part);
> > > > > > +	else
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	return RTE_ALIGN_CEIL(size, RTE_CACHE_LINE_SIZE); }
> > > > > > +
> > > > > > +struct rte_pdcp_entity *
> > > > > > +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf
> *conf) {
> > > > > > +	struct rte_pdcp_entity *entity = NULL;
> > > > > > +	struct entity_priv *en_priv;
> > > > > > +	int ret, entity_size;
> > > > > > +
> > > > > > +	if (conf == NULL || conf->cop_pool == NULL) {
> > > > > > +		rte_errno = -EINVAL;
> > > > > > +		return NULL;
> > > > > > +	}
> > > > >
> > > > > errnos are normally set as positive values.
> > > >
> > > > [Anoob] I do not think so. I checked rte_ethdev.h, rte_flow.h etc
> > > > and all APIs are returning negative values in case of errors.
> > >
> > > Check again lib/ethdev/rte_ethdev.c
> > > rte_errno are set as positive values only and APIs return error
> > > numbers as negative values.
> >
> > [Anoob] Okay. There are many APIs were this is not done correctly
> > (check
> > rte_flow.h) . For lib PDCP additions, I'll have this handled. Some of
> > these conventions should be documented to avoid confusion.
> 
> I am not sure what you are referring to.
> I cannot find any discrepancy in rte_flow.c as well.
> 
> Can you give an example where it is wrong. We can ask specific people to fix
> that.
> 
> Regarding documentation.
> It is mentioned in rte_errno.h
> /**
>  * Error number value, stored per-thread, which can be queried after
>  * calls to certain functions to determine why those functions failed.
>  *
>  * Uses standard values from errno.h wherever possible, with a small
> number
>  * of additional possible values for RTE-specific conditions.
>  */
> #define rte_errno RTE_PER_LCORE(_rte_errno)
> 
> And errno.h has all positive values defined.

[Anoob] Agreed. There are descriptions in rte_flow.h which got us confused. Thanks for the clarification. Changes in PDCP will come in next version.