| Message ID | e2fcabe78795552264f045bd224f83609903708f.1724749026.git.ritesh.list@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc: Use printk instead of WARN in change_memory_attr | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
Le 27/08/2024 à 11:12, Ritesh Harjani (IBM) a écrit : > [Vous ne recevez pas souvent de courriers de ritesh.list@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > Use pr_warn_once instead of WARN_ON_ONCE as discussed here [1] > for printing possible use of set_memory_* on linear map on Hash. > > [1]: https://lore.kernel.org/all/877cc2fpi2.fsf@mail.lhotse/#t > > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > --- > arch/powerpc/mm/pageattr.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c > index ac22bf28086f..c8c2d664c6f3 100644 > --- a/arch/powerpc/mm/pageattr.c > +++ b/arch/powerpc/mm/pageattr.c > @@ -94,8 +94,11 @@ int change_memory_attr(unsigned long addr, int numpages, long action) > if (!radix_enabled()) { > int region = get_region_id(addr); > > - if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID)) > + if (region != VMALLOC_REGION_ID && region != IO_REGION_ID) { > + pr_warn_once("%s: possible use of set_memory_* on linear map on Hash from (%ps)\n", > + __func__, __builtin_return_address(0)); Is it really only linear map ? What about "possible user of set_memory_* outside of vmalloc or io region. Maybe a show_stack() would also be worth it ? But in principle I think it would be better to keep the WARN_ONCE until we can add __must_check to set_memory_xxx() functions to be sure all callers check the result, as mandated by https://github.com/KSPP/linux/issues/7 Christophe
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 27/08/2024 à 11:12, Ritesh Harjani (IBM) a écrit : >> [Vous ne recevez pas souvent de courriers de ritesh.list@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >> >> Use pr_warn_once instead of WARN_ON_ONCE as discussed here [1] >> for printing possible use of set_memory_* on linear map on Hash. >> >> [1]: https://lore.kernel.org/all/877cc2fpi2.fsf@mail.lhotse/#t >> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >> --- >> arch/powerpc/mm/pageattr.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c >> index ac22bf28086f..c8c2d664c6f3 100644 >> --- a/arch/powerpc/mm/pageattr.c >> +++ b/arch/powerpc/mm/pageattr.c >> @@ -94,8 +94,11 @@ int change_memory_attr(unsigned long addr, int numpages, long action) >> if (!radix_enabled()) { >> int region = get_region_id(addr); >> >> - if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID)) >> + if (region != VMALLOC_REGION_ID && region != IO_REGION_ID) { >> + pr_warn_once("%s: possible use of set_memory_* on linear map on Hash from (%ps)\n", >> + __func__, __builtin_return_address(0)); > > Is it really only linear map ? > > What about "possible user of set_memory_* outside of vmalloc or io region. "warning: possible user of set_memory_* outside of vmalloc and io region." I am thinking of adding a word "warning" too. I can make above change and send v2. > > Maybe a show_stack() would also be worth it ? IMO, since we have the caller, we need not pollute the dmesg with the entire call stack. Besides I am not aware of dump_stack_once() style prints. > > > But in principle I think it would be better to keep the WARN_ONCE until > we can add __must_check to set_memory_xxx() functions to be sure all > callers check the result, as mandated by > https://github.com/KSPP/linux/issues/7 Fixing the callers to check the return value is something that need not depend on this change no? The intention of this change is to mainly remove the heavy WARN_ON_ONCE from powerpc specific change_memory_attr() and convert to printk warn. -ritesh
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 27/08/2024 à 11:12, Ritesh Harjani (IBM) a écrit : >> [Vous ne recevez pas souvent de courriers de ritesh.list@gmail.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] >> >> Use pr_warn_once instead of WARN_ON_ONCE as discussed here [1] >> for printing possible use of set_memory_* on linear map on Hash. >> >> [1]: https://lore.kernel.org/all/877cc2fpi2.fsf@mail.lhotse/#t >> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >> --- >> arch/powerpc/mm/pageattr.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c >> index ac22bf28086f..c8c2d664c6f3 100644 >> --- a/arch/powerpc/mm/pageattr.c >> +++ b/arch/powerpc/mm/pageattr.c >> @@ -94,8 +94,11 @@ int change_memory_attr(unsigned long addr, int numpages, long action) >> if (!radix_enabled()) { >> int region = get_region_id(addr); >> >> - if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID)) >> + if (region != VMALLOC_REGION_ID && region != IO_REGION_ID) { >> + pr_warn_once("%s: possible use of set_memory_* on linear map on Hash from (%ps)\n", >> + __func__, __builtin_return_address(0)); > > Is it really only linear map ? > > What about "possible user of set_memory_* outside of vmalloc or io region. Yeah that's more accurate. Can drop "possible" too I think? We're pretty sure that's what happened. > Maybe a show_stack() would also be worth it ? > > > But in principle I think it would be better to keep the WARN_ONCE until > we can add __must_check to set_memory_xxx() functions to be sure all > callers check the result, as mandated by > https://github.com/KSPP/linux/issues/7 Will that help in this case though? If the caller checks the return value they'll see that the call failed, and presumably error out, possibly breaking boot or something else crucial. Unless we change change_memory_attr() to return EOPNOTSUPP or something in this case, and expect callers to handle that. cheers
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c index ac22bf28086f..c8c2d664c6f3 100644 --- a/arch/powerpc/mm/pageattr.c +++ b/arch/powerpc/mm/pageattr.c @@ -94,8 +94,11 @@ int change_memory_attr(unsigned long addr, int numpages, long action) if (!radix_enabled()) { int region = get_region_id(addr); - if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID)) + if (region != VMALLOC_REGION_ID && region != IO_REGION_ID) { + pr_warn_once("%s: possible use of set_memory_* on linear map on Hash from (%ps)\n", + __func__, __builtin_return_address(0)); return -EINVAL; + } } #endif
Use pr_warn_once instead of WARN_ON_ONCE as discussed here [1] for printing possible use of set_memory_* on linear map on Hash. [1]: https://lore.kernel.org/all/877cc2fpi2.fsf@mail.lhotse/#t Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> --- arch/powerpc/mm/pageattr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) -- 2.39.2