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

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 17/08/2026 04:45, Jonathan Cameron wrote:
> On Mon, 10 Aug 2026 10:55:03 +0300
> Matti Vaittinen <matti.vaittinen@linux.dev> wrote:
> 
>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> 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@gmail.com>
>> 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! ~~