Re: [PATCH 11/12] iio: accel: kionix-kx022a: Prevent memory leak and fix state

[Matti Vaittinen]

On 17/08/2026 04:45, Jonathan Cameron wrote:
> On Mon, 10 Aug 2026 10:55:03 +0300
> Matti Vaittinen <matti.vaittinen@xxxxxxxxx> wrote:
>
> > From: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> >
> > The driver allocates memory for samples at buffer enable path. If regmap
> > operation fails in the kx022a_fifo_enable() at the buffer enable path, the
> > allocated memory is never freed. Furthermore, the state information and
> > previous hardware configuration(s) aren't undone, potentially leaving
> > WMI interrupts and buffers enabled, or driver state flags wrong.
> >
> > Free the memory and revert the hardware configuration and state flags on
> > error path.
> >
> > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> > Fixes: e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")
> > ---
> >   drivers/iio/accel/kionix-kx022a.c | 27 ++++++++++++++++++++++-----
> >   1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> > index 8a13f78aeab0..49e8b4b943da 100644
> > --- a/drivers/iio/accel/kionix-kx022a.c
> > +++ b/drivers/iio/accel/kionix-kx022a.c
> > @@ -980,26 +980,43 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
> >   	guard(mutex)(&data->mutex);
> >   	ret = __kx022a_turn_on_off(data, false);
> >   	if (ret)
> > -		return ret;
> > +		goto err_free_out;
>
> If we follow this path we are assuming that turn_on_off hasn't
> had any side effects in failing...
>
>
> >   
> >   	/* Update watermark to HW */
> >   	ret = kx022a_fifo_set_wmi(data);
> >   	if (ret)
> > -		return ret;
> > +		goto err_free_out;
> >   
> >   	/* Enable buffer */
> >   	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
> >   			      KX022A_MASK_BUF_EN);
> >   	if (ret)
> > -		return ret;
> > +		goto err_free_out;
> >   
> >   	data->state |= KX022A_STATE_FIFO;
> >   	ret = regmap_set_bits(data->regmap, data->ien_reg,
> >   			      KX022A_MASK_WMI);
> >   	if (ret)
> > -		return ret;
> > +		goto err_wmi_out;
> >   
> > -	return __kx022a_turn_on_off(data, true);
> > +	ret = __kx022a_turn_on_off(data, true);
> > +	if (ret)
> > +		goto err_on_out;
>
>
> > +
> > +	return ret;
> > +
> > +err_on_out:
> > +	regmap_clear_bits(data->regmap, data->ien_reg,
> > +			  KX022A_MASK_WMI);
> > +err_wmi_out:
> > +	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
> > +			  KX022A_MASK_BUF_EN);
> > +err_free_out:
> > +	kfree(data->fifo_buffer);
>
> This thing is fine here.
>
> > +	data->state &= ~KX022A_STATE_FIFO;
>
> This should also only occur when we have set it in the first place -
> so under err_wmi_out:
>
>
> > +	__kx022a_turn_on_off(data, true);
>
> So following path above we should not be calling this.  It might
> be safe to do so but it isn't logically correct.  It should be a few
> lines earlier.
>
> > +
> > +	return ret;
> >   }
> >   
> >   static int kx022a_buffer_postenable(struct iio_dev *idev)

Thanks Jonathan. I think you're right. I've no idea what I was thinking 
when writing this fix...

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~