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

[Bartosz Golaszewski]

On Mon, 29 Jan 2024 at 11:13, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>
> * Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> wrote:
>
> > On Fri, 26 Jan 2024 at 22:05, Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
> > >
> > > > 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:
> > >
> >
> > 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@xxxxxxxxxxxxxx/

Bartosz