[v1] of: reserved_mem: fix error log for reserved mem init failure

[PATCH] of: reserved_mem: fix error log for reserved mem init failure

Posted by Pintu Kumar 2 years ago

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",
 					rmem->name);
 				if (nomap)
 					memblock_clear_nomap(rmem->base, rmem->size);
-- 
2.17.1

Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure

Posted by Rob Herring 2 years ago

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.

>  					rmem->name);
>  				if (nomap)
>  					memblock_clear_nomap(rmem->base, rmem->size);
> -- 
> 2.17.1
>

Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure

Posted by Pintu Agarwal 2 years ago

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

Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure

Posted by Pintu Agarwal 2 years ago

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 ?

Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure

Posted by Pintu Agarwal 1 year, 11 months ago

Hi,

On Thu, 14 Dec 2023 at 22:47, Pintu Agarwal <pintu.ping@gmail.com> wrote:
>
> 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 ?

Any further comments or suggestions on the above ?
Shall we just fix the log message, or correct the if/else case as well ?

Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure

Posted by Rob Herring 1 year, 11 months ago

On Sat, Jan 06, 2024 at 11:31:12PM +0530, Pintu Agarwal wrote:
> Hi,
> 
> On Thu, 14 Dec 2023 at 22:47, Pintu Agarwal <pintu.ping@gmail.com> wrote:
> >
> > 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 ?
> 
> Any further comments or suggestions on the above ?
> Shall we just fix the log message, or correct the if/else case as well ?

It looked to me like the original author's intent was this is not an 
error. Either convince me otherwise or wait for me to study this 
further. This code gets a lot of drive-by patches and what is "correct" 
isn't always clear.

Rob

Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure

Posted by Pintu Agarwal 1 year, 11 months ago

On Tue, 16 Jan 2024 at 20:05, Rob Herring <robh@kernel.org> wrote:
>
> On Sat, Jan 06, 2024 at 11:31:12PM +0530, Pintu Agarwal wrote:
> > Hi,
> >
> > On Thu, 14 Dec 2023 at 22:47, Pintu Agarwal <pintu.ping@gmail.com> wrote:
> > >
> > > 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 ?
> >
> > Any further comments or suggestions on the above ?
> > Shall we just fix the log message, or correct the if/else case as well ?
>
> It looked to me like the original author's intent was this is not an
> error. Either convince me otherwise or wait for me to study this
> further. This code gets a lot of drive-by patches and what is "correct"
> isn't always clear.
>
Thank you so much for looking into this.
Yes, I agree that the author's intention might be to present this as
an info message to the user.
But I think he might have not realised that info msg may not be printed always.
If we change loglevel to 4 (quiet) this info msg may be missed.
Normally, in the final production system we do this.
So, ideally, I think, error msg logging is more suitable in either case.

Sure, I will wait for your further findings.
I will also have a further look and see if I can convince you more
with another set :)

Thanks,
Pintu