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

[Tom Rix <trix@redhat.com>]

On 5/9/22 2:45 PM, Chautru, Nicolas wrote:
> Hi Tom,
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Sunday, May 8, 2022 6:56 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> gakhil@marvell.com
>> Cc: thomas@monjalon.net; Kinsella, Ray <ray.kinsella@intel.com>; Richardson,
>> Bruce <bruce.richardson@intel.com>; hemant.agrawal@nxp.com; Zhang,
>> Mingshan <mingshan.zhang@intel.com>; david.marchand@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.chautru@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;