Re: [dpdk-dev] [PATCH v4] baseband/fpga_lte_fec: adding driver for FEC on FPGA

["Chautru, Nicolas" <nicolas.chautru@intel.com>]

>-----Original Message-----
>From: Chalupnik, KamilX 
>Sent: Wednesday, June 12, 2019 9:03 AM
>To: Chautru, Nicolas <nicolas.chautru@intel.com>; thomas@monjalon.net; akhil.goyal@nxp.com; dev@dpdk.org
>Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Mokhtar, Amr <amr.mokhtar@intel.com>; Chautru, Nicolas <nicolas.chautru@intel.com>
>Subject: RE: [dpdk-dev] [PATCH v4] baseband/fpga_lte_fec: adding driver for FEC on FPGA
>
>Hi Nic,
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Nicolas Chautru
>> Sent: Monday, June 10, 2019 7:01 PM
>> To: thomas@monjalon.net; akhil.goyal@nxp.com; dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Mokhtar, Amr 
>> <amr.mokhtar@intel.com>; Chautru, Nicolas <nicolas.chautru@intel.com>
>> Subject: [dpdk-dev] [PATCH v4] baseband/fpga_lte_fec: adding driver 
>> for FEC on FPGA
>> 
>> Supports for FEC 4G PMD Driver on FPGA card PAC N3000
>> 
>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>> ---
>>  config/common_base                                 |    6 +
>>  doc/guides/bbdevs/fpga_lte_fec.rst                 |  318 +++
>>  doc/guides/bbdevs/index.rst                        |    1 +
>>  drivers/baseband/Makefile                          |    2 +
>>  drivers/baseband/fpga_lte_fec/Makefile             |   29 +
>>  drivers/baseband/fpga_lte_fec/fpga_lte_fec.c       | 2674
>> ++++++++++++++++++++
>>  drivers/baseband/fpga_lte_fec/fpga_lte_fec.h       |   73 +
>>  drivers/baseband/fpga_lte_fec/meson.build          |    7 +
>>  .../rte_pmd_bbdev_fpga_lte_fec_version.map         |    3 +
>>  drivers/baseband/meson.build                       |    2 +-
>>  mk/rte.app.mk                                      |    1 +
>>  11 files changed, 3115 insertions(+), 1 deletion(-)  create mode 
>> 100644 doc/guides/bbdevs/fpga_lte_fec.rst
>>  create mode 100644 drivers/baseband/fpga_lte_fec/Makefile
>>  create mode 100644 drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
>>  create mode 100644 drivers/baseband/fpga_lte_fec/fpga_lte_fec.h
>>  create mode 100644 drivers/baseband/fpga_lte_fec/meson.build
>>  create mode 100644
>> drivers/baseband/fpga_lte_fec/rte_pmd_bbdev_fpga_lte_fec_version.map
>
>
>Generally code looks ok. Just few comments.
>In all new files please change "Copyright(c) 2018" to "2019"
>

Thanks for the catch. Will update now.

>
>
>> diff --git a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
>...
>> +static inline int
>> +enqueue_enc_one_op_cb(struct fpga_queue *q, struct
>> rte_bbdev_enc_op *op,
>> +		uint16_t desc_offset)
>> +{
>...
>> +	if (check_bit(op->turbo_enc.op_flags,
>> RTE_BBDEV_TURBO_RATE_MATCH))
>> +		out_length = (e + 7) >> 3;
>> +	else
>> +		out_length = (k >> 3) * 3 + 2;
>> +
>> +	mbuf_append(output, output, out_length);
>> +
>	 	
>Value returned by mbuf_append should be check. I guess we can put that check under RTE_LIBRTE_BBDEV_DEBUG flag.
>Please apply this to all places where mbuf_append is used.
>

We discussed in parallel that the driver is not meant to catch all user
errors condition even in debug mode. We agreed that this was not required but thanks
for the thorough review.

>
>> diff --git
>> a/drivers/baseband/fpga_lte_fec/rte_pmd_bbdev_fpga_lte_fec_version.m
>> ap
>> b/drivers/baseband/fpga_lte_fec/rte_pmd_bbdev_fpga_lte_fec_version.m
>> ap
>> +DPDK_18.08 {
>> +    local: *;
>> +};
>
>Shouldn't be DPDK_19.08?
>

You are right. Time flies since that driver started! 

>
>Best regards,
>Kamil
>

Take care, Thanks, 
Nic