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

[Baoquan He]

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


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

Sure. After we decide the hooks, I will update the log accordingly.

> 
> > 
> >>
> >>
> >>>
> >>> ===
> >>>    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@xxxxxxxxxx/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.

OK, I thought the function interface change is not related to functional
change. I will remove the 'no functionality change' sentence to avoid
misleading. Thanks.