Received-SPF: pass (google.com: domain of linux-kernel+bounces-40736-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240122124243.44002-5-brgl@bgdev.pl> <170627361652.398.12825437185563577604.tip-bot2@tip-bot2>
 <20240126210509.GA1212219@dev-arch.thelio-3990X>
In-Reply-To: <20240126210509.GA1212219@dev-arch.thelio-3990X>
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Date: Fri, 26 Jan 2024 23:15:57 +0100
Message-ID: <CACMJSesVR_3-PBt1ScricSKNMRzH5gesqtTVW3mqN=gg0-O-7w@mail.gmail.com>
Subject: Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup helpers
To: Nathan Chancellor <nathan@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org, linux-tip-commits@vger.kernel.org, 
	maz@kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Fri, 26 Jan 2024 at 22:05, Nathan Chancellor <nathan@kernel.org> wrote:
>
> 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@linaro.org>
> > AuthorDate:    Mon, 22 Jan 2024 13:42:43 +01:00
> > Committer:     Thomas Gleixner <tglx@linutronix.de>
> > 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@linaro.org>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Link: https://lore.kernel.org/r/20240122124243.44002-5-brgl@bgdev.pl
> >
> > ---
> >  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@baylibre.com>
> >   */
> >
> > +#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:
>

I did not see any other report. I don't know what happened here but
this was a ninja edit as it's not what I sent. If Thomas' intention
was to move the variable declaration and detach it from the assignment
then 'pending' should at least be set to NULL and __free() must
decorate the declaration.

But the coding style of declaring variables when they're first
assigned their auto-cleaned value is what Linus Torvalds explicitly
asked me to do when I first started sending PRs containing uses of
linux/cleanup.h.

Bartosz

>   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);
> >