Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot

[Christophe Leroy]

Hi Baoquan,

Le 12/09/2022 à 04:55, Baoquan He a écrit :
> Hi Christophe,
> 
> On 08/28/22 at 07:10pm, Baoquan He wrote:
>> On 08/23/22 at 07:03pm, Christophe Leroy wrote:
> ......
>>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers
>>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of
>>>>>>> doing it inside the arch_ioremap() function ?
>>>>>>
>>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but
>>>>>> feel the current way taken in this patchset is better. Because not all
>>>>>> architecutres need the address fix up, only parisc, and only few need
>>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only
>>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP
>>>>>> method for people to understand and take.
>>>>>
>>>>> I can't understand. Why is it difficult to do something like:
>>>>>
>>>>> #ifndef ioremap_adjust_prot
>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>>>>> {
>>>>> 	return flags;
>>>>> }
>>>>> #endif
>>>>>
>>>>> Then for arc you do
>>>>>
>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags)
>>>>> {
>>>>> 	return pgprot_val(pgprot_noncached(__pgprot(flags)));
>>>>> }
>>>>> #define ioremap_adjust_prot ioremap_adjust_prot
>>>>
>>>> My thinking is we have four things to do in the added hookers.
>>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from
>>>> ioremap_prot();
>>>> 2) handling the mapping io address specifically on ARCHes, e.g arc,
>>>> ia64, s390;
>>>> 3) the original physical address passed into ioremap_prot() need be
>>>> fixed up, e.g arc;
>>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc
>>>> and xtensa.
>>>>
>>>> With Kefeng's patches, the case 1) is handled with introduced
>>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is
>>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1),
>>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each
>>>> case is not difficult from my side. I worry that as time goes by, those
>>>> several hooks my cause issue, e.g if a new adjustment need be done,
>>>> should we introduce a new helper or make do with the existed hook; how
>>>>
>>>> When I investigated this, one arch_ioremap() looks not complicated
>>>> since not all ARCHes need cover all above 4 cases. That's why I finally
>>>> choose one hook. I am open to new idea, please let me know if we should
>>>> change it to introduce several different helpers.
>>>>
>>>
>>> A new idea that would have my preference would be to do just like we did
>>> with arch_get_unmapped_area(). Look at
>>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638
>>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131
>>>
>>> Instead of having the generic that calls the arch specific, make it the
>>> other way round, have the arch specific call the generic after doing its
>>> specialties.
>>
>> This sounds good. I made a draft patch to change code in generic code
>> part, just showing what it looks like.
>>
>> Both arch_ioremap() way and the arch sepcific call the generic way look
>> good to me. Just it will take less effort for me to continue the
>> arch_ioremap() way. I would like to hear Christoph's opinion since he
>> introduced the GENERIC_IOREMAP method and suggested the earlier
>> arch_ioremap() way and change in this patchset.
> 
> I will make another round change and post. Since Christoph doesn't
> reply, I would like to continue with the existing
> arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion
> on the new idea to reintroduce ioremap().
> 

I still dislike you approach with the architectures modifying local vars 
by reference, and as you said earlier I'm not the only one : "This is 
suggested too by Alexander during his v1 reviewing".

So I'd really prefer you to reconsider your approach and avoid passign 
pointers to local vars to architecture helpers.

Thanks
Christophe