[v2,5/5] baseband/acc100: add protection for some negative scenario
Checks
Commit Message
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
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;
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;
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;
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.
@@ -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;