Re: [PATCH] soc: renesas: renesas-soc: release 'chipid' from ioremap()

[Geert Uytterhoeven]

Hi Dan,

On Mon, Apr 3, 2023 at 6:29 AM Dan Carpenter <error27@xxxxxxxxx> wrote:
> On Fri, Mar 31, 2023 at 02:13:10PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Mar 31, 2023 at 12:14 PM Li Yang <lidaxian@xxxxxxxxxxx> wrote:
> > > Smatch reports:
> > >
> > > drivers/soc/renesas/renesas-soc.c:536 renesas_soc_init() warn:
> > > 'chipid' from ioremap() not released on lines: 475.
> > >
> > > If soc_dev_atrr allocation is failed, function renesas_soc_init()
> > > will return without releasing 'chipid' from ioremap().
> > >
> > > Fix this by adding function iounmap().
> > >
> > > Fixes: cb5508e47e60 ("soc: renesas: Add support for reading product revision for RZ/G2L family")
> > > Signed-off-by: Li Yang <lidaxian@xxxxxxxxxxx>
> > > Reviewed-by: Dan Carpenter <error27@xxxxxxxxx>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/soc/renesas/renesas-soc.c
> > > +++ b/drivers/soc/renesas/renesas-soc.c
> > > @@ -471,8 +471,11 @@ static int __init renesas_soc_init(void)
> > >         }
> > >
> > >         soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > > -       if (!soc_dev_attr)
> > > +       if (!soc_dev_attr) {
> > > +               if (chipid)
> > > +                       iounmap(chipid);
> >
> > We don't really care, as the system is dead at this point anyway.
>
> Why even have the check for NULL then?  The kzalloc() is small enough

Because else someone will submit a patch to add that check? ;-)

> to the point where it litterally cannot fail.

I still don't understand how it can be guaranteed that small allocations
never fail... "while (1) kmalloc(16, GFP_KERNEL);"

> This patch is already written, it's way less effort for us to apply it
> than it is to debate its worth.  I kind of feel like it's better to just
> fix the bugs even when they don't affect real life.  Otherwise it's a
> constant debate with bugs if they're worth fixing.
>
> This is a university group and they're looking for bugs to fix.  I'm
> like, "I'm drowning in resource leak warnings and I don't even look at
> them any more because they're basically all trash that no one cares
> about."  So I was hoping to maybe clean up the trash a bit to the point
> where we can start caring about the leaks.

Fair enough.
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
i.e. will queue in renesas-devel for v6.4.

Perhaps we need a different mechanism to annotate error handling code
that cannot ever happen in a real product, so it can be thrown away by
the compiler, while still pleasing the static checkers?  All these
checks and error handling code do affect kernel size.  There are
Linux products running on SoCs with 8 MiB of internal SRAM.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds