Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 Aug 2022 08:20:22 +0800
From:   Baoquan He <bhe@redhat.com>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "hch@infradead.org" <hch@infradead.org>,
        "agordeev@linux.ibm.com" <agordeev@linux.ibm.com>,
        "wangkefeng.wang@huawei.com" <wangkefeng.wang@huawei.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 01/11] mm/ioremap: change the return value of
 io[re|un]map_allowed and rename
Message-ID: <YwQdRg/IS0+3tbNu@MiWiFi-R3L-srv>
References: <20220820003125.353570-1-bhe@redhat.com>
 <20220820003125.353570-2-bhe@redhat.com>
 <d5272f42-f3e3-b2a8-428e-bd7815cf7518@csgroup.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d5272f42-f3e3-b2a8-428e-bd7815cf7518@csgroup.eu>
Precedence: bulk

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?


> 
> 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.

> 
> 
> > 
> > ===
> >   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.

Thanks
Baoquan