[PATCH v1 0/9] baseband/acc: VRB PMD fixes

[PATCH v1 0/9] baseband/acc: VRB PMD fixes

[Nicolas Chautru]

Hi, 

Series of mainly fixes for corner-cases and protection
in the VRB PMD.
The last 2 commits are not fixes but add support for
missing minor capability, as well as a request to remove printf
from code (function called from bbdev-test).

Thanks
Nic

Nicolas Chautru (9):
  baseband/acc: protection for TB negative scenario
  baseband/acc: add support for 4GUL with SO and TB
  baseband/acc: remove interrupt support on VRB1
  baseband/acc: add explicit mbuf apend for soft output
  baseband/acc: prevent to dequeue more than requested
  baseband/acc: fix iteration counter in TB mode
  baseband/acc: fix potential arithmetic overflow Fix potential issue of
    overflow causing coverity warning.
  baseband/acc: add support for 4GUL CRC drop in VRB PMD
  baseband/acc: remove printf from PMD function

 drivers/baseband/acc/rte_acc100_pmd.c |  18 ++---
 drivers/baseband/acc/rte_vrb_pmd.c    | 103 ++++++++++++++++++--------
 drivers/baseband/acc/vrb_pmd.h        |   1 +
 3 files changed, 84 insertions(+), 38 deletions(-)

-- 
2.34.1

[PATCH v1 1/9] baseband/acc: protection for TB negative scenario

[Nicolas Chautru]

Adding handling of negative scenario for malformed
Transport Block mode operations.

Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 34e42d1f6e..797330a5dd 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -1820,6 +1820,9 @@ enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op *op,
 	r = op->turbo_enc.tb_params.r;
 
 	while (mbuf_total_left > 0 && r < c) {
+		if (unlikely((input == NULL) || (output == NULL)))
+			return -1;
+
 		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
 		/* Set up DMA descriptor */
 		desc = acc_desc(q, total_enqueued_cbs);
@@ -1854,6 +1857,10 @@ enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op *op,
 		r++;
 	}
 
+	/* In case the number of CB doesn't match, the configuration was invalid. */
+	if (current_enqueued_cbs != cbs_in_tb)
+		return -1;
+
 	/* Set SDone on last CB descriptor for TB mode. */
 	desc->req.sdone_enable = 1;
 
@@ -2100,6 +2107,9 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 	}
 
 	while (mbuf_total_left > 0 && r < c) {
+		if (unlikely((input == NULL) || (h_output == NULL)))
+			return -1;
+
 		if (check_bit(op->ldpc_dec.op_flags, RTE_BBDEV_LDPC_DEC_SCATTER_GATHER))
 			seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
 		else
@@ -2145,6 +2155,10 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 		r++;
 	}
 
+	/* In case the number of CB doesn't match, the configuration was invalid. */
+	if (current_enqueued_cbs != cbs_in_tb)
+		return -1;
+
 #ifdef RTE_LIBRTE_BBDEV_DEBUG
 	if (check_mbuf_total_left(mbuf_total_left) != 0)
 		return -EINVAL;
@@ -2187,6 +2201,8 @@ enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 	r = op->turbo_dec.tb_params.r;
 
 	while (mbuf_total_left > 0 && r < c) {
+		if (unlikely((input == NULL) || (h_output == NULL)))
+			return -1;
 
 		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
 
@@ -2237,6 +2253,10 @@ enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 		r++;
 	}
 
+	/* In case the number of CB doesn't match, the configuration was invalid. */
+	if (current_enqueued_cbs != cbs_in_tb)
+		return -1;
+
 	/* Set SDone on last CB descriptor for TB mode */
 	desc->req.sdone_enable = 1;
 
-- 
2.34.1

Re: [PATCH v1 1/9] baseband/acc: protection for TB negative scenario

[Maxime Coquelin]

On 2/9/23 23:19, Nicolas Chautru wrote:
> Adding handling of negative scenario for malformed
> Transport Block mode operations.
> 
> Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index 34e42d1f6e..797330a5dd 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -1820,6 +1820,9 @@ enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op *op,
>   	r = op->turbo_enc.tb_params.r;
>   
>   	while (mbuf_total_left > 0 && r < c) {
> +		if (unlikely((input == NULL) || (output == NULL)))
> +			return -1;
> +
>   		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
>   		/* Set up DMA descriptor */
>   		desc = acc_desc(q, total_enqueued_cbs);
> @@ -1854,6 +1857,10 @@ enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op *op,
>   		r++;
>   	}
>   
> +	/* In case the number of CB doesn't match, the configuration was invalid. */
> +	if (current_enqueued_cbs != cbs_in_tb)

Maybe wrap it with unlikely()?

> +		return -1;
> +
>   	/* Set SDone on last CB descriptor for TB mode. */
>   	desc->req.sdone_enable = 1;
>   
> @@ -2100,6 +2107,9 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   	}
>   
>   	while (mbuf_total_left > 0 && r < c) {
> +		if (unlikely((input == NULL) || (h_output == NULL)))
> +			return -1;
> +
>   		if (check_bit(op->ldpc_dec.op_flags, RTE_BBDEV_LDPC_DEC_SCATTER_GATHER))
>   			seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
>   		else
> @@ -2145,6 +2155,10 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   		r++;
>   	}
>   
> +	/* In case the number of CB doesn't match, the configuration was invalid. */
> +	if (current_enqueued_cbs != cbs_in_tb)
> +		return -1;
> +
>   #ifdef RTE_LIBRTE_BBDEV_DEBUG
>   	if (check_mbuf_total_left(mbuf_total_left) != 0)
>   		return -EINVAL;
> @@ -2187,6 +2201,8 @@ enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   	r = op->turbo_dec.tb_params.r;
>   
>   	while (mbuf_total_left > 0 && r < c) {
> +		if (unlikely((input == NULL) || (h_output == NULL)))
> +			return -1;
>   
>   		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
>   
> @@ -2237,6 +2253,10 @@ enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   		r++;
>   	}
>   
> +	/* In case the number of CB doesn't match, the configuration was invalid. */
> +	if (current_enqueued_cbs != cbs_in_tb)
> +		return -1;
> +
>   	/* Set SDone on last CB descriptor for TB mode */
>   	desc->req.sdone_enable = 1;
>   

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

RE: [PATCH v1 1/9] baseband/acc: protection for TB negative scenario

[Chautru, Nicolas]

Hi Maxime, 
Thanks, will do in v2. 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, February 10, 2023 12:17 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org
> Cc: Vargas, Hernan <hernan.vargas@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v1 1/9] baseband/acc: protection for TB negative scenario
> 
> 
> 
> On 2/9/23 23:19, Nicolas Chautru wrote:
> > Adding handling of negative scenario for malformed Transport Block
> > mode operations.
> >
> > Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   drivers/baseband/acc/rte_vrb_pmd.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/baseband/acc/rte_vrb_pmd.c
> > b/drivers/baseband/acc/rte_vrb_pmd.c
> > index 34e42d1f6e..797330a5dd 100644
> > --- a/drivers/baseband/acc/rte_vrb_pmd.c
> > +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> > @@ -1820,6 +1820,9 @@ enqueue_enc_one_op_tb(struct acc_queue *q,
> struct rte_bbdev_enc_op *op,
> >   	r = op->turbo_enc.tb_params.r;
> >
> >   	while (mbuf_total_left > 0 && r < c) {
> > +		if (unlikely((input == NULL) || (output == NULL)))
> > +			return -1;
> > +
> >   		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
> >   		/* Set up DMA descriptor */
> >   		desc = acc_desc(q, total_enqueued_cbs); @@ -1854,6
> +1857,10 @@
> > enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op
> *op,
> >   		r++;
> >   	}
> >
> > +	/* In case the number of CB doesn't match, the configuration was
> invalid. */
> > +	if (current_enqueued_cbs != cbs_in_tb)
> 
> Maybe wrap it with unlikely()?
> 
> > +		return -1;
> > +
> >   	/* Set SDone on last CB descriptor for TB mode. */
> >   	desc->req.sdone_enable = 1;
> >
> > @@ -2100,6 +2107,9 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct
> acc_queue *q, struct rte_bbdev_dec_op *op,
> >   	}
> >
> >   	while (mbuf_total_left > 0 && r < c) {
> > +		if (unlikely((input == NULL) || (h_output == NULL)))
> > +			return -1;
> > +
> >   		if (check_bit(op->ldpc_dec.op_flags,
> RTE_BBDEV_LDPC_DEC_SCATTER_GATHER))
> >   			seg_total_left = rte_pktmbuf_data_len(input) -
> in_offset;
> >   		else
> > @@ -2145,6 +2155,10 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct
> acc_queue *q, struct rte_bbdev_dec_op *op,
> >   		r++;
> >   	}
> >
> > +	/* In case the number of CB doesn't match, the configuration was
> invalid. */
> > +	if (current_enqueued_cbs != cbs_in_tb)
> > +		return -1;
> > +
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	if (check_mbuf_total_left(mbuf_total_left) != 0)
> >   		return -EINVAL;
> > @@ -2187,6 +2201,8 @@ enqueue_dec_one_op_tb(struct acc_queue *q,
> struct rte_bbdev_dec_op *op,
> >   	r = op->turbo_dec.tb_params.r;
> >
> >   	while (mbuf_total_left > 0 && r < c) {
> > +		if (unlikely((input == NULL) || (h_output == NULL)))
> > +			return -1;
> >
> >   		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
> >
> > @@ -2237,6 +2253,10 @@ enqueue_dec_one_op_tb(struct acc_queue *q,
> struct rte_bbdev_dec_op *op,
> >   		r++;
> >   	}
> >
> > +	/* In case the number of CB doesn't match, the configuration was
> invalid. */
> > +	if (current_enqueued_cbs != cbs_in_tb)
> > +		return -1;
> > +
> >   	/* Set SDone on last CB descriptor for TB mode */
> >   	desc->req.sdone_enable = 1;
> >
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime

[PATCH v2 0/9] baseband/acc: VRB PMD fixes

[Nicolas Chautru]

Hi, 

v2: updated based on Maxime's review (commit messages and adding
unlikely statement). Thanks. Let me know if further
change is required. 

Series of mainly fixes for corner-cases and protection
in the VRB PMD.
The last 2 commits are not fixes but add support for
missing minor capability, as well as a request to remove printf
from code (function called from bbdev-test).

Thanks
Nic

Nicolas Chautru (9):
  baseband/acc: protection for TB negative scenario
  baseband/acc: remove interrupt support on VRB1
  baseband/acc: add explicit mbuf append for soft output
  baseband/acc: prevent to dequeue more than requested
  baseband/acc: fix iteration counter in TB mode
  baseband/acc: fix potential arithmetic overflow
  baseband/acc: add support for 4GUL CRC drop in VRB PMD
  baseband/acc: add support for 4GUL with SO and TB
  baseband/acc: remove printf from PMD function

 drivers/baseband/acc/rte_acc100_pmd.c |  18 ++---
 drivers/baseband/acc/rte_vrb_pmd.c    | 103 ++++++++++++++++++--------
 drivers/baseband/acc/vrb_pmd.h        |   1 +
 3 files changed, 84 insertions(+), 38 deletions(-)

-- 
2.34.1

[PATCH v2 1/9] baseband/acc: protection for TB negative scenario

[Nicolas Chautru]

Adding handling of negative scenario for malformed
Transport Block mode operations.

Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 34e42d1f6e..3afaea71a3 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -1820,6 +1820,9 @@ enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op *op,
 	r = op->turbo_enc.tb_params.r;
 
 	while (mbuf_total_left > 0 && r < c) {
+		if (unlikely((input == NULL) || (output == NULL)))
+			return -1;
+
 		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
 		/* Set up DMA descriptor */
 		desc = acc_desc(q, total_enqueued_cbs);
@@ -1854,6 +1857,10 @@ enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op *op,
 		r++;
 	}
 
+	/* In case the number of CB doesn't match, the configuration was invalid. */
+	if (unlikely(current_enqueued_cbs != cbs_in_tb))
+		return -1;
+
 	/* Set SDone on last CB descriptor for TB mode. */
 	desc->req.sdone_enable = 1;
 
@@ -2100,6 +2107,9 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 	}
 
 	while (mbuf_total_left > 0 && r < c) {
+		if (unlikely((input == NULL) || (h_output == NULL)))
+			return -1;
+
 		if (check_bit(op->ldpc_dec.op_flags, RTE_BBDEV_LDPC_DEC_SCATTER_GATHER))
 			seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
 		else
@@ -2145,6 +2155,10 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 		r++;
 	}
 
+	/* In case the number of CB doesn't match, the configuration was invalid. */
+	if (unlikely(current_enqueued_cbs != cbs_in_tb))
+		return -1;
+
 #ifdef RTE_LIBRTE_BBDEV_DEBUG
 	if (check_mbuf_total_left(mbuf_total_left) != 0)
 		return -EINVAL;
@@ -2187,6 +2201,8 @@ enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 	r = op->turbo_dec.tb_params.r;
 
 	while (mbuf_total_left > 0 && r < c) {
+		if (unlikely((input == NULL) || (h_output == NULL)))
+			return -1;
 
 		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
 
@@ -2237,6 +2253,10 @@ enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 		r++;
 	}
 
+	/* In case the number of CB doesn't match, the configuration was invalid. */
+	if (unlikely(current_enqueued_cbs != cbs_in_tb))
+		return -1;
+
 	/* Set SDone on last CB descriptor for TB mode */
 	desc->req.sdone_enable = 1;
 
-- 
2.34.1

Re: [PATCH v2 1/9] baseband/acc: protection for TB negative scenario

[Maxime Coquelin]

On 2/10/23 18:58, Nicolas Chautru wrote:
> Adding handling of negative scenario for malformed
> Transport Block mode operations.
> 
> Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")

The format is invalid, the sha-1 should be 12B long, checkpatch
complains about it:

WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12 chars 
of sha1> ("<title line>")' - ie: 'Fixes: bec597b78a0e ("baseband/acc200: 
add LTE processing")'
#9:
Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")


I will fix it here and in other patches.
To avoid such issues, you can add an alias in your git global config
file (e.g. ~/.gitconfig):

[alias]
	fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'

With this, to generate a Fixes tag, you just have to do:

$ git fixline bec597b78a0e347f7a82a0d51e4a9fc61dea0a16
Fixes: bec597b78a0e ("baseband/acc200: add LTE processing")
Cc: nicolas.chautru@intel.com

Hernan, this issue is also present in "baseband/acc: fix check after
deref and dead code", I will also fix it while applying, but please
consider adding the git alias.

Thanks,
Maxime

> Cc: stable@dpdk.org
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index 34e42d1f6e..3afaea71a3 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -1820,6 +1820,9 @@ enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op *op,
>   	r = op->turbo_enc.tb_params.r;
>   
>   	while (mbuf_total_left > 0 && r < c) {
> +		if (unlikely((input == NULL) || (output == NULL)))
> +			return -1;
> +
>   		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
>   		/* Set up DMA descriptor */
>   		desc = acc_desc(q, total_enqueued_cbs);
> @@ -1854,6 +1857,10 @@ enqueue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op *op,
>   		r++;
>   	}
>   
> +	/* In case the number of CB doesn't match, the configuration was invalid. */
> +	if (unlikely(current_enqueued_cbs != cbs_in_tb))
> +		return -1;
> +
>   	/* Set SDone on last CB descriptor for TB mode. */
>   	desc->req.sdone_enable = 1;
>   
> @@ -2100,6 +2107,9 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   	}
>   
>   	while (mbuf_total_left > 0 && r < c) {
> +		if (unlikely((input == NULL) || (h_output == NULL)))
> +			return -1;
> +
>   		if (check_bit(op->ldpc_dec.op_flags, RTE_BBDEV_LDPC_DEC_SCATTER_GATHER))
>   			seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
>   		else
> @@ -2145,6 +2155,10 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   		r++;
>   	}
>   
> +	/* In case the number of CB doesn't match, the configuration was invalid. */
> +	if (unlikely(current_enqueued_cbs != cbs_in_tb))
> +		return -1;
> +
>   #ifdef RTE_LIBRTE_BBDEV_DEBUG
>   	if (check_mbuf_total_left(mbuf_total_left) != 0)
>   		return -EINVAL;
> @@ -2187,6 +2201,8 @@ enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   	r = op->turbo_dec.tb_params.r;
>   
>   	while (mbuf_total_left > 0 && r < c) {
> +		if (unlikely((input == NULL) || (h_output == NULL)))
> +			return -1;
>   
>   		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
>   
> @@ -2237,6 +2253,10 @@ enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   		r++;
>   	}
>   
> +	/* In case the number of CB doesn't match, the configuration was invalid. */
> +	if (unlikely(current_enqueued_cbs != cbs_in_tb))
> +		return -1;
> +
>   	/* Set SDone on last CB descriptor for TB mode */
>   	desc->req.sdone_enable = 1;
>   

RE: [PATCH v2 1/9] baseband/acc: protection for TB negative scenario

[Chautru, Nicolas]

Thanks Maxime.

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, February 23, 2023 12:57 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Vargas, Hernan
> <hernan.vargas@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [PATCH v2 1/9] baseband/acc: protection for TB negative scenario
> 
> 
> 
> On 2/10/23 18:58, Nicolas Chautru wrote:
> > Adding handling of negative scenario for malformed Transport Block
> > mode operations.
> >
> > Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
> 
> The format is invalid, the sha-1 should be 12B long, checkpatch complains about
> it:
> 
> WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12 chars of
> sha1> ("<title line>")' - ie: 'Fixes: bec597b78a0e ("baseband/acc200:
> add LTE processing")'
> #9:
> Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
> 
> 
> I will fix it here and in other patches.
> To avoid such issues, you can add an alias in your git global config
> file (e.g. ~/.gitconfig):
> 
> [alias]
> 	fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'
> 
> With this, to generate a Fixes tag, you just have to do:
> 
> $ git fixline bec597b78a0e347f7a82a0d51e4a9fc61dea0a16
> Fixes: bec597b78a0e ("baseband/acc200: add LTE processing")
> Cc: nicolas.chautru@intel.com
> 
> Hernan, this issue is also present in "baseband/acc: fix check after
> deref and dead code", I will also fix it while applying, but please
> consider adding the git alias.
> 
> Thanks,
> Maxime
> 
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> >   drivers/baseband/acc/rte_vrb_pmd.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/baseband/acc/rte_vrb_pmd.c
> b/drivers/baseband/acc/rte_vrb_pmd.c
> > index 34e42d1f6e..3afaea71a3 100644
> > --- a/drivers/baseband/acc/rte_vrb_pmd.c
> > +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> > @@ -1820,6 +1820,9 @@ enqueue_enc_one_op_tb(struct acc_queue *q,
> struct rte_bbdev_enc_op *op,
> >   	r = op->turbo_enc.tb_params.r;
> >
> >   	while (mbuf_total_left > 0 && r < c) {
> > +		if (unlikely((input == NULL) || (output == NULL)))
> > +			return -1;
> > +
> >   		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
> >   		/* Set up DMA descriptor */
> >   		desc = acc_desc(q, total_enqueued_cbs);
> > @@ -1854,6 +1857,10 @@ enqueue_enc_one_op_tb(struct acc_queue *q,
> struct rte_bbdev_enc_op *op,
> >   		r++;
> >   	}
> >
> > +	/* In case the number of CB doesn't match, the configuration was
> invalid. */
> > +	if (unlikely(current_enqueued_cbs != cbs_in_tb))
> > +		return -1;
> > +
> >   	/* Set SDone on last CB descriptor for TB mode. */
> >   	desc->req.sdone_enable = 1;
> >
> > @@ -2100,6 +2107,9 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct
> acc_queue *q, struct rte_bbdev_dec_op *op,
> >   	}
> >
> >   	while (mbuf_total_left > 0 && r < c) {
> > +		if (unlikely((input == NULL) || (h_output == NULL)))
> > +			return -1;
> > +
> >   		if (check_bit(op->ldpc_dec.op_flags,
> RTE_BBDEV_LDPC_DEC_SCATTER_GATHER))
> >   			seg_total_left = rte_pktmbuf_data_len(input) -
> in_offset;
> >   		else
> > @@ -2145,6 +2155,10 @@ vrb_enqueue_ldpc_dec_one_op_tb(struct
> acc_queue *q, struct rte_bbdev_dec_op *op,
> >   		r++;
> >   	}
> >
> > +	/* In case the number of CB doesn't match, the configuration was
> invalid. */
> > +	if (unlikely(current_enqueued_cbs != cbs_in_tb))
> > +		return -1;
> > +
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	if (check_mbuf_total_left(mbuf_total_left) != 0)
> >   		return -EINVAL;
> > @@ -2187,6 +2201,8 @@ enqueue_dec_one_op_tb(struct acc_queue *q,
> struct rte_bbdev_dec_op *op,
> >   	r = op->turbo_dec.tb_params.r;
> >
> >   	while (mbuf_total_left > 0 && r < c) {
> > +		if (unlikely((input == NULL) || (h_output == NULL)))
> > +			return -1;
> >
> >   		seg_total_left = rte_pktmbuf_data_len(input) - in_offset;
> >
> > @@ -2237,6 +2253,10 @@ enqueue_dec_one_op_tb(struct acc_queue *q,
> struct rte_bbdev_dec_op *op,
> >   		r++;
> >   	}
> >
> > +	/* In case the number of CB doesn't match, the configuration was
> invalid. */
> > +	if (unlikely(current_enqueued_cbs != cbs_in_tb))
> > +		return -1;
> > +
> >   	/* Set SDone on last CB descriptor for TB mode */
> >   	desc->req.sdone_enable = 1;
> >

[PATCH v2 2/9] baseband/acc: remove interrupt support on VRB1

[Nicolas Chautru]

Not enabling interrupt in VRB1 PMD variant to avoid
potential corner case.

Fixes: 3cabc8eaf524 ("baseband/acc200: support interrupt")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 3afaea71a3..fc68f47ca6 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -586,6 +586,14 @@ vrb_intr_enable(struct rte_bbdev *dev)
 {
 	int ret;
 	struct acc_device *d = dev->data->dev_private;
+
+	if (d->device_variant == VRB1_VARIANT) {
+		/* On VRB1: cannot enable MSI/IR to avoid potential back-pressure corner case. */
+		rte_bbdev_log(ERR, "VRB1 (%s) doesn't support any MSI/MSI-X interrupt\n",
+				dev->data->name);
+		return -ENOTSUP;
+	}
+
 	/*
 	 * MSI/MSI-X are supported.
 	 * Option controlled by vfio-intr through EAL parameter.
-- 
2.34.1

[PATCH v2 3/9] baseband/acc: add explicit mbuf append for soft output

[Nicolas Chautru]

Adding an explicit mbuf append in the case of soft output
mbuf being provided.

Fixes: e640f6cdfa84 ("baseband/acc200: add LDPC processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index fc68f47ca6..b1134f244d 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -2066,6 +2066,10 @@ vrb_enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 		}
 	}
 
+	if (op->ldpc_dec.soft_output.length > 0)
+		mbuf_append(op->ldpc_dec.soft_output.data, op->ldpc_dec.soft_output.data,
+				op->ldpc_dec.soft_output.length);
+
 #ifdef RTE_LIBRTE_BBDEV_DEBUG
 	rte_memdump(stderr, "FCW", &desc->req.fcw_ld,
 			sizeof(desc->req.fcw_ld) - 8);
-- 
2.34.1

Re: [PATCH v2 3/9] baseband/acc: add explicit mbuf append for soft output

[Maxime Coquelin]

On 2/10/23 18:58, Nicolas Chautru wrote:
> Adding an explicit mbuf append in the case of soft output
> mbuf being provided.
> 
> Fixes: e640f6cdfa84 ("baseband/acc200: add LDPC processing")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I don't remember having put my R-by on this patch.

As I suggested to Hernan, one way to avoid such mistakes is to start the
next revision by applying the patchwork version. Just get the series
link from patchwork and apply it.

E.g. for this specific case:

$ curl http://patches.dpdk.org/series/26933/mbox/ | git am -3

There may be other ways, but that's how I do it.

The patch looks good to me, based on your explanation on V1, so don't
remove it in next revision (if any).

Maxime
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index fc68f47ca6..b1134f244d 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -2066,6 +2066,10 @@ vrb_enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   		}
>   	}
>   
> +	if (op->ldpc_dec.soft_output.length > 0)
> +		mbuf_append(op->ldpc_dec.soft_output.data, op->ldpc_dec.soft_output.data,
> +				op->ldpc_dec.soft_output.length);
> +
>   #ifdef RTE_LIBRTE_BBDEV_DEBUG
>   	rte_memdump(stderr, "FCW", &desc->req.fcw_ld,
>   			sizeof(desc->req.fcw_ld) - 8);

[PATCH v2 4/9] baseband/acc: prevent to dequeue more than requested

[Nicolas Chautru]

Add support for corner-case when more operations are
requested than expected, in the case of encoder muxing
operations.

Fixes: e640f6cdfa84 ("baseband/acc200: add LDPC processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index b1134f244d..a7d0b1e33c 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -2640,7 +2640,8 @@ vrb_enqueue_ldpc_dec(struct rte_bbdev_queue_data *q_data,
 /* Dequeue one encode operations from device in CB mode. */
 static inline int
 vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
-		uint16_t *dequeued_ops, uint32_t *aq_dequeued, uint16_t *dequeued_descs)
+		uint16_t *dequeued_ops, uint32_t *aq_dequeued, uint16_t *dequeued_descs,
+		uint16_t max_requested_ops)
 {
 	union acc_dma_desc *desc, atom_desc;
 	union acc_dma_rsp_desc rsp;
@@ -2653,6 +2654,9 @@ vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
 	desc = q->ring_addr + desc_idx;
 	atom_desc.atom_hdr = __atomic_load_n((uint64_t *)desc, __ATOMIC_RELAXED);
 
+	if (*dequeued_ops + desc->req.numCBs > max_requested_ops)
+		return -1;
+
 	/* Check fdone bit. */
 	if (!(atom_desc.rsp.val & ACC_FDONE))
 		return -1;
@@ -2694,7 +2698,7 @@ vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
 static inline int
 vrb_dequeue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
 		uint16_t *dequeued_ops, uint32_t *aq_dequeued,
-		uint16_t *dequeued_descs)
+		uint16_t *dequeued_descs, uint16_t max_requested_ops)
 {
 	union acc_dma_desc *desc, *last_desc, atom_desc;
 	union acc_dma_rsp_desc rsp;
@@ -2705,6 +2709,9 @@ vrb_dequeue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
 	desc = acc_desc_tail(q, *dequeued_descs);
 	atom_desc.atom_hdr = __atomic_load_n((uint64_t *)desc, __ATOMIC_RELAXED);
 
+	if (*dequeued_ops + 1 > max_requested_ops)
+		return -1;
+
 	/* Check fdone bit. */
 	if (!(atom_desc.rsp.val & ACC_FDONE))
 		return -1;
@@ -2965,25 +2972,23 @@ vrb_dequeue_enc(struct rte_bbdev_queue_data *q_data,
 
 	cbm = op->turbo_enc.code_block_mode;
 
-	for (i = 0; i < num; i++) {
+	for (i = 0; i < avail; i++) {
 		if (cbm == RTE_BBDEV_TRANSPORT_BLOCK)
 			ret = vrb_dequeue_enc_one_op_tb(q, &ops[dequeued_ops],
 					&dequeued_ops, &aq_dequeued,
-					&dequeued_descs);
+					&dequeued_descs, num);
 		else
 			ret = vrb_dequeue_enc_one_op_cb(q, &ops[dequeued_ops],
 					&dequeued_ops, &aq_dequeued,
-					&dequeued_descs);
+					&dequeued_descs, num);
 		if (ret < 0)
 			break;
-		if (dequeued_ops >= num)
-			break;
 	}
 
 	q->aq_dequeued += aq_dequeued;
 	q->sw_ring_tail += dequeued_descs;
 
-	/* Update enqueue stats */
+	/* Update enqueue stats. */
 	q_data->queue_stats.dequeued_count += dequeued_ops;
 
 	return dequeued_ops;
@@ -3009,15 +3014,13 @@ vrb_dequeue_ldpc_enc(struct rte_bbdev_queue_data *q_data,
 		if (cbm == RTE_BBDEV_TRANSPORT_BLOCK)
 			ret = vrb_dequeue_enc_one_op_tb(q, &ops[dequeued_ops],
 					&dequeued_ops, &aq_dequeued,
-					&dequeued_descs);
+					&dequeued_descs, num);
 		else
 			ret = vrb_dequeue_enc_one_op_cb(q, &ops[dequeued_ops],
 					&dequeued_ops, &aq_dequeued,
-					&dequeued_descs);
+					&dequeued_descs, num);
 		if (ret < 0)
 			break;
-		if (dequeued_ops >= num)
-			break;
 	}
 
 	q->aq_dequeued += aq_dequeued;
-- 
2.34.1

[PATCH v2 5/9] baseband/acc: fix iteration counter in TB mode

[Nicolas Chautru]

The iteration count was not using correct structure (4G vs 5G)
in TB mode.

Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index a7d0b1e33c..ccd3adaf93 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -2875,7 +2875,7 @@ vrb_dequeue_ldpc_dec_one_op_cb(struct rte_bbdev_queue_data *q_data,
 	return 1;
 }
 
-/* Dequeue one decode operations from device in TB mode. */
+/* Dequeue one decode operations from device in TB mode for 4G or 5G. */
 static inline int
 vrb_dequeue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op **ref_op,
 		uint16_t dequeued_cbs, uint32_t *aq_dequeued)
@@ -2929,8 +2929,12 @@ vrb_dequeue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op **ref_op,
 		/* CRC invalid if error exists. */
 		if (!op->status)
 			op->status |= rsp.crc_status << RTE_BBDEV_CRC_ERROR;
-		op->turbo_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
-				op->turbo_dec.iter_count);
+		if (q->op_type == RTE_BBDEV_OP_LDPC_DEC)
+			op->ldpc_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
+					op->ldpc_dec.iter_count);
+		else
+			op->turbo_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
+					op->turbo_dec.iter_count);
 
 		/* Check if this is the last desc in batch (Atomic Queue). */
 		if (desc->req.last_desc_in_batch) {
-- 
2.34.1

[PATCH v2 6/9] baseband/acc: fix potential arithmetic overflow

[Nicolas Chautru]

Fix potential issue of overflow causing coverity warning.

Coverity issue: 383154
Fixes: 8e16839937 ("baseband/acc: extension of the device structure")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 2 +-
 drivers/baseband/acc/vrb_pmd.h     | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index ccd3adaf93..82dbfcaa53 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -518,7 +518,7 @@ vrb_setup_queues(struct rte_bbdev *dev, uint16_t num_queues, int socket_id)
 	if (d->tail_ptrs == NULL)
 		d->tail_ptrs = rte_zmalloc_socket(
 				dev->device->driver->name,
-				d->num_qgroups * d->num_aqs * sizeof(uint32_t),
+				VRB_MAX_QGRPS * VRB_MAX_AQS * sizeof(uint32_t),
 				RTE_CACHE_LINE_SIZE, socket_id);
 	if (d->tail_ptrs == NULL) {
 		rte_bbdev_log(ERR, "Failed to allocate tail ptr for %s:%u",
diff --git a/drivers/baseband/acc/vrb_pmd.h b/drivers/baseband/acc/vrb_pmd.h
index f3c9239881..01028273e7 100644
--- a/drivers/baseband/acc/vrb_pmd.h
+++ b/drivers/baseband/acc/vrb_pmd.h
@@ -36,6 +36,7 @@
 
 #define VRB_NUM_ACCS                 6
 #define VRB_MAX_QGRPS                32
+#define VRB_MAX_AQS                  32
 
 #define ACC_STATUS_WAIT      10
 #define ACC_STATUS_TO        100
-- 
2.34.1

[PATCH v2 7/9] baseband/acc: add support for 4GUL CRC drop in VRB PMD

[Nicolas Chautru]

Adding support for the capability to drop the CRC24B
when using the turbo-decoder.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 82dbfcaa53..5d385ce1a5 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -996,6 +996,7 @@ vrb_dev_info_get(struct rte_bbdev *dev, struct rte_bbdev_driver_info *dev_info)
 				.capability_flags =
 					RTE_BBDEV_TURBO_SUBBLOCK_DEINTERLEAVE |
 					RTE_BBDEV_TURBO_CRC_TYPE_24B |
+					RTE_BBDEV_TURBO_DEC_CRC_24B_DROP |
 					RTE_BBDEV_TURBO_EQUALIZER |
 					RTE_BBDEV_TURBO_SOFT_OUT_SATURATE |
 					RTE_BBDEV_TURBO_HALF_ITERATION_EVEN |
@@ -1407,8 +1408,12 @@ vrb_dma_desc_td_fill(struct rte_bbdev_dec_op *op,
 	}
 
 	if ((op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK)
-		&& !check_bit(op->turbo_dec.op_flags,
-		RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP))
+			&& !check_bit(op->turbo_dec.op_flags,
+			RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP))
+		crc24_overlap = 24;
+	if ((op->turbo_dec.code_block_mode == RTE_BBDEV_CODE_BLOCK)
+			&& check_bit(op->turbo_dec.op_flags,
+			RTE_BBDEV_TURBO_DEC_CRC_24B_DROP))
 		crc24_overlap = 24;
 
 	/* Calculates circular buffer size.
-- 
2.34.1

[PATCH v2 8/9] baseband/acc: add support for 4GUL with SO and TB

[Nicolas Chautru]

Implementation to support the case when using LTE
decoder with soft output and transport block mode.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 5d385ce1a5..236f21dca3 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -1199,15 +1199,12 @@ vrb_fcw_td_fill(const struct rte_bbdev_dec_op *op, struct acc_fcw_td *fcw)
 	fcw->bypass_sb_deint = !check_bit(op->turbo_dec.op_flags,
 			RTE_BBDEV_TURBO_SUBBLOCK_DEINTERLEAVE);
 	if (op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK) {
-		/* FIXME for TB block */
+		fcw->c = op->turbo_dec.tb_params.c;
 		fcw->k_pos = op->turbo_dec.tb_params.k_pos;
-		fcw->k_neg = op->turbo_dec.tb_params.k_neg;
 	} else {
+		fcw->c = 1;
 		fcw->k_pos = op->turbo_dec.cb_params.k;
-		fcw->k_neg = op->turbo_dec.cb_params.k;
 	}
-	fcw->c = 1;
-	fcw->c_neg = 1;
 	if (check_bit(op->turbo_dec.op_flags, RTE_BBDEV_TURBO_SOFT_OUTPUT)) {
 		fcw->soft_output_en = 1;
 		fcw->sw_soft_out_dis = 0;
@@ -1218,8 +1215,14 @@ vrb_fcw_td_fill(const struct rte_bbdev_dec_op *op, struct acc_fcw_td *fcw)
 		if (check_bit(op->turbo_dec.op_flags,
 				RTE_BBDEV_TURBO_EQUALIZER)) {
 			fcw->bypass_teq = 0;
-			fcw->ea = op->turbo_dec.cb_params.e;
-			fcw->eb = op->turbo_dec.cb_params.e;
+			if (op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK) {
+				fcw->cab = op->turbo_dec.tb_params.cab;
+				fcw->ea = op->turbo_dec.tb_params.ea;
+				fcw->eb = op->turbo_dec.tb_params.eb;
+			} else {
+				fcw->ea = op->turbo_dec.cb_params.e;
+				fcw->eb = op->turbo_dec.cb_params.e;
+			}
 			if (op->turbo_dec.rv_index == 0)
 				fcw->k0_start_col = ACC_FCW_TD_RVIDX_0;
 			else if (op->turbo_dec.rv_index == 1)
@@ -1396,9 +1399,7 @@ vrb_dma_desc_td_fill(struct rte_bbdev_dec_op *op,
 	desc->numCBs = 1;
 
 	if (op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK) {
-		k = (r < op->turbo_dec.tb_params.c_neg)
-			? op->turbo_dec.tb_params.k_neg
-			: op->turbo_dec.tb_params.k_pos;
+		k = op->turbo_dec.tb_params.k_pos;
 		e = (r < op->turbo_dec.tb_params.cab)
 			? op->turbo_dec.tb_params.ea
 			: op->turbo_dec.tb_params.eb;
-- 
2.34.1

[PATCH v2 9/9] baseband/acc: remove printf from PMD function

[Nicolas Chautru]

Replacing usage of printf in companion function for
bbdev-test by rte_log.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/baseband/acc/rte_acc100_pmd.c | 18 +++++++++---------
 drivers/baseband/acc/rte_vrb_pmd.c    |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/baseband/acc/rte_acc100_pmd.c b/drivers/baseband/acc/rte_acc100_pmd.c
index 36f6fec5ad..40b5eaf089 100644
--- a/drivers/baseband/acc/rte_acc100_pmd.c
+++ b/drivers/baseband/acc/rte_acc100_pmd.c
@@ -4347,7 +4347,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
 {
 	int i, template_idx, qg_idx;
 	uint32_t address, status, value;
-	printf("Need to clear power-on 5GUL status in internal memory\n");
+	rte_bbdev_log(WARNING, "Need to clear power-on 5GUL status in internal memory");
 	/* Reset LDPC Cores */
 	for (i = 0; i < ACC100_ENGINES_MAX; i++)
 		acc_reg_write(d, HWPfFecUl5gCntrlReg +
@@ -4421,7 +4421,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
 	/* Force each engine which is in unspecified state */
 	for (i = 0; i < num_failed_engine; i++) {
 		int failed_engine = engines_to_restart[i];
-		printf("Force engine %d\n", failed_engine);
+		rte_bbdev_log(WARNING, "Force engine %d", failed_engine);
 		for (template_idx = ACC100_SIG_UL_5G;
 				template_idx <= ACC100_SIG_UL_5G_LAST;
 				template_idx++) {
@@ -4450,7 +4450,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
 		acc_reg_write(d, HWPfQmgrIngressAq + 0x100, enq_req.val);
 		usleep(ACC_LONG_WAIT * 100);
 		if (desc->req.word0 != 2)
-			printf("DMA Response %#"PRIx32"\n", desc->req.word0);
+			rte_bbdev_log(WARNING, "DMA Response %#"PRIx32"\n", desc->req.word0);
 	}
 
 	/* Reset LDPC Cores */
@@ -4482,7 +4482,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
 		} else
 			acc_reg_write(d, address, 0);
 	}
-	printf("Number of 5GUL engines %d\n", numEngines);
+	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
 
 	rte_free(d->sw_rings_base);
 	usleep(ACC_LONG_WAIT);
@@ -4671,7 +4671,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
 		} else
 			acc_reg_write(d, address, 0);
 	}
-	printf("Number of 5GUL engines %d\n", numEngines);
+	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
 	/* 4GDL */
 	numQqsAcc += numQgs;
 	numQgs	= conf->q_dl_4g.num_qgroups;
@@ -4801,7 +4801,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
 		version += acc_reg_read(d,
 				HWPfDdrPhyIdtmFwVersion + 4 * i) << (8 * i);
 	if (version != ACC100_PRQ_DDR_VER) {
-		printf("* Note: Not on DDR PRQ version %8x != %08x\n",
+		rte_bbdev_log(ERR, "* Note: Not on DDR PRQ version %8x != %08x",
 				version, ACC100_PRQ_DDR_VER);
 	} else if (firstCfg) {
 		/* ---- DDR configuration at boot up --- */
@@ -4871,7 +4871,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
 			if (value & 1)
 				break;
 		}
-		printf("DDR Training completed in %d ms", i);
+		rte_bbdev_log(INFO, "DDR Training completed in %d ms", i);
 		/* Enable Memory Controller */
 		acc_reg_write(d, HWPfDdrUmmcCtrl, 0x401);
 		/* Release AXI interface reset */
@@ -5047,7 +5047,7 @@ acc101_configure(const char *dev_name, struct rte_acc_conf *conf)
 		} else
 			acc_reg_write(d, address, 0);
 	}
-	printf("Number of 5GUL engines %d\n", numEngines);
+	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
 	/* 4GDL */
 	numQqsAcc += numQgs;
 	numQgs	= conf->q_dl_4g.num_qgroups;
@@ -5185,7 +5185,7 @@ rte_acc_configure(const char *dev_name, struct rte_acc_conf *conf)
 		return -ENODEV;
 	}
 	struct rte_pci_device *pci_dev = RTE_DEV_TO_PCI(bbdev->device);
-	printf("Configure dev id %x\n", pci_dev->id.device_id);
+	rte_bbdev_log(INFO, "Configure dev id %x", pci_dev->id.device_id);
 	if (pci_dev->id.device_id == ACC100_PF_DEVICE_ID)
 		return acc100_configure(dev_name, conf);
 	else if (pci_dev->id.device_id == ACC101_PF_DEVICE_ID)
diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 236f21dca3..9e5a73c9c7 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -3633,7 +3633,7 @@ vrb1_configure(const char *dev_name, struct rte_acc_conf *conf)
 		} else
 			acc_reg_write(d, address, 0);
 	}
-	printf("Number of 5GUL engines %d\n", numEngines);
+	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
 	/* 4GDL */
 	numQqsAcc += numQgs;
 	numQgs	= conf->q_dl_4g.num_qgroups;
-- 
2.34.1

Re: [PATCH v2 0/9] baseband/acc: VRB PMD fixes

[Maxime Coquelin]

On 2/10/23 18:58, Nicolas Chautru wrote:
> Hi,
> 
> v2: updated based on Maxime's review (commit messages and adding
> unlikely statement). Thanks. Let me know if further
> change is required.
> 
> Series of mainly fixes for corner-cases and protection
> in the VRB PMD.
> The last 2 commits are not fixes but add support for
> missing minor capability, as well as a request to remove printf
> from code (function called from bbdev-test).
> 
> Thanks
> Nic
> 
> Nicolas Chautru (9):
>    baseband/acc: protection for TB negative scenario
>    baseband/acc: remove interrupt support on VRB1
>    baseband/acc: add explicit mbuf append for soft output
>    baseband/acc: prevent to dequeue more than requested
>    baseband/acc: fix iteration counter in TB mode
>    baseband/acc: fix potential arithmetic overflow
>    baseband/acc: add support for 4GUL CRC drop in VRB PMD
>    baseband/acc: add support for 4GUL with SO and TB
>    baseband/acc: remove printf from PMD function
> 
>   drivers/baseband/acc/rte_acc100_pmd.c |  18 ++---
>   drivers/baseband/acc/rte_vrb_pmd.c    | 103 ++++++++++++++++++--------
>   drivers/baseband/acc/vrb_pmd.h        |   1 +
>   3 files changed, 84 insertions(+), 38 deletions(-)
> 

Applied to dpdk-next-baseband/for-main.

Thanks,
Maxime
11

[PATCH v1 2/9] baseband/acc: add support for 4GUL with SO and TB

[Nicolas Chautru]

Implementation to support the case when using LTE
decoder with soft output and transport block mode.

Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 797330a5dd..c43c08afe8 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -1190,15 +1190,12 @@ vrb_fcw_td_fill(const struct rte_bbdev_dec_op *op, struct acc_fcw_td *fcw)
 	fcw->bypass_sb_deint = !check_bit(op->turbo_dec.op_flags,
 			RTE_BBDEV_TURBO_SUBBLOCK_DEINTERLEAVE);
 	if (op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK) {
-		/* FIXME for TB block */
+		fcw->c = op->turbo_dec.tb_params.c;
 		fcw->k_pos = op->turbo_dec.tb_params.k_pos;
-		fcw->k_neg = op->turbo_dec.tb_params.k_neg;
 	} else {
+		fcw->c = 1;
 		fcw->k_pos = op->turbo_dec.cb_params.k;
-		fcw->k_neg = op->turbo_dec.cb_params.k;
 	}
-	fcw->c = 1;
-	fcw->c_neg = 1;
 	if (check_bit(op->turbo_dec.op_flags, RTE_BBDEV_TURBO_SOFT_OUTPUT)) {
 		fcw->soft_output_en = 1;
 		fcw->sw_soft_out_dis = 0;
@@ -1209,8 +1206,14 @@ vrb_fcw_td_fill(const struct rte_bbdev_dec_op *op, struct acc_fcw_td *fcw)
 		if (check_bit(op->turbo_dec.op_flags,
 				RTE_BBDEV_TURBO_EQUALIZER)) {
 			fcw->bypass_teq = 0;
-			fcw->ea = op->turbo_dec.cb_params.e;
-			fcw->eb = op->turbo_dec.cb_params.e;
+			if (op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK) {
+				fcw->cab = op->turbo_dec.tb_params.cab;
+				fcw->ea = op->turbo_dec.tb_params.ea;
+				fcw->eb = op->turbo_dec.tb_params.eb;
+			} else {
+				fcw->ea = op->turbo_dec.cb_params.e;
+				fcw->eb = op->turbo_dec.cb_params.e;
+			}
 			if (op->turbo_dec.rv_index == 0)
 				fcw->k0_start_col = ACC_FCW_TD_RVIDX_0;
 			else if (op->turbo_dec.rv_index == 1)
@@ -1387,9 +1390,7 @@ vrb_dma_desc_td_fill(struct rte_bbdev_dec_op *op,
 	desc->numCBs = 1;
 
 	if (op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK) {
-		k = (r < op->turbo_dec.tb_params.c_neg)
-			? op->turbo_dec.tb_params.k_neg
-			: op->turbo_dec.tb_params.k_pos;
+		k = op->turbo_dec.tb_params.k_pos;
 		e = (r < op->turbo_dec.tb_params.cab)
 			? op->turbo_dec.tb_params.ea
 			: op->turbo_dec.tb_params.eb;
-- 
2.34.1

Re: [PATCH v1 2/9] baseband/acc: add support for 4GUL with SO and TB

[Maxime Coquelin]

On 2/9/23 23:19, Nicolas Chautru wrote:
> Implementation to support the case when using LTE
> decoder with soft output and transport block mode.
> 
> Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
> Cc: stable@dpdk.org

Should it really be a fix and be backported?
It looks like a new feature.

> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
> 

Thanks,
Maxime

RE: [PATCH v1 2/9] baseband/acc: add support for 4GUL with SO and TB

[Chautru, Nicolas]

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, February 10, 2023 12:31 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org
> Cc: Vargas, Hernan <hernan.vargas@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v1 2/9] baseband/acc: add support for 4GUL with SO and
> TB
> 
> 
> 
> On 2/9/23 23:19, Nicolas Chautru wrote:
> > Implementation to support the case when using LTE decoder with soft
> > output and transport block mode.
> >
> > Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
> > Cc: stable@dpdk.org
> 
> Should it really be a fix and be backported?
> It looks like a new feature.

I was on a fence. The capability exposed would imply this should be supported already, but indeed no need to backport.
I will remove the Fixes in v2, and reorder accordingly the series. 

> 
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   drivers/baseband/acc/rte_vrb_pmd.c | 21 +++++++++++----------
> >   1 file changed, 11 insertions(+), 10 deletions(-)
> >
> 
> Thanks,
> Maxime

[PATCH v1 3/9] baseband/acc: remove interrupt support on VRB1

[Nicolas Chautru]

Not enabling interrupt in VRB1 PMD variant to avoid
potential corner case.

Fixes: 3cabc8eaf524 ("baseband/acc200: support interrupt")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index c43c08afe8..a111836e51 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -586,6 +586,14 @@ vrb_intr_enable(struct rte_bbdev *dev)
 {
 	int ret;
 	struct acc_device *d = dev->data->dev_private;
+
+	if (d->device_variant == VRB1_VARIANT) {
+		/* On VRB1: cannot enable MSI/IR to avoid potential back-pressure corner case. */
+		rte_bbdev_log(ERR, "VRB1 (%s) doesn't support any MSI/MSI-X interrupt\n",
+				dev->data->name);
+		return -ENOTSUP;
+	}
+
 	/*
 	 * MSI/MSI-X are supported.
 	 * Option controlled by vfio-intr through EAL parameter.
-- 
2.34.1

Re: [PATCH v1 3/9] baseband/acc: remove interrupt support on VRB1

[Maxime Coquelin]

On 2/9/23 23:19, Nicolas Chautru wrote:
> Not enabling interrupt in VRB1 PMD variant to avoid
> potential corner case.
> 
> Fixes: 3cabc8eaf524 ("baseband/acc200: support interrupt")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index c43c08afe8..a111836e51 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -586,6 +586,14 @@ vrb_intr_enable(struct rte_bbdev *dev)
>   {
>   	int ret;
>   	struct acc_device *d = dev->data->dev_private;
> +
> +	if (d->device_variant == VRB1_VARIANT) {
> +		/* On VRB1: cannot enable MSI/IR to avoid potential back-pressure corner case. */
> +		rte_bbdev_log(ERR, "VRB1 (%s) doesn't support any MSI/MSI-X interrupt\n",
> +				dev->data->name);
> +		return -ENOTSUP;
> +	}
> +
>   	/*
>   	 * MSI/MSI-X are supported.
>   	 * Option controlled by vfio-intr through EAL parameter.

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[PATCH v1 4/9] baseband/acc: add explicit mbuf apend for soft output

[Nicolas Chautru]

Adding an explicit mbuf append in the case of soft output
mbuf being provided.

Fixes: e640f6cdfa84 ("baseband/acc200: add LDPC processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index a111836e51..8540e3d31c 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -2067,6 +2067,10 @@ vrb_enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 		}
 	}
 
+	if (op->ldpc_dec.soft_output.length > 0)
+		mbuf_append(op->ldpc_dec.soft_output.data, op->ldpc_dec.soft_output.data,
+				op->ldpc_dec.soft_output.length);
+
 #ifdef RTE_LIBRTE_BBDEV_DEBUG
 	rte_memdump(stderr, "FCW", &desc->req.fcw_ld,
 			sizeof(desc->req.fcw_ld) - 8);
-- 
2.34.1

Re: [PATCH v1 4/9] baseband/acc: add explicit mbuf apend for soft output

[Maxime Coquelin]

On 2/9/23 23:19, Nicolas Chautru wrote:
> Adding an explicit mbuf append in the case of soft output
> mbuf being provided.
> 
> Fixes: e640f6cdfa84 ("baseband/acc200: add LDPC processing")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index a111836e51..8540e3d31c 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -2067,6 +2067,10 @@ vrb_enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   		}
>   	}
>   
> +	if (op->ldpc_dec.soft_output.length > 0)
> +		mbuf_append(op->ldpc_dec.soft_output.data, op->ldpc_dec.soft_output.data,
> +				op->ldpc_dec.soft_output.length);

No need to check the return value?
IOW, are we sure the buffer we try to append fits into the mbuf?

> +
>   #ifdef RTE_LIBRTE_BBDEV_DEBUG
>   	rte_memdump(stderr, "FCW", &desc->req.fcw_ld,
>   			sizeof(desc->req.fcw_ld) - 8);

RE: [PATCH v1 4/9] baseband/acc: add explicit mbuf apend for soft output

[Chautru, Nicolas]

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, February 10, 2023 12:41 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org
> Cc: Vargas, Hernan <hernan.vargas@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v1 4/9] baseband/acc: add explicit mbuf apend for soft
> output
> 
> 
> 
> On 2/9/23 23:19, Nicolas Chautru wrote:
> > Adding an explicit mbuf append in the case of soft output mbuf being
> > provided.
> >
> > Fixes: e640f6cdfa84 ("baseband/acc200: add LDPC processing")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   drivers/baseband/acc/rte_vrb_pmd.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/baseband/acc/rte_vrb_pmd.c
> > b/drivers/baseband/acc/rte_vrb_pmd.c
> > index a111836e51..8540e3d31c 100644
> > --- a/drivers/baseband/acc/rte_vrb_pmd.c
> > +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> > @@ -2067,6 +2067,10 @@ vrb_enqueue_ldpc_dec_one_op_cb(struct
> acc_queue *q, struct rte_bbdev_dec_op *op,
> >   		}
> >   	}
> >
> > +	if (op->ldpc_dec.soft_output.length > 0)
> > +		mbuf_append(op->ldpc_dec.soft_output.data, op-
> >ldpc_dec.soft_output.data,
> > +				op->ldpc_dec.soft_output.length);
> 
> No need to check the return value?
> IOW, are we sure the buffer we try to append fits into the mbuf?

There are many places we currently do not check on mbuf _append() return. Notably as there are many cases where the mbuf structure may not be a genuine mbuf. Ie. when the size may actually not be compatible with mbuf structure (much larger than max supported size). 
In the case when we know the size is bounded in a way that it would be implemented as true mbuf (HARQ circular buffer whose max size is 66 x 384) then we can do an explicit check. 
For other case we manipulate them as mbuf when relevant but still don’t impose the assumption and limitation to have an underlying real mbuf. Keep in mind we do not process packets but perform signal processing (some memory can be extremely large). 

> 
> > +
> >   #ifdef RTE_LIBRTE_BBDEV_DEBUG
> >   	rte_memdump(stderr, "FCW", &desc->req.fcw_ld,
> >   			sizeof(desc->req.fcw_ld) - 8);

[PATCH v1 5/9] baseband/acc: prevent to dequeue more than requested

[Nicolas Chautru]

Add support for corner-case when more operations are
requested than expected, in the case of encoder muxing
operations.

Fixes: e640f6cdfa84 ("baseband/acc200: add LDPC processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 8540e3d31c..b251ad25c6 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -2641,7 +2641,8 @@ vrb_enqueue_ldpc_dec(struct rte_bbdev_queue_data *q_data,
 /* Dequeue one encode operations from device in CB mode. */
 static inline int
 vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
-		uint16_t *dequeued_ops, uint32_t *aq_dequeued, uint16_t *dequeued_descs)
+		uint16_t *dequeued_ops, uint32_t *aq_dequeued, uint16_t *dequeued_descs,
+		uint16_t max_requested_ops)
 {
 	union acc_dma_desc *desc, atom_desc;
 	union acc_dma_rsp_desc rsp;
@@ -2654,6 +2655,9 @@ vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
 	desc = q->ring_addr + desc_idx;
 	atom_desc.atom_hdr = __atomic_load_n((uint64_t *)desc, __ATOMIC_RELAXED);
 
+	if (*dequeued_ops + desc->req.numCBs > max_requested_ops)
+		return -1;
+
 	/* Check fdone bit. */
 	if (!(atom_desc.rsp.val & ACC_FDONE))
 		return -1;
@@ -2695,7 +2699,7 @@ vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
 static inline int
 vrb_dequeue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
 		uint16_t *dequeued_ops, uint32_t *aq_dequeued,
-		uint16_t *dequeued_descs)
+		uint16_t *dequeued_descs, uint16_t max_requested_ops)
 {
 	union acc_dma_desc *desc, *last_desc, atom_desc;
 	union acc_dma_rsp_desc rsp;
@@ -2706,6 +2710,9 @@ vrb_dequeue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
 	desc = acc_desc_tail(q, *dequeued_descs);
 	atom_desc.atom_hdr = __atomic_load_n((uint64_t *)desc, __ATOMIC_RELAXED);
 
+	if (*dequeued_ops + 1 > max_requested_ops)
+		return -1;
+
 	/* Check fdone bit. */
 	if (!(atom_desc.rsp.val & ACC_FDONE))
 		return -1;
@@ -2966,25 +2973,23 @@ vrb_dequeue_enc(struct rte_bbdev_queue_data *q_data,
 
 	cbm = op->turbo_enc.code_block_mode;
 
-	for (i = 0; i < num; i++) {
+	for (i = 0; i < avail; i++) {
 		if (cbm == RTE_BBDEV_TRANSPORT_BLOCK)
 			ret = vrb_dequeue_enc_one_op_tb(q, &ops[dequeued_ops],
 					&dequeued_ops, &aq_dequeued,
-					&dequeued_descs);
+					&dequeued_descs, num);
 		else
 			ret = vrb_dequeue_enc_one_op_cb(q, &ops[dequeued_ops],
 					&dequeued_ops, &aq_dequeued,
-					&dequeued_descs);
+					&dequeued_descs, num);
 		if (ret < 0)
 			break;
-		if (dequeued_ops >= num)
-			break;
 	}
 
 	q->aq_dequeued += aq_dequeued;
 	q->sw_ring_tail += dequeued_descs;
 
-	/* Update enqueue stats */
+	/* Update enqueue stats. */
 	q_data->queue_stats.dequeued_count += dequeued_ops;
 
 	return dequeued_ops;
@@ -3010,15 +3015,13 @@ vrb_dequeue_ldpc_enc(struct rte_bbdev_queue_data *q_data,
 		if (cbm == RTE_BBDEV_TRANSPORT_BLOCK)
 			ret = vrb_dequeue_enc_one_op_tb(q, &ops[dequeued_ops],
 					&dequeued_ops, &aq_dequeued,
-					&dequeued_descs);
+					&dequeued_descs, num);
 		else
 			ret = vrb_dequeue_enc_one_op_cb(q, &ops[dequeued_ops],
 					&dequeued_ops, &aq_dequeued,
-					&dequeued_descs);
+					&dequeued_descs, num);
 		if (ret < 0)
 			break;
-		if (dequeued_ops >= num)
-			break;
 	}
 
 	q->aq_dequeued += aq_dequeued;
-- 
2.34.1

Re: [PATCH v1 5/9] baseband/acc: prevent to dequeue more than requested

[Maxime Coquelin]

On 2/9/23 23:19, Nicolas Chautru wrote:
> Add support for corner-case when more operations are
> requested than expected, in the case of encoder muxing
> operations.
> 
> Fixes: e640f6cdfa84 ("baseband/acc200: add LDPC processing")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index 8540e3d31c..b251ad25c6 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -2641,7 +2641,8 @@ vrb_enqueue_ldpc_dec(struct rte_bbdev_queue_data *q_data,
>   /* Dequeue one encode operations from device in CB mode. */
>   static inline int
>   vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
> -		uint16_t *dequeued_ops, uint32_t *aq_dequeued, uint16_t *dequeued_descs)
> +		uint16_t *dequeued_ops, uint32_t *aq_dequeued, uint16_t *dequeued_descs,
> +		uint16_t max_requested_ops)
>   {
>   	union acc_dma_desc *desc, atom_desc;
>   	union acc_dma_rsp_desc rsp;
> @@ -2654,6 +2655,9 @@ vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
>   	desc = q->ring_addr + desc_idx;
>   	atom_desc.atom_hdr = __atomic_load_n((uint64_t *)desc, __ATOMIC_RELAXED);
>   
> +	if (*dequeued_ops + desc->req.numCBs > max_requested_ops)
> +		return -1;
> +
>   	/* Check fdone bit. */
>   	if (!(atom_desc.rsp.val & ACC_FDONE))
>   		return -1;
> @@ -2695,7 +2699,7 @@ vrb_dequeue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
>   static inline int
>   vrb_dequeue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
>   		uint16_t *dequeued_ops, uint32_t *aq_dequeued,
> -		uint16_t *dequeued_descs)
> +		uint16_t *dequeued_descs, uint16_t max_requested_ops)
>   {
>   	union acc_dma_desc *desc, *last_desc, atom_desc;
>   	union acc_dma_rsp_desc rsp;
> @@ -2706,6 +2710,9 @@ vrb_dequeue_enc_one_op_tb(struct acc_queue *q, struct rte_bbdev_enc_op **ref_op,
>   	desc = acc_desc_tail(q, *dequeued_descs);
>   	atom_desc.atom_hdr = __atomic_load_n((uint64_t *)desc, __ATOMIC_RELAXED);
>   
> +	if (*dequeued_ops + 1 > max_requested_ops)
> +		return -1;
> +
>   	/* Check fdone bit. */
>   	if (!(atom_desc.rsp.val & ACC_FDONE))
>   		return -1;
> @@ -2966,25 +2973,23 @@ vrb_dequeue_enc(struct rte_bbdev_queue_data *q_data,
>   
>   	cbm = op->turbo_enc.code_block_mode;
>   
> -	for (i = 0; i < num; i++) {
> +	for (i = 0; i < avail; i++) {
>   		if (cbm == RTE_BBDEV_TRANSPORT_BLOCK)
>   			ret = vrb_dequeue_enc_one_op_tb(q, &ops[dequeued_ops],
>   					&dequeued_ops, &aq_dequeued,
> -					&dequeued_descs);
> +					&dequeued_descs, num);
>   		else
>   			ret = vrb_dequeue_enc_one_op_cb(q, &ops[dequeued_ops],
>   					&dequeued_ops, &aq_dequeued,
> -					&dequeued_descs);
> +					&dequeued_descs, num);
>   		if (ret < 0)
>   			break;
> -		if (dequeued_ops >= num)
> -			break;
>   	}
>   
>   	q->aq_dequeued += aq_dequeued;
>   	q->sw_ring_tail += dequeued_descs;
>   
> -	/* Update enqueue stats */
> +	/* Update enqueue stats. */
>   	q_data->queue_stats.dequeued_count += dequeued_ops;
>   
>   	return dequeued_ops;
> @@ -3010,15 +3015,13 @@ vrb_dequeue_ldpc_enc(struct rte_bbdev_queue_data *q_data,
>   		if (cbm == RTE_BBDEV_TRANSPORT_BLOCK)
>   			ret = vrb_dequeue_enc_one_op_tb(q, &ops[dequeued_ops],
>   					&dequeued_ops, &aq_dequeued,
> -					&dequeued_descs);
> +					&dequeued_descs, num);
>   		else
>   			ret = vrb_dequeue_enc_one_op_cb(q, &ops[dequeued_ops],
>   					&dequeued_ops, &aq_dequeued,
> -					&dequeued_descs);
> +					&dequeued_descs, num);
>   		if (ret < 0)
>   			break;
> -		if (dequeued_ops >= num)
> -			break;
>   	}
>   
>   	q->aq_dequeued += aq_dequeued;

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[PATCH v1 6/9] baseband/acc: fix iteration counter in TB mode

[Nicolas Chautru]

The iteration count was not using correct structure (4G vs 5G)
in TB mode.

Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index b251ad25c6..0bd5c65177 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -2876,7 +2876,7 @@ vrb_dequeue_ldpc_dec_one_op_cb(struct rte_bbdev_queue_data *q_data,
 	return 1;
 }
 
-/* Dequeue one decode operations from device in TB mode. */
+/* Dequeue one decode operations from device in TB mode for 4G or 5G. */
 static inline int
 vrb_dequeue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op **ref_op,
 		uint16_t dequeued_cbs, uint32_t *aq_dequeued)
@@ -2930,8 +2930,12 @@ vrb_dequeue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op **ref_op,
 		/* CRC invalid if error exists. */
 		if (!op->status)
 			op->status |= rsp.crc_status << RTE_BBDEV_CRC_ERROR;
-		op->turbo_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
-				op->turbo_dec.iter_count);
+		if (q->op_type == RTE_BBDEV_OP_LDPC_DEC)
+			op->ldpc_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
+					op->ldpc_dec.iter_count);
+		else
+			op->turbo_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
+					op->turbo_dec.iter_count);
 
 		/* Check if this is the last desc in batch (Atomic Queue). */
 		if (desc->req.last_desc_in_batch) {
-- 
2.34.1

Re: [PATCH v1 6/9] baseband/acc: fix iteration counter in TB mode

[Maxime Coquelin]

On 2/9/23 23:19, Nicolas Chautru wrote:
> The iteration count was not using correct structure (4G vs 5G)
> in TB mode.
> 
> Fixes: bec597b78a0 ("baseband/acc200: add LTE processing")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index b251ad25c6..0bd5c65177 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -2876,7 +2876,7 @@ vrb_dequeue_ldpc_dec_one_op_cb(struct rte_bbdev_queue_data *q_data,
>   	return 1;
>   }
>   
> -/* Dequeue one decode operations from device in TB mode. */
> +/* Dequeue one decode operations from device in TB mode for 4G or 5G. */
>   static inline int
>   vrb_dequeue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op **ref_op,
>   		uint16_t dequeued_cbs, uint32_t *aq_dequeued)
> @@ -2930,8 +2930,12 @@ vrb_dequeue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op **ref_op,
>   		/* CRC invalid if error exists. */
>   		if (!op->status)
>   			op->status |= rsp.crc_status << RTE_BBDEV_CRC_ERROR;
> -		op->turbo_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
> -				op->turbo_dec.iter_count);
> +		if (q->op_type == RTE_BBDEV_OP_LDPC_DEC)
> +			op->ldpc_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
> +					op->ldpc_dec.iter_count);
> +		else
> +			op->turbo_dec.iter_count = RTE_MAX((uint8_t) rsp.iter_cnt,
> +					op->turbo_dec.iter_count);
>   
>   		/* Check if this is the last desc in batch (Atomic Queue). */
>   		if (desc->req.last_desc_in_batch) {

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[PATCH v1 7/9] baseband/acc: fix potential arithmetic overflow Fix potential issue of overflow causing coverity warning.

[Nicolas Chautru]

Coverity issue: 383154
Fixes: 8e16839937 ("baseband/acc: extension of the device structure")
Cc: stable@dpdk.org

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 2 +-
 drivers/baseband/acc/vrb_pmd.h     | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 0bd5c65177..8fcb06b4ff 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -518,7 +518,7 @@ vrb_setup_queues(struct rte_bbdev *dev, uint16_t num_queues, int socket_id)
 	if (d->tail_ptrs == NULL)
 		d->tail_ptrs = rte_zmalloc_socket(
 				dev->device->driver->name,
-				d->num_qgroups * d->num_aqs * sizeof(uint32_t),
+				VRB_MAX_QGRPS * VRB_MAX_AQS * sizeof(uint32_t),
 				RTE_CACHE_LINE_SIZE, socket_id);
 	if (d->tail_ptrs == NULL) {
 		rte_bbdev_log(ERR, "Failed to allocate tail ptr for %s:%u",
diff --git a/drivers/baseband/acc/vrb_pmd.h b/drivers/baseband/acc/vrb_pmd.h
index f3c9239881..01028273e7 100644
--- a/drivers/baseband/acc/vrb_pmd.h
+++ b/drivers/baseband/acc/vrb_pmd.h
@@ -36,6 +36,7 @@
 
 #define VRB_NUM_ACCS                 6
 #define VRB_MAX_QGRPS                32
+#define VRB_MAX_AQS                  32
 
 #define ACC_STATUS_WAIT      10
 #define ACC_STATUS_TO        100
-- 
2.34.1

Re: [PATCH v1 7/9] baseband/acc: fix potential arithmetic overflow Fix potential issue of overflow causing coverity warning.

[Maxime Coquelin]

There seems to be formatting issue of the commit log,
the commit message is appended to the title.


On 2/9/23 23:19, Nicolas Chautru wrote:
> Coverity issue: 383154
> Fixes: 8e16839937 ("baseband/acc: extension of the device structure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 2 +-
>   drivers/baseband/acc/vrb_pmd.h     | 1 +
>   2 files changed, 2 insertions(+), 1 deletion(-)

With commit log fixed:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index 0bd5c65177..8fcb06b4ff 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -518,7 +518,7 @@ vrb_setup_queues(struct rte_bbdev *dev, uint16_t num_queues, int socket_id)
>   	if (d->tail_ptrs == NULL)
>   		d->tail_ptrs = rte_zmalloc_socket(
>   				dev->device->driver->name,
> -				d->num_qgroups * d->num_aqs * sizeof(uint32_t),
> +				VRB_MAX_QGRPS * VRB_MAX_AQS * sizeof(uint32_t),
>   				RTE_CACHE_LINE_SIZE, socket_id);
>   	if (d->tail_ptrs == NULL) {
>   		rte_bbdev_log(ERR, "Failed to allocate tail ptr for %s:%u",
> diff --git a/drivers/baseband/acc/vrb_pmd.h b/drivers/baseband/acc/vrb_pmd.h
> index f3c9239881..01028273e7 100644
> --- a/drivers/baseband/acc/vrb_pmd.h
> +++ b/drivers/baseband/acc/vrb_pmd.h
> @@ -36,6 +36,7 @@
>   
>   #define VRB_NUM_ACCS                 6
>   #define VRB_MAX_QGRPS                32
> +#define VRB_MAX_AQS                  32
>   
>   #define ACC_STATUS_WAIT      10
>   #define ACC_STATUS_TO        100

[PATCH v1 8/9] baseband/acc: add support for 4GUL CRC drop in VRB PMD

[Nicolas Chautru]

Adding support for the capability to drop the CRC24B
when using the turbo-decoder.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_vrb_pmd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 8fcb06b4ff..18b5dab25f 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -996,6 +996,7 @@ vrb_dev_info_get(struct rte_bbdev *dev, struct rte_bbdev_driver_info *dev_info)
 				.capability_flags =
 					RTE_BBDEV_TURBO_SUBBLOCK_DEINTERLEAVE |
 					RTE_BBDEV_TURBO_CRC_TYPE_24B |
+					RTE_BBDEV_TURBO_DEC_CRC_24B_DROP |
 					RTE_BBDEV_TURBO_EQUALIZER |
 					RTE_BBDEV_TURBO_SOFT_OUT_SATURATE |
 					RTE_BBDEV_TURBO_HALF_ITERATION_EVEN |
@@ -1408,8 +1409,12 @@ vrb_dma_desc_td_fill(struct rte_bbdev_dec_op *op,
 	}
 
 	if ((op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK)
-		&& !check_bit(op->turbo_dec.op_flags,
-		RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP))
+			&& !check_bit(op->turbo_dec.op_flags,
+			RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP))
+		crc24_overlap = 24;
+	if ((op->turbo_dec.code_block_mode == RTE_BBDEV_CODE_BLOCK)
+			&& check_bit(op->turbo_dec.op_flags,
+			RTE_BBDEV_TURBO_DEC_CRC_24B_DROP))
 		crc24_overlap = 24;
 
 	/* Calculates circular buffer size.
-- 
2.34.1

Re: [PATCH v1 8/9] baseband/acc: add support for 4GUL CRC drop in VRB PMD

[Maxime Coquelin]

On 2/9/23 23:19, Nicolas Chautru wrote:
> Adding support for the capability to drop the CRC24B
> when using the turbo-decoder.
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_vrb_pmd.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index 8fcb06b4ff..18b5dab25f 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -996,6 +996,7 @@ vrb_dev_info_get(struct rte_bbdev *dev, struct rte_bbdev_driver_info *dev_info)
>   				.capability_flags =
>   					RTE_BBDEV_TURBO_SUBBLOCK_DEINTERLEAVE |
>   					RTE_BBDEV_TURBO_CRC_TYPE_24B |
> +					RTE_BBDEV_TURBO_DEC_CRC_24B_DROP |
>   					RTE_BBDEV_TURBO_EQUALIZER |
>   					RTE_BBDEV_TURBO_SOFT_OUT_SATURATE |
>   					RTE_BBDEV_TURBO_HALF_ITERATION_EVEN |
> @@ -1408,8 +1409,12 @@ vrb_dma_desc_td_fill(struct rte_bbdev_dec_op *op,
>   	}
>   
>   	if ((op->turbo_dec.code_block_mode == RTE_BBDEV_TRANSPORT_BLOCK)
> -		&& !check_bit(op->turbo_dec.op_flags,
> -		RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP))
> +			&& !check_bit(op->turbo_dec.op_flags,
> +			RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP))
> +		crc24_overlap = 24;
> +	if ((op->turbo_dec.code_block_mode == RTE_BBDEV_CODE_BLOCK)
> +			&& check_bit(op->turbo_dec.op_flags,
> +			RTE_BBDEV_TURBO_DEC_CRC_24B_DROP))
>   		crc24_overlap = 24;
>   
>   	/* Calculates circular buffer size.

Normally only fixes should be proposed at this stage, but this is small
enough:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[PATCH v1 9/9] baseband/acc: remove printf from PMD function

[Nicolas Chautru]

Replacing usage of printf in companion function for
bbdev-test by rte_log.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 drivers/baseband/acc/rte_acc100_pmd.c | 18 +++++++++---------
 drivers/baseband/acc/rte_vrb_pmd.c    |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/baseband/acc/rte_acc100_pmd.c b/drivers/baseband/acc/rte_acc100_pmd.c
index 36f6fec5ad..40b5eaf089 100644
--- a/drivers/baseband/acc/rte_acc100_pmd.c
+++ b/drivers/baseband/acc/rte_acc100_pmd.c
@@ -4347,7 +4347,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
 {
 	int i, template_idx, qg_idx;
 	uint32_t address, status, value;
-	printf("Need to clear power-on 5GUL status in internal memory\n");
+	rte_bbdev_log(WARNING, "Need to clear power-on 5GUL status in internal memory");
 	/* Reset LDPC Cores */
 	for (i = 0; i < ACC100_ENGINES_MAX; i++)
 		acc_reg_write(d, HWPfFecUl5gCntrlReg +
@@ -4421,7 +4421,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
 	/* Force each engine which is in unspecified state */
 	for (i = 0; i < num_failed_engine; i++) {
 		int failed_engine = engines_to_restart[i];
-		printf("Force engine %d\n", failed_engine);
+		rte_bbdev_log(WARNING, "Force engine %d", failed_engine);
 		for (template_idx = ACC100_SIG_UL_5G;
 				template_idx <= ACC100_SIG_UL_5G_LAST;
 				template_idx++) {
@@ -4450,7 +4450,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
 		acc_reg_write(d, HWPfQmgrIngressAq + 0x100, enq_req.val);
 		usleep(ACC_LONG_WAIT * 100);
 		if (desc->req.word0 != 2)
-			printf("DMA Response %#"PRIx32"\n", desc->req.word0);
+			rte_bbdev_log(WARNING, "DMA Response %#"PRIx32"\n", desc->req.word0);
 	}
 
 	/* Reset LDPC Cores */
@@ -4482,7 +4482,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
 		} else
 			acc_reg_write(d, address, 0);
 	}
-	printf("Number of 5GUL engines %d\n", numEngines);
+	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
 
 	rte_free(d->sw_rings_base);
 	usleep(ACC_LONG_WAIT);
@@ -4671,7 +4671,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
 		} else
 			acc_reg_write(d, address, 0);
 	}
-	printf("Number of 5GUL engines %d\n", numEngines);
+	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
 	/* 4GDL */
 	numQqsAcc += numQgs;
 	numQgs	= conf->q_dl_4g.num_qgroups;
@@ -4801,7 +4801,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
 		version += acc_reg_read(d,
 				HWPfDdrPhyIdtmFwVersion + 4 * i) << (8 * i);
 	if (version != ACC100_PRQ_DDR_VER) {
-		printf("* Note: Not on DDR PRQ version %8x != %08x\n",
+		rte_bbdev_log(ERR, "* Note: Not on DDR PRQ version %8x != %08x",
 				version, ACC100_PRQ_DDR_VER);
 	} else if (firstCfg) {
 		/* ---- DDR configuration at boot up --- */
@@ -4871,7 +4871,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
 			if (value & 1)
 				break;
 		}
-		printf("DDR Training completed in %d ms", i);
+		rte_bbdev_log(INFO, "DDR Training completed in %d ms", i);
 		/* Enable Memory Controller */
 		acc_reg_write(d, HWPfDdrUmmcCtrl, 0x401);
 		/* Release AXI interface reset */
@@ -5047,7 +5047,7 @@ acc101_configure(const char *dev_name, struct rte_acc_conf *conf)
 		} else
 			acc_reg_write(d, address, 0);
 	}
-	printf("Number of 5GUL engines %d\n", numEngines);
+	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
 	/* 4GDL */
 	numQqsAcc += numQgs;
 	numQgs	= conf->q_dl_4g.num_qgroups;
@@ -5185,7 +5185,7 @@ rte_acc_configure(const char *dev_name, struct rte_acc_conf *conf)
 		return -ENODEV;
 	}
 	struct rte_pci_device *pci_dev = RTE_DEV_TO_PCI(bbdev->device);
-	printf("Configure dev id %x\n", pci_dev->id.device_id);
+	rte_bbdev_log(INFO, "Configure dev id %x", pci_dev->id.device_id);
 	if (pci_dev->id.device_id == ACC100_PF_DEVICE_ID)
 		return acc100_configure(dev_name, conf);
 	else if (pci_dev->id.device_id == ACC101_PF_DEVICE_ID)
diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
index 18b5dab25f..c37f725415 100644
--- a/drivers/baseband/acc/rte_vrb_pmd.c
+++ b/drivers/baseband/acc/rte_vrb_pmd.c
@@ -3633,7 +3633,7 @@ vrb1_configure(const char *dev_name, struct rte_acc_conf *conf)
 		} else
 			acc_reg_write(d, address, 0);
 	}
-	printf("Number of 5GUL engines %d\n", numEngines);
+	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
 	/* 4GDL */
 	numQqsAcc += numQgs;
 	numQgs	= conf->q_dl_4g.num_qgroups;
-- 
2.34.1

Re: [PATCH v1 9/9] baseband/acc: remove printf from PMD function

[Maxime Coquelin]

On 2/9/23 23:19, Nicolas Chautru wrote:
> Replacing usage of printf in companion function for
> bbdev-test by rte_log.

I think it could be backported. If you agree please add Fixes tag and Cc
stable.

> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   drivers/baseband/acc/rte_acc100_pmd.c | 18 +++++++++---------
>   drivers/baseband/acc/rte_vrb_pmd.c    |  2 +-
>   2 files changed, 10 insertions(+), 10 deletions(-)

For the patch itself:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


> diff --git a/drivers/baseband/acc/rte_acc100_pmd.c b/drivers/baseband/acc/rte_acc100_pmd.c
> index 36f6fec5ad..40b5eaf089 100644
> --- a/drivers/baseband/acc/rte_acc100_pmd.c
> +++ b/drivers/baseband/acc/rte_acc100_pmd.c
> @@ -4347,7 +4347,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
>   {
>   	int i, template_idx, qg_idx;
>   	uint32_t address, status, value;
> -	printf("Need to clear power-on 5GUL status in internal memory\n");
> +	rte_bbdev_log(WARNING, "Need to clear power-on 5GUL status in internal memory");
>   	/* Reset LDPC Cores */
>   	for (i = 0; i < ACC100_ENGINES_MAX; i++)
>   		acc_reg_write(d, HWPfFecUl5gCntrlReg +
> @@ -4421,7 +4421,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
>   	/* Force each engine which is in unspecified state */
>   	for (i = 0; i < num_failed_engine; i++) {
>   		int failed_engine = engines_to_restart[i];
> -		printf("Force engine %d\n", failed_engine);
> +		rte_bbdev_log(WARNING, "Force engine %d", failed_engine);
>   		for (template_idx = ACC100_SIG_UL_5G;
>   				template_idx <= ACC100_SIG_UL_5G_LAST;
>   				template_idx++) {
> @@ -4450,7 +4450,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
>   		acc_reg_write(d, HWPfQmgrIngressAq + 0x100, enq_req.val);
>   		usleep(ACC_LONG_WAIT * 100);
>   		if (desc->req.word0 != 2)
> -			printf("DMA Response %#"PRIx32"\n", desc->req.word0);
> +			rte_bbdev_log(WARNING, "DMA Response %#"PRIx32"\n", desc->req.word0);
>   	}
>   
>   	/* Reset LDPC Cores */
> @@ -4482,7 +4482,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
>   		} else
>   			acc_reg_write(d, address, 0);
>   	}
> -	printf("Number of 5GUL engines %d\n", numEngines);
> +	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
>   
>   	rte_free(d->sw_rings_base);
>   	usleep(ACC_LONG_WAIT);
> @@ -4671,7 +4671,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
>   		} else
>   			acc_reg_write(d, address, 0);
>   	}
> -	printf("Number of 5GUL engines %d\n", numEngines);
> +	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
>   	/* 4GDL */
>   	numQqsAcc += numQgs;
>   	numQgs	= conf->q_dl_4g.num_qgroups;
> @@ -4801,7 +4801,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
>   		version += acc_reg_read(d,
>   				HWPfDdrPhyIdtmFwVersion + 4 * i) << (8 * i);
>   	if (version != ACC100_PRQ_DDR_VER) {
> -		printf("* Note: Not on DDR PRQ version %8x != %08x\n",
> +		rte_bbdev_log(ERR, "* Note: Not on DDR PRQ version %8x != %08x",
>   				version, ACC100_PRQ_DDR_VER);
>   	} else if (firstCfg) {
>   		/* ---- DDR configuration at boot up --- */
> @@ -4871,7 +4871,7 @@ acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
>   			if (value & 1)
>   				break;
>   		}
> -		printf("DDR Training completed in %d ms", i);
> +		rte_bbdev_log(INFO, "DDR Training completed in %d ms", i);
>   		/* Enable Memory Controller */
>   		acc_reg_write(d, HWPfDdrUmmcCtrl, 0x401);
>   		/* Release AXI interface reset */
> @@ -5047,7 +5047,7 @@ acc101_configure(const char *dev_name, struct rte_acc_conf *conf)
>   		} else
>   			acc_reg_write(d, address, 0);
>   	}
> -	printf("Number of 5GUL engines %d\n", numEngines);
> +	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
>   	/* 4GDL */
>   	numQqsAcc += numQgs;
>   	numQgs	= conf->q_dl_4g.num_qgroups;
> @@ -5185,7 +5185,7 @@ rte_acc_configure(const char *dev_name, struct rte_acc_conf *conf)
>   		return -ENODEV;
>   	}
>   	struct rte_pci_device *pci_dev = RTE_DEV_TO_PCI(bbdev->device);
> -	printf("Configure dev id %x\n", pci_dev->id.device_id);
> +	rte_bbdev_log(INFO, "Configure dev id %x", pci_dev->id.device_id);
>   	if (pci_dev->id.device_id == ACC100_PF_DEVICE_ID)
>   		return acc100_configure(dev_name, conf);
>   	else if (pci_dev->id.device_id == ACC101_PF_DEVICE_ID)
> diff --git a/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c
> index 18b5dab25f..c37f725415 100644
> --- a/drivers/baseband/acc/rte_vrb_pmd.c
> +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> @@ -3633,7 +3633,7 @@ vrb1_configure(const char *dev_name, struct rte_acc_conf *conf)
>   		} else
>   			acc_reg_write(d, address, 0);
>   	}
> -	printf("Number of 5GUL engines %d\n", numEngines);
> +	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
>   	/* 4GDL */
>   	numQqsAcc += numQgs;
>   	numQgs	= conf->q_dl_4g.num_qgroups;

RE: [PATCH v1 9/9] baseband/acc: remove printf from PMD function

[Chautru, Nicolas]

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, February 10, 2023 1:45 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org
> Cc: Vargas, Hernan <hernan.vargas@intel.com>
> Subject: Re: [PATCH v1 9/9] baseband/acc: remove printf from PMD function
> 
> 
> 
> On 2/9/23 23:19, Nicolas Chautru wrote:
> > Replacing usage of printf in companion function for bbdev-test by
> > rte_log.
> 
> I think it could be backported. If you agree please add Fixes tag and Cc stable.

That function is only used for bbdev-test so would not really fix anything. 
We are changing this mainly to help imposing a rule moving forward for no printf in that PMD directory.
Based on this I don’t think we have to backport. But if you are convinced this is required, ok as well. 

> 
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   drivers/baseband/acc/rte_acc100_pmd.c | 18 +++++++++---------
> >   drivers/baseband/acc/rte_vrb_pmd.c    |  2 +-
> >   2 files changed, 10 insertions(+), 10 deletions(-)
> 
> For the patch itself:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime
> 
> 
> > diff --git a/drivers/baseband/acc/rte_acc100_pmd.c
> > b/drivers/baseband/acc/rte_acc100_pmd.c
> > index 36f6fec5ad..40b5eaf089 100644
> > --- a/drivers/baseband/acc/rte_acc100_pmd.c
> > +++ b/drivers/baseband/acc/rte_acc100_pmd.c
> > @@ -4347,7 +4347,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct
> acc_device *d,
> >   {
> >   	int i, template_idx, qg_idx;
> >   	uint32_t address, status, value;
> > -	printf("Need to clear power-on 5GUL status in internal memory\n");
> > +	rte_bbdev_log(WARNING, "Need to clear power-on 5GUL status in
> > +internal memory");
> >   	/* Reset LDPC Cores */
> >   	for (i = 0; i < ACC100_ENGINES_MAX; i++)
> >   		acc_reg_write(d, HWPfFecUl5gCntrlReg + @@ -4421,7
> +4421,7 @@
> > poweron_cleanup(struct rte_bbdev *bbdev, struct acc_device *d,
> >   	/* Force each engine which is in unspecified state */
> >   	for (i = 0; i < num_failed_engine; i++) {
> >   		int failed_engine = engines_to_restart[i];
> > -		printf("Force engine %d\n", failed_engine);
> > +		rte_bbdev_log(WARNING, "Force engine %d", failed_engine);
> >   		for (template_idx = ACC100_SIG_UL_5G;
> >   				template_idx <= ACC100_SIG_UL_5G_LAST;
> >   				template_idx++) {
> > @@ -4450,7 +4450,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct
> acc_device *d,
> >   		acc_reg_write(d, HWPfQmgrIngressAq + 0x100, enq_req.val);
> >   		usleep(ACC_LONG_WAIT * 100);
> >   		if (desc->req.word0 != 2)
> > -			printf("DMA Response %#"PRIx32"\n", desc-
> >req.word0);
> > +			rte_bbdev_log(WARNING, "DMA Response
> %#"PRIx32"\n",
> > +desc->req.word0);
> >   	}
> >
> >   	/* Reset LDPC Cores */
> > @@ -4482,7 +4482,7 @@ poweron_cleanup(struct rte_bbdev *bbdev, struct
> acc_device *d,
> >   		} else
> >   			acc_reg_write(d, address, 0);
> >   	}
> > -	printf("Number of 5GUL engines %d\n", numEngines);
> > +	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
> >
> >   	rte_free(d->sw_rings_base);
> >   	usleep(ACC_LONG_WAIT);
> > @@ -4671,7 +4671,7 @@ acc100_configure(const char *dev_name, struct
> rte_acc_conf *conf)
> >   		} else
> >   			acc_reg_write(d, address, 0);
> >   	}
> > -	printf("Number of 5GUL engines %d\n", numEngines);
> > +	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
> >   	/* 4GDL */
> >   	numQqsAcc += numQgs;
> >   	numQgs	= conf->q_dl_4g.num_qgroups;
> > @@ -4801,7 +4801,7 @@ acc100_configure(const char *dev_name, struct
> rte_acc_conf *conf)
> >   		version += acc_reg_read(d,
> >   				HWPfDdrPhyIdtmFwVersion + 4 * i) << (8 * i);
> >   	if (version != ACC100_PRQ_DDR_VER) {
> > -		printf("* Note: Not on DDR PRQ version %8x != %08x\n",
> > +		rte_bbdev_log(ERR, "* Note: Not on DDR PRQ version %8x !=
> %08x",
> >   				version, ACC100_PRQ_DDR_VER);
> >   	} else if (firstCfg) {
> >   		/* ---- DDR configuration at boot up --- */ @@ -4871,7 +4871,7
> @@
> > acc100_configure(const char *dev_name, struct rte_acc_conf *conf)
> >   			if (value & 1)
> >   				break;
> >   		}
> > -		printf("DDR Training completed in %d ms", i);
> > +		rte_bbdev_log(INFO, "DDR Training completed in %d ms", i);
> >   		/* Enable Memory Controller */
> >   		acc_reg_write(d, HWPfDdrUmmcCtrl, 0x401);
> >   		/* Release AXI interface reset */
> > @@ -5047,7 +5047,7 @@ acc101_configure(const char *dev_name, struct
> rte_acc_conf *conf)
> >   		} else
> >   			acc_reg_write(d, address, 0);
> >   	}
> > -	printf("Number of 5GUL engines %d\n", numEngines);
> > +	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
> >   	/* 4GDL */
> >   	numQqsAcc += numQgs;
> >   	numQgs	= conf->q_dl_4g.num_qgroups;
> > @@ -5185,7 +5185,7 @@ rte_acc_configure(const char *dev_name, struct
> rte_acc_conf *conf)
> >   		return -ENODEV;
> >   	}
> >   	struct rte_pci_device *pci_dev = RTE_DEV_TO_PCI(bbdev->device);
> > -	printf("Configure dev id %x\n", pci_dev->id.device_id);
> > +	rte_bbdev_log(INFO, "Configure dev id %x", pci_dev->id.device_id);
> >   	if (pci_dev->id.device_id == ACC100_PF_DEVICE_ID)
> >   		return acc100_configure(dev_name, conf);
> >   	else if (pci_dev->id.device_id == ACC101_PF_DEVICE_ID) diff --git
> > a/drivers/baseband/acc/rte_vrb_pmd.c
> > b/drivers/baseband/acc/rte_vrb_pmd.c
> > index 18b5dab25f..c37f725415 100644
> > --- a/drivers/baseband/acc/rte_vrb_pmd.c
> > +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> > @@ -3633,7 +3633,7 @@ vrb1_configure(const char *dev_name, struct
> rte_acc_conf *conf)
> >   		} else
> >   			acc_reg_write(d, address, 0);
> >   	}
> > -	printf("Number of 5GUL engines %d\n", numEngines);
> > +	rte_bbdev_log(INFO, "Number of 5GUL engines %d", numEngines);
> >   	/* 4GDL */
> >   	numQqsAcc += numQgs;
> >   	numQgs	= conf->q_dl_4g.num_qgroups;