Re: [PATCH 2/3] of: Use scope based kfree() cleanups

[Saravana Kannan]

On Fri, Apr 5, 2024 at 5:44 AM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Thu, Apr 4, 2024 at 6:16 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> >
> > On Thu, Apr 4, 2024 at 7:15 AM Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > Use the relatively new scope based kfree() cleanup to simplify error
> > > handling. Doing so reduces the chances of memory leaks and simplifies
> > > error paths by avoiding the need for goto statements.
> > >
> > > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > > ---
> > >  drivers/of/base.c     | 34 ++++++++--------------------------
> > >  drivers/of/dynamic.c  | 11 ++++-------
> > >  drivers/of/resolver.c | 35 +++++++++++++----------------------
> > >  3 files changed, 25 insertions(+), 55 deletions(-)
> > >
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index 8856c67c466a..20603d3c9931 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -16,6 +16,7 @@
> > >
> > >  #define pr_fmt(fmt)    "OF: " fmt
> > >
> > > +#include <linux/cleanup.h>
> > >  #include <linux/console.h>
> > >  #include <linux/ctype.h>
> > >  #include <linux/cpu.h>
> > > @@ -1393,8 +1394,10 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
> > >                                    const char *stem_name,
> > >                                    int index, struct of_phandle_args *out_args)
> > >  {
> > > -       char *cells_name, *map_name = NULL, *mask_name = NULL;
> > > -       char *pass_name = NULL;
> > > +       char *cells_name __free(kfree) = kasprintf(GFP_KERNEL, "#%s-cells", stem_name);
> > > +       char *map_name __free(kfree) = kasprintf(GFP_KERNEL, "%s-map", stem_name);
> > > +       char *mask_name __free(kfree) = kasprintf(GFP_KERNEL, "%s-map-mask", stem_name);
> > > +       char *pass_name __free(kfree) = kasprintf(GFP_KERNEL, "%s-map-pass-thru", stem_name);
> >
> > With the scoped stuff, do these function calls need to be in the same
> > line we are defining these variables? If not, I'd rather that the
> > calls remain where they were. It feels like a lote to visually parse
> > and take in from a readability perspective.
>
> They don't have to be, but if you don't want to get yelled at by the
> chief penguin, then yes, they should be together. See the discussions
> on adding the scoped iterators. But with the C99 adoption, we can move
> the declaration to where the assignment was original.

Thanks for the context and the link in the other email.

Review-by without reservations.

-Saravana