diff mbox series

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

Message ID 1651083423-33202-6-git-send-email-nicolas.chautru@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series drivers/baseband: PMD to support ACC101 device |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Chautru, Nicolas April 27, 2022, 6:17 p.m. UTC 
  Catch exception in PMD in case of invalid input parameter.

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

Comments

Tom Rix May 8, 2022, 1:55 p.m. UTC | #1 
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.

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

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;
Chautru, Nicolas May 9, 2022, 9:45 p.m. UTC | #2 
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.  

> >
> > 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;
Tom Rix May 10, 2022, 12:11 p.m. UTC | #3 
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;
Thomas Monjalon May 10, 2022, 2:44 p.m. UTC | #4 
10/05/2022 14:11, Tom Rix:
> On 5/9/22 2:45 PM, Chautru, Nicolas wrote:
> > From: Tom Rix <trix@redhat.com>
> >> 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.

Well explained, and I agree with Tom.
diff mbox series

Patch

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