Message-ID: <a4a9ba6f-9891-cc4c-e512-d221141d998f@csgroup.eu>
Date:   Tue, 23 Aug 2022 05:24:14 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Baoquan He <bhe@...hat.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "hch@...radead.org" <hch@...radead.org>,
        "agordeev@...ux.ibm.com" <agordeev@...ux.ibm.com>,
        "wangkefeng.wang@...wei.com" <wangkefeng.wang@...wei.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 01/11] mm/ioremap: change the return value of
 io[re|un]map_allowed and rename



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

> 
> 
>>
>> And why does it need a name change ? The new name suggests that what was
>> simply a check function becomes now a function doing the job. Is that
>> the intention ?
> 
> Yes, it's not a simple checking any more. It could do io address mapping
> inside arch_ioremap(), and could modify the passed in 'phys_addr' and
> 'prot' in patch 2. The ioremap_allowed() isn't appropriate to reflect
> those.

Fair enough, then all this needs to be explained in the commit message.

> 
>>
>>
>>>
>>> ===
>>>    arch_ioremap() return a bool,
>>
>> It is not a bool. A bool is either true or false.
> 
> Thanks, I forgot to update this accordingly.
> 
>>
>>>      - IS_ERR means return an error
>>>      - NULL means continue to remap
>>>      - a non-NULL, non-IS_ERR pointer is returned directly
>>>    arch_iounmap() return a bool,
>>
>> Same here, not a bool either.
> 
> And this place.
>>
>>>      - 0 means continue to vunmap
>>>      - error code means skip vunmap and return directly
>>>
>>> This is taken from Kefeng's below old patch. Christoph suggested the
>>> return value because he foresaw the doablity of converting to take
>>> GENERIC_IOREMAP on more architectures.
>>>    - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap()
>>>    - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u
>>>
>>> While at it, the invocation of arch_ioremap() need be moved to the
>>> beginning of ioremap_prot() because architectures like sh, openrisc,
>>> ia64, need do the ARCH specific io address mapping on the original
>>> physical address. And in the later patch, the address fix up code
>>> in arch_ioremap() also need be done on the original addre on some
>>> architectures.
>>>
>>> This is preparation for later patch, no functionality change.
>>
>> No functionnal change, really ?
> 
> You mean the new arch_ioremap() owning different definition or the
> invocation of arch_ioremap() moved up is functional change? Now I am
> not sure about the latter one, may need update my knowledge base.

Both indeed. I understand that this first step is not changing much to 
the logic, but I think the simple fact to change the arguments and name 
are some how a functionnal change.

> 
> Thanks
> Baoquan
> 

Powered by blists - more mailing lists