[PATCH v3 2/2] arch/*/io.h: remove ioremap_uc in some architectures

[PATCH v3 2/2] arch/*/io.h: remove ioremap_uc in some architectures

[Baoquan He]

ioremap_uc() is only meaningful on old x86-32 systems with the PAT
extension, and on ia64 with its slightly unconventional ioremap()
behavior, everywhere else this is the same as ioremap() anyway.

Here, 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.

Note: This changes the existing behaviour and could break code
calling ioremap_uc(). If any ARCH meets this breakage and really
needs a specific ioremap_uc() for its own usage, one ioremap_uc()
can be added in the ARCH.

Link: https://lore.kernel.org/all/20191112105507.GA7122@lst.de/#t
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: linux-alpha@vger.kernel.org
Cc: linux-hexagon@vger.kernel.org
Cc: linux-m68k@lists.linux-m68k.org
Cc: linux-mips@vger.kernel.org
Cc: linux-parisc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-sh@vger.kernel.org
Cc: sparclinux@vger.kernel.org

---
 Documentation/driver-api/device-io.rst | 14 +++++++++-----
 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, 9 insertions(+), 17 deletions(-)

diff --git a/Documentation/driver-api/device-io.rst b/Documentation/driver-api/device-io.rst
index 4d2baac0311c..12c2ebbaa41e 100644
--- a/Documentation/driver-api/device-io.rst
+++ b/Documentation/driver-api/device-io.rst
@@ -408,11 +408,15 @@ 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.
-
-Portable drivers should avoid the use of ioremap_uc().
+ioremap_uc() will be obsoleted since it's only expected on x86 and ia64.
+X86 non-PAT system marks memory as uncached even when the MTRR has designated
+it as cacheable in ioremap_uc()(see Documentation/x86/pat.rst). Ia64 system
+need check if attributes match, otherwise fails. Other than these two
+architectures, ioremap_uc() will default to NULL. Any architectures which
+meets breakage by ioremap_uc() returning NULL should provide a specific
+implementation to override the default version.
+
+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

Re: [PATCH v3 2/2] arch/*/io.h: remove ioremap_uc in some architectures

[Michael Ellerman]

Baoquan He <bhe@redhat.com> writes:
> ioremap_uc() is only meaningful on old x86-32 systems with the PAT
> extension, and on ia64 with its slightly unconventional ioremap()
> behavior, everywhere else this is the same as ioremap() anyway.
>
> Here, 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.
>
> Note: This changes the existing behaviour and could break code
> calling ioremap_uc(). If any ARCH meets this breakage and really
> needs a specific ioremap_uc() for its own usage, one ioremap_uc()
> can be added in the ARCH.

I see one use in:

drivers/video/fbdev/aty/atyfb_base.c:        par->ati_regbase = ioremap_uc(info->fix.mmio_start, 0x1000);


Which isn't obviously x86/ia64 specific.

I'm pretty sure some powermacs (powerpc) use that driver.

Maybe that exact code path is only reachable on x86/ia64? But if so
please explain why.

Otherwise it looks like this series could break that driver on powerpc
at least.

cheers

Re: [PATCH v3 2/2] arch/*/io.h: remove ioremap_uc in some architectures

[Geert Uytterhoeven]

Hi Michael,

On Sun, Mar 5, 2023 at 10:23 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> Baoquan He <bhe@redhat.com> writes:
> > ioremap_uc() is only meaningful on old x86-32 systems with the PAT
> > extension, and on ia64 with its slightly unconventional ioremap()
> > behavior, everywhere else this is the same as ioremap() anyway.
> >
> > Here, 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.
> >
> > Note: This changes the existing behaviour and could break code
> > calling ioremap_uc(). If any ARCH meets this breakage and really
> > needs a specific ioremap_uc() for its own usage, one ioremap_uc()
> > can be added in the ARCH.
>
> I see one use in:
>
> drivers/video/fbdev/aty/atyfb_base.c:        par->ati_regbase = ioremap_uc(info->fix.mmio_start, 0x1000);
>
>
> Which isn't obviously x86/ia64 specific.
>
> I'm pretty sure some powermacs (powerpc) use that driver.

I originally wrote that driver for CHRP, so yes.

> Maybe that exact code path is only reachable on x86/ia64? But if so
> please explain why.
>
> Otherwise it looks like this series could break that driver on powerpc
> at least.

Indeed.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/2] arch/*/io.h: remove ioremap_uc in some architectures

[Arnd Bergmann]

On Sun, Mar 5, 2023, at 10:29, Geert Uytterhoeven wrote:
>
> On Sun, Mar 5, 2023 at 10:23 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Maybe that exact code path is only reachable on x86/ia64? But if so
>> please explain why.
>>
>> Otherwise it looks like this series could break that driver on powerpc
>> at least.
>
> Indeed.

When I last looked into this, I sent a patch to use ioremap()
on non-x86:

https://lore.kernel.org/all/20191111192258.2234502-1-arnd@arndb.de/

    Arnd

Re: [PATCH v3 2/2] arch/*/io.h: remove ioremap_uc in some architectures

[Michael Ellerman]

"Arnd Bergmann" <arnd@arndb.de> writes:
> On Sun, Mar 5, 2023, at 10:29, Geert Uytterhoeven wrote:
>> On Sun, Mar 5, 2023 at 10:23 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>> Maybe that exact code path is only reachable on x86/ia64? But if so
>>> please explain why.
>>>
>>> Otherwise it looks like this series could break that driver on powerpc
>>> at least.
>>
>> Indeed.
>
> When I last looked into this, I sent a patch to use ioremap()
> on non-x86:
>
> https://lore.kernel.org/all/20191111192258.2234502-1-arnd@arndb.de/

OK thanks.

Baoquan can you add that patch to the start of this series if/when you
post the next version?

cheers

Re: [PATCH v3 2/2] arch/*/io.h: remove ioremap_uc in some architectures

[Baoquan He]

On 03/07/23 at 11:58am, Michael Ellerman wrote:
> "Arnd Bergmann" <arnd@arndb.de> writes:
> > On Sun, Mar 5, 2023, at 10:29, Geert Uytterhoeven wrote:
> >> On Sun, Mar 5, 2023 at 10:23 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >>> Maybe that exact code path is only reachable on x86/ia64? But if so
> >>> please explain why.
> >>>
> >>> Otherwise it looks like this series could break that driver on powerpc
> >>> at least.
> >>
> >> Indeed.
> >
> > When I last looked into this, I sent a patch to use ioremap()
> > on non-x86:
> >
> > https://lore.kernel.org/all/20191111192258.2234502-1-arnd@arndb.de/
> 
> OK thanks.
> 
> Baoquan can you add that patch to the start of this series if/when you
> post the next version?

Sure, will do. Wondering if we need make change to cover powerpc other
than x86 and ia64 in Arnd's patch as you and Geert pointed out.

Re: [PATCH v3 2/2] arch/*/io.h: remove ioremap_uc in some architectures

[Arnd Bergmann]

On Tue, Mar 7, 2023, at 02:30, Baoquan He wrote:
> On 03/07/23 at 11:58am, Michael Ellerman wrote:
>> "Arnd Bergmann" <arnd@arndb.de> writes:
>> > On Sun, Mar 5, 2023, at 10:29, Geert Uytterhoeven wrote:
>> >> On Sun, Mar 5, 2023 at 10:23 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> >>> Maybe that exact code path is only reachable on x86/ia64? But if so
>> >>> please explain why.
>> >>>
>> >>> Otherwise it looks like this series could break that driver on powerpc
>> >>> at least.
>> >>
>> >> Indeed.
>> >
>> > When I last looked into this, I sent a patch to use ioremap()
>> > on non-x86:
>> >
>> > https://lore.kernel.org/all/20191111192258.2234502-1-arnd@arndb.de/
>> 
>> OK thanks.
>> 
>> Baoquan can you add that patch to the start of this series if/when you
>> post the next version?
>
> Sure, will do. Wondering if we need make change to cover powerpc other
> than x86 and ia64 in Arnd's patch as you and Geert pointed out.

The patch fixes the aty driver for all architectures, including the
ones that were already broken before your series with the 'return NULL'
version.

The only other callers of ioremap_uc() and devm_ioremap_uc() are
in architecture specific code and in drivers/mfd/intel-lpss.c, which
is x86 specific.

     Arnd