| Message ID | 20231206151600.26833-1-quic_pintu@quicinc.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | of: reserved_mem: fix error log for reserved mem init failure | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
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 >
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
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 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
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(-)