Re: [PATCH] powerpc: Use printk instead of WARN in change_memory_attr

[Michael Ellerman <mpe@ellerman.id.au>]

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