diff mbox series

powerpc: Use printk instead of WARN in change_memory_attr

Message ID e2fcabe78795552264f045bd224f83609903708f.1724749026.git.ritesh.list@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series powerpc: Use printk instead of WARN in change_memory_attr | expand

Checks

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.

Commit Message

Ritesh Harjani (IBM) Aug. 27, 2024, 9:12 a.m. UTC
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

Comments

Christophe Leroy Aug. 27, 2024, 2:41 p.m. UTC | #1
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
Ritesh Harjani (IBM) Aug. 27, 2024, 4:06 p.m. UTC | #2
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
Michael Ellerman Aug. 30, 2024, 11:13 a.m. UTC | #3
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 mbox series

Patch

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