[PATCH v4 2/2] iio: trigger: fix device initialization order in viio_trigger_alloc

[PATCH v4 2/2] iio: trigger: fix device initialization order in viio_trigger_alloc

[Ma Ke]

Move device initialization to the end of viio_trigger_alloc() to
simplify error handling. This follows the pattern used in similar
functions like spi_alloc_device(), where device_initialize() is called
only after all resources have been successfully allocated.

This change eliminates the need for complex cleanup in error paths and
ensures that the device release callback only runs when the device was
fully initialized.

By moving device_initialize() after all resource allocations, we can
use simple kfree() in error paths instead of put_device(), making the
code more straightforward and less error-prone.

Found by code review.

Suggested-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
Changes in v4:
- split the patch into two independent patches and modified according to developer's suggestions;
Changes in v3:
- modified the patch;
Changes in v2:
- modified the patch, thanks for developer's suggestions.
---
 drivers/iio/industrialio-trigger.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 5baa83349e8f..760ae3e60639 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -562,12 +562,6 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
 	if (!trig)
 		return NULL;
 
-	trig->dev.parent = parent;
-	trig->dev.type = &iio_trig_type;
-	trig->dev.bus = &iio_bus_type;
-	device_initialize(&trig->dev);
-	INIT_WORK(&trig->reenable_work, iio_reenable_work_fn);
-
 	mutex_init(&trig->pool_lock);
 	trig->subirq_base = irq_alloc_descs(-1, 0,
 					    CONFIG_IIO_CONSUMERS_PER_TRIGGER,
@@ -593,6 +587,13 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
 				  IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
 	}
 
+	/* Initialize device only after all resources are allocated */
+	trig->dev.parent = parent;
+	trig->dev.type = &iio_trig_type;
+	trig->dev.bus = &iio_bus_type;
+	device_initialize(&trig->dev);
+	INIT_WORK(&trig->reenable_work, iio_reenable_work_fn);
+
 	return trig;
 
 free_descs:
-- 
2.17.1

Re: [PATCH v4 2/2] iio: trigger: fix device initialization order in viio_trigger_alloc

[Andy Shevchenko]

On Mon, Nov 10, 2025 at 11:58:38AM +0800, Ma Ke wrote:
> Move device initialization to the end of viio_trigger_alloc() to
> simplify error handling. This follows the pattern used in similar
> functions like spi_alloc_device(), where device_initialize() is called
> only after all resources have been successfully allocated.
> 
> This change eliminates the need for complex cleanup in error paths and
> ensures that the device release callback only runs when the device was
> fully initialized.
> 
> By moving device_initialize() after all resource allocations, we can
> use simple kfree() in error paths instead of put_device(), making the
> code more straightforward and less error-prone.
> 
> Found by code review.


Thanks for the update, my comments below.

...

> -	trig->dev.parent = parent;
> -	trig->dev.type = &iio_trig_type;
> -	trig->dev.bus = &iio_bus_type;
> -	device_initialize(&trig->dev);
> -	INIT_WORK(&trig->reenable_work, iio_reenable_work_fn);


> +	/* Initialize device only after all resources are allocated */
> +	trig->dev.parent = parent;
> +	trig->dev.type = &iio_trig_type;
> +	trig->dev.bus = &iio_bus_type;
> +	device_initialize(&trig->dev);

> +	INIT_WORK(&trig->reenable_work, iio_reenable_work_fn);

Why has this been moved?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] iio: trigger: fix device initialization order in viio_trigger_alloc

[Nuno Sá]

On Mon, 2025-11-10 at 11:58 +0800, Ma Ke wrote:
> Move device initialization to the end of viio_trigger_alloc() to
> simplify error handling. This follows the pattern used in similar
> functions like spi_alloc_device(), where device_initialize() is called
> only after all resources have been successfully allocated.
> 
> This change eliminates the need for complex cleanup in error paths and
> ensures that the device release callback only runs when the device was
> fully initialized.
> 
> By moving device_initialize() after all resource allocations, we can
> use simple kfree() in error paths instead of put_device(), making the
> code more straightforward and less error-prone.
> 
> Found by code review.
> 
> Suggested-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---

Have the same question Andy has... With that addressed:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

> Changes in v4:
> - split the patch into two independent patches and modified according to developer's suggestions;
> Changes in v3:
> - modified the patch;
> Changes in v2:
> - modified the patch, thanks for developer's suggestions.
> ---
>  drivers/iio/industrialio-trigger.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 5baa83349e8f..760ae3e60639 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -562,12 +562,6 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
>  	if (!trig)
>  		return NULL;
>  
> -	trig->dev.parent = parent;
> -	trig->dev.type = &iio_trig_type;
> -	trig->dev.bus = &iio_bus_type;
> -	device_initialize(&trig->dev);
> -	INIT_WORK(&trig->reenable_work, iio_reenable_work_fn);
> -
>  	mutex_init(&trig->pool_lock);
>  	trig->subirq_base = irq_alloc_descs(-1, 0,
>  					    CONFIG_IIO_CONSUMERS_PER_TRIGGER,
> @@ -593,6 +587,13 @@ struct iio_trigger *viio_trigger_alloc(struct device *parent,
>  				  IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
>  	}
>  
> +	/* Initialize device only after all resources are allocated */
> +	trig->dev.parent = parent;
> +	trig->dev.type = &iio_trig_type;
> +	trig->dev.bus = &iio_bus_type;
> +	device_initialize(&trig->dev);
> +	INIT_WORK(&trig->reenable_work, iio_reenable_work_fn);
> +
>  	return trig;
>  
>  free_descs: