Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup helpers

[Nathan Chancellor]

On Fri, Jan 26, 2024 at 12:53:36PM -0000, tip-bot2 for Bartosz Golaszewski wrote:
> The following commit has been merged into the irq/core branch of tip:
> 
> Commit-ID:     590610d72a790458431cbbebc71ee24521533b5e
> Gitweb:        https://git.kernel.org/tip/590610d72a790458431cbbebc71ee24521533b5e
> Author:        Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> AuthorDate:    Mon, 22 Jan 2024 13:42:43 +01:00
> Committer:     Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> CommitterDate: Fri, 26 Jan 2024 13:44:48 +01:00
> 
> genirq/irq_sim: Shrink code by using cleanup helpers
> 
> Use the new __free() mechanism to remove all gotos and simplify the error
> paths.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Link: https://lore.kernel.org/r/20240122124243.44002-5-brgl@xxxxxxxx
> 
> ---
>  kernel/irq/irq_sim.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> index b0d50b4..fe8fd30 100644
> --- a/kernel/irq/irq_sim.c
> +++ b/kernel/irq/irq_sim.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2020 Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irq_sim.h>
> @@ -163,33 +164,27 @@ static const struct irq_domain_ops irq_sim_domain_ops = {
>  struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
>  					 unsigned int num_irqs)
>  {
> -	struct irq_sim_work_ctx *work_ctx;
> +	struct irq_sim_work_ctx *work_ctx __free(kfree) = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
> +	unsigned long *pending;
>  
> -	work_ctx = kmalloc(sizeof(*work_ctx), GFP_KERNEL);
>  	if (!work_ctx)
> -		goto err_out;
> +		return ERR_PTR(-ENOMEM);
>  
> -	work_ctx->pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
> -	if (!work_ctx->pending)
> -		goto err_free_work_ctx;
> +	pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);

Apologies if this has already been reported elsewhere. This does not
match what was sent and it causes the build to break with both GCC:

  In file included from include/linux/compiler_types.h:89,
                   from <command-line>:
  kernel/irq/irq_sim.c: In function 'irq_domain_create_sim':
  include/linux/compiler_attributes.h:76:41: error: expected expression before '__attribute__'
     76 | #define __cleanup(func)                 __attribute__((__cleanup__(func)))
        |                                         ^~~~~~~~~~~~~
  include/linux/cleanup.h:64:25: note: in expansion of macro '__cleanup'
     64 | #define __free(_name)   __cleanup(__free_##_name)
        |                         ^~~~~~~~~
  kernel/irq/irq_sim.c:173:19: note: in expansion of macro '__free'
    173 |         pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
        |                   ^~~~~~

and Clang:

  kernel/irq/irq_sim.c:173:12: error: expected expression
    173 |         pending = __free(bitmap) = bitmap_zalloc(num_irqs, GFP_KERNEL);
        |                   ^
  include/linux/cleanup.h:64:23: note: expanded from macro '__free'
     64 | #define __free(_name)   __cleanup(__free_##_name)
        |                         ^
  include/linux/compiler-clang.h:15:25: note: expanded from macro '__cleanup'
     15 | #define __cleanup(func) __maybe_unused __attribute__((__cleanup__(func)))
        |                         ^
  include/linux/compiler_attributes.h:344:41: note: expanded from macro '__maybe_unused'
    344 | #define __maybe_unused                  __attribute__((__unused__))
        |                                         ^
  1 error generated.

This was initially noticed by our CI:

https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/7671789235/job/20915505965
https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2bVGKZUmat8fRr582Nh8hNA6FXD/build.log

Cheers,
Nathan

> +	if (!pending)
> +		return ERR_PTR(-ENOMEM);
>  
>  	work_ctx->domain = irq_domain_create_linear(fwnode, num_irqs,
>  						    &irq_sim_domain_ops,
>  						    work_ctx);
>  	if (!work_ctx->domain)
> -		goto err_free_bitmap;
> +		return ERR_PTR(-ENOMEM);
>  
>  	work_ctx->irq_count = num_irqs;
>  	work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
> +	work_ctx->pending = no_free_ptr(pending);
>  
> -	return work_ctx->domain;
> -
> -err_free_bitmap:
> -	bitmap_free(work_ctx->pending);
> -err_free_work_ctx:
> -	kfree(work_ctx);
> -err_out:
> -	return ERR_PTR(-ENOMEM);
> +	return no_free_ptr(work_ctx)->domain;
>  }
>  EXPORT_SYMBOL_GPL(irq_domain_create_sim);
>