* [PATCH] iio: gp2ap020a00f: Fix signedness bug
@ 2022-04-14 10:45 Haowen Bai
2022-04-14 13:47 ` Andy Shevchenko
0 siblings, 1 reply; 10+ messages in thread
From: Haowen Bai @ 2022-04-14 10:45 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen; +Cc: Haowen Bai, linux-iio, linux-kernel
function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
we have to change u8 -> s8.
Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
drivers/iio/light/gp2ap020a00f.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index b820041159f7..b0e62d3c6fa0 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -994,7 +994,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
return IRQ_HANDLED;
}
-static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
+static s8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
enum iio_event_direction event_dir)
{
switch (chan->type) {
@@ -1025,7 +1025,7 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
struct gp2ap020a00f_data *data = iio_priv(indio_dev);
bool event_en = false;
u8 thresh_val_id;
- u8 thresh_reg_l;
+ s8 thresh_reg_l;
int err = 0;
mutex_lock(&data->lock);
@@ -1082,7 +1082,7 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
int *val, int *val2)
{
struct gp2ap020a00f_data *data = iio_priv(indio_dev);
- u8 thresh_reg_l;
+ s8 thresh_reg_l;
int err = IIO_VAL_INT;
mutex_lock(&data->lock);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: gp2ap020a00f: Fix signedness bug
2022-04-14 10:45 [PATCH] iio: gp2ap020a00f: Fix signedness bug Haowen Bai
@ 2022-04-14 13:47 ` Andy Shevchenko
2022-04-15 1:20 ` [PATCH V2] " Haowen Bai
2022-04-15 1:21 ` [PATCH] " baihaowen
0 siblings, 2 replies; 10+ messages in thread
From: Andy Shevchenko @ 2022-04-14 13:47 UTC (permalink / raw)
To: Haowen Bai
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio,
Linux Kernel Mailing List
On Thu, Apr 14, 2022 at 2:19 PM Haowen Bai <baihaowen@meizu.com> wrote:
>
> function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
> errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
> we have to change u8 -> s8.
s8 is not enough to hold an (arbitrary) error code. To be on the safe
side you need to use int.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V2] iio: gp2ap020a00f: Fix signedness bug
2022-04-14 13:47 ` Andy Shevchenko
@ 2022-04-15 1:20 ` Haowen Bai
2022-04-15 17:52 ` Jonathan Cameron
2022-04-15 1:21 ` [PATCH] " baihaowen
1 sibling, 1 reply; 10+ messages in thread
From: Haowen Bai @ 2022-04-15 1:20 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen; +Cc: Haowen Bai, linux-iio, linux-kernel
function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
we have to change u8 -> int.
Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: s8 is not enough to hold an (arbitrary) error code. To be on the safe
side we need to use int
drivers/iio/light/gp2ap020a00f.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index b820041159f7..b0e62d3c6fa0 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -994,7 +994,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
return IRQ_HANDLED;
}
-static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
+static int gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
enum iio_event_direction event_dir)
{
switch (chan->type) {
@@ -1025,7 +1025,7 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
struct gp2ap020a00f_data *data = iio_priv(indio_dev);
bool event_en = false;
u8 thresh_val_id;
- u8 thresh_reg_l;
+ int thresh_reg_l;
int err = 0;
mutex_lock(&data->lock);
@@ -1082,7 +1082,7 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
int *val, int *val2)
{
struct gp2ap020a00f_data *data = iio_priv(indio_dev);
- u8 thresh_reg_l;
+ int thresh_reg_l;
int err = IIO_VAL_INT;
mutex_lock(&data->lock);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: gp2ap020a00f: Fix signedness bug
2022-04-14 13:47 ` Andy Shevchenko
2022-04-15 1:20 ` [PATCH V2] " Haowen Bai
@ 2022-04-15 1:21 ` baihaowen
1 sibling, 0 replies; 10+ messages in thread
From: baihaowen @ 2022-04-15 1:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio,
Linux Kernel Mailing List
在 4/14/22 9:47 PM, Andy Shevchenko 写道:
> On Thu, Apr 14, 2022 at 2:19 PM Haowen Bai <baihaowen@meizu.com> wrote:
>> function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
>> errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
>> we have to change u8 -> s8.
> s8 is not enough to hold an (arbitrary) error code. To be on the safe
> side you need to use int.
>
Thank you for your suggestion.
--
Haowen Bai
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] iio: gp2ap020a00f: Fix signedness bug
2022-04-15 1:20 ` [PATCH V2] " Haowen Bai
@ 2022-04-15 17:52 ` Jonathan Cameron
2022-04-18 2:19 ` [PATCH V3] " Haowen Bai
2022-04-18 2:20 ` [PATCH V2] " baihaowen
0 siblings, 2 replies; 10+ messages in thread
From: Jonathan Cameron @ 2022-04-15 17:52 UTC (permalink / raw)
To: Haowen Bai; +Cc: Lars-Peter Clausen, linux-iio, linux-kernel
On Fri, 15 Apr 2022 09:20:37 +0800
Haowen Bai <baihaowen@meizu.com> wrote:
> function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
> errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
> we have to change u8 -> int.
>
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
Hi,
The return value is not checked in *read_event_val(), so if we actually got
-EINVAL (in reality we can't because the switch in *_get_thresh_reg*) always
matches) then we'd use it to index an array (after performing some maths on
the value). So please also add a check that the return value is not
negative in read_event_val()
Same is true or write_event_val.
Note that the bug here is probably the fact we return -EINVAL in the
first place. We could just stop doing that but it would be non obvious
when looking at the code that we couldn't get a failure to match in
the switch statement - so fixing as you have done (plus the extra
check I'm requesting) is probably the neatest solution.
Thanks,
Jonathan
> ---
> V1->V2: s8 is not enough to hold an (arbitrary) error code. To be on the safe
> side we need to use int
>
> drivers/iio/light/gp2ap020a00f.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
> index b820041159f7..b0e62d3c6fa0 100644
> --- a/drivers/iio/light/gp2ap020a00f.c
> +++ b/drivers/iio/light/gp2ap020a00f.c
> @@ -994,7 +994,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
> +static int gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
> enum iio_event_direction event_dir)
> {
> switch (chan->type) {
> @@ -1025,7 +1025,7 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
> struct gp2ap020a00f_data *data = iio_priv(indio_dev);
> bool event_en = false;
> u8 thresh_val_id;
> - u8 thresh_reg_l;
> + int thresh_reg_l;
> int err = 0;
>
> mutex_lock(&data->lock);
> @@ -1082,7 +1082,7 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
> int *val, int *val2)
> {
> struct gp2ap020a00f_data *data = iio_priv(indio_dev);
> - u8 thresh_reg_l;
> + int thresh_reg_l;
> int err = IIO_VAL_INT;
>
> mutex_lock(&data->lock);
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V3] iio: gp2ap020a00f: Fix signedness bug
2022-04-15 17:52 ` Jonathan Cameron
@ 2022-04-18 2:19 ` Haowen Bai
2022-04-28 18:41 ` Jonathan Cameron
2022-04-18 2:20 ` [PATCH V2] " baihaowen
1 sibling, 1 reply; 10+ messages in thread
From: Haowen Bai @ 2022-04-18 2:19 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen; +Cc: Haowen Bai, linux-iio, linux-kernel
function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
we have to change u8 -> int. Also we need to do index bound check at
gp2ap020a00f_read_event_val().
Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: s8 is not enough to hold an (arbitrary) error code. To be on the safe
side we need to use int.
V2->V3: add bound check at gp2ap020a00f_read_event_val().
drivers/iio/light/gp2ap020a00f.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index b820041159f7..f80d30786035 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -994,7 +994,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
return IRQ_HANDLED;
}
-static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
+static int gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
enum iio_event_direction event_dir)
{
switch (chan->type) {
@@ -1025,7 +1025,7 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
struct gp2ap020a00f_data *data = iio_priv(indio_dev);
bool event_en = false;
u8 thresh_val_id;
- u8 thresh_reg_l;
+ int thresh_reg_l;
int err = 0;
mutex_lock(&data->lock);
@@ -1082,14 +1082,14 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
int *val, int *val2)
{
struct gp2ap020a00f_data *data = iio_priv(indio_dev);
- u8 thresh_reg_l;
+ int thresh_reg_l;
int err = IIO_VAL_INT;
mutex_lock(&data->lock);
thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
- if (thresh_reg_l > GP2AP020A00F_PH_L_REG) {
+ if (thresh_reg_l < 0 || thresh_reg_l > GP2AP020A00F_PH_L_REG) {
err = -EINVAL;
goto error_unlock;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V2] iio: gp2ap020a00f: Fix signedness bug
2022-04-15 17:52 ` Jonathan Cameron
2022-04-18 2:19 ` [PATCH V3] " Haowen Bai
@ 2022-04-18 2:20 ` baihaowen
1 sibling, 0 replies; 10+ messages in thread
From: baihaowen @ 2022-04-18 2:20 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Lars-Peter Clausen, linux-iio, linux-kernel
在 4/16/22 1:52 AM, Jonathan Cameron 写道:
> On Fri, 15 Apr 2022 09:20:37 +0800
> Haowen Bai <baihaowen@meizu.com> wrote:
>
>> function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
>> errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
>> we have to change u8 -> int.
>>
>> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> Hi,
>
> The return value is not checked in *read_event_val(), so if we actually got
> -EINVAL (in reality we can't because the switch in *_get_thresh_reg*) always
> matches) then we'd use it to index an array (after performing some maths on
> the value). So please also add a check that the return value is not
> negative in read_event_val()
>
> Same is true or write_event_val.
>
>
> Note that the bug here is probably the fact we return -EINVAL in the
> first place. We could just stop doing that but it would be non obvious
> when looking at the code that we couldn't get a failure to match in
> the switch statement - so fixing as you have done (plus the extra
> check I'm requesting) is probably the neatest solution.
>
> Thanks,
>
> Jonathan
>
>
>> ---
>> V1->V2: s8 is not enough to hold an (arbitrary) error code. To be on the safe
>> side we need to use int
>>
>> drivers/iio/light/gp2ap020a00f.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
>> index b820041159f7..b0e62d3c6fa0 100644
>> --- a/drivers/iio/light/gp2ap020a00f.c
>> +++ b/drivers/iio/light/gp2ap020a00f.c
>> @@ -994,7 +994,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>>
>> -static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
>> +static int gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
>> enum iio_event_direction event_dir)
>> {
>> switch (chan->type) {
>> @@ -1025,7 +1025,7 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
>> struct gp2ap020a00f_data *data = iio_priv(indio_dev);
>> bool event_en = false;
>> u8 thresh_val_id;
>> - u8 thresh_reg_l;
>> + int thresh_reg_l;
>> int err = 0;
>>
>> mutex_lock(&data->lock);
>> @@ -1082,7 +1082,7 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
>> int *val, int *val2)
>> {
>> struct gp2ap020a00f_data *data = iio_priv(indio_dev);
>> - u8 thresh_reg_l;
>> + int thresh_reg_l;
>> int err = IIO_VAL_INT;
>>
>> mutex_lock(&data->lock);
hi Jonathan Cameron
Thank you for your suggestion. resended :).
--
Haowen Bai
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V3] iio: gp2ap020a00f: Fix signedness bug
2022-04-18 2:19 ` [PATCH V3] " Haowen Bai
@ 2022-04-28 18:41 ` Jonathan Cameron
2022-04-29 1:36 ` [PATCH V4] " Haowen Bai
0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2022-04-28 18:41 UTC (permalink / raw)
To: Haowen Bai; +Cc: Lars-Peter Clausen, linux-iio, linux-kernel
On Mon, 18 Apr 2022 10:19:35 +0800
Haowen Bai <baihaowen@meizu.com> wrote:
> function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
> errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
> we have to change u8 -> int. Also we need to do index bound check at
> gp2ap020a00f_read_event_val().
>
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
Please add a Fixes tag if possible.
> ---
> V1->V2: s8 is not enough to hold an (arbitrary) error code. To be on the safe
> side we need to use int.
> V2->V3: add bound check at gp2ap020a00f_read_event_val().
>
>
> drivers/iio/light/gp2ap020a00f.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
> index b820041159f7..f80d30786035 100644
> --- a/drivers/iio/light/gp2ap020a00f.c
> +++ b/drivers/iio/light/gp2ap020a00f.c
> @@ -994,7 +994,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
> +static int gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
> enum iio_event_direction event_dir)
> {
> switch (chan->type) {
> @@ -1025,7 +1025,7 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
> struct gp2ap020a00f_data *data = iio_priv(indio_dev);
> bool event_en = false;
> u8 thresh_val_id;
> - u8 thresh_reg_l;
> + int thresh_reg_l;
You need to check this val after the function call, but before it is used.
> int err = 0;
>
> mutex_lock(&data->lock);
> @@ -1082,14 +1082,14 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
> int *val, int *val2)
> {
> struct gp2ap020a00f_data *data = iio_priv(indio_dev);
> - u8 thresh_reg_l;
> + int thresh_reg_l;
> int err = IIO_VAL_INT;
>
> mutex_lock(&data->lock);
>
> thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
>
> - if (thresh_reg_l > GP2AP020A00F_PH_L_REG) {
> + if (thresh_reg_l < 0 || thresh_reg_l > GP2AP020A00F_PH_L_REG) {
> err = -EINVAL;
If a function returns an error we should pass that on unchanged
Here the value is the same, but none the less we should have this look something
like.
if (thresh_reg_l < 0)
return thresh_reg_l;
if (thresh_reg_l > GP2AP020A00F_PH_L_REG)
...
> goto error_unlock;
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V4] iio: gp2ap020a00f: Fix signedness bug
2022-04-28 18:41 ` Jonathan Cameron
@ 2022-04-29 1:36 ` Haowen Bai
2022-05-01 15:03 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Haowen Bai @ 2022-04-29 1:36 UTC (permalink / raw)
To: jic23; +Cc: baihaowen, lars, linux-iio, linux-kernel
function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
we have to change u8 -> int. Also we need to do index bound check at
gp2ap020a00f_read_event_val().
Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
V1->V2: s8 is not enough to hold an (arbitrary) error code. To be on the safe
side we need to use int.
V2->V3: add bound check at gp2ap020a00f_read_event_val().
V3->V4:
1. add fix tag.
2. add check before use at gp2ap020a00f_write_event_val().
3. returns an error we should pass that on unchanged at
gp2ap020a00f_read_event_val()
drivers/iio/light/gp2ap020a00f.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index b820041159f7..13583e1191d4 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -994,7 +994,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
return IRQ_HANDLED;
}
-static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
+static int gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
enum iio_event_direction event_dir)
{
switch (chan->type) {
@@ -1025,12 +1025,18 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
struct gp2ap020a00f_data *data = iio_priv(indio_dev);
bool event_en = false;
u8 thresh_val_id;
- u8 thresh_reg_l;
+ int thresh_reg_l;
int err = 0;
mutex_lock(&data->lock);
thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
+
+ if (thresh_reg_l < 0){
+ err = thresh_reg_l;
+ goto error_unlock;
+ }
+
thresh_val_id = GP2AP020A00F_THRESH_VAL_ID(thresh_reg_l);
if (thresh_val_id > GP2AP020A00F_THRESH_PH) {
@@ -1082,13 +1088,16 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
int *val, int *val2)
{
struct gp2ap020a00f_data *data = iio_priv(indio_dev);
- u8 thresh_reg_l;
+ int thresh_reg_l;
int err = IIO_VAL_INT;
mutex_lock(&data->lock);
thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
+ if (thresh_reg_l < 0)
+ return thresh_reg_l;
+
if (thresh_reg_l > GP2AP020A00F_PH_L_REG) {
err = -EINVAL;
goto error_unlock;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH V4] iio: gp2ap020a00f: Fix signedness bug
2022-04-29 1:36 ` [PATCH V4] " Haowen Bai
@ 2022-05-01 15:03 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2022-05-01 15:03 UTC (permalink / raw)
To: Haowen Bai; +Cc: lars, linux-iio, linux-kernel
On Fri, 29 Apr 2022 09:36:54 +0800
Haowen Bai <baihaowen@meizu.com> wrote:
> function gp2ap020a00f_get_thresh_reg() is unsigned but returning -EINVAL
> errcode, and thresh_reg_l is unsigned but receiving -EINVAL errcode. so
> we have to change u8 -> int. Also we need to do index bound check at
> gp2ap020a00f_read_event_val().
>
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
Hi Haowen Bai,
Please add a fixes tag. Also, this thread is getting rather deep
and illustrates why I tend to ask for each new version of IIO patches
to be posted without replying to previous thread (so start a new thread).
One issue below with locking.
Thanks,
Jonathan
> ---
> V1->V2: s8 is not enough to hold an (arbitrary) error code. To be on the safe
> side we need to use int.
> V2->V3: add bound check at gp2ap020a00f_read_event_val().
> V3->V4:
> 1. add fix tag.
> 2. add check before use at gp2ap020a00f_write_event_val().
> 3. returns an error we should pass that on unchanged at
> gp2ap020a00f_read_event_val()
>
> drivers/iio/light/gp2ap020a00f.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
> index b820041159f7..13583e1191d4 100644
> --- a/drivers/iio/light/gp2ap020a00f.c
> +++ b/drivers/iio/light/gp2ap020a00f.c
> @@ -994,7 +994,7 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
> +static int gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
> enum iio_event_direction event_dir)
> {
> switch (chan->type) {
> @@ -1025,12 +1025,18 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
> struct gp2ap020a00f_data *data = iio_priv(indio_dev);
> bool event_en = false;
> u8 thresh_val_id;
> - u8 thresh_reg_l;
> + int thresh_reg_l;
> int err = 0;
>
> mutex_lock(&data->lock);
>
> thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
> +
> + if (thresh_reg_l < 0){
> + err = thresh_reg_l;
> + goto error_unlock;
> + }
> +
> thresh_val_id = GP2AP020A00F_THRESH_VAL_ID(thresh_reg_l);
>
> if (thresh_val_id > GP2AP020A00F_THRESH_PH) {
> @@ -1082,13 +1088,16 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
> int *val, int *val2)
> {
> struct gp2ap020a00f_data *data = iio_priv(indio_dev);
> - u8 thresh_reg_l;
> + int thresh_reg_l;
> int err = IIO_VAL_INT;
>
> mutex_lock(&data->lock);
>
> thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
>
> + if (thresh_reg_l < 0)
mutex_unlock() ?
I'd expect something like
if (thresh_reg_l < 0) {
err = thresh_reg_l;
goto error_unlock;
}
> + return thresh_reg_l;
> +
> if (thresh_reg_l > GP2AP020A00F_PH_L_REG) {
> err = -EINVAL;
> goto error_unlock;
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-05-01 14:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-14 10:45 [PATCH] iio: gp2ap020a00f: Fix signedness bug Haowen Bai
2022-04-14 13:47 ` Andy Shevchenko
2022-04-15 1:20 ` [PATCH V2] " Haowen Bai
2022-04-15 17:52 ` Jonathan Cameron
2022-04-18 2:19 ` [PATCH V3] " Haowen Bai
2022-04-28 18:41 ` Jonathan Cameron
2022-04-29 1:36 ` [PATCH V4] " Haowen Bai
2022-05-01 15:03 ` Jonathan Cameron
2022-04-18 2:20 ` [PATCH V2] " baihaowen
2022-04-15 1:21 ` [PATCH] " baihaowen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.