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

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

[Li Yang]

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@hust.edu.cn>
Reviewed-by: Dan Carpenter <error27@gmail.com>
---
 drivers/soc/renesas/renesas-soc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
index 468ebce1ea88..51191d1a6dd1 100644
--- 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);
 		return -ENOMEM;
+	}
 
 	np = of_find_node_by_path("/");
 	of_property_read_string(np, "model", &soc_dev_attr->machine);
-- 
2.34.1

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

[Geert Uytterhoeven]

Hi Li,

On Fri, Mar 31, 2023 at 12:14 PM Li Yang <lidaxian@hust.edu.cn> 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@hust.edu.cn>
> Reviewed-by: Dan Carpenter <error27@gmail.com>

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.

>                 return -ENOMEM;
> +       }
>
>         np = of_find_node_by_path("/");
>         of_property_read_string(np, "model", &soc_dev_attr->machine);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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

[Dan Carpenter]

On Fri, Mar 31, 2023 at 02:13:10PM +0200, Geert Uytterhoeven wrote:
> Hi Li,
> 
> On Fri, Mar 31, 2023 at 12:14 PM Li Yang <lidaxian@hust.edu.cn> 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@hust.edu.cn>
> > Reviewed-by: Dan Carpenter <error27@gmail.com>
> 
> 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
to the point where it litterally cannot fail.

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.

regards,
dan carpenter

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@gmail.com> 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@hust.edu.cn> 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@hust.edu.cn>
> > > Reviewed-by: Dan Carpenter <error27@gmail.com>
> >
> > 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@glider.be>
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@linux-m68k.org

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

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

[Dan Carpenter]

On Mon, Apr 03, 2023 at 09:12:55AM +0200, Geert Uytterhoeven wrote:
> Hi Dan,
> 
> On Mon, Apr 3, 2023 at 6:29 AM Dan Carpenter <error27@gmail.com> 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@hust.edu.cn> 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@hust.edu.cn>
> > > > Reviewed-by: Dan Carpenter <error27@gmail.com>
> > >
> > > 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);"
> 

I read an lwn article on it and I think I once looked it up to try
figure out how small the definition of "small" was and it was
surprisingly large...  But I have no idea.  I think maybe small atomic
allocations can fail and GFP_KERNEL allocations sleep forever?  (These
guesses are worthless).

> 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.

People sometimes call BUG_ON(!soc_dev_attr).  It's sort of rare these
days.  It would be easy to make a function which silences Smatch...

	__system_is_dead();

regards,
dan carpenter

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

[Geert Uytterhoeven]

Hi Dan,

On Mon, Apr 3, 2023 at 9:34 AM Dan Carpenter <error27@gmail.com> wrote:
> On Mon, Apr 03, 2023 at 09:12:55AM +0200, Geert Uytterhoeven wrote:
> > 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.
>
> People sometimes call BUG_ON(!soc_dev_attr).  It's sort of rare these

BUG_ON() is also not cheap, space-wise (except if CONFIG_BUG=n).

> days.  It would be easy to make a function which silences Smatch...
>
>         __system_is_dead();

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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