[PATCH] soc: fsl: dpio: Use common error handling code in two functions

[PATCH] soc: fsl: dpio: Use common error handling code in two functions

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 11 Jun 2026 11:56:18 +0200

Use additional labels so that a bit of exception handling can be better
reused at the end of two function implementations.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/soc/fsl/dpio/dpio-service.c | 30 ++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/soc/fsl/dpio/dpio-service.c b/drivers/soc/fsl/dpio/dpio-service.c
index 317ca50b0c2b..025d9fb5660d 100644
--- a/drivers/soc/fsl/dpio/dpio-service.c
+++ b/drivers/soc/fsl/dpio/dpio-service.c
@@ -140,10 +140,8 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
 		return NULL;
 
 	/* check if CPU is out of range (-1 means any cpu) */
-	if (desc->cpu != DPAA2_IO_ANY_CPU && desc->cpu >= num_possible_cpus()) {
-		kfree(obj);
-		return NULL;
-	}
+	if (desc->cpu != DPAA2_IO_ANY_CPU && desc->cpu >= num_possible_cpus())
+		goto free_obj;
 
 	obj->dpio_desc = *desc;
 	obj->swp_desc.cena_bar = obj->dpio_desc.regs_cena;
@@ -158,11 +156,8 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
 	qman_256_cycles_per_ns = 256000 / (obj->swp_desc.qman_clk / 1000000);
 	obj->swp_desc.qman_256_cycles_per_ns = qman_256_cycles_per_ns;
 	obj->swp = qbman_swp_init(&obj->swp_desc);
-
-	if (!obj->swp) {
-		kfree(obj);
-		return NULL;
-	}
+	if (!obj->swp)
+		goto free_obj;
 
 	INIT_LIST_HEAD(&obj->node);
 	spin_lock_init(&obj->lock_mgmt_cmd);
@@ -192,6 +187,10 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
 	obj->frames = 0;
 
 	return obj;
+
+free_obj:
+	kfree(obj);
+	return NULL;
 }
 
 /**
@@ -665,10 +664,8 @@ struct dpaa2_io_store *dpaa2_io_store_create(unsigned int max_frames,
 	ret->max = max_frames;
 	size = max_frames * sizeof(struct dpaa2_dq) + 64;
 	ret->alloced_addr = kzalloc(size, GFP_KERNEL);
-	if (!ret->alloced_addr) {
-		kfree(ret);
-		return NULL;
-	}
+	if (!ret->alloced_addr)
+		goto free_ret;
 
 	ret->vaddr = PTR_ALIGN(ret->alloced_addr, 64);
 	ret->paddr = dma_map_single(dev, ret->vaddr,
@@ -676,14 +673,17 @@ struct dpaa2_io_store *dpaa2_io_store_create(unsigned int max_frames,
 				    DMA_FROM_DEVICE);
 	if (dma_mapping_error(dev, ret->paddr)) {
 		kfree(ret->alloced_addr);
-		kfree(ret);
-		return NULL;
+		goto free_ret;
 	}
 
 	ret->idx = 0;
 	ret->dev = dev;
 
 	return ret;
+
+free_ret:
+	kfree(ret);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(dpaa2_io_store_create);
 
-- 
2.54.0

Re: [PATCH] soc: fsl: dpio: Use common error handling code in two functions

[Christophe Leroy (CS GROUP)]

Le 11/06/2026 à 12:02, Markus Elfring a écrit :
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 11 Jun 2026 11:56:18 +0200
> 
> Use additional labels so that a bit of exception handling can be better
> reused at the end of two function implementations.
> 
> This issue was detected by using the Coccinelle software.

I think it would be better to declare obj and ret as automatic free instead:

	struct dpaa2_io __free(kfree) *obj = kmalloc_obj(*obj);



> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/soc/fsl/dpio/dpio-service.c | 30 ++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/fsl/dpio/dpio-service.c b/drivers/soc/fsl/dpio/dpio-service.c
> index 317ca50b0c2b..025d9fb5660d 100644
> --- a/drivers/soc/fsl/dpio/dpio-service.c
> +++ b/drivers/soc/fsl/dpio/dpio-service.c
> @@ -140,10 +140,8 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
>   		return NULL;
>   
>   	/* check if CPU is out of range (-1 means any cpu) */
> -	if (desc->cpu != DPAA2_IO_ANY_CPU && desc->cpu >= num_possible_cpus()) {
> -		kfree(obj);
> -		return NULL;
> -	}
> +	if (desc->cpu != DPAA2_IO_ANY_CPU && desc->cpu >= num_possible_cpus())
> +		goto free_obj;
>   
>   	obj->dpio_desc = *desc;
>   	obj->swp_desc.cena_bar = obj->dpio_desc.regs_cena;
> @@ -158,11 +156,8 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
>   	qman_256_cycles_per_ns = 256000 / (obj->swp_desc.qman_clk / 1000000);
>   	obj->swp_desc.qman_256_cycles_per_ns = qman_256_cycles_per_ns;
>   	obj->swp = qbman_swp_init(&obj->swp_desc);
> -
> -	if (!obj->swp) {
> -		kfree(obj);
> -		return NULL;
> -	}
> +	if (!obj->swp)
> +		goto free_obj;
>   
>   	INIT_LIST_HEAD(&obj->node);
>   	spin_lock_init(&obj->lock_mgmt_cmd);
> @@ -192,6 +187,10 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
>   	obj->frames = 0;
>   
>   	return obj;
> +
> +free_obj:
> +	kfree(obj);
> +	return NULL;
>   }
>   
>   /**
> @@ -665,10 +664,8 @@ struct dpaa2_io_store *dpaa2_io_store_create(unsigned int max_frames,
>   	ret->max = max_frames;
>   	size = max_frames * sizeof(struct dpaa2_dq) + 64;
>   	ret->alloced_addr = kzalloc(size, GFP_KERNEL);
> -	if (!ret->alloced_addr) {
> -		kfree(ret);
> -		return NULL;
> -	}
> +	if (!ret->alloced_addr)
> +		goto free_ret;
>   
>   	ret->vaddr = PTR_ALIGN(ret->alloced_addr, 64);
>   	ret->paddr = dma_map_single(dev, ret->vaddr,
> @@ -676,14 +673,17 @@ struct dpaa2_io_store *dpaa2_io_store_create(unsigned int max_frames,
>   				    DMA_FROM_DEVICE);
>   	if (dma_mapping_error(dev, ret->paddr)) {
>   		kfree(ret->alloced_addr);
> -		kfree(ret);
> -		return NULL;
> +		goto free_ret;
>   	}
>   
>   	ret->idx = 0;
>   	ret->dev = dev;
>   
>   	return ret;
> +
> +free_ret:
> +	kfree(ret);
> +	return NULL;
>   }
>   EXPORT_SYMBOL_GPL(dpaa2_io_store_create);
>   

Re: [PATCH] soc: fsl: dpio: Use common error handling code in two functions

[Markus Elfring]

>> Use additional labels so that a bit of exception handling can be better
>> reused at the end of two function implementations.
>>
>> This issue was detected by using the Coccinelle software.
> 
> I think it would be better to declare obj and ret as automatic free instead:

Please reconsider such information in more detail.


>     struct dpaa2_io __free(kfree) *obj = kmalloc_obj(*obj);

The application of scope-base resource management can eventually be increased further.

Regards,
Markus

[PATCH v2 0/2] soc: fsl: dpio: Use scope-based resource management in two functions

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 6 Jul 2026 11:42:10 +0200

Some adjustment opportunities were picked up.

Markus Elfring (2):
  Use scope-based resource management in dpaa2_io_store_create()
  Use scope-based resource management in dpaa2_io_create()


v2:
Christophe Leroy requested to apply the attribute “__free(kfree)”.


 drivers/soc/fsl/dpio/dpio-service.c | 37 ++++++++++-------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

-- 
2.54.0

[PATCH v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 6 Jul 2026 10:34:43 +0200

Scope-based resource management became supported for some
programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
Introduce __cleanup() based infrastructure").

* Thus use the attribute “__free(kfree)”.

* Reduce the scope for the local variable “ret”.

* Omit two kfree() calls accordingly.

* Omit the local variable “size” (for another memory allocation).

* Use the macro call “return_ptr(ret)” at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/soc/fsl/dpio/dpio-service.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/soc/fsl/dpio/dpio-service.c b/drivers/soc/fsl/dpio/dpio-service.c
index 317ca50b0c2b..b252c3c7fa65 100644
--- a/drivers/soc/fsl/dpio/dpio-service.c
+++ b/drivers/soc/fsl/dpio/dpio-service.c
@@ -652,23 +652,17 @@ EXPORT_SYMBOL_GPL(dpaa2_io_service_acquire);
 struct dpaa2_io_store *dpaa2_io_store_create(unsigned int max_frames,
 					     struct device *dev)
 {
-	struct dpaa2_io_store *ret;
-	size_t size;
-
 	if (!max_frames || (max_frames > 32))
 		return NULL;
 
-	ret = kmalloc_obj(*ret);
+	struct dpaa2_io_store *ret __free(kfree) = kmalloc_obj(*ret);
 	if (!ret)
 		return NULL;
 
 	ret->max = max_frames;
-	size = max_frames * sizeof(struct dpaa2_dq) + 64;
-	ret->alloced_addr = kzalloc(size, GFP_KERNEL);
-	if (!ret->alloced_addr) {
-		kfree(ret);
+	ret->alloced_addr = kzalloc(max_frames * sizeof(struct dpaa2_dq) + 64, GFP_KERNEL);
+	if (!ret->alloced_addr)
 		return NULL;
-	}
 
 	ret->vaddr = PTR_ALIGN(ret->alloced_addr, 64);
 	ret->paddr = dma_map_single(dev, ret->vaddr,
@@ -676,14 +670,13 @@ struct dpaa2_io_store *dpaa2_io_store_create(unsigned int max_frames,
 				    DMA_FROM_DEVICE);
 	if (dma_mapping_error(dev, ret->paddr)) {
 		kfree(ret->alloced_addr);
-		kfree(ret);
 		return NULL;
 	}
 
 	ret->idx = 0;
 	ret->dev = dev;
 
-	return ret;
+	return_ptr(ret);
 }
 EXPORT_SYMBOL_GPL(dpaa2_io_store_create);
 
-- 
2.54.0

[PATCH v2 2/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_create()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 6 Jul 2026 11:18:39 +0200

Scope-based resource management became supported for some
programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
Introduce __cleanup() based infrastructure").

* Thus use the attribute “__free(kfree)”.

* Omit two kfree() calls accordingly.

* Reduce the scopes for the local variables “obj”
  and “qman_256_cycles_per_ns”.

* Use the macro call “return_ptr(obj)” at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/soc/fsl/dpio/dpio-service.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/soc/fsl/dpio/dpio-service.c b/drivers/soc/fsl/dpio/dpio-service.c
index b252c3c7fa65..80fb11206160 100644
--- a/drivers/soc/fsl/dpio/dpio-service.c
+++ b/drivers/soc/fsl/dpio/dpio-service.c
@@ -133,17 +133,14 @@ static void dpaa2_io_dim_work(struct work_struct *w)
 struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
 				 struct device *dev)
 {
-	struct dpaa2_io *obj = kmalloc_obj(*obj);
-	u32 qman_256_cycles_per_ns;
-
-	if (!obj)
+	/* check if CPU is out of range (-1 means any cpu) */
+	if (desc->cpu != DPAA2_IO_ANY_CPU && desc->cpu >= num_possible_cpus())
 		return NULL;
 
-	/* check if CPU is out of range (-1 means any cpu) */
-	if (desc->cpu != DPAA2_IO_ANY_CPU && desc->cpu >= num_possible_cpus()) {
-		kfree(obj);
+	struct dpaa2_io *obj __free(kfree) = kmalloc_obj(*obj);
+
+	if (!obj)
 		return NULL;
-	}
 
 	obj->dpio_desc = *desc;
 	obj->swp_desc.cena_bar = obj->dpio_desc.regs_cena;
@@ -155,14 +152,11 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
 	 * the interrupt timeout period register needs to be specified in QBMAN
 	 * clock cycles in increments of 256.
 	 */
-	qman_256_cycles_per_ns = 256000 / (obj->swp_desc.qman_clk / 1000000);
+	u32 qman_256_cycles_per_ns = 256000 / (obj->swp_desc.qman_clk / 1000000);
 	obj->swp_desc.qman_256_cycles_per_ns = qman_256_cycles_per_ns;
 	obj->swp = qbman_swp_init(&obj->swp_desc);
-
-	if (!obj->swp) {
-		kfree(obj);
+	if (!obj->swp)
 		return NULL;
-	}
 
 	INIT_LIST_HEAD(&obj->node);
 	spin_lock_init(&obj->lock_mgmt_cmd);
@@ -191,7 +185,7 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc,
 	obj->bytes = 0;
 	obj->frames = 0;
 
-	return obj;
+	return_ptr(obj);
 }
 
 /**
-- 
2.54.0

Re: [PATCH v2 0/2] soc: fsl: dpio: Use scope-based resource management in two functions

[Christophe Leroy (CS GROUP)]

Hi Ioana,

Le 06/07/2026 à 12:36, Markus Elfring a écrit :
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 6 Jul 2026 11:42:10 +0200
> 
> Some adjustment opportunities were picked up.
> 
> Markus Elfring (2):
>    Use scope-based resource management in dpaa2_io_store_create()
>    Use scope-based resource management in dpaa2_io_create()
> 

As the (new) maintainer of drivers/soc/fsl/dpio, could you have a look 
at this series ?

Thanks
Christophe

> 
> v2:
> Christophe Leroy requested to apply the attribute “__free(kfree)”.
> 
> 
>   drivers/soc/fsl/dpio/dpio-service.c | 37 ++++++++++-------------------
>   1 file changed, 12 insertions(+), 25 deletions(-)
> 

Re: [PATCH v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Dan Carpenter]

On Mon, Jul 06, 2026 at 12:38:08PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 6 Jul 2026 10:34:43 +0200
> 
> Scope-based resource management became supported for some
> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
> Introduce __cleanup() based infrastructure").
> 
> * Thus use the attribute “__free(kfree)”.
> 
> * Reduce the scope for the local variable “ret”.
> 
> * Omit two kfree() calls accordingly.
> 
> * Omit the local variable “size” (for another memory allocation).

Why?  This seems unrelated...

> 
> * Use the macro call “return_ptr(ret)” at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/soc/fsl/dpio/dpio-service.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/soc/fsl/dpio/dpio-service.c b/drivers/soc/fsl/dpio/dpio-service.c
> index 317ca50b0c2b..b252c3c7fa65 100644
> --- a/drivers/soc/fsl/dpio/dpio-service.c
> +++ b/drivers/soc/fsl/dpio/dpio-service.c
> @@ -652,23 +652,17 @@ EXPORT_SYMBOL_GPL(dpaa2_io_service_acquire);

Missing #include <linux/cleanup.h>

>  struct dpaa2_io_store *dpaa2_io_store_create(unsigned int max_frames,
>  					     struct device *dev)
>  {
> -	struct dpaa2_io_store *ret;
> -	size_t size;
> -
>  	if (!max_frames || (max_frames > 32))
>  		return NULL;
>  
> -	ret = kmalloc_obj(*ret);
> +	struct dpaa2_io_store *ret __free(kfree) = kmalloc_obj(*ret);
>  	if (!ret)
>  		return NULL;
>  
>  	ret->max = max_frames;
> -	size = max_frames * sizeof(struct dpaa2_dq) + 64;
> -	ret->alloced_addr = kzalloc(size, GFP_KERNEL);
> -	if (!ret->alloced_addr) {
> -		kfree(ret);
> +	ret->alloced_addr = kzalloc(max_frames * sizeof(struct dpaa2_dq) + 64, GFP_KERNEL);
> +	if (!ret->alloced_addr)
>  		return NULL;

Why convert the ret allocation but not the ret->alloced_addr allocation?

regards,
dan carpenter

> -	}
>  
>  	ret->vaddr = PTR_ALIGN(ret->alloced_addr, 64);
>  	ret->paddr = dma_map_single(dev, ret->vaddr,
> @@ -676,14 +670,13 @@ struct dpaa2_io_store *dpaa2_io_store_create(unsigned int max_frames,
>  				    DMA_FROM_DEVICE);
>  	if (dma_mapping_error(dev, ret->paddr)) {
>  		kfree(ret->alloced_addr);
> -		kfree(ret);
>  		return NULL;
>  	}
>  
>  	ret->idx = 0;
>  	ret->dev = dev;
>  
> -	return ret;
> +	return_ptr(ret);
>  }
>  EXPORT_SYMBOL_GPL(dpaa2_io_store_create);
>  
> -- 
> 2.54.0
> 

Re: [PATCH v2 2/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_create()

[Dan Carpenter]

On Mon, Jul 06, 2026 at 12:40:09PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 6 Jul 2026 11:18:39 +0200
> 
> Scope-based resource management became supported for some
> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
> Introduce __cleanup() based infrastructure").
> 
> * Thus use the attribute “__free(kfree)”.
> 
> * Omit two kfree() calls accordingly.
> 
> * Reduce the scopes for the local variables “obj”
>   and “qman_256_cycles_per_ns”.

Why?  No one does this except bcachefs and that was removed.

regards,
dan carpenter

Re: [PATCH v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Ioana Ciornei]

On Mon, Jul 06, 2026 at 12:38:08PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 6 Jul 2026 10:34:43 +0200
> 
> Scope-based resource management became supported for some
> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
> Introduce __cleanup() based infrastructure").
> 
> * Thus use the attribute “__free(kfree)”.
> 
> * Reduce the scope for the local variable “ret”.

Please don't. Let all the variables be defined at the start of each
function.

> 
> * Omit two kfree() calls accordingly.
> 
> * Omit the local variable “size” (for another memory allocation).

Please remove this change, not related to the intention of the patch.

Ioana

Re: [PATCH v2 2/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_create()

[Ioana Ciornei]

On Mon, Jul 06, 2026 at 12:40:09PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 6 Jul 2026 11:18:39 +0200
> 
> Scope-based resource management became supported for some
> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
> Introduce __cleanup() based infrastructure").
> 
> * Thus use the attribute “__free(kfree)”.
> 
> * Omit two kfree() calls accordingly.
> 
> * Reduce the scopes for the local variables “obj”
>   and “qman_256_cycles_per_ns”.

Remove these changes. All variables should be defined at the start of
the function, especially since there is no strong reason not to.

Ioana

Re: [v2 2/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_create()

[Markus Elfring]

>> Scope-based resource management became supported for some
>> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
>> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
>> Introduce __cleanup() based infrastructure").
>>
>> * Thus use the attribute “__free(kfree)”.
>>
>> * Omit two kfree() calls accordingly.
>>
>> * Reduce the scopes for the local variables “obj”
>>   and “qman_256_cycles_per_ns”.
> 
> Why?  No one does this except bcachefs and that was removed.
Have you still got development difficulties with scope reductions for
selected local variables?
https://elixir.bootlin.com/linux/v7.2-rc4/source/include/linux/cleanup.h#L142-L146

Regards,
Markus

Re: [v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Markus Elfring]

>> Scope-based resource management became supported for some
>> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
>> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
>> Introduce __cleanup() based infrastructure").
>>
>> * Thus use the attribute “__free(kfree)”.
>>
>> * Reduce the scope for the local variable “ret”.
> 
> Please don't. Let all the variables be defined at the start of each function.

Do we stumble on another coding style preferences conflict here?
https://elixir.bootlin.com/linux/v7.2-rc4/source/include/linux/cleanup.h#L142-L146


>> * Omit two kfree() calls accordingly.
>>
>> * Omit the local variable “size” (for another memory allocation).
> 
> Please remove this change, not related to the intention of the patch.
What does hinder to perform the required size determination as a direct parameter
for a kzalloc() call?

Regards,
Markus

Re: [v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Ioana Ciornei]

On Mon, Jul 27, 2026 at 02:24:55PM +0200, Markus Elfring wrote:
> >> Scope-based resource management became supported for some
> >> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
> >> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
> >> Introduce __cleanup() based infrastructure").
> >>
> >> * Thus use the attribute “__free(kfree)”.
> >>
> >> * Reduce the scope for the local variable “ret”.
> > 
> > Please don't. Let all the variables be defined at the start of each function.
> 
> Do we stumble on another coding style preferences conflict here?

Yes, especially when moving the variable definition is unnecessary.

> https://elixir.bootlin.com/linux/v7.2-rc4/source/include/linux/cleanup.h#L142-L146

The context for the recommendation linked by you is (also from
cleanup.h):

	 * "When multiple variables in the same scope have cleanup attributes,
	 * at exit from the scope their associated cleanup functions are run in
	 * reverse order of definition (last defined, first cleanup)."
	 *
	 * When the unwind order matters it requires that variables be defined
	 * mid-function scope rather than at the top of the file.  Take the
	 * following example and notice the bug highlighted by "!!"::

Which is not the case of these simple functions which only need a kfree.

> 
> 
> >> * Omit two kfree() calls accordingly.
> >>
> >> * Omit the local variable “size” (for another memory allocation).
> > 
> > Please remove this change, not related to the intention of the patch.
> What does hinder to perform the required size determination as a direct parameter
> for a kzalloc() call?

Nothing, but it's a different change. On top of this, it's just churning
the code without an added benefit.

Ioana

Re: [v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Markus Elfring]

>>>> * Omit the local variable “size” (for another memory allocation).
>>>
>>> Please remove this change, not related to the intention of the patch.
>> What does hinder to perform the required size determination as a direct parameter
>> for a kzalloc() call?
> 
> Nothing, but it's a different change. On top of this, it's just churning
> the code without an added benefit.
It influences development concerns also according to preferred variable scopes.

Regards,
Markus

Re: [v2 2/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_create()

[Markus Elfring]

>> Scope-based resource management became supported for some
>> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
>> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
>> Introduce __cleanup() based infrastructure").
>>
>> * Thus use the attribute “__free(kfree)”.
>>
>> * Omit two kfree() calls accordingly.
>>
>> * Reduce the scopes for the local variables “obj”
>>   and “qman_256_cycles_per_ns”.
> 
> Remove these changes. All variables should be defined at the start of
> the function, especially since there is no strong reason not to.
Can such a feedback be also interpreted in the way that you have got still difficulties
with complete support of scope-based resource management?

Regards,
Markus

Re: [v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Markus Elfring]

>> https://elixir.bootlin.com/linux/v7.2-rc4/source/include/linux/cleanup.h#L142-L146
> 
> The context for the recommendation linked by you is (also from
> cleanup.h):
> 
> 	 * "When multiple variables in the same scope have cleanup attributes,
> 	 * at exit from the scope their associated cleanup functions are run in
> 	 * reverse order of definition (last defined, first cleanup)."
> 	 *
> 	 * When the unwind order matters it requires that variables be defined
> 	 * mid-function scope rather than at the top of the file.  Take the
> 	 * following example and notice the bug highlighted by "!!"::
> 
> Which is not the case of these simple functions which only need a kfree.
How will development concerns evolve further also according to previous clarification attempts?

Clarifying confusion of our variable placement rules caused by cleanup.h
https://lore.kernel.org/ksummit/CAHk-=whPZoi03ZwphxiW6cuWPtC3nyKYS8_BThgztCdgPWP1WA@mail.gmail.com/
https://lkml.org/lkml/2025/11/18/1766

Regards,
Markus

Re: [v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Ioana Ciornei]

On Tue, Jul 28, 2026 at 08:55:38AM +0200, Markus Elfring wrote:
> >> https://elixir.bootlin.com/linux/v7.2-rc4/source/include/linux/cleanup.h#L142-L146
> > 
> > The context for the recommendation linked by you is (also from
> > cleanup.h):
> > 
> > 	 * "When multiple variables in the same scope have cleanup attributes,
> > 	 * at exit from the scope their associated cleanup functions are run in
> > 	 * reverse order of definition (last defined, first cleanup)."
> > 	 *
> > 	 * When the unwind order matters it requires that variables be defined
> > 	 * mid-function scope rather than at the top of the file.  Take the
> > 	 * following example and notice the bug highlighted by "!!"::
> > 
> > Which is not the case of these simple functions which only need a kfree.
> How will development concerns evolve further also according to previous clarification attempts?
> 
> Clarifying confusion of our variable placement rules caused by cleanup.h
> https://lore.kernel.org/ksummit/CAHk-=whPZoi03ZwphxiW6cuWPtC3nyKYS8_BThgztCdgPWP1WA@mail.gmail.com/
> https://lkml.org/lkml/2025/11/18/1766

My preference still stands and I do not think they contradict the views
expressed in the above links:
- Variables which do not have anything to do with cleanup.h should still
  be defined at the start of the function. This is the case of the
  qman_256_cycles_per_ns which you moved in 2/2 without any motive.
- In the case of a variable used with __free, if it can be done, I
  prefer to see the definition and initialization done at the start of
  the function.
  For example:

	struct dpaa2_io *obj __free(kfree) = kmalloc_obj(*obj);
	u32 qman_256_cycles_per_ns;

	if (!obj)
		return NULL;


I am not going to engage past this message.

Ioana

Re: [v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Markus Elfring]

>> How will development concerns evolve further also according to previous clarification attempts?
>>
>> Clarifying confusion of our variable placement rules caused by cleanup.h
>> https://lore.kernel.org/ksummit/CAHk-=whPZoi03ZwphxiW6cuWPtC3nyKYS8_BThgztCdgPWP1WA@mail.gmail.com/
>> https://lkml.org/lkml/2025/11/18/1766
> 
> My preference still stands and I do not think they contradict the views
> expressed in the above links:

It seems that more time and guidance adjustments would be needed
to adapt development habits into further directions.


> - Variables which do not have anything to do with cleanup.h should still
>   be defined at the start of the function. This is the case of the
>   qman_256_cycles_per_ns which you moved in 2/2 without any motive.
> - In the case of a variable used with __free, if it can be done, I
>   prefer to see the definition and initialization done at the start of
>   the function.

The discussed function implementations contain also input parameter validation.
Thus I imagine that it would occasionally be nicer to avoid extra data processing accordingly.

Regards,
Markus

Re: [PATCH v2 1/2] soc: fsl: dpio: Use scope-based resource management in dpaa2_io_store_create()

[Markus Elfring]

…
>> +++ b/drivers/soc/fsl/dpio/dpio-service.c
…
>> -	size = max_frames * sizeof(struct dpaa2_dq) + 64;
>> -	ret->alloced_addr = kzalloc(size, GFP_KERNEL);
>> -	if (!ret->alloced_addr) {
>> -		kfree(ret);
>> +	ret->alloced_addr = kzalloc(max_frames * sizeof(struct dpaa2_dq) + 64, GFP_KERNEL);
>> +	if (!ret->alloced_addr)
>>  		return NULL;
> 
> Why convert the ret allocation but not the ret->alloced_addr allocation?
I suggest to reconsider the suggested influence of the attribute “__free(kfree)”
once more also according to data structure members.

Regards,
Markus