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@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. Thanks Baoquan
