iio: light: gp2ap020a00f: Fix signedness bug in gp2ap020a00f_get_thresh_reg()

[PATCH] iio: light: gp2ap020a00f: Fix signedness bug in gp2ap020a00f_get_thresh_reg()

Posted by Alper Ak 1 month, 1 week ago

gp2ap020a00f_get_thresh_reg() returns -EINVAL on error,
but it was declared as a u8.

Change the return type to int and update callers to use int type for
the return value and properly check for negative error codes.

Fixes: 5d6a25bad035 ("iio:gp2ap020a00f: Switch to new event config interface")
Signed-off-by: Alper Ak <alperyasinak1@gmail.com>
---
 drivers/iio/light/gp2ap020a00f.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index c7df4b258e2c..9f2441fe8c52 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -992,7 +992,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) {
@@ -1023,12 +1023,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) {
@@ -1080,15 +1086,15 @@ 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) {
-		err = -EINVAL;
+	if (thresh_reg_l < 0) {
+		err = thresh_reg_l;
 		goto error_unlock;
 	}
 
-- 
2.43.0

Re: [PATCH] iio: light: gp2ap020a00f: Fix signedness bug in gp2ap020a00f_get_thresh_reg()

Posted by Jonathan Cameron 1 month, 1 week ago

On Fri, 26 Dec 2025 18:45:21 +0300
Alper Ak <alperyasinak1@gmail.com> wrote:

> gp2ap020a00f_get_thresh_reg() returns -EINVAL on error,
> but it was declared as a u8.
> 
> Change the return type to int and update callers to use int type for
> the return value and properly check for negative error codes.
> 
> Fixes: 5d6a25bad035 ("iio:gp2ap020a00f: Switch to new event config interface")
Hi Alper,

It's not a real bug because the values that can be passed to this
must already be constrained to be ones that will match the non error
cases in that call.

The type issue is worth tidying up though.  A few comments inline.


> Signed-off-by: Alper Ak <alperyasinak1@gmail.com>
> ---
>  drivers/iio/light/gp2ap020a00f.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
> index c7df4b258e2c..9f2441fe8c52 100644
> --- a/drivers/iio/light/gp2ap020a00f.c
> +++ b/drivers/iio/light/gp2ap020a00f.c
> @@ -992,7 +992,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) {
> @@ -1023,12 +1023,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;

This never needed to be initialized as this value is never used
(that matters for suggestion that follows)


>  
>  	mutex_lock(&data->lock);
Given this isn't a fix as such (see above). You could precede it with
a patch using cleanup.h and guard() to handle the locking and remove
the need for a err_unlock; label and gotos.

>  
>  	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
> +

Drop this blank line to keep the cause of the return value and the check
on it tightly coupled. I'd prefer this use err until we know if the value is valid.

	err = gp2ap020a00f_get_thresh_reg(chan, dir);
	if (err < 0)
		goto error_unlock;
	thresh_reg_l = err;

	

> +	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) {
> @@ -1080,15 +1086,15 @@ 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) {
> -		err = -EINVAL;
> +	if (thresh_reg_l < 0) {
Similar comments to above apply here as well.

Thanks

Jonathan

> +		err = thresh_reg_l;
>  		goto error_unlock;
>  	}
>

Re: [PATCH] iio: light: gp2ap020a00f: Fix signedness bug in gp2ap020a00f_get_thresh_reg()

Posted by Andy Shevchenko 1 month, 1 week ago

On Fri, Dec 26, 2025 at 06:45:21PM +0300, Alper Ak wrote:
> gp2ap020a00f_get_thresh_reg() returns -EINVAL on error,
> but it was declared as a u8.
> 
> Change the return type to int and update callers to use int type for
> the return value and properly check for negative error codes.

...

>  	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);

> +

This blank line is redundant.

> +	if (thresh_reg_l < 0) {
> +		err = thresh_reg_l;
> +		goto error_unlock;
> +	}

And entire code can be rewritten in a shorter way
(in this case the type not needed to be changed):

	err = gp2ap020a00f_get_thresh_reg(chan, dir);
	if (err < 0)
		goto error_unlock;
	thresh_reg_l = err;

but this one is up to maintainers as I know Jonathan likes the clear naming
to be assigned to (however when written as above it clear to me what's the
semantics of the non-negative returns).

...

Ditto for the other function.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: light: gp2ap020a00f: Fix signedness bug in gp2ap020a00f_get_thresh_reg()

Posted by Jonathan Cameron 1 month, 1 week ago

On Sat, 27 Dec 2025 17:00:46 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Fri, Dec 26, 2025 at 06:45:21PM +0300, Alper Ak wrote:
> > gp2ap020a00f_get_thresh_reg() returns -EINVAL on error,
> > but it was declared as a u8.
> > 
> > Change the return type to int and update callers to use int type for
> > the return value and properly check for negative error codes.  
> 
> ...
> 
> >  	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);  
> 
> > +  
> 
> This blank line is redundant.
> 
> > +	if (thresh_reg_l < 0) {
> > +		err = thresh_reg_l;
> > +		goto error_unlock;
> > +	}  
> 
> And entire code can be rewritten in a shorter way
> (in this case the type not needed to be changed):
> 
> 	err = gp2ap020a00f_get_thresh_reg(chan, dir);
> 	if (err < 0)
> 		goto error_unlock;
> 	thresh_reg_l = err;
> 
> but this one is up to maintainers as I know Jonathan likes the clear naming
> to be assigned to (however when written as above it clear to me what's the
> semantics of the non-negative returns).

This pattern is absolutely fine.  I don't mind using an err, ret or similar
briefly like this as the real meaning of the successful value is right there
2 lines down

Jonathan

> 
> ...
> 
> Ditto for the other function.
>