Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 1 Apr 2023 11:22:52 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Geert Uytterhoeven <geert@linux-m68k.org>
Cc:     Li Yang <lidaxian@hust.edu.cn>,
        Magnus Damm <magnus.damm@gmail.com>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Biju Das <biju.das.jz@bp.renesas.com>,
        linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] soc: renesas: renesas-soc: release 'chipid' from
 ioremap()
Message-ID: <d2688eb2-d7b6-4e80-a13e-55ed541ac9b8@kili.mountain>
References: <20230331095545.31823-1-lidaxian@hust.edu.cn>
 <CAMuHMdV5guFbo76nq27aZjWsYqneOfGNf0Ozyh0C53+VgnXgXw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMuHMdV5guFbo76nq27aZjWsYqneOfGNf0Ozyh0C53+VgnXgXw@mail.gmail.com>
Precedence: bulk

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