Received-SPF: pass (google.com: domain of linux-kernel+bounces-42522-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date: Mon, 29 Jan 2024 11:13:00 +0100
From: Ingo Molnar <mingo@kernel.org>
To: Bartosz Golaszewski <bartosz.golaszewski@linaro.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>
Subject: Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup
 helpers
Message-ID: <Zbd6LPDRFxCWZnqb@gmail.com>
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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CACMJSesVR_3-PBt1ScricSKNMRzH5gesqtTVW3mqN=gg0-O-7w@mail.gmail.com>


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

Thanks,

	Ingo