Received-SPF: pass (google.com: domain of linux-kernel+bounces-42546-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
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> <CACMJSesVR_3-PBt1ScricSKNMRzH5gesqtTVW3mqN=gg0-O-7w@mail.gmail.com>
 <Zbd6LPDRFxCWZnqb@gmail.com>
In-Reply-To: <Zbd6LPDRFxCWZnqb@gmail.com>
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Date: Mon, 29 Jan 2024 11:37:37 +0100
Message-ID: <CACMJSet0s+FqMDucvHi7x3Q6MSexUNH7_2eE8+g-gS=Z+CXVdw@mail.gmail.com>
Subject: Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup helpers
To: Ingo Molnar <mingo@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>, Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org, 
	linux-tip-commits@vger.kernel.org, maz@kernel.org, 
	linux-kernel@vger.kernel.org, Linus Torvalds <torvalds@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"

On Mon, 29 Jan 2024 at 11:13, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Bartosz Golaszewski <bartosz.golaszewski@linaro.org> wrote:
>
> > On Fri, 26 Jan 2024 at 22:05, Nathan Chancellor <nathan@kernel.org> wrote:
> > >
> > > > 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.
>
> Ok - I've rebased tip:irq/core with the original patch.
>
> Do you have a reference to Linus's mail about C++ style definition
> of variables? I can see the validity of the pattern in this context,
> but it's explicitly against the kernel coding style AFAICS, which
> I suppose prompted Thomas's edit. I'd like to have an URL handy when the
> inevitable checkpatch 'fix' gets submitted. ;-)
>

Sure, here's one rant I was the target of:
https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@mail.gmail.com/

Bartosz