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

[Jonathan Cameron <jic23@kernel.org>]

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