Re: [PATCH v2] iio: adc: at91_adc: move to devm_* to fix NULL pointer deref

[Andy Shevchenko]

On Tue, Nov 10, 2026 at 11:28:48AM +0200, Abdelnasser Hussein wrote:
> If 'atmel,adc-use-external-triggers' is not set in the device tree,
> at91_adc_trigger_init() leaves the external trigger slots in st->trig
> as NULL. The error paths and remove function unconditionally called
> iio_trigger_unregister() and iio_trigger_free() on them, causing a
> NULL pointer dereference.
> 
> Instead of just checking for NULL, refactor the driver to use the
> device-managed (devm_*) API for registration and allocation. This
> modernizes the code, eliminates the need for manual cleanup functions
> (at91_adc_trigger_remove, at91_adc_buffer_remove, at91_ts_unregister,
> and at91_adc_remove), and inherently fixes the NULL pointer dereference
> since the core handles the cleanup of properly allocated triggers.
> 
> As part of this simplification, the remaining trigger initialization
> logic has been inlined into at91_adc_probe().

...

> static int at91_ts_register(struct iio_dev *idev,

>  {
>  	struct at91_adc_state *st = iio_priv(idev);
>  	struct input_dev *input;
> -	int ret;
>  
> -	input = input_allocate_device();
> +	input = devm_input_allocate_device(&pdev->dev);
>  	if (!input) {
>  		dev_err(&idev->dev, "Failed to allocate TS device!\n");

Side note:
This message needs to be killed, we don't print -ENOMEM errors.

>  		return -ENOMEM;

>  		if (st->touchscreen_type != ATMEL_ADC_TOUCHSCREEN_4WIRE) {
>  			dev_err(&pdev->dev,
>  				"This touchscreen controller only support 4 wires\n");
> -			ret = -EINVAL;
> -			goto err;
> +			return -EINVAL;

Another side note: this should move to dev_err_probe() (along with similar
cases) at some point.

>  		}
>  
>  		input_set_abs_params(input, ABS_X, 0, (1 << MAX_RLPOS_BITS) - 1,

...

>  static int at91_adc_probe(struct platform_device *pdev)
>  {
>  	unsigned int prsc, mstrclk, ticks, adc_clk, adc_clk_khz, shtim;
>  	struct device_node *node = pdev->dev.of_node;
> -	int ret;
> +	int i, ret;

No, don't add it here as it's local to the for-loop.

>  	struct iio_dev *idev;
>  	struct at91_adc_state *st;
>  	u32 reg, prop;

> -		ret = at91_adc_trigger_init(idev);
> -		if (ret < 0) {
> -			dev_err(&pdev->dev, "Couldn't setup the triggers.\n");
> -			at91_adc_buffer_remove(idev);
> -			return ret;

> +		st->trig = devm_kcalloc(&pdev->dev,

Add at the top

	struct device *dev = &pdev->dev;

and use in the code you modified or add.

> +					st->caps->trigger_number, sizeof(*st->trig),
> +					GFP_KERNEL);
> +		if (!st->trig)
> +			return -ENOMEM;

> +		for (i = 0; i < st->caps->trigger_number; i++) {

		for (int i = 0; i < st->caps->trigger_number; i++) {
		// align the type of i based on the type of trigger_number

> +			if (st->caps->triggers[i].is_external && !(st->use_external))
> +				continue;
> +
> +			st->trig[i] = at91_adc_allocate_trigger(idev,
> +								st->caps->triggers + i);
> +			if (!st->trig[i]) {
> +				dev_err(&idev->dev,
> +					"Could not allocate trigger %d\n", i);
> +				return -ENOMEM;
> +			}
>  		}

-- 
With Best Regards,
Andy Shevchenko