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:   Sun, 28 Aug 2022 22:44:28 +0800
From:   Baoquan He <bhe@redhat.com>
To:     David Laight <David.Laight@aculab.com>
Cc:     'Christophe Leroy' <christophe.leroy@csgroup.eu>,
        "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: <Ywt/TB0rsKWrFcVy@MiWiFi-R3L-srv>
References: <20220820003125.353570-1-bhe@redhat.com>
 <20220820003125.353570-2-bhe@redhat.com>
 <d5272f42-f3e3-b2a8-428e-bd7815cf7518@csgroup.eu>
 <YwQdRg/IS0+3tbNu@MiWiFi-R3L-srv>
 <a4a9ba6f-9891-cc4c-e512-d221141d998f@csgroup.eu>
 <YwTu1kzcIUxsTL4j@MiWiFi-R3L-srv>
 <429cb367-923f-bb3d-ccf0-57dce0c7f35b@csgroup.eu>
 <b193efbff3a74343839c3dee1702f73c@AcuMS.aculab.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <b193efbff3a74343839c3dee1702f73c@AcuMS.aculab.com>
Precedence: bulk

Hi David,

On 08/24/22 at 08:16am, David Laight 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.
> 
> I think you also need to summarise the change itself.
> If the success/fail return actually changes then you really
> need to change something so the compiler errors unchanged code.
> Otherwise it is a complete recipe for disaster.

Thanks for looking into this and sorry for late response.

I am not sure if I follow you. In this patch, I just rename the old
ioremap_allowed() to arch_ioremap(), and change its return value. Except
of arm64 which has taken GENERIC_IOREMAP way to provide
ioremap_allowed(), no other ARCHes are affected yet. This is what have
been changed. Could you be more specific what I should add?

Or are you suggesting words like below sentences need be added to patch
log?

If the success/fail return actually changes then you really
need to change something so the compiler errors unchanged code.
Otherwise it is a complete recipe for disaster.

Thanks
Baoquan