| Message ID | 20231206151600.26833-1-quic_pintu@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | of: reserved_mem: fix error log for reserved mem init failure | expand |
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 >
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 ?
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 ?
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
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);
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(-)