| Message ID | 20220820003125.353570-2-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: ioremap: Convert architectures to take GENERIC_IOREMAP way | expand |
On Sat, Aug 20, 2022 at 08:31:15AM +0800, Baoquan He wrote: > +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); Please avoid the overly long lines. I also wonder if we just want a common definition with a __weak default instead of duplicating it in many arch headers. > + ioaddr = arch_ioremap(phys_addr, size, prot); > + if (IS_ERR(ioaddr)) > + return NULL; > + else if (ioaddr) > + return ioaddr; No need for the else here.
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. Please don't just say you need to change the return value. Explain why. 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 ? > > === > arch_ioremap() return a bool, It is not a bool. A bool is either true or false. > - 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. > - 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 ? > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > arch/arm64/include/asm/io.h | 4 ++-- > arch/arm64/mm/ioremap.c | 15 ++++++++++----- > include/asm-generic/io.h | 29 +++++++++++++++-------------- > mm/ioremap.c | 12 ++++++++---- > 4 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index 877495a0fd0c..dd7e1c2dc86c 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -139,8 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t); > * I/O memory mapping functions. > */ > > -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot); > -#define ioremap_allowed ioremap_allowed > +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); > +#define arch_ioremap arch_ioremap > > #define _PAGE_IOREMAP PROT_DEVICE_nGnRE > > diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c > index c5af103d4ad4..b0f4cea86f0e 100644 > --- a/arch/arm64/mm/ioremap.c > +++ b/arch/arm64/mm/ioremap.c > @@ -3,19 +3,24 @@ > #include <linux/mm.h> > #include <linux/io.h> > > -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot) > +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) > { > - unsigned long last_addr = phys_addr + size - 1; > + unsigned long last_addr, offset; > + > + offset = phys_addr & (~PAGE_MASK); > + phys_addr -= offset; > + size = PAGE_ALIGN(size + offset); > + last_addr = phys_addr + size - 1; > > /* Don't allow outside PHYS_MASK */ > if (last_addr & ~PHYS_MASK) > - return false; > + return IOMEM_ERR_PTR(-EINVAL); > > /* Don't allow RAM to be mapped. */ > if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr)))) > - return false; > + return IOMEM_ERR_PTR(-EINVAL); > > - return true; > + return NULL; > } > > /* > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index a68f8fbf423b..7b6bfb62ef80 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -1049,27 +1049,28 @@ static inline void iounmap(volatile void __iomem *addr) > > /* > * Arch code can implement the following two hooks when using GENERIC_IOREMAP > - * ioremap_allowed() return a bool, > - * - true means continue to remap > - * - false means skip remap and return directly > - * iounmap_allowed() return a bool, > - * - true means continue to vunmap > - * - false means skip vunmap and return directly > + * arch_ioremap() return a bool, > + * - 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, > + * - 0 means continue to vunmap > + * - error code means skip vunmap and return directly > */ > -#ifndef ioremap_allowed > -#define ioremap_allowed ioremap_allowed > -static inline bool ioremap_allowed(phys_addr_t phys_addr, size_t size, > +#ifndef arch_ioremap > +#define arch_ioremap arch_ioremap > +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, > unsigned long prot) > { > - return true; > + return NULL; > } > #endif > > -#ifndef iounmap_allowed > -#define iounmap_allowed iounmap_allowed > -static inline bool iounmap_allowed(void *addr) > +#ifndef arch_iounmap > +#define arch_iounmap arch_iounmap > +static inline int arch_iounmap(void __iomem *addr) > { > - return true; > + return 0; > } > #endif > > diff --git a/mm/ioremap.c b/mm/ioremap.c > index 8652426282cc..99fde69becc7 100644 > --- a/mm/ioremap.c > +++ b/mm/ioremap.c > @@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > unsigned long offset, vaddr; > phys_addr_t last_addr; > struct vm_struct *area; > + void __iomem *ioaddr; > + > + ioaddr = arch_ioremap(phys_addr, size, prot); > + if (IS_ERR(ioaddr)) > + return NULL; > + else if (ioaddr) > + return ioaddr; > > /* Disallow wrap-around or zero size */ > last_addr = phys_addr + size - 1; > @@ -28,9 +35,6 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > phys_addr -= offset; > size = PAGE_ALIGN(size + offset); > > - if (!ioremap_allowed(phys_addr, size, prot)) > - return NULL; > - > area = get_vm_area_caller(size, VM_IOREMAP, > __builtin_return_address(0)); > if (!area) > @@ -52,7 +56,7 @@ void iounmap(volatile void __iomem *addr) > { > void *vaddr = (void *)((unsigned long)addr & PAGE_MASK); > > - if (!iounmap_allowed(vaddr)) > + if (arch_iounmap((void __iomem *)addr)) > return; > > if (is_vmalloc_addr(vaddr))
On 08/20/22 at 11:53pm, Christoph Hellwig wrote: > On Sat, Aug 20, 2022 at 08:31:15AM +0800, Baoquan He wrote: > > +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); > > Please avoid the overly long lines. Thanks for reviewing. Will break the line. > > I also wonder if we just want a common definition with a __weak default > instead of duplicating it in many arch headers. Seems __weak symbol is not suggested any more in kernel. Please see below thread. [PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add] https://lore.kernel.org/all/20220518181828.645877-1-naveen.n.rao@linux.vnet.ibm.com/T/#u > > > + ioaddr = arch_ioremap(phys_addr, size, prot); > > + if (IS_ERR(ioaddr)) > > + return NULL; > > + else if (ioaddr) > > + return ioaddr; > > No need for the else here. Do you mean changing it like this? It's fine to me if I get it correctly. ioaddr = arch_ioremap(phys_addr, size, prot); if (IS_ERR(ioaddr)) return NULL; if (ioaddr) return ioaddr; Thanks Baoquan
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
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@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. 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 >
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@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. > > 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.
Le 23/08/2022 à 17:14, Baoquan He a écrit : > 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. > ==== > That looks more in line with the type of explanation I foresee in the commit message, thanks. Christophe
From: Christophe Leroy > Sent: 23 August 2022 16:26 > > Le 23/08/2022 à 17:14, Baoquan He a écrit : > > 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. > > ==== > > > > 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. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sat, Aug 20, 2022 at 08:31:15AM +0800, Baoquan He wrote: Hi Baoquan, > arch_ioremap() return a bool, > - 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, > - 0 means continue to vunmap > - error code means skip vunmap and return directly It would make more sense if the return values were described from the prospective of an architecture, not the caller. I.e true - unmapped, false - not supported, etc. > diff --git a/mm/ioremap.c b/mm/ioremap.c > index 8652426282cc..99fde69becc7 100644 > --- a/mm/ioremap.c > +++ b/mm/ioremap.c > @@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > unsigned long offset, vaddr; > phys_addr_t last_addr; > struct vm_struct *area; > + void __iomem *ioaddr; > + > + ioaddr = arch_ioremap(phys_addr, size, prot); > + if (IS_ERR(ioaddr)) > + return NULL; > + else if (ioaddr) > + return ioaddr; It seems to me arch_ioremap() could simply return an address or an error. Then IOMEM_ERR_PTR(-ENOSYS) if the architecture does not support it reads much better than the cryptic NULL. Probably arch_iounmap() returning error would look better too, though not sure about that. Thanks!
On 08/28/22 at 10:36am, Alexander Gordeev wrote: > On Sat, Aug 20, 2022 at 08:31:15AM +0800, Baoquan He wrote: > > Hi Baoquan, > > > arch_ioremap() return a bool, > > - 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, > > - 0 means continue to vunmap > > - error code means skip vunmap and return directly > > It would make more sense if the return values were described > from the prospective of an architecture, not the caller. > I.e true - unmapped, false - not supported, etc. Yes, sounds reasonable to me, thanks. While ChristopheL suggested to take another way. Please see below link. I will reply to Christophe to discuss that. https://lore.kernel.org/all/8df89136-a7f2-9b66-d522-a4fb9860bf22@csgroup.eu/T/#u If the current arch_ioremap() way is taken, I will change the description as you said. > > > diff --git a/mm/ioremap.c b/mm/ioremap.c > > index 8652426282cc..99fde69becc7 100644 > > --- a/mm/ioremap.c > > +++ b/mm/ioremap.c > > @@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > > unsigned long offset, vaddr; > > phys_addr_t last_addr; > > struct vm_struct *area; > > + void __iomem *ioaddr; > > + > > + ioaddr = arch_ioremap(phys_addr, size, prot); > > + if (IS_ERR(ioaddr)) > > + return NULL; > > + else if (ioaddr) > > + return ioaddr; > > It seems to me arch_ioremap() could simply return an address > or an error. Then IOMEM_ERR_PTR(-ENOSYS) if the architecture > does not support it reads much better than the cryptic NULL. I may not follow. Returning NULL means arch_ioremap() doesn't give out a mapped address and doesn't encounter wrong thing. NULL is a little twisting, maybe '0' is better? > > Probably arch_iounmap() returning error would look better too, > though not sure about that. Don't follow either. arch_iounmap() is returning error now.
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
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index 877495a0fd0c..dd7e1c2dc86c 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -139,8 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t); * I/O memory mapping functions. */ -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot); -#define ioremap_allowed ioremap_allowed +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); +#define arch_ioremap arch_ioremap #define _PAGE_IOREMAP PROT_DEVICE_nGnRE diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c index c5af103d4ad4..b0f4cea86f0e 100644 --- a/arch/arm64/mm/ioremap.c +++ b/arch/arm64/mm/ioremap.c @@ -3,19 +3,24 @@ #include <linux/mm.h> #include <linux/io.h> -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot) +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) { - unsigned long last_addr = phys_addr + size - 1; + unsigned long last_addr, offset; + + offset = phys_addr & (~PAGE_MASK); + phys_addr -= offset; + size = PAGE_ALIGN(size + offset); + last_addr = phys_addr + size - 1; /* Don't allow outside PHYS_MASK */ if (last_addr & ~PHYS_MASK) - return false; + return IOMEM_ERR_PTR(-EINVAL); /* Don't allow RAM to be mapped. */ if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr)))) - return false; + return IOMEM_ERR_PTR(-EINVAL); - return true; + return NULL; } /* diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index a68f8fbf423b..7b6bfb62ef80 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -1049,27 +1049,28 @@ static inline void iounmap(volatile void __iomem *addr) /* * Arch code can implement the following two hooks when using GENERIC_IOREMAP - * ioremap_allowed() return a bool, - * - true means continue to remap - * - false means skip remap and return directly - * iounmap_allowed() return a bool, - * - true means continue to vunmap - * - false means skip vunmap and return directly + * arch_ioremap() return a bool, + * - 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, + * - 0 means continue to vunmap + * - error code means skip vunmap and return directly */ -#ifndef ioremap_allowed -#define ioremap_allowed ioremap_allowed -static inline bool ioremap_allowed(phys_addr_t phys_addr, size_t size, +#ifndef arch_ioremap +#define arch_ioremap arch_ioremap +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) { - return true; + return NULL; } #endif -#ifndef iounmap_allowed -#define iounmap_allowed iounmap_allowed -static inline bool iounmap_allowed(void *addr) +#ifndef arch_iounmap +#define arch_iounmap arch_iounmap +static inline int arch_iounmap(void __iomem *addr) { - return true; + return 0; } #endif diff --git a/mm/ioremap.c b/mm/ioremap.c index 8652426282cc..99fde69becc7 100644 --- a/mm/ioremap.c +++ b/mm/ioremap.c @@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long offset, vaddr; phys_addr_t last_addr; struct vm_struct *area; + void __iomem *ioaddr; + + ioaddr = arch_ioremap(phys_addr, size, prot); + if (IS_ERR(ioaddr)) + return NULL; + else if (ioaddr) + return ioaddr; /* Disallow wrap-around or zero size */ last_addr = phys_addr + size - 1; @@ -28,9 +35,6 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, phys_addr -= offset; size = PAGE_ALIGN(size + offset); - if (!ioremap_allowed(phys_addr, size, prot)) - return NULL; - area = get_vm_area_caller(size, VM_IOREMAP, __builtin_return_address(0)); if (!area) @@ -52,7 +56,7 @@ void iounmap(volatile void __iomem *addr) { void *vaddr = (void *)((unsigned long)addr & PAGE_MASK); - if (!iounmap_allowed(vaddr)) + if (arch_iounmap((void __iomem *)addr)) return; if (is_vmalloc_addr(vaddr))
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. === arch_ioremap() return a bool, - 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, - 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. Signed-off-by: Baoquan He <bhe@redhat.com> --- arch/arm64/include/asm/io.h | 4 ++-- arch/arm64/mm/ioremap.c | 15 ++++++++++----- include/asm-generic/io.h | 29 +++++++++++++++-------------- mm/ioremap.c | 12 ++++++++---- 4 files changed, 35 insertions(+), 25 deletions(-)