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

  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(+)
  

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);
  

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);