Re: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename

[Christophe Leroy]

Le 23/08/2022 à 17:14, Baoquan He a écrit :
> On 08/23/22 at 05:24am, Christophe Leroy wrote:
>>
>>
>> Le 23/08/2022 à 02:20, Baoquan He a écrit :
>>> On 08/22/22 at 06:25am, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 20/08/2022 à 02:31, Baoquan He a écrit :
>>>>> In some architectures, there are ARCH specifici io address mapping
>>>>> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
>>>>> openrisc, s390, sh.
>>>>>
>>>>> In oder to convert them to take GENERIC_IOREMAP method, we need change
>>>>> the return value of hook ioremap_allowed() and iounmap_allowed().
>>>>> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
>>>>> their current behaviour.
>>>
>>> Thanks for reviewing.
>>>
>>>>
>>>> Please don't just say you need to change the return value. Explain why.
>>>
>>> The 1st paragraph and the sentence 'In oder to convert them to take
>>> GENERIC_IOREMAP method' tell the reason, no?
>>
>> What I would like to read is _why_ you need to change the return value
>> in order to convert to GENERIC_IOREMAP
> 
> I rephrase the log as below, it's OK to you? Or please help check and
> tell what I need to improve to better explain the reason.
> 
> ====
> The current io[re|un]map_allowed() hooks are used to check if the
> io[re|un]map() actions are qualified to proceed when taking
> GENERIC_IOREMAP way to do ioremap()/iounmap(). Otherwise io[re|un]map()
> will return NULL.
> 
> On some architectures like arc, ia64, openris, s390, sh, there are
> ARCH specific io address mapping to translate the passed in physical
> address to io address when calling ioremap(). In order to convert
> these architectures to take GENERIC_IOREMAP way to ioremap(), we need
> change the return value of hook ioremap_allowed() and iounmap_allowed().
> With the change, we can move the architecture specific io address
> mapping into ioremap_allowed() hook, and give the mapped io address
> out to let ioremap_prot() return it. While at it, rename the hooks to
> arch_ioremap() and arch_iounmap() to reflect their new behaviour.
> ====
> 

That looks more in line with the type of explanation I foresee in the 
commit message, thanks.


Christophe