2023-03-08 13:11:42

by Baoquan He

[permalink] [raw]
Subject: [PATCH v4 0/4] arch/*/io.h: remove ioremap_uc in some architectures

This patchset tries to remove ioremap_uc() in the current architectures
except of x86 and ia64. They will use the default ioremap_uc version
in <asm-generic/io.h> which returns NULL. Anyone who wants to add new
invocation of ioremap_uc(), please consider using ioremap() instead or
adding a new ARCH specific ioremap_uc(), or refer to the callsite
in drivers/video/fbdev/aty/atyfb_base.c.

This change won't cuase breakage to the current kernel because in the
only ioremap_uc callsite, an adjustment is made to eliminate impact in
patch 1 of this series.

To get rid of all of them other than x86 and ia64, add asm-generic/io.h
to asm/io.h of mips ARCH. With this adding, we can get rid of the
ioremap_uc() in mips too. This is done in patch 2. And a followup patch
4 is added to remove duplicated code according to Arnd's suggestion.

v3->v4:
- Add patch 1 to adjust code in the only ioremap_uc() callsite so that
later removing ioremap_uc() won't cause breakage.
- Update log and document writing in patch 3.
- Add followup patch 4 to clean up duplicated code in asm/io.h of MIPS.
v2->v3:
- In patch 1, move those macro definition of functio near its function
declaration according to Arnd's suggestion. And remove the unneeded
change in asm/mmiowb.h introduced in old version.
- In patch 2, clean up and rewrite the messy document related to
ioremap_uc() in Documentation/driver-api/device-io.rst.
v1->v2:
- Update log of patch 2, and document related to ioremap_uc()
according to Geert's comment.
- Add Geert's Acked-by.

Arnd Bergmann (1):
video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64

Baoquan He (3):
mips: add <asm-generic/io.h> including
arch/*/io.h: remove ioremap_uc in some architectures
mips: io: remove duplicated codes

Documentation/driver-api/device-io.rst | 9 +-
arch/alpha/include/asm/io.h | 1 -
arch/hexagon/include/asm/io.h | 3 -
arch/m68k/include/asm/kmap.h | 1 -
arch/mips/include/asm/io.h | 112 +++++++++++++++----------
arch/parisc/include/asm/io.h | 2 -
arch/powerpc/include/asm/io.h | 1 -
arch/sh/include/asm/io.h | 2 -
arch/sparc/include/asm/io_64.h | 1 -
drivers/video/fbdev/aty/atyfb_base.c | 4 +
10 files changed, 78 insertions(+), 58 deletions(-)

--
2.34.1



2023-03-08 13:11:47

by Baoquan He

[permalink] [raw]
Subject: [PATCH v4 2/4] mips: add <asm-generic/io.h> including

With the adding, some default ioremap_xx methods defined in
asm-generic/io.h can be used. E.g the default ioremap_uc() returning
NULL.

Signed-off-by: Baoquan He <[email protected]>
Reviewed-by: Arnd Bergmann <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Serge Semin <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Huacai Chen <[email protected]>
Cc: Jiaxun Yang <[email protected]>
Cc: [email protected]
---
arch/mips/include/asm/io.h | 78 ++++++++++++++++++++++++++++++++++----
1 file changed, 70 insertions(+), 8 deletions(-)

diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
index cec8347f0b85..6756baadba6c 100644
--- a/arch/mips/include/asm/io.h
+++ b/arch/mips/include/asm/io.h
@@ -126,6 +126,7 @@ static inline phys_addr_t virt_to_phys(const volatile void *x)
* almost all conceivable cases a device driver should not be using
* this function
*/
+#define phys_to_virt phys_to_virt
static inline void * phys_to_virt(unsigned long address)
{
return __va(address);
@@ -359,6 +360,27 @@ __BUILD_MEMORY_PFX(__raw_, q, u64, 0)
__BUILD_MEMORY_PFX(__mem_, q, u64, 0)
#endif

+#define readb readb
+#define readw readw
+#define readl readl
+#define writeb writeb
+#define writew writew
+#define writel writel
+
+#ifdef CONFIG_64BIT
+#define readq readq
+#define writeq writeq
+#define __raw_readq __raw_readq
+#define __raw_writeq __raw_writeq
+#endif
+
+#define __raw_readb __raw_readb
+#define __raw_readw __raw_readw
+#define __raw_readl __raw_readl
+#define __raw_writeb __raw_writeb
+#define __raw_writew __raw_writew
+#define __raw_writel __raw_writel
+
#define __BUILD_IOPORT_PFX(bus, bwlq, type) \
__BUILD_IOPORT_SINGLE(bus, bwlq, type, 1, 0,) \
__BUILD_IOPORT_SINGLE(bus, bwlq, type, 1, 0, _p)
@@ -374,6 +396,27 @@ BUILDIO_IOPORT(l, u32)
BUILDIO_IOPORT(q, u64)
#endif

+#define inb inb
+#define inw inw
+#define inl inl
+#define inb_p inb_p
+#define inw_p inw_p
+#define inl_p inl_p
+
+#define outb outb
+#define outw outw
+#define outl outl
+#define outb_p outb_p
+#define outw_p outw_p
+#define outl_p outl_p
+
+#ifdef CONFIG_64BIT
+#define inq inq
+#define outq outq
+#define inq_p inq_p
+#define outq_p outq_p
+#endif
+
#define __BUILDIO(bwlq, type) \
\
__BUILD_MEMORY_SINGLE(____raw_, bwlq, type, 1, 0, 0)
@@ -412,14 +455,6 @@ __BUILDIO(q, u64)
#define writeq_be(val, addr) \
__raw_writeq(cpu_to_be64((val)), (__force unsigned *)(addr))

-/*
- * Some code tests for these symbols
- */
-#ifdef CONFIG_64BIT
-#define readq readq
-#define writeq writeq
-#endif
-
#define __BUILD_MEMORY_STRING(bwlq, type) \
\
static inline void writes##bwlq(volatile void __iomem *mem, \
@@ -480,14 +515,39 @@ BUILDSTRING(l, u32)
BUILDSTRING(q, u64)
#endif

+#define insb insb
+#define insw insw
+#define insl insl
+#define outsb outsb
+#define outsw outsw
+#define outsl outsl
+
+#define readsb readsb
+#define readsw readsw
+#define readsl readsl
+#define writesb writesb
+#define writesw writesw
+#define writesl writesl
+
+#ifdef CONFIG_64BIT
+#define insq insq
+#define readsq readsq
+#define readsq readsq
+#define writesq writesq
+#endif
+
+
+#define memset_io memset_io
static inline void memset_io(volatile void __iomem *addr, unsigned char val, int count)
{
memset((void __force *) addr, val, count);
}
+#define memcpy_fromio memcpy_fromio
static inline void memcpy_fromio(void *dst, const volatile void __iomem *src, int count)
{
memcpy(dst, (void __force *) src, count);
}
+#define memcpy_toio memcpy_toio
static inline void memcpy_toio(volatile void __iomem *dst, const void *src, int count)
{
memcpy((void __force *) dst, src, count);
@@ -556,4 +616,6 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size);

void __ioread64_copy(void *to, const void __iomem *from, size_t count);

+#include <asm-generic/io.h>
+
#endif /* _ASM_IO_H */
--
2.34.1


2023-03-08 13:12:30

by Baoquan He

[permalink] [raw]
Subject: [PATCH v4 3/4] arch/*/io.h: remove ioremap_uc in some architectures

ioremap_uc() is only meaningful on old x86-32 systems with the PAT
extension, and on ia64 with its slightly unconventional ioremap()
behavior. So remove the ioremap_uc() definition in architecutures
other than x86 and ia64. These architectures all have asm-generic/io.h
included and will have the default ioremap_uc() definition which
returns NULL.

This changes the existing behaviour, while no need to worry about
any breakage because in the only callsite of ioremap_uc(), code
has been adjusted to eliminate the impact. Please see
atyfb_setup_generic() of drivers/video/fbdev/aty/atyfb_base.c.

If any new invocation of ioremap_uc() need be added, please consider
using ioremap() intead or adding a ARCH specific version if necessary.

Acked-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Baoquan He <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Documentation/driver-api/device-io.rst | 9 +++++----
arch/alpha/include/asm/io.h | 1 -
arch/hexagon/include/asm/io.h | 3 ---
arch/m68k/include/asm/kmap.h | 1 -
arch/mips/include/asm/io.h | 1 -
arch/parisc/include/asm/io.h | 2 --
arch/powerpc/include/asm/io.h | 1 -
arch/sh/include/asm/io.h | 2 --
arch/sparc/include/asm/io_64.h | 1 -
9 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/Documentation/driver-api/device-io.rst b/Documentation/driver-api/device-io.rst
index 4d2baac0311c..d55384b106bd 100644
--- a/Documentation/driver-api/device-io.rst
+++ b/Documentation/driver-api/device-io.rst
@@ -408,11 +408,12 @@ functions for details on the CPU side of things.
ioremap_uc()
------------

-ioremap_uc() behaves like ioremap() except that on the x86 architecture without
-'PAT' mode, it marks memory as uncached even when the MTRR has designated
-it as cacheable, see Documentation/x86/pat.rst.
+ioremap_uc() is only meaningful on old x86-32 systems with the PAT extension,
+and on ia64 with its slightly unconventional ioremap() behavior, everywhere
+elss ioremap_uc() defaults to return NULL.

-Portable drivers should avoid the use of ioremap_uc().
+
+Portable drivers should avoid the use of ioremap_uc(), use ioremap() instead.

ioremap_cache()
---------------
diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
index 7aeaf7c30a6f..076f0e4e7f1e 100644
--- a/arch/alpha/include/asm/io.h
+++ b/arch/alpha/include/asm/io.h
@@ -308,7 +308,6 @@ static inline void __iomem *ioremap(unsigned long port, unsigned long size)
}

#define ioremap_wc ioremap
-#define ioremap_uc ioremap

static inline void iounmap(volatile void __iomem *addr)
{
diff --git a/arch/hexagon/include/asm/io.h b/arch/hexagon/include/asm/io.h
index dcd9cbbf5934..b9847472f25c 100644
--- a/arch/hexagon/include/asm/io.h
+++ b/arch/hexagon/include/asm/io.h
@@ -176,9 +176,6 @@ static inline void writel(u32 data, volatile void __iomem *addr)
#define _PAGE_IOREMAP (_PAGE_PRESENT | _PAGE_READ | _PAGE_WRITE | \
(__HEXAGON_C_DEV << 6))

-#define ioremap_uc(addr, size) ioremap((addr), (size))
-
-
#define __raw_writel writel

static inline void memcpy_fromio(void *dst, const volatile void __iomem *src,
diff --git a/arch/m68k/include/asm/kmap.h b/arch/m68k/include/asm/kmap.h
index 4efb3efa593a..b778f015c917 100644
--- a/arch/m68k/include/asm/kmap.h
+++ b/arch/m68k/include/asm/kmap.h
@@ -25,7 +25,6 @@ static inline void __iomem *ioremap(unsigned long physaddr, unsigned long size)
return __ioremap(physaddr, size, IOMAP_NOCACHE_SER);
}

-#define ioremap_uc ioremap
#define ioremap_wt ioremap_wt
static inline void __iomem *ioremap_wt(unsigned long physaddr,
unsigned long size)
diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
index 6756baadba6c..da0a625c3c6d 100644
--- a/arch/mips/include/asm/io.h
+++ b/arch/mips/include/asm/io.h
@@ -167,7 +167,6 @@ void iounmap(const volatile void __iomem *addr);
*/
#define ioremap(offset, size) \
ioremap_prot((offset), (size), _CACHE_UNCACHED)
-#define ioremap_uc ioremap

/*
* ioremap_cache - map bus memory into CPU space
diff --git a/arch/parisc/include/asm/io.h b/arch/parisc/include/asm/io.h
index 366537042465..48630c78714a 100644
--- a/arch/parisc/include/asm/io.h
+++ b/arch/parisc/include/asm/io.h
@@ -132,8 +132,6 @@ static inline void gsc_writeq(unsigned long long val, unsigned long addr)

#define ioremap_wc(addr, size) \
ioremap_prot((addr), (size), _PAGE_IOREMAP)
-#define ioremap_uc(addr, size) \
- ioremap_prot((addr), (size), _PAGE_IOREMAP)

#define pci_iounmap pci_iounmap

diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 978d687edf32..7873fc83c82c 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -863,7 +863,6 @@ void __iomem *ioremap_wt(phys_addr_t address, unsigned long size);
#endif

void __iomem *ioremap_coherent(phys_addr_t address, unsigned long size);
-#define ioremap_uc(addr, size) ioremap((addr), (size))
#define ioremap_cache(addr, size) \
ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))

diff --git a/arch/sh/include/asm/io.h b/arch/sh/include/asm/io.h
index b3a26b405c8d..12a892804082 100644
--- a/arch/sh/include/asm/io.h
+++ b/arch/sh/include/asm/io.h
@@ -278,8 +278,6 @@ unsigned long long poke_real_address_q(unsigned long long addr,
ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))
#endif /* CONFIG_MMU */

-#define ioremap_uc ioremap
-
/*
* Convert a physical pointer to a virtual kernel pointer for /dev/mem
* access
diff --git a/arch/sparc/include/asm/io_64.h b/arch/sparc/include/asm/io_64.h
index 9303270b22f3..d8ee1442f303 100644
--- a/arch/sparc/include/asm/io_64.h
+++ b/arch/sparc/include/asm/io_64.h
@@ -423,7 +423,6 @@ static inline void __iomem *ioremap(unsigned long offset, unsigned long size)
return (void __iomem *)offset;
}

-#define ioremap_uc(X,Y) ioremap((X),(Y))
#define ioremap_wc(X,Y) ioremap((X),(Y))
#define ioremap_wt(X,Y) ioremap((X),(Y))
static inline void __iomem *ioremap_np(unsigned long offset, unsigned long size)
--
2.34.1


2023-03-08 13:13:27

by Baoquan He

[permalink] [raw]
Subject: [PATCH v4 4/4] mips: io: remove duplicated codes

By adding asm-generic/io.h support, there are some duplicated function
implementation, like phys_to_virt, memset_io, memcpy_(from|to)io.
Let's remove them to use the default version in asm-neneric/io.h.

Meanwhile move isa_bus_to_virt() down below <asm-generic/io.h> including
line to fix the compiling error of missing phys_to_virt definition.

Signed-off-by: Baoquan He <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Serge Semin <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Huacai Chen <[email protected]>
Cc: Jiaxun Yang <[email protected]>
Cc: [email protected]
---
arch/mips/include/asm/io.h | 45 +++++---------------------------------
1 file changed, 5 insertions(+), 40 deletions(-)

diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
index da0a625c3c6d..1b38f02bc608 100644
--- a/arch/mips/include/asm/io.h
+++ b/arch/mips/include/asm/io.h
@@ -114,24 +114,6 @@ static inline phys_addr_t virt_to_phys(const volatile void *x)
return __virt_to_phys(x);
}

-/*
- * phys_to_virt - map physical address to virtual
- * @address: address to remap
- *
- * The returned virtual address is a current CPU mapping for
- * the memory address given. It is only valid to use this function on
- * addresses that have a kernel mapping
- *
- * This function does not handle bus mappings for DMA transfers. In
- * almost all conceivable cases a device driver should not be using
- * this function
- */
-#define phys_to_virt phys_to_virt
-static inline void * phys_to_virt(unsigned long address)
-{
- return __va(address);
-}
-
/*
* ISA I/O bus memory addresses are 1:1 with the physical address.
*/
@@ -140,11 +122,6 @@ static inline unsigned long isa_virt_to_bus(volatile void *address)
return virt_to_phys(address);
}

-static inline void *isa_bus_to_virt(unsigned long address)
-{
- return phys_to_virt(address);
-}
-
/*
* Change "struct page" to physical address.
*/
@@ -535,23 +512,6 @@ BUILDSTRING(q, u64)
#define writesq writesq
#endif

-
-#define memset_io memset_io
-static inline void memset_io(volatile void __iomem *addr, unsigned char val, int count)
-{
- memset((void __force *) addr, val, count);
-}
-#define memcpy_fromio memcpy_fromio
-static inline void memcpy_fromio(void *dst, const volatile void __iomem *src, int count)
-{
- memcpy(dst, (void __force *) src, count);
-}
-#define memcpy_toio memcpy_toio
-static inline void memcpy_toio(volatile void __iomem *dst, const void *src, int count)
-{
- memcpy((void __force *) dst, src, count);
-}
-
/*
* The caches on some architectures aren't dma-coherent and have need to
* handle this in software. There are three types of operations that
@@ -617,4 +577,9 @@ void __ioread64_copy(void *to, const void __iomem *from, size_t count);

#include <asm-generic/io.h>

+static inline void *isa_bus_to_virt(unsigned long address)
+{
+ return phys_to_virt(address);
+}
+
#endif /* _ASM_IO_H */
--
2.34.1


2023-03-09 14:37:18

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH v4 3/4] arch/*/io.h: remove ioremap_uc in some architectures

On Wed, Mar 08, 2023 at 09:07:09PM +0800, Baoquan He wrote:
> ioremap_uc() is only meaningful on old x86-32 systems with the PAT
> extension, and on ia64 with its slightly unconventional ioremap()
> behavior. So remove the ioremap_uc() definition in architecutures
> other than x86 and ia64. These architectures all have asm-generic/io.h
> included and will have the default ioremap_uc() definition which
> returns NULL.
>
> This changes the existing behaviour, while no need to worry about
> any breakage because in the only callsite of ioremap_uc(), code
> has been adjusted to eliminate the impact. Please see
> atyfb_setup_generic() of drivers/video/fbdev/aty/atyfb_base.c.
>
> If any new invocation of ioremap_uc() need be added, please consider
> using ioremap() intead or adding a ARCH specific version if necessary.
>
> Acked-by: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Baoquan He <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> Documentation/driver-api/device-io.rst | 9 +++++----
> arch/alpha/include/asm/io.h | 1 -
> arch/hexagon/include/asm/io.h | 3 ---
> arch/m68k/include/asm/kmap.h | 1 -
> arch/mips/include/asm/io.h | 1 -
> arch/parisc/include/asm/io.h | 2 --
> arch/powerpc/include/asm/io.h | 1 -
> arch/sh/include/asm/io.h | 2 --
> arch/sparc/include/asm/io_64.h | 1 -
> 9 files changed, 5 insertions(+), 16 deletions(-)

this doesn't apply to v6.3-rc1... what tree is this based on ?

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]

2023-03-09 22:55:25

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v4 3/4] arch/*/io.h: remove ioremap_uc in some architectures

Baoquan He <[email protected]> writes:
> ioremap_uc() is only meaningful on old x86-32 systems with the PAT
> extension, and on ia64 with its slightly unconventional ioremap()
> behavior. So remove the ioremap_uc() definition in architecutures
> other than x86 and ia64. These architectures all have asm-generic/io.h
> included and will have the default ioremap_uc() definition which
> returns NULL.
>
> This changes the existing behaviour, while no need to worry about
> any breakage because in the only callsite of ioremap_uc(), code
> has been adjusted to eliminate the impact. Please see
> atyfb_setup_generic() of drivers/video/fbdev/aty/atyfb_base.c.
>
> If any new invocation of ioremap_uc() need be added, please consider
> using ioremap() intead or adding a ARCH specific version if necessary.
>
> Acked-by: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Baoquan He <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> Documentation/driver-api/device-io.rst | 9 +++++----
> arch/alpha/include/asm/io.h | 1 -
> arch/hexagon/include/asm/io.h | 3 ---
> arch/m68k/include/asm/kmap.h | 1 -
> arch/mips/include/asm/io.h | 1 -
> arch/parisc/include/asm/io.h | 2 --
> arch/powerpc/include/asm/io.h | 1 -

Acked-by: Michael Ellerman <[email protected]> (powerpc)

cheers

2023-03-10 01:46:30

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH v4 3/4] arch/*/io.h: remove ioremap_uc in some architectures

On 03/09/23 at 03:36pm, Thomas Bogendoerfer wrote:
> On Wed, Mar 08, 2023 at 09:07:09PM +0800, Baoquan He wrote:
> > ioremap_uc() is only meaningful on old x86-32 systems with the PAT
> > extension, and on ia64 with its slightly unconventional ioremap()
> > behavior. So remove the ioremap_uc() definition in architecutures
> > other than x86 and ia64. These architectures all have asm-generic/io.h
> > included and will have the default ioremap_uc() definition which
> > returns NULL.
> >
> > This changes the existing behaviour, while no need to worry about
> > any breakage because in the only callsite of ioremap_uc(), code
> > has been adjusted to eliminate the impact. Please see
> > atyfb_setup_generic() of drivers/video/fbdev/aty/atyfb_base.c.
> >
> > If any new invocation of ioremap_uc() need be added, please consider
> > using ioremap() intead or adding a ARCH specific version if necessary.
> >
> > Acked-by: Geert Uytterhoeven <[email protected]>
> > Signed-off-by: Baoquan He <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > Documentation/driver-api/device-io.rst | 9 +++++----
> > arch/alpha/include/asm/io.h | 1 -
> > arch/hexagon/include/asm/io.h | 3 ---
> > arch/m68k/include/asm/kmap.h | 1 -
> > arch/mips/include/asm/io.h | 1 -
> > arch/parisc/include/asm/io.h | 2 --
> > arch/powerpc/include/asm/io.h | 1 -
> > arch/sh/include/asm/io.h | 2 --
> > arch/sparc/include/asm/io_64.h | 1 -
> > 9 files changed, 5 insertions(+), 16 deletions(-)
>
> this doesn't apply to v6.3-rc1... what tree is this based on ?

Sorry, I forgot mentioning this in cover letter. This series is
followup of below patchset, so it's on top of below patchset and based
on v6.3-rc1.

https://lore.kernel.org/all/[email protected]/T/#u
[PATCH v5 00/17] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way

Thanks
Baoquan


2023-03-10 21:14:45

by Helge Deller

[permalink] [raw]
Subject: Re: [PATCH v4 3/4] arch/*/io.h: remove ioremap_uc in some architectures

On 3/10/23 02:45, Baoquan He wrote:
> On 03/09/23 at 03:36pm, Thomas Bogendoerfer wrote:
>> On Wed, Mar 08, 2023 at 09:07:09PM +0800, Baoquan He wrote:
>>> ioremap_uc() is only meaningful on old x86-32 systems with the PAT
>>> extension, and on ia64 with its slightly unconventional ioremap()
>>> behavior. So remove the ioremap_uc() definition in architecutures
>>> other than x86 and ia64. These architectures all have asm-generic/io.h
>>> included and will have the default ioremap_uc() definition which
>>> returns NULL.
>>>
>>> This changes the existing behaviour, while no need to worry about
>>> any breakage because in the only callsite of ioremap_uc(), code
>>> has been adjusted to eliminate the impact. Please see
>>> atyfb_setup_generic() of drivers/video/fbdev/aty/atyfb_base.c.
>>>
>>> If any new invocation of ioremap_uc() need be added, please consider
>>> using ioremap() intead or adding a ARCH specific version if necessary.
>>>
>>> Acked-by: Geert Uytterhoeven <[email protected]>
>>> Signed-off-by: Baoquan He <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> ---
>>> Documentation/driver-api/device-io.rst | 9 +++++----
>>> arch/alpha/include/asm/io.h | 1 -
>>> arch/hexagon/include/asm/io.h | 3 ---
>>> arch/m68k/include/asm/kmap.h | 1 -
>>> arch/mips/include/asm/io.h | 1 -
>>> arch/parisc/include/asm/io.h | 2 --
>>> arch/powerpc/include/asm/io.h | 1 -
>>> arch/sh/include/asm/io.h | 2 --
>>> arch/sparc/include/asm/io_64.h | 1 -
>>> 9 files changed, 5 insertions(+), 16 deletions(-)
>>
>> this doesn't apply to v6.3-rc1... what tree is this based on ?
>
> Sorry, I forgot mentioning this in cover letter. This series is
> followup of below patchset, so it's on top of below patchset and based
> on v6.3-rc1.
>
> https://lore.kernel.org/all/[email protected]/T/#u
> [PATCH v5 00/17] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way

I've applied both patch series on top of v6.3-rc1 and
tested it with success on the parisc platform (32- and 64-bit kernel).

You may add to both patch series:

Acked-by: Helge Deller <[email protected]> # parisc

Thank you!
Helge

2023-03-13 17:55:50

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] mips: add <asm-generic/io.h> including

On Wed, Mar 08, 2023 at 09:07:08PM +0800, Baoquan He wrote:
> With the adding, some default ioremap_xx methods defined in
> asm-generic/io.h can be used. E.g the default ioremap_uc() returning
> NULL.
>
> Signed-off-by: Baoquan He <[email protected]>
> Reviewed-by: Arnd Bergmann <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Helge Deller <[email protected]>
> Cc: Serge Semin <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Huacai Chen <[email protected]>
> Cc: Jiaxun Yang <[email protected]>
> Cc: [email protected]
> ---
> arch/mips/include/asm/io.h | 78 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 70 insertions(+), 8 deletions(-)
>
> diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h
> index cec8347f0b85..6756baadba6c 100644
> --- a/arch/mips/include/asm/io.h
> +++ b/arch/mips/include/asm/io.h
> @@ -126,6 +126,7 @@ static inline phys_addr_t virt_to_phys(const volatile void *x)
> * almost all conceivable cases a device driver should not be using
> * this function
> */
> +#define phys_to_virt phys_to_virt
> static inline void * phys_to_virt(unsigned long address)
> {
> return __va(address);
> @@ -359,6 +360,27 @@ __BUILD_MEMORY_PFX(__raw_, q, u64, 0)
> __BUILD_MEMORY_PFX(__mem_, q, u64, 0)
> #endif
>
> +#define readb readb
> +#define readw readw
> +#define readl readl
> +#define writeb writeb
> +#define writew writew
> +#define writel writel
> +
> +#ifdef CONFIG_64BIT
> +#define readq readq
> +#define writeq writeq
> +#define __raw_readq __raw_readq
> +#define __raw_writeq __raw_writeq
> +#endif
> +
> +#define __raw_readb __raw_readb
> +#define __raw_readw __raw_readw
> +#define __raw_readl __raw_readl
> +#define __raw_writeb __raw_writeb
> +#define __raw_writew __raw_writew
> +#define __raw_writel __raw_writel
> +
> #define __BUILD_IOPORT_PFX(bus, bwlq, type) \
> __BUILD_IOPORT_SINGLE(bus, bwlq, type, 1, 0,) \
> __BUILD_IOPORT_SINGLE(bus, bwlq, type, 1, 0, _p)
> @@ -374,6 +396,27 @@ BUILDIO_IOPORT(l, u32)
> BUILDIO_IOPORT(q, u64)
> #endif
>
> +#define inb inb
> +#define inw inw
> +#define inl inl
> +#define inb_p inb_p
> +#define inw_p inw_p
> +#define inl_p inl_p
> +
> +#define outb outb
> +#define outw outw
> +#define outl outl
> +#define outb_p outb_p
> +#define outw_p outw_p
> +#define outl_p outl_p
> +
> +#ifdef CONFIG_64BIT
> +#define inq inq
> +#define outq outq
> +#define inq_p inq_p
> +#define outq_p outq_p
> +#endif
> +
> #define __BUILDIO(bwlq, type) \
> \
> __BUILD_MEMORY_SINGLE(____raw_, bwlq, type, 1, 0, 0)
> @@ -412,14 +455,6 @@ __BUILDIO(q, u64)
> #define writeq_be(val, addr) \
> __raw_writeq(cpu_to_be64((val)), (__force unsigned *)(addr))
>
> -/*
> - * Some code tests for these symbols
> - */
> -#ifdef CONFIG_64BIT
> -#define readq readq
> -#define writeq writeq
> -#endif
> -
> #define __BUILD_MEMORY_STRING(bwlq, type) \
> \
> static inline void writes##bwlq(volatile void __iomem *mem, \
> @@ -480,14 +515,39 @@ BUILDSTRING(l, u32)
> BUILDSTRING(q, u64)
> #endif
>
> +#define insb insb
> +#define insw insw
> +#define insl insl
> +#define outsb outsb
> +#define outsw outsw
> +#define outsl outsl
> +
> +#define readsb readsb
> +#define readsw readsw
> +#define readsl readsl
> +#define writesb writesb
> +#define writesw writesw
> +#define writesl writesl
> +
> +#ifdef CONFIG_64BIT
> +#define insq insq
> +#define readsq readsq
> +#define readsq readsq
> +#define writesq writesq
> +#endif
> +
> +
> +#define memset_io memset_io
> static inline void memset_io(volatile void __iomem *addr, unsigned char val, int count)
> {
> memset((void __force *) addr, val, count);
> }
> +#define memcpy_fromio memcpy_fromio
> static inline void memcpy_fromio(void *dst, const volatile void __iomem *src, int count)
> {
> memcpy(dst, (void __force *) src, count);
> }
> +#define memcpy_toio memcpy_toio
> static inline void memcpy_toio(volatile void __iomem *dst, const void *src, int count)
> {
> memcpy((void __force *) dst, src, count);
> @@ -556,4 +616,6 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size);
>
> void __ioread64_copy(void *to, const void __iomem *from, size_t count);
>
> +#include <asm-generic/io.h>

this #include blows up builds with:

GEN Makefile
Checking missing-syscalls for N32
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
Checking missing-syscalls for O32
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
CC init/version.o
In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
from /local/tbogendoerfer/korg/linux/init/version.c:17:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_trylock’:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:3: error: implicit declaration of function ‘spin_acquire’ [-Werror=implicit-function-declaration]
spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
^~~~~~~~~~~~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:21: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
^~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_lock_irqsave’:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:110:20: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
^~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
^~~~~~~~~~~~~~
GEN Makefile
Checking missing-syscalls for N32
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
Checking missing-syscalls for O32
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
CC init/version.o
In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
from /local/tbogendoerfer/korg/linux/init/version.c:17:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_trylock’:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:3: error: implicit declaration of function ‘spin_acquire’ [-Werror=implicit-function-declaration]
spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
^~~~~~~~~~~~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:21: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
^~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_lock_irqsave’:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:110:20: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
^~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
^~~~~~~~~~~~~~
GEN Makefile
Checking missing-syscalls for N32
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
Checking missing-syscalls for O32
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
CC init/version.o
In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
from /local/tbogendoerfer/korg/linux/init/version.c:17:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_trylock’:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:3: error: implicit declaration of function ‘spin_acquire’ [-Werror=implicit-function-declaration]
spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
^~~~~~~~~~~~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:21: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
^~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_lock_irqsave’:
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:110:20: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
^~
/local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
^~~~~~~~~~~~~~
[...]

I've cut the compiler output. Removing the asm-generic doesn't show this
problem, but so far I fail to see the reason...

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]

2023-03-14 02:57:46

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] mips: add <asm-generic/io.h> including

On 03/13/23 at 06:55pm, Thomas Bogendoerfer wrote:
......
> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
> LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> ^~~~~~~~~~~~~~
> GEN Makefile
> Checking missing-syscalls for N32
> CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
> Checking missing-syscalls for O32
> CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
> CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
> CC init/version.o
> In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
> from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
> from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
> from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
> from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
> from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
> from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
> from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
> from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
> from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
> from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
> from /local/tbogendoerfer/korg/linux/init/version.c:17:
> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_trylock’:
> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:3: error: implicit declaration of function ‘spin_acquire’ [-Werror=implicit-function-declaration]
> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
> ^~~~~~~~~~~~
> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:21: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
> ^~
> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_lock_irqsave’:
> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:110:20: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
> spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
> ^~
> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
> LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> ^~~~~~~~~~~~~~
> [...]
>
> I've cut the compiler output. Removing the asm-generic doesn't show this
> problem, but so far I fail to see the reason...

Thanks for trying this.

Do you have the kernel config file, I can try to reproduce on my local
machine. And by the way, it could be fixed with below patch, not very
sure. Earlier, Arnd suggested this to fix a similar case.


From b3310e58c063b695ba7ab3966c57269f57f16585 Mon Sep 17 00:00:00 2001
From: Baoquan He <[email protected]>
Date: Tue, 14 Mar 2023 08:53:15 +0800
Subject: [PATCH] mips: use wmb() instead to replace iobarrier_w()
Content-type: text/plain

Otherwise nested including of asm/io.h and asm/mmiowb.h will cause
compiling error.

Signed-off-by: Baoquan He <[email protected]>
---
arch/mips/include/asm/mmiowb.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/mmiowb.h b/arch/mips/include/asm/mmiowb.h
index a40824e3ef8e..dd206792abed 100644
--- a/arch/mips/include/asm/mmiowb.h
+++ b/arch/mips/include/asm/mmiowb.h
@@ -2,9 +2,7 @@
#ifndef _ASM_MMIOWB_H
#define _ASM_MMIOWB_H

-#include <asm/io.h>
-
-#define mmiowb() iobarrier_w()
+#define mmiowb() wmb()

#include <asm-generic/mmiowb.h>

--
2.34.1


2023-03-14 15:36:53

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] mips: add <asm-generic/io.h> including

On Tue, Mar 14, 2023 at 10:56:36AM +0800, Baoquan He wrote:
> On 03/13/23 at 06:55pm, Thomas Bogendoerfer wrote:
> ......
> > /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
> > LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> > ^~~~~~~~~~~~~~
> > GEN Makefile
> > Checking missing-syscalls for N32
> > CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
> > Checking missing-syscalls for O32
> > CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
> > CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
> > CC init/version.o
> > In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
> > from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
> > from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
> > from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
> > from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
> > from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
> > from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
> > from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
> > from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
> > from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
> > from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
> > from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
> > from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
> > from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
> > from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
> > from /local/tbogendoerfer/korg/linux/init/version.c:17:
> > /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_trylock’:
> > /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:3: error: implicit declaration of function ‘spin_acquire’ [-Werror=implicit-function-declaration]
> > spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
> > ^~~~~~~~~~~~
> > /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:21: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
> > spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
> > ^~
> > /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_lock_irqsave’:
> > /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:110:20: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
> > spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
> > ^~
> > /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
> > LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> > ^~~~~~~~~~~~~~
> > [...]
> >
> > I've cut the compiler output. Removing the asm-generic doesn't show this
> > problem, but so far I fail to see the reason...
>
> Thanks for trying this.
>
> Do you have the kernel config file, I can try to reproduce on my local
> machine. And by the way, it could be fixed with below patch, not very
> sure. Earlier, Arnd suggested this to fix a similar case.

already tried it, but it doesn't fix the issue. I've attached the
config.

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]


Attachments:
(No filename) (3.87 kB)
config-bigsur (69.38 kB)
Download all attachments

2023-03-14 16:31:37

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] mips: add <asm-generic/io.h> including

On 3/14/23 08:34, Thomas Bogendoerfer wrote:
> On Tue, Mar 14, 2023 at 10:56:36AM +0800, Baoquan He wrote:
>> On 03/13/23 at 06:55pm, Thomas Bogendoerfer wrote:
>> ......
>>> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
>>> LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
>>> ^~~~~~~~~~~~~~
>>> GEN Makefile
>>> Checking missing-syscalls for N32
>>> CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
>>> Checking missing-syscalls for O32
>>> CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
>>> CALL /local/tbogendoerfer/korg/linux/scripts/checksyscalls.sh
>>> CC init/version.o
>>> In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
>>> from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
>>> from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
>>> from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
>>> from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
>>> from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
>>> from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
>>> from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
>>> from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
>>> from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
>>> from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
>>> from /local/tbogendoerfer/korg/linux/init/version.c:17:
>>> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_trylock’:
>>> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:3: error: implicit declaration of function ‘spin_acquire’ [-Werror=implicit-function-declaration]
>>> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
>>> ^~~~~~~~~~~~
>>> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:90:21: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
>>> spin_acquire(&lock->dep_map, 0, 1, _RET_IP_);
>>> ^~
>>> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h: In function ‘__raw_spin_lock_irqsave’:
>>> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:110:20: error: ‘raw_spinlock_t {aka struct raw_spinlock}’ has no member named ‘dep_map’
>>> spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
>>> ^~
>>> /local/tbogendoerfer/korg/linux/include/linux/spinlock_api_smp.h:111:2: error: implicit declaration of function ‘LOCK_CONTENDED’ [-Werror=implicit-function-declaration]
>>> LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
>>> ^~~~~~~~~~~~~~
>>> [...]
>>>
>>> I've cut the compiler output. Removing the asm-generic doesn't show this
>>> problem, but so far I fail to see the reason...
>>
>> Thanks for trying this.
>>
>> Do you have the kernel config file, I can try to reproduce on my local
>> machine. And by the way, it could be fixed with below patch, not very
>> sure. Earlier, Arnd suggested this to fix a similar case.
>
> already tried it, but it doesn't fix the issue. I've attached the
> config.

I had attempted a similar approach before as Baoquan did, but met the
same build issue as Thomas that was not immediately clear to me why it
popped up. I would be curious to see how this can be resolved.
--
Florian


2023-03-14 17:20:10

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] mips: add <asm-generic/io.h> including

On Tue, Mar 14, 2023, at 17:31, Florian Fainelli wrote:
> On 3/14/23 08:34, Thomas Bogendoerfer wrote:
>> On Tue, Mar 14, 2023 at 10:56:36AM +0800, Baoquan He wrote:
>>>> In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
>>>> from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
>>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
>>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
>>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
>>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
>>>> from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
>>>> from /local/tbogendoerfer/korg/linux/init/version.c:17:
>>
>> already tried it, but it doesn't fix the issue. I've attached the
>> config.
>
> I had attempted a similar approach before as Baoquan did, but met the
> same build issue as Thomas that was not immediately clear to me why it
> popped up. I would be curious to see how this can be resolved.

I think this is the result of recursive header inclusion:
spinlock.h includes lockdep.h, but its header guard is already
there from the include chain.

There is probably something in one of the mips asm/*.h headers that
causes this recursion that is not present elsewhere.

I think this should fix it, but is likely to cause another problem elsewhere:

--- a/arch/mips/include/asm/smp-ops.h
+++ b/arch/mips/include/asm/smp-ops.h
@@ -13,8 +13,6 @@

#include <linux/errno.h>

-#include <asm/mips-cps.h>
-
#ifdef CONFIG_SMP

#include <linux/cpumask.h>


Arnd

2023-03-15 00:51:06

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] mips: add <asm-generic/io.h> including

On 03/14/23 at 06:19pm, Arnd Bergmann wrote:
> On Tue, Mar 14, 2023, at 17:31, Florian Fainelli wrote:
> > On 3/14/23 08:34, Thomas Bogendoerfer wrote:
> >> On Tue, Mar 14, 2023 at 10:56:36AM +0800, Baoquan He wrote:
> >>>> In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
> >>>> from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
> >>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
> >>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
> >>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
> >>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
> >>>> from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
> >>>> from /local/tbogendoerfer/korg/linux/init/version.c:17:
> >>
> >> already tried it, but it doesn't fix the issue. I've attached the
> >> config.
> >
> > I had attempted a similar approach before as Baoquan did, but met the
> > same build issue as Thomas that was not immediately clear to me why it
> > popped up. I would be curious to see how this can be resolved.
>
> I think this is the result of recursive header inclusion:
> spinlock.h includes lockdep.h, but its header guard is already
> there from the include chain.
>
> There is probably something in one of the mips asm/*.h headers that
> causes this recursion that is not present elsewhere.
>
> I think this should fix it, but is likely to cause another problem elsewhere:
>
> --- a/arch/mips/include/asm/smp-ops.h
> +++ b/arch/mips/include/asm/smp-ops.h
> @@ -13,8 +13,6 @@
>
> #include <linux/errno.h>
>
> -#include <asm/mips-cps.h>
> -
> #ifdef CONFIG_SMP
>
> #include <linux/cpumask.h>

Will meet below compiling error after appllying above patch. Adding
asm/mips-cps.h including in arch/mips/kernel/setup.c will fix it as below.

arch/mips/kernel/setup.c: In function ‘setup_arch’:
arch/mips/kernel/setup.c:781:9: error: implicit declaration of function ‘mips_cm_probe’ [-Werror=implicit-function-declaration]
781 | mips_cm_probe();
| ^~~~~~~~~~~~~
cc1: all warnings being treated as errors


diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index f1c88f8a1dc5..e8c4020ef367 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -43,6 +43,7 @@
#include <asm/smp-ops.h>
#include <asm/prom.h>
#include <asm/fw/fw.h>
+#include <asm/mips-cps.h>

#ifdef CONFIG_MIPS_ELF_APPENDED_DTB
char __section(".appended_dtb") __appended_dtb[0x100000];


2023-03-15 13:10:56

by Thomas Bogendoerfer

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] mips: add <asm-generic/io.h> including

On Wed, Mar 15, 2023 at 08:49:53AM +0800, Baoquan He wrote:
> On 03/14/23 at 06:19pm, Arnd Bergmann wrote:
> > On Tue, Mar 14, 2023, at 17:31, Florian Fainelli wrote:
> > > On 3/14/23 08:34, Thomas Bogendoerfer wrote:
> > >> On Tue, Mar 14, 2023 at 10:56:36AM +0800, Baoquan He wrote:
> > >>>> In file included from /local/tbogendoerfer/korg/linux/include/linux/spinlock.h:311:0,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/vmalloc.h:5,
> > >>>> from /local/tbogendoerfer/korg/linux/include/asm-generic/io.h:994,
> > >>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/io.h:618,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/io.h:13,
> > >>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/mips-cps.h:11,
> > >>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp-ops.h:16,
> > >>>> from /local/tbogendoerfer/korg/linux/arch/mips/include/asm/smp.h:21,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/smp.h:113,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/lockdep.h:14,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/rcupdate.h:29,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/rculist.h:11,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/pid.h:5,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/sched.h:14,
> > >>>> from /local/tbogendoerfer/korg/linux/include/linux/utsname.h:6,
> > >>>> from /local/tbogendoerfer/korg/linux/init/version.c:17:
> > >>
> > >> already tried it, but it doesn't fix the issue. I've attached the
> > >> config.
> > >
> > > I had attempted a similar approach before as Baoquan did, but met the
> > > same build issue as Thomas that was not immediately clear to me why it
> > > popped up. I would be curious to see how this can be resolved.
> >
> > I think this is the result of recursive header inclusion:
> > spinlock.h includes lockdep.h, but its header guard is already
> > there from the include chain.
> >
> > There is probably something in one of the mips asm/*.h headers that
> > causes this recursion that is not present elsewhere.
> >
> > I think this should fix it, but is likely to cause another problem elsewhere:
> >
> > --- a/arch/mips/include/asm/smp-ops.h
> > +++ b/arch/mips/include/asm/smp-ops.h
> > @@ -13,8 +13,6 @@
> >
> > #include <linux/errno.h>
> >
> > -#include <asm/mips-cps.h>
> > -
> > #ifdef CONFIG_SMP
> >
> > #include <linux/cpumask.h>
>
> Will meet below compiling error after appllying above patch. Adding
> asm/mips-cps.h including in arch/mips/kernel/setup.c will fix it as below.
>
> arch/mips/kernel/setup.c: In function ‘setup_arch’:
> arch/mips/kernel/setup.c:781:9: error: implicit declaration of function ‘mips_cm_probe’ [-Werror=implicit-function-declaration]
> 781 | mips_cm_probe();
> | ^~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index f1c88f8a1dc5..e8c4020ef367 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -43,6 +43,7 @@
> #include <asm/smp-ops.h>
> #include <asm/prom.h>
> #include <asm/fw/fw.h>
> +#include <asm/mips-cps.h>
>
> #ifdef CONFIG_MIPS_ELF_APPENDED_DTB
> char __section(".appended_dtb") __appended_dtb[0x100000];

with

diff --git a/arch/mips/pci/pci-ip27.c b/arch/mips/pci/pci-ip27.c
index d85cbf84e41c..973faea61cad 100644
--- a/arch/mips/pci/pci-ip27.c
+++ b/arch/mips/pci/pci-ip27.c
@@ -7,6 +7,9 @@
* Copyright (C) 1999, 2000, 04 Ralf Baechle ([email protected])
* Copyright (C) 1999, 2000 Silicon Graphics, Inc.
*/
+
+#include <linux/io.h>
+
#include <asm/sn/addrs.h>
#include <asm/sn/types.h>
#include <asm/sn/klconfig.h>

on top, all my builds are ok. But my malta qemu setup crashes:

Unhandled kernel unaligned access[#1]:
CPU: 0 PID: 1 Comm: systemd Not tainted 6.3.0-rc1-00022-gff6bffcfbf9e #290
$ 0 : 00000000 00000001 81000640 00000001
$ 4 : 80bd8641 00000001 00000000 00000000
$ 8 : 00000001 00000001 77c5abae ffffffff
$12 : 00000000 7b3eeedb 820f7380 bdd54e39
$16 : 80bd8641 820fbdc4 81000640 80ad0000
$20 : 00000008 80ad0000 80ad0000 80ad0000
$24 : 00000000 f592e7d4
$28 : 820f8000 820fbd48 80c20000 801c44f4
Hi : 001f93e2
Lo : 18073cf3
epc : 80902188 _raw_spin_lock+0xc/0x54
ra : 801c44f4 add_timer_on+0x94/0x1b0
Status: 1400a402 KERNEL EXL
Cause : 10800010 (ExcCode 04)
BadVA : 80bd8641
PrId : 00019300 (MIPS 24Kc)
Modules linked in:
Process systemd (pid: 1, threadinfo=(ptrval), task=(ptrval), tls=76f2b6e0)
Stack : 80bd8641 820fbdc4 80ad0000 80ad0000 00000008 80ad0000 80ad0000 801c44f4
80ad0000 80ad0000 820f8000 820fbd98 00000001 f01a83f8 00000008 820fbdc0
80ad0000 820fbdc4 80ad0000 808f4f20 00000001 822f7640 822f7940 f01a83f8
00000000 820fbec0 00000001 00000001 00000001 77d3fafc 3e01e433 00000122
00000000 ffff8c77 808f513c 0dc40000 00000001 00000001 00000001 82a1f900
...
Call Trace:
[<80902188>] _raw_spin_lock+0xc/0x54
[<801c44f4>] add_timer_on+0x94/0x1b0
[<808f4f20>] try_to_generate_entropy+0x1e8/0x270
[<805c7408>] urandom_read_iter+0x10c/0x114
[<802d77f8>] vfs_read+0x21c/0x2bc
[<802d8284>] ksys_read+0x7c/0x118
[<8011b130>] syscall_common+0x34/0x58

Code: 27bdffe0 afbf001c 24030001 <c0850000> 14a00005 00000000 00600825 e0810000 1020fffa

---[ end trace 0000000000000000 ]---

I guess this has something to do with the removal of #include <asm/mips-cps.h>

Thomas.

--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]