[PATCH 0/4] iomap: fix multiple consistency issues, interface cleanup

[PATCH 0/4] iomap: fix multiple consistency issues, interface cleanup

[Hugo Lefeuvre]

Hi,

This patch cleans up the iomap interface.

The first patch makes the include/asm-generic/iomap.h header compliant
with the kernel style guidelines by adding missing function args
identifier names.

The second and fourth patches address multiple compilation warnings due
to missing const qualifiers in mmio_ins* and ioread*/iowrite
definitions in include/asm-generic/iomap.h and lib/iomap.c.

The third patch modifies io*_rep definitions from asm-generic/io.h to
take unsigned long count parameter instead of unsigned int which is
inconsistent with other definitions in the kernel.

Hugo Lefeuvre (4):
  iomap: add missing function args identifier names
  iomap: add missing const to ioread*/iowrite addr arg
  io: change io*_rep definitions to take ulong count
  lib/iomap: add missing const to mmio_ins* addr arg

 include/asm-generic/io.h    | 16 ++++++++--------
 include/asm-generic/iomap.h | 38 ++++++++++++++++++-------------------
 lib/iomap.c                 | 22 ++++++++++-----------
 3 files changed, 38 insertions(+), 38 deletions(-)

-- 
2.20.1

[PATCH 1/4] iomap: add missing function args identifier names

[Hugo Lefeuvre]

Add missing function arguments identifier names to asm-generic/iomap.h
definitions. This addresses multiple checkpatch.pl code style
warnings.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 include/asm-generic/iomap.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 5b63b94ef6b5..7e26f21b466f 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -26,24 +26,24 @@
  * in the low address range. Architectures for which this is not
  * true can't use this generic implementation.
  */
-extern unsigned int ioread8(void __iomem *);
-extern unsigned int ioread16(void __iomem *);
-extern unsigned int ioread16be(void __iomem *);
-extern unsigned int ioread32(void __iomem *);
-extern unsigned int ioread32be(void __iomem *);
+extern unsigned int ioread8(void __iomem *addr);
+extern unsigned int ioread16(void __iomem *addr);
+extern unsigned int ioread16be(void __iomem *addr);
+extern unsigned int ioread32(void __iomem *addr);
+extern unsigned int ioread32be(void __iomem *addr);
 #ifdef CONFIG_64BIT
-extern u64 ioread64(void __iomem *);
-extern u64 ioread64be(void __iomem *);
+extern u64 ioread64(void __iomem *addr);
+extern u64 ioread64be(void __iomem *addr);
 #endif
 
-extern void iowrite8(u8, void __iomem *);
-extern void iowrite16(u16, void __iomem *);
-extern void iowrite16be(u16, void __iomem *);
-extern void iowrite32(u32, void __iomem *);
-extern void iowrite32be(u32, void __iomem *);
+extern void iowrite8(u8 value, void __iomem *addr);
+extern void iowrite16(u16 value, void __iomem *addr);
+extern void iowrite16be(u16 value, void __iomem *addr);
+extern void iowrite32(u32 value, void __iomem *addr);
+extern void iowrite32be(u32 value, void __iomem *addr);
 #ifdef CONFIG_64BIT
-extern void iowrite64(u64, void __iomem *);
-extern void iowrite64be(u64, void __iomem *);
+extern void iowrite64(u64 value, void __iomem *addr);
+extern void iowrite64be(u64 value, void __iomem *addr);
 #endif
 
 /*
@@ -68,7 +68,7 @@ extern void iowrite32_rep(void __iomem *port, const void *buf, unsigned long cou
 #ifdef CONFIG_HAS_IOPORT_MAP
 /* Create a virtual mapping cookie for an IO port range */
 extern void __iomem *ioport_map(unsigned long port, unsigned int nr);
-extern void ioport_unmap(void __iomem *);
+extern void ioport_unmap(void __iomem *addr);
 #endif
 
 #ifndef ARCH_HAS_IOREMAP_WC
@@ -82,7 +82,7 @@ extern void ioport_unmap(void __iomem *);
 #ifdef CONFIG_PCI
 /* Destroy a virtual mapping cookie for a PCI BAR (memory or IO) */
 struct pci_dev;
-extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
+extern void pci_iounmap(struct pci_dev *dev, void __iomem *addr);
 #elif defined(CONFIG_GENERIC_IOMAP)
 struct pci_dev;
 static inline void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
-- 
2.20.1

[PATCH 2/4] iomap: add missing const to ioread*/iowrite addr arg

[Hugo Lefeuvre]

ioread* and iowrite* definitions from asm-generic/iomap.h and
lib/iomap.c are missing const qualifiers. This is inconsistent with
the definitions from asm-generic/io.h and results in compilation
warnings when compiling drivers.

Add missing const qualifiers.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 include/asm-generic/iomap.h | 20 ++++++++++----------
 lib/iomap.c                 | 16 ++++++++--------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index 7e26f21b466f..cd92d32a4d87 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -26,14 +26,14 @@
  * in the low address range. Architectures for which this is not
  * true can't use this generic implementation.
  */
-extern unsigned int ioread8(void __iomem *addr);
-extern unsigned int ioread16(void __iomem *addr);
-extern unsigned int ioread16be(void __iomem *addr);
-extern unsigned int ioread32(void __iomem *addr);
-extern unsigned int ioread32be(void __iomem *addr);
+extern unsigned int ioread8(const void __iomem *addr);
+extern unsigned int ioread16(const void __iomem *addr);
+extern unsigned int ioread16be(const void __iomem *addr);
+extern unsigned int ioread32(const void __iomem *addr);
+extern unsigned int ioread32be(const void __iomem *addr);
 #ifdef CONFIG_64BIT
-extern u64 ioread64(void __iomem *addr);
-extern u64 ioread64be(void __iomem *addr);
+extern u64 ioread64(const void __iomem *addr);
+extern u64 ioread64be(const void __iomem *addr);
 #endif
 
 extern void iowrite8(u8 value, void __iomem *addr);
@@ -57,9 +57,9 @@ extern void iowrite64be(u64 value, void __iomem *addr);
  * memory across multiple ports, use "memcpy_toio()"
  * and friends.
  */
-extern void ioread8_rep(void __iomem *port, void *buf, unsigned long count);
-extern void ioread16_rep(void __iomem *port, void *buf, unsigned long count);
-extern void ioread32_rep(void __iomem *port, void *buf, unsigned long count);
+extern void ioread8_rep(const void __iomem *port, void *buf, unsigned long count);
+extern void ioread16_rep(const void __iomem *port, void *buf, unsigned long count);
+extern void ioread32_rep(const void __iomem *port, void *buf, unsigned long count);
 
 extern void iowrite8_rep(void __iomem *port, const void *buf, unsigned long count);
 extern void iowrite16_rep(void __iomem *port, const void *buf, unsigned long count);
diff --git a/lib/iomap.c b/lib/iomap.c
index 541d926da95e..8cc3270697e9 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -69,27 +69,27 @@ static void bad_io_access(unsigned long port, const char *access)
 #define mmio_read32be(addr) be32_to_cpu(__raw_readl(addr))
 #endif
 
-unsigned int ioread8(void __iomem *addr)
+unsigned int ioread8(const void __iomem *addr)
 {
 	IO_COND(addr, return inb(port), return readb(addr));
 	return 0xff;
 }
-unsigned int ioread16(void __iomem *addr)
+unsigned int ioread16(const void __iomem *addr)
 {
 	IO_COND(addr, return inw(port), return readw(addr));
 	return 0xffff;
 }
-unsigned int ioread16be(void __iomem *addr)
+unsigned int ioread16be(const void __iomem *addr)
 {
 	IO_COND(addr, return pio_read16be(port), return mmio_read16be(addr));
 	return 0xffff;
 }
-unsigned int ioread32(void __iomem *addr)
+unsigned int ioread32(const void __iomem *addr)
 {
 	IO_COND(addr, return inl(port), return readl(addr));
 	return 0xffffffff;
 }
-unsigned int ioread32be(void __iomem *addr)
+unsigned int ioread32be(const void __iomem *addr)
 {
 	IO_COND(addr, return pio_read32be(port), return mmio_read32be(addr));
 	return 0xffffffff;
@@ -193,15 +193,15 @@ static inline void mmio_outsl(void __iomem *addr, const u32 *src, int count)
 }
 #endif
 
-void ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
+void ioread8_rep(const void __iomem *addr, void *dst, unsigned long count)
 {
 	IO_COND(addr, insb(port,dst,count), mmio_insb(addr, dst, count));
 }
-void ioread16_rep(void __iomem *addr, void *dst, unsigned long count)
+void ioread16_rep(const void __iomem *addr, void *dst, unsigned long count)
 {
 	IO_COND(addr, insw(port,dst,count), mmio_insw(addr, dst, count));
 }
-void ioread32_rep(void __iomem *addr, void *dst, unsigned long count)
+void ioread32_rep(const void __iomem *addr, void *dst, unsigned long count)
 {
 	IO_COND(addr, insl(port,dst,count), mmio_insl(addr, dst, count));
 }
-- 
2.20.1

[PATCH 3/4] io: change io*_rep definitions to take ulong count

[Hugo Lefeuvre]

ioread*_rep and iowrite*_rep from asm-generic/io.h expect unsigned
int count parameter. This is inconsistent with all other definitions
in the kernel which take unsigned long count.

Change io*_rep definitions to take unsigned long count instead of
unsigned int.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 include/asm-generic/io.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index d356f802945a..345ae6a09bca 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -811,7 +811,7 @@ static inline void iowrite64be(u64 value, volatile void __iomem *addr)
 #ifndef ioread8_rep
 #define ioread8_rep ioread8_rep
 static inline void ioread8_rep(const volatile void __iomem *addr, void *buffer,
-			       unsigned int count)
+			       unsigned long count)
 {
 	readsb(addr, buffer, count);
 }
@@ -820,7 +820,7 @@ static inline void ioread8_rep(const volatile void __iomem *addr, void *buffer,
 #ifndef ioread16_rep
 #define ioread16_rep ioread16_rep
 static inline void ioread16_rep(const volatile void __iomem *addr,
-				void *buffer, unsigned int count)
+				void *buffer, unsigned long count)
 {
 	readsw(addr, buffer, count);
 }
@@ -829,7 +829,7 @@ static inline void ioread16_rep(const volatile void __iomem *addr,
 #ifndef ioread32_rep
 #define ioread32_rep ioread32_rep
 static inline void ioread32_rep(const volatile void __iomem *addr,
-				void *buffer, unsigned int count)
+				void *buffer, unsigned long count)
 {
 	readsl(addr, buffer, count);
 }
@@ -839,7 +839,7 @@ static inline void ioread32_rep(const volatile void __iomem *addr,
 #ifndef ioread64_rep
 #define ioread64_rep ioread64_rep
 static inline void ioread64_rep(const volatile void __iomem *addr,
-				void *buffer, unsigned int count)
+				void *buffer, unsigned long count)
 {
 	readsq(addr, buffer, count);
 }
@@ -850,7 +850,7 @@ static inline void ioread64_rep(const volatile void __iomem *addr,
 #define iowrite8_rep iowrite8_rep
 static inline void iowrite8_rep(volatile void __iomem *addr,
 				const void *buffer,
-				unsigned int count)
+				unsigned long count)
 {
 	writesb(addr, buffer, count);
 }
@@ -860,7 +860,7 @@ static inline void iowrite8_rep(volatile void __iomem *addr,
 #define iowrite16_rep iowrite16_rep
 static inline void iowrite16_rep(volatile void __iomem *addr,
 				 const void *buffer,
-				 unsigned int count)
+				 unsigned long count)
 {
 	writesw(addr, buffer, count);
 }
@@ -870,7 +870,7 @@ static inline void iowrite16_rep(volatile void __iomem *addr,
 #define iowrite32_rep iowrite32_rep
 static inline void iowrite32_rep(volatile void __iomem *addr,
 				 const void *buffer,
-				 unsigned int count)
+				 unsigned long count)
 {
 	writesl(addr, buffer, count);
 }
@@ -881,7 +881,7 @@ static inline void iowrite32_rep(volatile void __iomem *addr,
 #define iowrite64_rep iowrite64_rep
 static inline void iowrite64_rep(volatile void __iomem *addr,
 				 const void *buffer,
-				 unsigned int count)
+				 unsigned long count)
 {
 	writesq(addr, buffer, count);
 }
-- 
2.20.1

[PATCH 4/4] lib/iomap: add missing const to mmio_ins* addr arg

[Hugo Lefeuvre]

mmio_ins* definitions from lib/iomap.c are missing const qualifiers
for the addr argument. This results in compilation warnings when
compiling drivers.

Add missing const qualifiers.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 lib/iomap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/iomap.c b/lib/iomap.c
index 8cc3270697e9..2bcc5d9d30b1 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -143,7 +143,7 @@ EXPORT_SYMBOL(iowrite32be);
  * order" (we also don't have IO barriers).
  */
 #ifndef mmio_insb
-static inline void mmio_insb(void __iomem *addr, u8 *dst, int count)
+static inline void mmio_insb(const void __iomem *addr, u8 *dst, int count)
 {
 	while (--count >= 0) {
 		u8 data = __raw_readb(addr);
@@ -151,7 +151,7 @@ static inline void mmio_insb(void __iomem *addr, u8 *dst, int count)
 		dst++;
 	}
 }
-static inline void mmio_insw(void __iomem *addr, u16 *dst, int count)
+static inline void mmio_insw(const void __iomem *addr, u16 *dst, int count)
 {
 	while (--count >= 0) {
 		u16 data = __raw_readw(addr);
@@ -159,7 +159,7 @@ static inline void mmio_insw(void __iomem *addr, u16 *dst, int count)
 		dst++;
 	}
 }
-static inline void mmio_insl(void __iomem *addr, u32 *dst, int count)
+static inline void mmio_insl(const void __iomem *addr, u32 *dst, int count)
 {
 	while (--count >= 0) {
 		u32 data = __raw_readl(addr);
-- 
2.20.1

Re: [PATCH 0/4] iomap: fix multiple consistency issues, interface cleanup

[Arnd Bergmann]

On Mon, Feb 18, 2019 at 11:53 AM Hugo Lefeuvre <hle@owl.eu.com> wrote:
>
> Hi,
>
> This patch cleans up the iomap interface.
>
> The first patch makes the include/asm-generic/iomap.h header compliant
> with the kernel style guidelines by adding missing function args
> identifier names.
>
> The second and fourth patches address multiple compilation warnings due
> to missing const qualifiers in mmio_ins* and ioread*/iowrite
> definitions in include/asm-generic/iomap.h and lib/iomap.c.
>
> The third patch modifies io*_rep definitions from asm-generic/io.h to
> take unsigned long count parameter instead of unsigned int which is
> inconsistent with other definitions in the kernel.

Nice cleanup! Applied to my asm-generic tree now.

I notice that you did not add a 'volatile' qualifier in addition to 'const'.
Is that something you considered doing? I think the traditional
readl/writel style accessors have them partly for historic reasons,
and partly to simply the trivial implementation that is based on
a volatile pointer dereference. Neither is needed here, and I don't
think we have any drivers that pass volatile pointers into ioread()/iowrite()
(which would cause a warning), so we can probably leave it at that,
but it's still a bit inconsistent.

    Arnd

Re: [PATCH 0/4] iomap: fix multiple consistency issues, interface cleanup

[Hugo Lefeuvre]

Hi,

> Nice cleanup! Applied to my asm-generic tree now.

Thanks!

> I notice that you did not add a 'volatile' qualifier in addition to 'const'.
> Is that something you considered doing? I think the traditional
> readl/writel style accessors have them partly for historic reasons,
> and partly to simply the trivial implementation that is based on
> a volatile pointer dereference. Neither is needed here, and I don't
> think we have any drivers that pass volatile pointers into ioread()/iowrite()
> (which would cause a warning), so we can probably leave it at that,
> but it's still a bit inconsistent.

Right, I intentionally omitted the volatile qualifier here.

Adding it to the definitions from asm-generic/iomap.h would require to
also update lib/iomap.c and arch/ implementations. This should, imo, only
be done if really necessary.

From what I've seen, almost all ioread() implementations simply redirect
to readl style accessors. I could not find one implementation that does
something with the addr argument which requires the volatile qualifier.

Also, I'm not even sure to understand why we need the volatile qualifier
in the ioread() definitions from asm-generic/io.h...

Drivers wishing to pass volatile pointers to ioread() should probably use
a more low level api.

regards,
 Hugo

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C