Re: [PATCH v2 3/4] iio: accel: kionix-kx022a: Prevent memory leak and fix state

[Jonathan Cameron]

> 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.
> 
> Fixes: e7123a4dfcd7 ("iio: accel: kionix-kx022a: Refactor driver and add chip_info structure")
> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> Reviewed-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxxxxxx>

I'm not happy with the label naming.  It had me very confused!

>
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 02dd1db7a646..42bac6d724a0 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -980,26 +980,44 @@ 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;
Label naming wise - this one is talking about what needs doing (works for me!)
>  
>  	/* Update watermark to HW */
>  	ret = kx022a_fifo_set_wmi(data);
>  	if (ret)
> -		return ret;
> +		goto err_wmi_out;

This one is talkign about why we went there.  For consistency would
need to be something about turning off not this.

>  
>  	/* Enable buffer */
>  	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
>  			      KX022A_MASK_BUF_EN);
>  	if (ret)
> -		return ret;
> +		goto err_wmi_out;

This I'd kind of expect to undo the set wmi but it doesn't...

>  
>  	data->state |= KX022A_STATE_FIFO;
>  	ret = regmap_set_bits(data->regmap, data->ien_reg,
>  			      KX022A_MASK_WMI);
>  	if (ret)
> -		return ret;
> +		goto err_buf_en_out;

This one is back to what we are undoing (good).

>  
> -	return __kx022a_turn_on_off(data, true);
> +	ret = __kx022a_turn_on_off(data, true);
> +	if (ret)
> +		goto err_on_out;

This is another where we came form, not where we are going to.

> +
> +	return ret;
> +
> +err_on_out:
> +	regmap_clear_bits(data->regmap, data->ien_reg,
> +			  KX022A_MASK_WMI);
> +err_buf_en_out:
> +	regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
> +			  KX022A_MASK_BUF_EN);
> +	data->state &= ~KX022A_STATE_FIFO;
> +err_wmi_out:
> +	__kx022a_turn_on_off(data, true);
> +err_free_out:
> +	kfree(data->fifo_buffer);
> +
> +	return ret;
>  }
>  
>  static int kx022a_buffer_postenable(struct iio_dev *idev)
Thanks,

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>