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