Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
MIME-Version: 1.0
References: <20231206151600.26833-1-quic_pintu@quicinc.com> <20231208203128.GA2646347-robh@kernel.org>
In-Reply-To: <20231208203128.GA2646347-robh@kernel.org>
From:   Pintu Agarwal <pintu.ping@gmail.com>
Date:   Mon, 11 Dec 2023 20:13:00 +0530
Message-ID: <CAOuPNLg90T69USVQ8Ti6c8fXb_XrnaR035in_CbJHmNMUYLqOg@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

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 ?


Thanks,
Pintu