Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <20231206151600.26833-1-quic_pintu@quicinc.com>
 <20231208203128.GA2646347-robh@kernel.org> <CAOuPNLg90T69USVQ8Ti6c8fXb_XrnaR035in_CbJHmNMUYLqOg@mail.gmail.com>
In-Reply-To: <CAOuPNLg90T69USVQ8Ti6c8fXb_XrnaR035in_CbJHmNMUYLqOg@mail.gmail.com>
From:   Pintu Agarwal <pintu.ping@gmail.com>
Date:   Thu, 14 Dec 2023 22:47:04 +0530
Message-ID: <CAOuPNLj4_pQiAHoER2VJpW_2NEaq8+zF8p1br+tf0Toe1t1UDg@mail.gmail.com>
Subject: Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure
To:     Rob Herring <robh@kernel.org>, vichy.kuo@gmail.com
Cc:     Pintu Kumar <quic_pintu@quicinc.com>, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, linux-mm@kvack.org,
        frowand.list@gmail.com, devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 11 Dec 2023 at 20:13, Pintu Agarwal <pintu.ping@gmail.com> wrote:
>
> Hi,
>
> On Sat, 9 Dec 2023 at 02:01, Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Dec 06, 2023 at 08:46:00PM +0530, Pintu Kumar wrote:
> > > During fdt_init_reserved_mem() when __reserved_mem_init_node()
> > > fail we are using pr_info to print error.
> > >
> > > So, if we change the loglevel to 4 (or below), this error
> > > message will be missed.
> > >
> > > Thus, change the pr_info to pr_err for fail case.
> > >
> > > Signed-off-by: Pintu Kumar <quic_pintu@quicinc.com>
> > > ---
> > >  drivers/of/of_reserved_mem.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > > index 7ec94cfcbddb..473665e76b6f 100644
> > > --- a/drivers/of/of_reserved_mem.c
> > > +++ b/drivers/of/of_reserved_mem.c
> > > @@ -334,7 +334,7 @@ void __init fdt_init_reserved_mem(void)
> > >               if (err == 0) {
> > >                       err = __reserved_mem_init_node(rmem);
> > >                       if (err != 0 && err != -ENOENT) {
> > > -                             pr_info("node %s compatible matching fail\n",
> > > +                             pr_err("node %s compatible matching fail\n",
> >
> > Isn't the message just wrong. If compatible match fails, we return
> > ENOENT. The failure here would be from the init function.
> >
> Okay.
> You mean to say, if __reserved_mem_init_node fails with default err
> (ENOENT) then it may not hit this condition.
> Instead it will hit the 'else' case which is wrong ?
> Also, the "initfn" inside "__reserved_mem_init_node" may fail in which
> case also it may return default err.
>
> Maybe, the initial author's intention was to free the memory only if
> the failure type is not the default ENOENT type.
>
> This seems to be a different issue.
> Can we address this separately in a different patch ?
>
> And how do we fix this ?
> One option is to add another "if" condition with just ENOENT error check ?
> if (err == -ENOENT) {
>     pr_err("node %s compatible matching fail\n", rmem->name);
>     return;
> }
> Then, correct the existing log with a different message:
> pr_err("node %s matching reserved mem not found.\n", rmem->name);
> Or, add one more "if else" condition ?
> Or, fix the calling function itself : __reserved_mem_init_node ?
>

Any further comments on this ?