[PATCH] iio: mlx90614: fix missing GPIO direction return value checks

[PATCH] iio: mlx90614: fix missing GPIO direction return value checks

[Salah Triki]

The functions gpiod_direction_output() and gpiod_direction_input() can
fail, but their return values were previously ignored.

If an error occurs during the GPIO configuration, the function should
abort the wake-up sequence and return the error code. More importantly,
failing to check these values could lead to the I2C bus remaining
locked if an error occurs after i2c_lock_bus() is called.

Add return value checks and ensure the I2C bus is properly unlocked
via a goto label in case of failure.

Fixes: eb4b07dae4d4 ("iio: mlx90614: Add power management")
Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 drivers/iio/temperature/mlx90614.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index 1ad21b73e1b4..ce12b44c84e0 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -489,6 +489,7 @@ static int mlx90614_sleep(struct mlx90614_data *data)
 static int mlx90614_wakeup(struct mlx90614_data *data)
 {
 	const struct mlx_chip_info *chip_info = data->chip_info;
+	int ret;
 
 	if (!data->wakeup_gpio) {
 		dev_dbg(&data->client->dev, "Wake-up disabled");
@@ -498,9 +499,17 @@ static int mlx90614_wakeup(struct mlx90614_data *data)
 	dev_dbg(&data->client->dev, "Requesting wake-up");
 
 	i2c_lock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
-	gpiod_direction_output(data->wakeup_gpio, 0);
+
+	ret = gpiod_direction_output(data->wakeup_gpio, 0);
+	if (ret)
+		goto out_unlock;
+
 	msleep(chip_info->wakeup_delay_ms);
-	gpiod_direction_input(data->wakeup_gpio);
+
+	ret = gpiod_direction_input(data->wakeup_gpio);
+	if (ret)
+		goto out_unlock;
+
 	i2c_unlock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
 
 	data->ready_timestamp = jiffies +
@@ -515,6 +524,10 @@ static int mlx90614_wakeup(struct mlx90614_data *data)
 	i2c_smbus_read_word_data(data->client, chip_info->op_eeprom_config1);
 
 	return 0;
+
+out_unlock:
+	i2c_unlock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
+	return ret;
 }
 
 /* Return wake-up GPIO or NULL if sleep functionality should be disabled. */
-- 
2.43.0

Re: [PATCH] iio: mlx90614: fix missing GPIO direction return value checks

[Andy Shevchenko]

On Mon, Apr 27, 2026 at 10:58:00PM +0100, Salah Triki wrote:
> The functions gpiod_direction_output() and gpiod_direction_input() can
> fail, but their return values were previously ignored.
> 
> If an error occurs during the GPIO configuration, the function should
> abort the wake-up sequence and return the error code. More importantly,
> failing to check these values could lead to the I2C bus remaining
> locked if an error occurs after i2c_lock_bus() is called.
> 
> Add return value checks and ensure the I2C bus is properly unlocked
> via a goto label in case of failure.

...

> -	gpiod_direction_output(data->wakeup_gpio, 0);
> +
> +	ret = gpiod_direction_output(data->wakeup_gpio, 0);
> +	if (ret)
> +		goto out_unlock;
> +
>  	msleep(chip_info->wakeup_delay_ms);
> -	gpiod_direction_input(data->wakeup_gpio);
> +
> +	ret = gpiod_direction_input(data->wakeup_gpio);
> +	if (ret)
> +		goto out_unlock;

While technically it sounds correct, the potential problem here is that you may
fail this in case CONFIG_GPIOLIB=n. Is this GPIO optional?
What may happen if GPIO is not optional, but for some reason setting it fails?

TL;DR:
I am not sure about this patch. At least I'm not comfortable to take it without
testing on real HW.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: mlx90614: fix missing GPIO direction return value checks

[Salah Triki]

On Tue, Apr 28, 2026 at 11:00:04AM +0300, Andy Shevchenko wrote:
> On Mon, Apr 27, 2026 at 10:58:00PM +0100, Salah Triki wrote:
> > The functions gpiod_direction_output() and gpiod_direction_input() can
> > fail, but their return values were previously ignored.
> > 
> > If an error occurs during the GPIO configuration, the function should
> > abort the wake-up sequence and return the error code. More importantly,
> > failing to check these values could lead to the I2C bus remaining
> > locked if an error occurs after i2c_lock_bus() is called.
> > 
> > Add return value checks and ensure the I2C bus is properly unlocked
> > via a goto label in case of failure.
> 
> ...
> 
> > -	gpiod_direction_output(data->wakeup_gpio, 0);
> > +
> > +	ret = gpiod_direction_output(data->wakeup_gpio, 0);
> > +	if (ret)
> > +		goto out_unlock;
> > +
> >  	msleep(chip_info->wakeup_delay_ms);
> > -	gpiod_direction_input(data->wakeup_gpio);
> > +
> > +	ret = gpiod_direction_input(data->wakeup_gpio);
> > +	if (ret)
> > +		goto out_unlock;
> 
> While technically it sounds correct, the potential problem here is that you may
> fail this in case CONFIG_GPIOLIB=n. Is this GPIO optional?
> What may happen if GPIO is not optional, but for some reason setting it fails?
> 
> TL;DR:
> I am not sure about this patch. At least I'm not comfortable to take it without
> testing on real HW.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Thank you for your feedback. Since I don't have the physical hardware to 
perform the necessary tests and ensure there are no regressions, I agree
that it is safer to drop this patch. I don't want to risk breaking the 
driver for a minor cleanup.

Best regards,
--
Salah Triki

Re: [PATCH] iio: mlx90614: fix missing GPIO direction return value checks

[Jonathan Cameron]

On Tue, 28 Apr 2026 09:38:47 +0100
Salah Triki <salah.triki@gmail.com> wrote:

> On Tue, Apr 28, 2026 at 11:00:04AM +0300, Andy Shevchenko wrote:
> > On Mon, Apr 27, 2026 at 10:58:00PM +0100, Salah Triki wrote:  
> > > The functions gpiod_direction_output() and gpiod_direction_input() can
> > > fail, but their return values were previously ignored.
> > > 
> > > If an error occurs during the GPIO configuration, the function should
> > > abort the wake-up sequence and return the error code. More importantly,
> > > failing to check these values could lead to the I2C bus remaining
> > > locked if an error occurs after i2c_lock_bus() is called.
> > > 
> > > Add return value checks and ensure the I2C bus is properly unlocked
> > > via a goto label in case of failure.  
> > 
> > ...
> >   
> > > -	gpiod_direction_output(data->wakeup_gpio, 0);
> > > +
> > > +	ret = gpiod_direction_output(data->wakeup_gpio, 0);
> > > +	if (ret)
> > > +		goto out_unlock;
> > > +
> > >  	msleep(chip_info->wakeup_delay_ms);
> > > -	gpiod_direction_input(data->wakeup_gpio);
> > > +
> > > +	ret = gpiod_direction_input(data->wakeup_gpio);
> > > +	if (ret)
> > > +		goto out_unlock;  
> > 
> > While technically it sounds correct, the potential problem here is that you may
> > fail this in case CONFIG_GPIOLIB=n. Is this GPIO optional?
> > What may happen if GPIO is not optional, but for some reason setting it fails?
> > 
> > TL;DR:
> > I am not sure about this patch. At least I'm not comfortable to take it without
> > testing on real HW.
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> >   
> 
> Thank you for your feedback. Since I don't have the physical hardware to 
> perform the necessary tests and ensure there are no regressions, I agree
> that it is safer to drop this patch. I don't want to risk breaking the 
> driver for a minor cleanup.
> 
Maybe Crt has the hardware to test. For now I'll assume this is not
going anywhere wrt to tracking in patchwork.

> Best regards,
> --
> Salah Triki

Re: [PATCH] iio: mlx90614: fix missing GPIO direction return value checks

[Crt Mori]

On Tue, 28 Apr 2026 at 18:04, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 28 Apr 2026 09:38:47 +0100
> Salah Triki <salah.triki@gmail.com> wrote:
>
> > On Tue, Apr 28, 2026 at 11:00:04AM +0300, Andy Shevchenko wrote:
> > > On Mon, Apr 27, 2026 at 10:58:00PM +0100, Salah Triki wrote:
> > > > The functions gpiod_direction_output() and gpiod_direction_input() can
> > > > fail, but their return values were previously ignored.
> > > >
> > > > If an error occurs during the GPIO configuration, the function should
> > > > abort the wake-up sequence and return the error code. More importantly,
> > > > failing to check these values could lead to the I2C bus remaining
> > > > locked if an error occurs after i2c_lock_bus() is called.
> > > >
> > > > Add return value checks and ensure the I2C bus is properly unlocked
> > > > via a goto label in case of failure.
> > >
> > > ...
> > >
> > > > - gpiod_direction_output(data->wakeup_gpio, 0);
> > > > +
> > > > + ret = gpiod_direction_output(data->wakeup_gpio, 0);
> > > > + if (ret)
> > > > +         goto out_unlock;
> > > > +
> > > >   msleep(chip_info->wakeup_delay_ms);
> > > > - gpiod_direction_input(data->wakeup_gpio);
> > > > +
> > > > + ret = gpiod_direction_input(data->wakeup_gpio);
> > > > + if (ret)
> > > > +         goto out_unlock;
> > >
> > > While technically it sounds correct, the potential problem here is that you may
> > > fail this in case CONFIG_GPIOLIB=n. Is this GPIO optional?
> > > What may happen if GPIO is not optional, but for some reason setting it fails?
> > >
> > > TL;DR:
> > > I am not sure about this patch. At least I'm not comfortable to take it without
> > > testing on real HW.
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> >
> > Thank you for your feedback. Since I don't have the physical hardware to
> > perform the necessary tests and ensure there are no regressions, I agree
> > that it is safer to drop this patch. I don't want to risk breaking the
> > driver for a minor cleanup.
> >
> Maybe Crt has the hardware to test. For now I'll assume this is not
> going anywhere wrt to tracking in patchwork.
>
I will dig the hardware to test as it was off my desk for some time
now. It seems like backwards compatible, but I am wondering why would
we want to be informed of this failing? The problem is that sensor
uses wakeup line (same as for i2c) to receive wakeup pulse, so in
theory this is already a workaround. @Salah did you see any issues in
the wild?

> > Best regards,
> > --
> > Salah Triki
>

Re: [PATCH] iio: mlx90614: fix missing GPIO direction return value checks

[Jonathan Cameron]

On Mon, 27 Apr 2026 22:58:00 +0100
Salah Triki <salah.triki@gmail.com> wrote:

> The functions gpiod_direction_output() and gpiod_direction_input() can
> fail, but their return values were previously ignored.
> 
> If an error occurs during the GPIO configuration, the function should
> abort the wake-up sequence and return the error code. More importantly,
> failing to check these values could lead to the I2C bus remaining
> locked if an error occurs after i2c_lock_bus() is called.
> 
> Add return value checks and ensure the I2C bus is properly unlocked
> via a goto label in case of failure.
> 
> Fixes: eb4b07dae4d4 ("iio: mlx90614: Add power management")
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
>  drivers/iio/temperature/mlx90614.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index 1ad21b73e1b4..ce12b44c84e0 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -489,6 +489,7 @@ static int mlx90614_sleep(struct mlx90614_data *data)
>  static int mlx90614_wakeup(struct mlx90614_data *data)
>  {
>  	const struct mlx_chip_info *chip_info = data->chip_info;
> +	int ret;
>  
>  	if (!data->wakeup_gpio) {
>  		dev_dbg(&data->client->dev, "Wake-up disabled");
> @@ -498,9 +499,17 @@ static int mlx90614_wakeup(struct mlx90614_data *data)
>  	dev_dbg(&data->client->dev, "Requesting wake-up");
>  
>  	i2c_lock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
> -	gpiod_direction_output(data->wakeup_gpio, 0);
> +
> +	ret = gpiod_direction_output(data->wakeup_gpio, 0);
> +	if (ret)
> +		goto out_unlock;
> +
>  	msleep(chip_info->wakeup_delay_ms);
> -	gpiod_direction_input(data->wakeup_gpio);
If this did go forwards, I'd prefer the section under the lock moved
into a helper function so that we can do

	ret = helper();
	i2c_unlock_bus();
	if (ret)
		return ret;

Rather than introducing a goto for a section of code in the middle of
the function that isn't appropriate if we were to later introduce some
error path after this.


> +
> +	ret = gpiod_direction_input(data->wakeup_gpio);
> +	if (ret)
> +		goto out_unlock;
> +
>  	i2c_unlock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
>  
>  	data->ready_timestamp = jiffies +
> @@ -515,6 +524,10 @@ static int mlx90614_wakeup(struct mlx90614_data *data)
>  	i2c_smbus_read_word_data(data->client, chip_info->op_eeprom_config1);
>  
>  	return 0;
> +
> +out_unlock:
> +	i2c_unlock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
> +	return ret;
>  }
>  
>  /* Return wake-up GPIO or NULL if sleep functionality should be disabled. */