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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zbd6LPDRFxCWZnqb@gmail.com>
Date: Mon, 29 Jan 2024 11:13:00 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Cc: Nathan Chancellor <nathan@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org,
	linux-tip-commits@...r.kernel.org, maz@...nel.org,
	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [tip: irq/core] genirq/irq_sim: Shrink code by using cleanup
 helpers


* Bartosz Golaszewski <bartosz.golaszewski@...aro.org> wrote:

> On Fri, 26 Jan 2024 at 22:05, Nathan Chancellor <nathan@...nel.org> wrote:
> >
> > > Committer:     Thomas Gleixner <tglx@...utronix.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@...aro.org>
> > > Signed-off-by: Thomas Gleixner <tglx@...utronix.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@...libre.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