Re: [PATCH v2 5/5] baseband/acc100: add protection for some negative scenario

[Tom Rix]

On 5/9/22 2:45 PM, Chautru, Nicolas wrote:
Hi Tom,

-----Original Message-----
From: Tom Rix <t...@redhat.com>
Sent: Sunday, May 8, 2022 6:56 AM
To: Chautru, Nicolas <nicolas.chau...@intel.com>; dev@dpdk.org;
gak...@marvell.com
Cc: tho...@monjalon.net; Kinsella, Ray <ray.kinse...@intel.com>; Richardson,
Bruce <bruce.richard...@intel.com>; hemant.agra...@nxp.com; Zhang,
Mingshan <mingshan.zh...@intel.com>; david.march...@redhat.com
Subject: Re: [PATCH v2 5/5] baseband/acc100: add protection for some
negative scenario


On 4/27/22 11:17 AM, Nicolas Chautru wrote:
Catch exception in PMD in case of invalid input parameter.
It is not clear if this is 1 fix or 2.

But it does look like an acc100 fix so it should be split from the
acc101 patchset.

What is the concern? This is a different commit related to acc100.

Bisecting patchsets.

A patchset like this that enables a new device should just enable the 
new device.

Not enable a new device and random other stuff.

If the patchset had to be reverted, the revert would wipe out the fixes.

That work is done by someone else without deep knowledge or time to 
analyze every patchset for misc parts.

The fixes are more important than the new device, so they should be 
submitted first.

Tom


Signed-off-by: Nicolas Chautru <nicolas.chau...@intel.com>
---
   drivers/baseband/acc100/rte_acc100_pmd.c | 6 ++++++
   1 file changed, 6 insertions(+)

diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
b/drivers/baseband/acc100/rte_acc100_pmd.c
index b588f5f..a13966c 100644
--- a/drivers/baseband/acc100/rte_acc100_pmd.c
+++ b/drivers/baseband/acc100/rte_acc100_pmd.c
@@ -1241,6 +1241,8 @@
                        return (bg == 1 ? ACC100_K0_3_1 : ACC100_K0_3_2)
* z_c;
        }
        /* LBRM case - includes a division by N */
+       if (unlikely(z_c == 0))
+               return 0;
This check should be moved to earlier, if 'n' is set to 0 in the statement 
above,
there is div by 0 later
N is purely a factor of z_c, I don’t see the concern is order.

Tom

        if (rv_index == 1)
                return (((bg == 1 ? ACC100_K0_1_1 : ACC100_K0_1_2) *
n_cb)
                                / n) * z_c;
@@ -1916,6 +1918,10 @@ static inline uint32_t hq_index(uint32_t
offset)

        /* Soft output */
        if (check_bit(op->turbo_dec.op_flags,
RTE_BBDEV_TURBO_SOFT_OUTPUT))
{
+               if (op->turbo_dec.soft_output.data == 0) {
+                       rte_bbdev_log(ERR, "Soft output is not defined");
+                       return -1;
+               }
                if (check_bit(op->turbo_dec.op_flags,
                                RTE_BBDEV_TURBO_EQUALIZER))
                        *s_out_length = e;