diff mbox series

[v6,5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

Message ID 20230510110557.14343-6-tzimmermann@suse.de
State New
Headers show
Series fbdev: Move framebuffer I/O helpers to <asm/fb.h> | expand

Commit Message

Thomas Zimmermann May 10, 2023, 11:05 a.m. UTC
Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
in the architecture's <asm/fb.h> header file or the generic one.

The common case has been the use of regular I/O functions, such as
__raw_readb() or memset_io(). A few architectures used plain system-
memory reads and writes. Sparc used helpers for its SBus.

The architectures that used special cases provide the same code in
their __raw_*() I/O helpers. So the patch replaces this code with the
__raw_*() functions and moves it to <asm-generic/fb.h> for all
architectures.

v6:
	* fix fb_readq()/fb_writeq() on 64-bit mips (kernel test robot)
v5:
	* include <linux/io.h> in <asm-generic/fb>; fix s390 build
v4:
	* ia64, loongarch, sparc64: add fb_mem*() to arch headers
	  to keep current semantics (Arnd)
v3:
	* implement all architectures with generic helpers
	* support reordering and native byte order (Geert, Arnd)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>

add mips fb_q()
---
 arch/ia64/include/asm/fb.h      |  20 +++++++
 arch/loongarch/include/asm/fb.h |  21 +++++++
 arch/mips/include/asm/fb.h      |  22 +++++++
 arch/sparc/include/asm/fb.h     |  20 +++++++
 include/asm-generic/fb.h        | 102 ++++++++++++++++++++++++++++++++
 include/linux/fb.h              |  53 -----------------
 6 files changed, 185 insertions(+), 53 deletions(-)

Comments

Geert Uytterhoeven May 10, 2023, 12:34 p.m. UTC | #1
Hi Thomas,

On Wed, May 10, 2023 at 1:06 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
> in the architecture's <asm/fb.h> header file or the generic one.
>
> The common case has been the use of regular I/O functions, such as
> __raw_readb() or memset_io(). A few architectures used plain system-
> memory reads and writes. Sparc used helpers for its SBus.
>
> The architectures that used special cases provide the same code in
> their __raw_*() I/O helpers. So the patch replaces this code with the
> __raw_*() functions and moves it to <asm-generic/fb.h> for all
> architectures.
>
> v6:
>         * fix fb_readq()/fb_writeq() on 64-bit mips (kernel test robot)
> v5:
>         * include <linux/io.h> in <asm-generic/fb>; fix s390 build
> v4:
>         * ia64, loongarch, sparc64: add fb_mem*() to arch headers
>           to keep current semantics (Arnd)
> v3:
>         * implement all architectures with generic helpers
>         * support reordering and native byte order (Geert, Arnd)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>
> add mips fb_q()

> --- a/arch/mips/include/asm/fb.h
> +++ b/arch/mips/include/asm/fb.h
> @@ -12,6 +12,28 @@ static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma,
>  }
>  #define fb_pgprotect fb_pgprotect
>
> +/*
> + * MIPS doesn't define __raw_ I/O macros, so the helpers
> + * in <asm-generic/fb.h> don't generate fb_readq() and
> + * fb_write(). We have to provide them here.

MIPS does not include <asm-generic/io.h>,  nor define its own
__raw_readq() and __raw_writeq()...

> + *
> + * TODO: Convert MIPS to generic I/O. The helpers below can
> + *       then be removed.
> + */
> +#ifdef CONFIG_64BIT
> +static inline u64 fb_readq(const volatile void __iomem *addr)
> +{
> +       return __raw_readq(addr);

... so how can this call work?

> +}
> +#define fb_readq fb_readq
> +
> +static inline void fb_writeq(u64 b, volatile void __iomem *addr)
> +{
> +       __raw_writeq(b, addr);
> +}
> +#define fb_writeq fb_writeq
> +#endif
> +
>  #include <asm-generic/fb.h>

Gr{oetje,eeting}s,

                        Geert
kernel test robot May 10, 2023, 2:03 p.m. UTC | #2
Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[cannot apply to deller-parisc/for-next arnd-asm-generic/master linus/master davem-sparc/master v6.4-rc1 next-20230510]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-matrox-Remove-trailing-whitespaces/20230510-190752
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230510110557.14343-6-tzimmermann%40suse.de
patch subject: [PATCH v6 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>
config: sh-randconfig-r024-20230509 (https://download.01.org/0day-ci/archive/20230510/202305102136.eMjTSPwH-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/46cc7edd7f28cc167c5b38d0e4f0aa8c3ac67328
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/fbdev-matrox-Remove-trailing-whitespaces/20230510-190752
        git checkout 46cc7edd7f28cc167c5b38d0e4f0aa8c3ac67328
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/video/fbdev/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305102136.eMjTSPwH-lkp@intel.com/

All warnings (new ones prefixed by >>):

   cc1: warning: arch/sh/include/mach-hp6xx: No such file or directory [-Wmissing-include-dirs]
   cc1: warning: arch/sh/include/mach-hp6xx: No such file or directory [-Wmissing-include-dirs]
   In file included from drivers/video/fbdev/hitfb.c:27:
   drivers/video/fbdev/hitfb.c: In function 'hitfb_accel_wait':
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 1 of 'fb_readw' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:93:33: note: in expansion of macro 'HD64461_IO_OFFSET'
      93 | #define HD64461_GRCFGR          HD64461_IO_OFFSET(0x1044)       /* Accelerator Configuration Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:47:25: note: in expansion of macro 'HD64461_GRCFGR'
      47 |         while (fb_readw(HD64461_GRCFGR) & HD64461_GRCFGR_ACCSTATUS) ;
         |                         ^~~~~~~~~~~~~~
   In file included from arch/sh/include/asm/fb.h:5,
                    from include/linux/fb.h:19,
                    from drivers/video/fbdev/hitfb.c:22:
   include/asm-generic/fb.h:52:57: note: expected 'const volatile void *' but argument is of type 'unsigned int'
      52 | static inline u16 fb_readw(const volatile void __iomem *addr)
         |                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
   drivers/video/fbdev/hitfb.c: In function 'hitfb_accel_start':
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:93:33: note: in expansion of macro 'HD64461_IO_OFFSET'
      93 | #define HD64461_GRCFGR          HD64461_IO_OFFSET(0x1044)       /* Accelerator Configuration Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:53:30: note: in expansion of macro 'HD64461_GRCFGR'
      53 |                 fb_writew(6, HD64461_GRCFGR);
         |                              ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:93:33: note: in expansion of macro 'HD64461_IO_OFFSET'
      93 | #define HD64461_GRCFGR          HD64461_IO_OFFSET(0x1044)       /* Accelerator Configuration Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:55:30: note: in expansion of macro 'HD64461_GRCFGR'
      55 |                 fb_writew(7, HD64461_GRCFGR);
         |                              ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
   drivers/video/fbdev/hitfb.c: In function 'hitfb_accel_set_dest':
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:116:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     116 | #define HD64461_BBTDWR          HD64461_IO_OFFSET(0x105c)       /* Destination Block Width Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:66:28: note: in expansion of macro 'HD64461_BBTDWR'
      66 |         fb_writew(width-1, HD64461_BBTDWR);
         |                            ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:117:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     117 | #define HD64461_BBTDHR          HD64461_IO_OFFSET(0x105e)       /* Destination Block Height Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:67:29: note: in expansion of macro 'HD64461_BBTDHR'
      67 |         fb_writew(height-1, HD64461_BBTDHR);
         |                             ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:115:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     115 | #define HD64461_BBTDSARL        HD64461_IO_OFFSET(0x105a)       /* Destination Start Address Register (L) */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:69:35: note: in expansion of macro 'HD64461_BBTDSARL'
      69 |         fb_writew(saddr & 0xffff, HD64461_BBTDSARL);
         |                                   ^~~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:114:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     114 | #define HD64461_BBTDSARH        HD64461_IO_OFFSET(0x1058)       /* Destination Start Address Register (H) */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:70:32: note: in expansion of macro 'HD64461_BBTDSARH'
      70 |         fb_writew(saddr >> 16, HD64461_BBTDSARH);
         |                                ^~~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
   drivers/video/fbdev/hitfb.c: In function 'hitfb_accel_bitblt':
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:122:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     122 | #define HD64461_BBTROPR         HD64461_IO_OFFSET(0x1068)       /* ROP Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:83:24: note: in expansion of macro 'HD64461_BBTROPR'
      83 |         fb_writew(rop, HD64461_BBTROPR);
         |                        ^~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:123:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     123 | #define HD64461_BBTMDR          HD64461_IO_OFFSET(0x106a)       /* BitBLT Mode Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:94:49: note: in expansion of macro 'HD64461_BBTMDR'
      94 |                         fb_writew((1 << 5) | 1, HD64461_BBTMDR);
         |                                                 ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:123:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     123 | #define HD64461_BBTMDR          HD64461_IO_OFFSET(0x106a)       /* BitBLT Mode Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:96:38: note: in expansion of macro 'HD64461_BBTMDR'
      96 |                         fb_writew(1, HD64461_BBTMDR);
         |                                      ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:123:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     123 | #define HD64461_BBTMDR          HD64461_IO_OFFSET(0x106a)       /* BitBLT Mode Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:101:45: note: in expansion of macro 'HD64461_BBTMDR'
     101 |                         fb_writew((1 << 5), HD64461_BBTMDR);
         |                                             ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:123:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     123 | #define HD64461_BBTMDR          HD64461_IO_OFFSET(0x106a)       /* BitBLT Mode Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:103:38: note: in expansion of macro 'HD64461_BBTMDR'
     103 |                         fb_writew(0, HD64461_BBTMDR);
         |                                      ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:116:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     116 | #define HD64461_BBTDWR          HD64461_IO_OFFSET(0x105c)       /* Destination Block Width Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:110:26: note: in expansion of macro 'HD64461_BBTDWR'
     110 |         fb_writew(width, HD64461_BBTDWR);
         |                          ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:117:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     117 | #define HD64461_BBTDHR          HD64461_IO_OFFSET(0x105e)       /* Destination Block Height Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:111:27: note: in expansion of macro 'HD64461_BBTDHR'
     111 |         fb_writew(height, HD64461_BBTDHR);
         |                           ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:113:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     113 | #define HD64461_BBTSSARL        HD64461_IO_OFFSET(0x1056)       /* Source Start Address Register (L) */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:112:35: note: in expansion of macro 'HD64461_BBTSSARL'
     112 |         fb_writew(saddr & 0xffff, HD64461_BBTSSARL);
         |                                   ^~~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:112:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     112 | #define HD64461_BBTSSARH        HD64461_IO_OFFSET(0x1054)       /* Source Start Address Register (H) */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:113:32: note: in expansion of macro 'HD64461_BBTSSARH'
     113 |         fb_writew(saddr >> 16, HD64461_BBTSSARH);
         |                                ^~~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:115:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     115 | #define HD64461_BBTDSARL        HD64461_IO_OFFSET(0x105a)       /* Destination Start Address Register (L) */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:114:35: note: in expansion of macro 'HD64461_BBTDSARL'
     114 |         fb_writew(daddr & 0xffff, HD64461_BBTDSARL);
         |                                   ^~~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:114:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     114 | #define HD64461_BBTDSARH        HD64461_IO_OFFSET(0x1058)       /* Destination Start Address Register (H) */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:115:32: note: in expansion of macro 'HD64461_BBTDSARH'
     115 |         fb_writew(daddr >> 16, HD64461_BBTDSARH);
         |                                ^~~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:121:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     121 | #define HD64461_BBTMARL         HD64461_IO_OFFSET(0x1066)       /* Mask Start Address Register (L) */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:118:43: note: in expansion of macro 'HD64461_BBTMARL'
     118 |                 fb_writew(maddr & 0xffff, HD64461_BBTMARL);
         |                                           ^~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:120:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     120 | #define HD64461_BBTMARH         HD64461_IO_OFFSET(0x1064)       /* Mask Start Address Register (H) */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:119:40: note: in expansion of macro 'HD64461_BBTMARH'
     119 |                 fb_writew(maddr >> 16, HD64461_BBTMARH);
         |                                        ^~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
   drivers/video/fbdev/hitfb.c: In function 'hitfb_fillrect':
   arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:122:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     122 | #define HD64461_BBTROPR         HD64461_IO_OFFSET(0x1068)       /* ROP Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:130:35: note: in expansion of macro 'HD64461_BBTROPR'
     130 |                 fb_writew(0x00f0, HD64461_BBTROPR);
         |                                   ^~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
   arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:123:33: note: in expansion of macro 'HD64461_IO_OFFSET'
     123 | #define HD64461_BBTMDR          HD64461_IO_OFFSET(0x106a)       /* BitBLT Mode Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:131:31: note: in expansion of macro 'HD64461_BBTMDR'
     131 |                 fb_writew(16, HD64461_BBTMDR);
         |                               ^~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
   arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:92:33: note: in expansion of macro 'HD64461_IO_OFFSET'
      92 | #define HD64461_GRSCR           HD64461_IO_OFFSET(0x1042)       /* Solid Color Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:135:35: note: in expansion of macro 'HD64461_GRSCR'
     135 |                                   HD64461_GRSCR);
         |                                   ^~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
   arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:92:33: note: in expansion of macro 'HD64461_IO_OFFSET'
      92 | #define HD64461_GRSCR           HD64461_IO_OFFSET(0x1042)       /* Solid Color Register */
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:140:48: note: in expansion of macro 'HD64461_GRSCR'
     140 |                         fb_writew(rect->color, HD64461_GRSCR);
         |                                                ^~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
   drivers/video/fbdev/hitfb.c: In function 'hitfb_pan_display':
   arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 2 of 'fb_writew' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:53:33: note: in expansion of macro 'HD64461_IO_OFFSET'
      53 | #define HD64461_LCDCBAR         HD64461_IO_OFFSET(0x1000)
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:165:56: note: in expansion of macro 'HD64461_LCDCBAR'
     165 |         fb_writew((yoffset*info->fix.line_length)>>10, HD64461_LCDCBAR);
         |                                                        ^~~~~~~~~~~~~~~
   include/asm-generic/fb.h:86:60: note: expected 'volatile void *' but argument is of type 'unsigned int'
      86 | static inline void fb_writew(u16 b, volatile void __iomem *addr)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~~^~~~
   drivers/video/fbdev/hitfb.c: At top level:
   drivers/video/fbdev/hitfb.c:170:5: warning: no previous prototype for 'hitfb_blank' [-Wmissing-prototypes]
     170 | int hitfb_blank(int blank_mode, struct fb_info *info)
         |     ^~~~~~~~~~~
   drivers/video/fbdev/hitfb.c: In function 'hitfb_blank':
   arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 1 of 'fb_readw' makes pointer from integer without a cast [-Wint-conversion]
      18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
         |                                 ^~~~~~~~~~~~~~~~~~~~~~
         |                                 |
         |                                 unsigned int
   arch/sh/include/asm/hd64461.h:70:33: note: in expansion of macro 'HD64461_IO_OFFSET'
      70 | #define HD64461_LDR1            HD64461_IO_OFFSET(0x1010)
         |                                 ^~~~~~~~~~~~~~~~~
   drivers/video/fbdev/hitfb.c:175:30: note: in expansion of macro 'HD64461_LDR1'
     175 |                 v = fb_readw(HD64461_LDR1);


vim +/fb_readw +18 arch/sh/include/asm/hd64461.h

^1da177e4c3f41 include/asm-sh/hd64461/hd64461.h Linus Torvalds     2005-04-16  15  
be15d65d97f924 include/asm-sh/hd64461.h         Kristoffer Ericson 2007-07-12  16  /* Area 6 - Slot 0 - memory and/or IO card */
bec36eca6f5d1d arch/sh/include/asm/hd64461.h    Paul Mundt         2009-05-15  17  #define HD64461_IOBASE		0xb0000000
bec36eca6f5d1d arch/sh/include/asm/hd64461.h    Paul Mundt         2009-05-15 @18  #define HD64461_IO_OFFSET(x)	(HD64461_IOBASE + (x))
bec36eca6f5d1d arch/sh/include/asm/hd64461.h    Paul Mundt         2009-05-15  19  #define	HD64461_PCC0_BASE	HD64461_IO_OFFSET(0x8000000)
be15d65d97f924 include/asm-sh/hd64461.h         Kristoffer Ericson 2007-07-12  20  #define	HD64461_PCC0_ATTR	(HD64461_PCC0_BASE)				/* 0xb80000000 */
be15d65d97f924 include/asm-sh/hd64461.h         Kristoffer Ericson 2007-07-12  21  #define	HD64461_PCC0_COMM	(HD64461_PCC0_BASE+HD64461_PCC_WINDOW)		/* 0xb90000000 */
be15d65d97f924 include/asm-sh/hd64461.h         Kristoffer Ericson 2007-07-12  22  #define	HD64461_PCC0_IO		(HD64461_PCC0_BASE+2*HD64461_PCC_WINDOW)	/* 0xba0000000 */
^1da177e4c3f41 include/asm-sh/hd64461/hd64461.h Linus Torvalds     2005-04-16  23
Arnd Bergmann May 10, 2023, 2:15 p.m. UTC | #3
On Wed, May 10, 2023, at 16:03, kernel test robot wrote:

>
>    cc1: warning: arch/sh/include/mach-hp6xx: No such file or directory 
> [-Wmissing-include-dirs]
>    cc1: warning: arch/sh/include/mach-hp6xx: No such file or directory 
> [-Wmissing-include-dirs]
>    In file included from drivers/video/fbdev/hitfb.c:27:
>    drivers/video/fbdev/hitfb.c: In function 'hitfb_accel_wait':
>>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 1 of 'fb_readw' makes pointer from integer without a cast [-Wint-conversion]
>       18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
>          |                                 ^~~~~~~~~~~~~~~~~~~~~~
>          |                                 |
>          |                                 unsigned int
>    arch/sh/include/asm/hd64461.h:93:33: note: in expansion of macro 
> 'HD64461_IO_OFFSET'
>       93 | #define HD64461_GRCFGR          HD64461_IO_OFFSET(0x1044)    
>    /* Accelerator Configuration Register */
>          |                                 ^~~~~~~~~~~~~~~~~
>    drivers/video/fbdev/hitfb.c:47:25: note: in expansion of macro 
> 'HD64461_GRCFGR'
>       47 |         while (fb_readw(HD64461_GRCFGR) & 
> HD64461_GRCFGR_ACCSTATUS) ;

I think that's a preexisting bug and I have no idea what the
correct solution is. Looking for HD64461 shows it being used
both with inw/outw and readw/writew, so there is no way to have
the correct type. The sh __raw_readw() definition hides this bug,
but that is a problem with arch/sh and it probably hides others
as well.

       Arnd
Thomas Zimmermann May 10, 2023, 2:20 p.m. UTC | #4
Hi Geert

Am 10.05.23 um 14:34 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Wed, May 10, 2023 at 1:06 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
>> in the architecture's <asm/fb.h> header file or the generic one.
>>
>> The common case has been the use of regular I/O functions, such as
>> __raw_readb() or memset_io(). A few architectures used plain system-
>> memory reads and writes. Sparc used helpers for its SBus.
>>
>> The architectures that used special cases provide the same code in
>> their __raw_*() I/O helpers. So the patch replaces this code with the
>> __raw_*() functions and moves it to <asm-generic/fb.h> for all
>> architectures.
>>
>> v6:
>>          * fix fb_readq()/fb_writeq() on 64-bit mips (kernel test robot)
>> v5:
>>          * include <linux/io.h> in <asm-generic/fb>; fix s390 build
>> v4:
>>          * ia64, loongarch, sparc64: add fb_mem*() to arch headers
>>            to keep current semantics (Arnd)
>> v3:
>>          * implement all architectures with generic helpers
>>          * support reordering and native byte order (Geert, Arnd)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>>
>> add mips fb_q()

Oops, left-over fom squashing patches.

> 
>> --- a/arch/mips/include/asm/fb.h
>> +++ b/arch/mips/include/asm/fb.h
>> @@ -12,6 +12,28 @@ static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma,
>>   }
>>   #define fb_pgprotect fb_pgprotect
>>
>> +/*
>> + * MIPS doesn't define __raw_ I/O macros, so the helpers
>> + * in <asm-generic/fb.h> don't generate fb_readq() and
>> + * fb_write(). We have to provide them here.
> 
> MIPS does not include <asm-generic/io.h>,  nor define its own

I know, that's why the TODO says to convert it to generic I/O.

> __raw_readq() and __raw_writeq()...

It doesn't define those macros, but it generates function calls of the 
same names. Follow the macros at

 
https://elixir.bootlin.com/linux/latest/source/arch/mips/include/asm/io.h#L357

It expands to a variety of helpers, including __raw_*().

> 
>> + *
>> + * TODO: Convert MIPS to generic I/O. The helpers below can
>> + *       then be removed.
>> + */
>> +#ifdef CONFIG_64BIT
>> +static inline u64 fb_readq(const volatile void __iomem *addr)
>> +{
>> +       return __raw_readq(addr);
> 
> ... so how can this call work?

On 64-bit builds, there's __raw_readq() and __raw_writeq().

At first, I tried to do the right thing and convert MIPS to work with 
<asm-generic/io.h>. But that created a ton of follow-up errors in other 
headers. So for now, it's better to handle this problem in asm/fb.h.

Best regards
Thomas

> 
>> +}
>> +#define fb_readq fb_readq
>> +
>> +static inline void fb_writeq(u64 b, volatile void __iomem *addr)
>> +{
>> +       __raw_writeq(b, addr);
>> +}
>> +#define fb_writeq fb_writeq
>> +#endif
>> +
>>   #include <asm-generic/fb.h>
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
Thomas Zimmermann May 10, 2023, 2:27 p.m. UTC | #5
Hi

Am 10.05.23 um 16:15 schrieb Arnd Bergmann:
> On Wed, May 10, 2023, at 16:03, kernel test robot wrote:
> 
>>
>>     cc1: warning: arch/sh/include/mach-hp6xx: No such file or directory
>> [-Wmissing-include-dirs]
>>     cc1: warning: arch/sh/include/mach-hp6xx: No such file or directory
>> [-Wmissing-include-dirs]
>>     In file included from drivers/video/fbdev/hitfb.c:27:
>>     drivers/video/fbdev/hitfb.c: In function 'hitfb_accel_wait':
>>>> arch/sh/include/asm/hd64461.h:18:33: warning: passing argument 1 of 'fb_readw' makes pointer from integer without a cast [-Wint-conversion]
>>        18 | #define HD64461_IO_OFFSET(x)    (HD64461_IOBASE + (x))
>>           |                                 ^~~~~~~~~~~~~~~~~~~~~~
>>           |                                 |
>>           |                                 unsigned int
>>     arch/sh/include/asm/hd64461.h:93:33: note: in expansion of macro
>> 'HD64461_IO_OFFSET'
>>        93 | #define HD64461_GRCFGR          HD64461_IO_OFFSET(0x1044)
>>     /* Accelerator Configuration Register */
>>           |                                 ^~~~~~~~~~~~~~~~~
>>     drivers/video/fbdev/hitfb.c:47:25: note: in expansion of macro
>> 'HD64461_GRCFGR'
>>        47 |         while (fb_readw(HD64461_GRCFGR) &
>> HD64461_GRCFGR_ACCSTATUS) ;
> 
> I think that's a preexisting bug and I have no idea what the
> correct solution is. Looking for HD64461 shows it being used
> both with inw/outw and readw/writew, so there is no way to have
> the correct type. The sh __raw_readw() definition hides this bug,
> but that is a problem with arch/sh and it probably hides others
> as well.

The constant HD64461_IOBASE is defined as integer at

 
https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/hd64461.h#L17

but fb_readw() expects a volatile-void pointer. I guess we could add a 
cast somewhere to silence the problem. In the current upstream code, 
that appears to be done by sh's __raw_readw() internally:

 
https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/io.h#L35

Best regards
Thomas

> 
>         Arnd
Geert Uytterhoeven May 10, 2023, 2:34 p.m. UTC | #6
Hi Thomas,

On Wed, May 10, 2023 at 4:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 10.05.23 um 14:34 schrieb Geert Uytterhoeven:
> > On Wed, May 10, 2023 at 1:06 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
> >> in the architecture's <asm/fb.h> header file or the generic one.
> >>
> >> The common case has been the use of regular I/O functions, such as
> >> __raw_readb() or memset_io(). A few architectures used plain system-
> >> memory reads and writes. Sparc used helpers for its SBus.
> >>
> >> The architectures that used special cases provide the same code in
> >> their __raw_*() I/O helpers. So the patch replaces this code with the
> >> __raw_*() functions and moves it to <asm-generic/fb.h> for all
> >> architectures.
> >>
> >> v6:
> >>          * fix fb_readq()/fb_writeq() on 64-bit mips (kernel test robot)
> >> v5:
> >>          * include <linux/io.h> in <asm-generic/fb>; fix s390 build
> >> v4:
> >>          * ia64, loongarch, sparc64: add fb_mem*() to arch headers
> >>            to keep current semantics (Arnd)
> >> v3:
> >>          * implement all architectures with generic helpers
> >>          * support reordering and native byte order (Geert, Arnd)
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> >> Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> >> --- a/arch/mips/include/asm/fb.h
> >> +++ b/arch/mips/include/asm/fb.h
> >> @@ -12,6 +12,28 @@ static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma,
> >>   }
> >>   #define fb_pgprotect fb_pgprotect
> >>
> >> +/*
> >> + * MIPS doesn't define __raw_ I/O macros, so the helpers
> >> + * in <asm-generic/fb.h> don't generate fb_readq() and
> >> + * fb_write(). We have to provide them here.
> >
> > MIPS does not include <asm-generic/io.h>,  nor define its own
>
> I know, that's why the TODO says to convert it to generic I/O.
>
> > __raw_readq() and __raw_writeq()...
>
> It doesn't define those macros, but it generates function calls of the
> same names. Follow the macros at
>
>
> https://elixir.bootlin.com/linux/latest/source/arch/mips/include/asm/io.h#L357
>
> It expands to a variety of helpers, including __raw_*().

Thanks, I forgot MIPS is using these grep-unfriendly factories...

> >> + *
> >> + * TODO: Convert MIPS to generic I/O. The helpers below can
> >> + *       then be removed.
> >> + */
> >> +#ifdef CONFIG_64BIT
> >> +static inline u64 fb_readq(const volatile void __iomem *addr)
> >> +{
> >> +       return __raw_readq(addr);
> >
> > ... so how can this call work?
>
> On 64-bit builds, there's __raw_readq() and __raw_writeq().
>
> At first, I tried to do the right thing and convert MIPS to work with
> <asm-generic/io.h>. But that created a ton of follow-up errors in other
> headers. So for now, it's better to handle this problem in asm/fb.h.

So isn't just adding

    #define __raw_readq __raw_readq
    #define __raw_writeq __raw_writeq

to arch/mips/include/asm/io.h sufficient to make <asm-generic/fb.h>
do the right thing?

Gr{oetje,eeting}s,

                        Geert
Thomas Zimmermann May 10, 2023, 3:11 p.m. UTC | #7
Hi Geert

Am 10.05.23 um 16:34 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Wed, May 10, 2023 at 4:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 10.05.23 um 14:34 schrieb Geert Uytterhoeven:
>>> On Wed, May 10, 2023 at 1:06 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
>>>> in the architecture's <asm/fb.h> header file or the generic one.
>>>>
>>>> The common case has been the use of regular I/O functions, such as
>>>> __raw_readb() or memset_io(). A few architectures used plain system-
>>>> memory reads and writes. Sparc used helpers for its SBus.
>>>>
>>>> The architectures that used special cases provide the same code in
>>>> their __raw_*() I/O helpers. So the patch replaces this code with the
>>>> __raw_*() functions and moves it to <asm-generic/fb.h> for all
>>>> architectures.
>>>>
>>>> v6:
>>>>           * fix fb_readq()/fb_writeq() on 64-bit mips (kernel test robot)
>>>> v5:
>>>>           * include <linux/io.h> in <asm-generic/fb>; fix s390 build
>>>> v4:
>>>>           * ia64, loongarch, sparc64: add fb_mem*() to arch headers
>>>>             to keep current semantics (Arnd)
>>>> v3:
>>>>           * implement all architectures with generic helpers
>>>>           * support reordering and native byte order (Geert, Arnd)
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
>>>> --- a/arch/mips/include/asm/fb.h
>>>> +++ b/arch/mips/include/asm/fb.h
>>>> @@ -12,6 +12,28 @@ static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma,
>>>>    }
>>>>    #define fb_pgprotect fb_pgprotect
>>>>
>>>> +/*
>>>> + * MIPS doesn't define __raw_ I/O macros, so the helpers
>>>> + * in <asm-generic/fb.h> don't generate fb_readq() and
>>>> + * fb_write(). We have to provide them here.
>>>
>>> MIPS does not include <asm-generic/io.h>,  nor define its own
>>
>> I know, that's why the TODO says to convert it to generic I/O.
>>
>>> __raw_readq() and __raw_writeq()...
>>
>> It doesn't define those macros, but it generates function calls of the
>> same names. Follow the macros at
>>
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/mips/include/asm/io.h#L357
>>
>> It expands to a variety of helpers, including __raw_*().
> 
> Thanks, I forgot MIPS is using these grep-unfriendly factories...
> 
>>>> + *
>>>> + * TODO: Convert MIPS to generic I/O. The helpers below can
>>>> + *       then be removed.
>>>> + */
>>>> +#ifdef CONFIG_64BIT
>>>> +static inline u64 fb_readq(const volatile void __iomem *addr)
>>>> +{
>>>> +       return __raw_readq(addr);
>>>
>>> ... so how can this call work?
>>
>> On 64-bit builds, there's __raw_readq() and __raw_writeq().
>>
>> At first, I tried to do the right thing and convert MIPS to work with
>> <asm-generic/io.h>. But that created a ton of follow-up errors in other
>> headers. So for now, it's better to handle this problem in asm/fb.h.
> 
> So isn't just adding
> 
>      #define __raw_readq __raw_readq
>      #define __raw_writeq __raw_writeq
> 
> to arch/mips/include/asm/io.h sufficient to make <asm-generic/fb.h>
> do the right thing?

That works. I had a patch that adds all missing defines to MIPS' io.h. 
Then I went with the current fix, which is self-contained within fbdev. 
But I'd leave it to arch maintainers.

Best regards
Thomas


> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
Arnd Bergmann May 10, 2023, 3:54 p.m. UTC | #8
On Wed, May 10, 2023, at 16:27, Thomas Zimmermann wrote:
> Am 10.05.23 um 16:15 schrieb Arnd Bergmann:
>> On Wed, May 10, 2023, at 16:03, kernel test robot wrote:
 
>> I think that's a preexisting bug and I have no idea what the
>> correct solution is. Looking for HD64461 shows it being used
>> both with inw/outw and readw/writew, so there is no way to have
>> the correct type. The sh __raw_readw() definition hides this bug,
>> but that is a problem with arch/sh and it probably hides others
>> as well.
>
> The constant HD64461_IOBASE is defined as integer at
>
> 
> https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/hd64461.h#L17
>
> but fb_readw() expects a volatile-void pointer. I guess we could add a 
> cast somewhere to silence the problem. In the current upstream code, 
> that appears to be done by sh's __raw_readw() internally:
>
> 
> https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/io.h#L35

Sure, that would make it build again, but that still doesn't make the
code correct, since it's completely unclear what base address the
HD64461_IOBASE is relative to. The hp6xx platform code only passes it
through inw()/outw(), which take an offset relative to sh_io_port_base,
but that is not initialized on hp6xx. I tried to find in the history
when it broke, apparently that was in 2007 commit 34a780a0afeb ("sh:
hp6xx pata_platform support."), which removed the custom inw/outw
implementations.

      Arnd
Thomas Zimmermann May 11, 2023, 10:53 a.m. UTC | #9
Hi

Am 10.05.23 um 17:54 schrieb Arnd Bergmann:
> On Wed, May 10, 2023, at 16:27, Thomas Zimmermann wrote:
>> Am 10.05.23 um 16:15 schrieb Arnd Bergmann:
>>> On Wed, May 10, 2023, at 16:03, kernel test robot wrote:
>   
>>> I think that's a preexisting bug and I have no idea what the
>>> correct solution is. Looking for HD64461 shows it being used
>>> both with inw/outw and readw/writew, so there is no way to have
>>> the correct type. The sh __raw_readw() definition hides this bug,
>>> but that is a problem with arch/sh and it probably hides others
>>> as well.
>>
>> The constant HD64461_IOBASE is defined as integer at
>>
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/hd64461.h#L17
>>
>> but fb_readw() expects a volatile-void pointer. I guess we could add a
>> cast somewhere to silence the problem. In the current upstream code,
>> that appears to be done by sh's __raw_readw() internally:
>>
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/io.h#L35
> 
> Sure, that would make it build again, but that still doesn't make the
> code correct, since it's completely unclear what base address the

Oh, OK. I thought it's like vgafb, which grabs the fixed VGA framebuffer 
range.

> HD64461_IOBASE is relative to. The hp6xx platform code only passes it
> through inw()/outw(), which take an offset relative to sh_io_port_base,
> but that is not initialized on hp6xx. I tried to find in the history
> when it broke, apparently that was in 2007 commit 34a780a0afeb ("sh:
> hp6xx pata_platform support."), which removed the custom inw/outw
> implementations.

Thanks for looking this up. If this driver has been broken for 15 years, 
the correct fix is to delete it. I've meanwhile prepared the second-best 
fix, which is the type casting. It'll be in the next patchset.

Best regards
Thomas

> 
>        Arnd
Geert Uytterhoeven May 11, 2023, 12:35 p.m. UTC | #10
Hi Arnd,

CC Artur, who's working on HP Jornada 680.

On Wed, May 10, 2023 at 5:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, May 10, 2023, at 16:27, Thomas Zimmermann wrote:
> > Am 10.05.23 um 16:15 schrieb Arnd Bergmann:
> >> On Wed, May 10, 2023, at 16:03, kernel test robot wrote:
>
> >> I think that's a preexisting bug and I have no idea what the
> >> correct solution is. Looking for HD64461 shows it being used
> >> both with inw/outw and readw/writew, so there is no way to have
> >> the correct type. The sh __raw_readw() definition hides this bug,
> >> but that is a problem with arch/sh and it probably hides others
> >> as well.
> >
> > The constant HD64461_IOBASE is defined as integer at
> >
> >
> > https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/hd64461.h#L17
> >
> > but fb_readw() expects a volatile-void pointer. I guess we could add a
> > cast somewhere to silence the problem. In the current upstream code,
> > that appears to be done by sh's __raw_readw() internally:
> >
> >
> > https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/io.h#L35
>
> Sure, that would make it build again, but that still doesn't make the
> code correct, since it's completely unclear what base address the
> HD64461_IOBASE is relative to. The hp6xx platform code only passes it
> through inw()/outw(), which take an offset relative to sh_io_port_base,
> but that is not initialized on hp6xx. I tried to find in the history
> when it broke, apparently that was in 2007 commit 34a780a0afeb ("sh:
> hp6xx pata_platform support."), which removed the custom inw/outw
> implementations.

See also commit 4aafae27d0ce73f8 ("sh: hd64461 tidying."), which
claims they are no longer needed.

Don't the I/O port macros just treat the port as an absolute base address
when sh_io_port_base isn't set?

Gr{oetje,eeting}s,

                        Geert
Arnd Bergmann May 11, 2023, 12:48 p.m. UTC | #11
On Thu, May 11, 2023, at 14:35, Geert Uytterhoeven wrote:
> CC Artur, who's working on HP Jornada 680.
>
> On Wed, May 10, 2023 at 5:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, May 10, 2023, at 16:27, Thomas Zimmermann wrote:
>> > Am 10.05.23 um 16:15 schrieb Arnd Bergmann:
>> >> On Wed, May 10, 2023, at 16:03, kernel test robot wrote:
>
> See also commit 4aafae27d0ce73f8 ("sh: hd64461 tidying."), which
> claims they are no longer needed.
>
> Don't the I/O port macros just treat the port as an absolute base address
> when sh_io_port_base isn't set?

As far as I can tell, sh_io_port_base gets initialized to '-1'
specifically to prevent that from working by accident. So it's
almost treated as an absolute base address, but the off-by-one
offset ensures this never actually works unless it was first
set to the correct value.

      Arnd
Thomas Zimmermann May 11, 2023, 1:12 p.m. UTC | #12
Hi

Am 10.05.23 um 17:54 schrieb Arnd Bergmann:
> On Wed, May 10, 2023, at 16:27, Thomas Zimmermann wrote:
>> Am 10.05.23 um 16:15 schrieb Arnd Bergmann:
>>> On Wed, May 10, 2023, at 16:03, kernel test robot wrote:
>   
>>> I think that's a preexisting bug and I have no idea what the
>>> correct solution is. Looking for HD64461 shows it being used
>>> both with inw/outw and readw/writew, so there is no way to have
>>> the correct type. The sh __raw_readw() definition hides this bug,
>>> but that is a problem with arch/sh and it probably hides others
>>> as well.
>>
>> The constant HD64461_IOBASE is defined as integer at
>>
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/hd64461.h#L17
>>
>> but fb_readw() expects a volatile-void pointer. I guess we could add a
>> cast somewhere to silence the problem. In the current upstream code,
>> that appears to be done by sh's __raw_readw() internally:
>>
>>
>> https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/io.h#L35
> 
> Sure, that would make it build again, but that still doesn't make the
> code correct, since it's completely unclear what base address the
> HD64461_IOBASE is relative to. The hp6xx platform code only passes it
> through inw()/outw(), which take an offset relative to sh_io_port_base,
> but that is not initialized on hp6xx. I tried to find in the history
> when it broke, apparently that was in 2007 commit 34a780a0afeb ("sh:
> hp6xx pata_platform support."), which removed the custom inw/outw
> implementations.

It just occured to me that these fb_read and fb_write calls are probably 
all wrong. The fb_ interfaces are for framebuffer I/O memory. The driver 
uses them to access the regular state registers. The writew() on sh is 
definitely different. [1]

I assume that it only works because CONFIG_SWAP_IO_SPACE [2] is not set 
in hp6xx_defconfig.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/io.h#L55
[2] 
https://elixir.bootlin.com/linux/latest/source/arch/sh/include/mach-common/mach/mangle-port.h#L22

> 
>        Arnd
Artur Rojek May 11, 2023, 1:22 p.m. UTC | #13
On 2023-05-11 14:35, Geert Uytterhoeven wrote:
> Hi Arnd,
> 
> CC Artur, who's working on HP Jornada 680.
Thanks for CC'ing me - I faced this exact issue while working on my
(still not upstreamed) hd6446x PCMCIA controller driver. The PCMCIA
subsystem uses `inb/outb`, which expect the `sh_io_port_base` to be set
to something else than the default `-1`. At first I tried to set it to
`0xa0000000`, so that all I/O goes through the fixed, non-cacheable P2
area. That however broke some other driver code (I had no time to debug
which one). Eventually I ended up taking a suggestion from a MIPS PCMCIA
driver [1] and simply substract the broken `sh_io_port_base` address
from `HD64461_IOBASE`, as the base for `socket.io_offset`. This way all
the PCMCIA `inb/outb` accesses are absolute, no matter what the
`sh_io_port_base` is set to. This of course is a very ugly solution and
we should instead fix the root cause of this mess. I will have a better
look at this patch set and the problem at hand at a later date.

Cheers,
Artur

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pcmcia/db1xxx_ss.c?h=v6.4-rc1#n527

> 
> On Wed, May 10, 2023 at 5:55 PM Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, May 10, 2023, at 16:27, Thomas Zimmermann wrote:
>> > Am 10.05.23 um 16:15 schrieb Arnd Bergmann:
>> >> On Wed, May 10, 2023, at 16:03, kernel test robot wrote:
>> 
>> >> I think that's a preexisting bug and I have no idea what the
>> >> correct solution is. Looking for HD64461 shows it being used
>> >> both with inw/outw and readw/writew, so there is no way to have
>> >> the correct type. The sh __raw_readw() definition hides this bug,
>> >> but that is a problem with arch/sh and it probably hides others
>> >> as well.
>> >
>> > The constant HD64461_IOBASE is defined as integer at
>> >
>> >
>> > https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/hd64461.h#L17
>> >
>> > but fb_readw() expects a volatile-void pointer. I guess we could add a
>> > cast somewhere to silence the problem. In the current upstream code,
>> > that appears to be done by sh's __raw_readw() internally:
>> >
>> >
>> > https://elixir.bootlin.com/linux/latest/source/arch/sh/include/asm/io.h#L35
>> 
>> Sure, that would make it build again, but that still doesn't make the
>> code correct, since it's completely unclear what base address the
>> HD64461_IOBASE is relative to. The hp6xx platform code only passes it
>> through inw()/outw(), which take an offset relative to 
>> sh_io_port_base,
>> but that is not initialized on hp6xx. I tried to find in the history
>> when it broke, apparently that was in 2007 commit 34a780a0afeb ("sh:
>> hp6xx pata_platform support."), which removed the custom inw/outw
>> implementations.
> 
> See also commit 4aafae27d0ce73f8 ("sh: hd64461 tidying."), which
> claims they are no longer needed.
> 
> Don't the I/O port macros just treat the port as an absolute base 
> address
> when sh_io_port_base isn't set?
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
Arnd Bergmann May 11, 2023, 1:40 p.m. UTC | #14
On Thu, May 11, 2023, at 15:22, Artur Rojek wrote:
> On 2023-05-11 14:35, Geert Uytterhoeven wrote:
>> 
>> CC Artur, who's working on HP Jornada 680.
> Thanks for CC'ing me - I faced this exact issue while working on my
> (still not upstreamed) hd6446x PCMCIA controller driver. The PCMCIA
> subsystem uses `inb/outb`, which expect the `sh_io_port_base` to be set
> to something else than the default `-1`. At first I tried to set it to
> `0xa0000000`, so that all I/O goes through the fixed, non-cacheable P2
> area. That however broke some other driver code (I had no time to debug
> which one). Eventually I ended up taking a suggestion from a MIPS PCMCIA
> driver [1] and simply substract the broken `sh_io_port_base` address
> from `HD64461_IOBASE`, as the base for `socket.io_offset`. This way all
> the PCMCIA `inb/outb` accesses are absolute, no matter what the
> `sh_io_port_base` is set to. This of course is a very ugly solution and
> we should instead fix the root cause of this mess. I will have a better
> look at this patch set and the problem at hand at a later date.
>
> Cheers,
> Artur
>
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pcmcia/db1xxx_ss.c?h=v6.4-rc1#n527

I think the best fix would be to change all those drivers away
from using inb/outb to readb/writeb, except when they access the
actual PCMCIA I/O space behind the bridge.

On most of the modern architectures, inb(addr) now turns into
approximately readb(PCI_IOBASE + addr), with a bit of extra
logic to deal with endianess and barrier semantics.

PCI_IOBASE in turn tends to be a hardcoded virtual address
to which the physical I/O space window gets mapped during
early boot, though you can also #define it to sh_io_port_base
if you want to allocate the virtual address dynamically and
leave the existing logic unchanged.

Setting sh_io_port_base to zero however is a problem for any
driver that passes a small port number into it -- this then
turns into a user space pointer dereference, which is trivially
exploitable.

     Arnd
diff mbox series

Patch

diff --git a/arch/ia64/include/asm/fb.h b/arch/ia64/include/asm/fb.h
index 0208f64a0da0..bcf982043a5c 100644
--- a/arch/ia64/include/asm/fb.h
+++ b/arch/ia64/include/asm/fb.h
@@ -2,7 +2,9 @@ 
 #ifndef _ASM_FB_H_
 #define _ASM_FB_H_
 
+#include <linux/compiler.h>
 #include <linux/efi.h>
+#include <linux/string.h>
 
 #include <asm/page.h>
 
@@ -18,6 +20,24 @@  static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma,
 }
 #define fb_pgprotect fb_pgprotect
 
+static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
+{
+	memcpy(to, (void __force *)from, n);
+}
+#define fb_memcpy_fromfb fb_memcpy_fromfb
+
+static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+{
+	memcpy((void __force *)to, from, n);
+}
+#define fb_memcpy_tofb fb_memcpy_tofb
+
+static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+{
+	memset((void __force *)addr, c, n);
+}
+#define fb_memset fb_memset
+
 #include <asm-generic/fb.h>
 
 #endif /* _ASM_FB_H_ */
diff --git a/arch/loongarch/include/asm/fb.h b/arch/loongarch/include/asm/fb.h
index ff82f20685c8..c6fc7ef374a4 100644
--- a/arch/loongarch/include/asm/fb.h
+++ b/arch/loongarch/include/asm/fb.h
@@ -5,6 +5,27 @@ 
 #ifndef _ASM_FB_H_
 #define _ASM_FB_H_
 
+#include <linux/compiler.h>
+#include <linux/string.h>
+
+static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
+{
+	memcpy(to, (void __force *)from, n);
+}
+#define fb_memcpy_fromfb fb_memcpy_fromfb
+
+static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+{
+	memcpy((void __force *)to, from, n);
+}
+#define fb_memcpy_tofb fb_memcpy_tofb
+
+static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+{
+	memset((void __force *)addr, c, n);
+}
+#define fb_memset fb_memset
+
 #include <asm-generic/fb.h>
 
 #endif /* _ASM_FB_H_ */
diff --git a/arch/mips/include/asm/fb.h b/arch/mips/include/asm/fb.h
index 6bda0a81d8ca..18b7226403ba 100644
--- a/arch/mips/include/asm/fb.h
+++ b/arch/mips/include/asm/fb.h
@@ -12,6 +12,28 @@  static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma,
 }
 #define fb_pgprotect fb_pgprotect
 
+/*
+ * MIPS doesn't define __raw_ I/O macros, so the helpers
+ * in <asm-generic/fb.h> don't generate fb_readq() and
+ * fb_write(). We have to provide them here.
+ *
+ * TODO: Convert MIPS to generic I/O. The helpers below can
+ *       then be removed.
+ */
+#ifdef CONFIG_64BIT
+static inline u64 fb_readq(const volatile void __iomem *addr)
+{
+	return __raw_readq(addr);
+}
+#define fb_readq fb_readq
+
+static inline void fb_writeq(u64 b, volatile void __iomem *addr)
+{
+	__raw_writeq(b, addr);
+}
+#define fb_writeq fb_writeq
+#endif
+
 #include <asm-generic/fb.h>
 
 #endif /* _ASM_FB_H_ */
diff --git a/arch/sparc/include/asm/fb.h b/arch/sparc/include/asm/fb.h
index 689ee5c60054..077da91aeba1 100644
--- a/arch/sparc/include/asm/fb.h
+++ b/arch/sparc/include/asm/fb.h
@@ -2,6 +2,8 @@ 
 #ifndef _SPARC_FB_H_
 #define _SPARC_FB_H_
 
+#include <linux/io.h>
+
 struct fb_info;
 struct file;
 struct vm_area_struct;
@@ -16,6 +18,24 @@  static inline void fb_pgprotect(struct file *file, struct vm_area_struct *vma,
 int fb_is_primary_device(struct fb_info *info);
 #define fb_is_primary_device fb_is_primary_device
 
+static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
+{
+	sbus_memcpy_fromio(to, from, n);
+}
+#define fb_memcpy_fromfb fb_memcpy_fromfb
+
+static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+{
+	sbus_memcpy_toio(to, from, n);
+}
+#define fb_memcpy_tofb fb_memcpy_tofb
+
+static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+{
+	sbus_memset_io(addr, c, n);
+}
+#define fb_memset fb_memset
+
 #include <asm-generic/fb.h>
 
 #endif /* _SPARC_FB_H_ */
diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h
index c8af99f5a535..0540eccdbeca 100644
--- a/include/asm-generic/fb.h
+++ b/include/asm-generic/fb.h
@@ -7,6 +7,7 @@ 
  * Only include this header file from your architecture's <asm/fb.h>.
  */
 
+#include <linux/io.h>
 #include <linux/mm_types.h>
 #include <linux/pgtable.h>
 
@@ -30,4 +31,105 @@  static inline int fb_is_primary_device(struct fb_info *info)
 }
 #endif
 
+/*
+ * I/O helpers for the framebuffer. Prefer these functions over their
+ * regular counterparts. The regular I/O functions provide in-order
+ * access and swap bytes to/from little-endian ordering. Neither is
+ * required for framebuffers. Instead, the helpers read and write
+ * raw framebuffer data. Independent operations can be reordered for
+ * improved performance.
+ */
+
+#ifndef fb_readb
+static inline u8 fb_readb(const volatile void __iomem *addr)
+{
+	return __raw_readb(addr);
+}
+#define fb_readb fb_readb
+#endif
+
+#ifndef fb_readw
+static inline u16 fb_readw(const volatile void __iomem *addr)
+{
+	return __raw_readw(addr);
+}
+#define fb_readw fb_readw
+#endif
+
+#ifndef fb_readl
+static inline u32 fb_readl(const volatile void __iomem *addr)
+{
+	return __raw_readl(addr);
+}
+#define fb_readl fb_readl
+#endif
+
+#ifndef fb_readq
+#if defined(__raw_readq)
+static inline u64 fb_readq(const volatile void __iomem *addr)
+{
+	return __raw_readq(addr);
+}
+#define fb_readq fb_readq
+#endif
+#endif
+
+#ifndef fb_writeb
+static inline void fb_writeb(u8 b, volatile void __iomem *addr)
+{
+	__raw_writeb(b, addr);
+}
+#define fb_writeb fb_writeb
+#endif
+
+#ifndef fb_writew
+static inline void fb_writew(u16 b, volatile void __iomem *addr)
+{
+	__raw_writew(b, addr);
+}
+#define fb_writew fb_writew
+#endif
+
+#ifndef fb_writel
+static inline void fb_writel(u32 b, volatile void __iomem *addr)
+{
+	__raw_writel(b, addr);
+}
+#define fb_writel fb_writel
+#endif
+
+#ifndef fb_writeq
+#if defined(__raw_writeq)
+static inline void fb_writeq(u64 b, volatile void __iomem *addr)
+{
+	__raw_writeq(b, addr);
+}
+#define fb_writeq fb_writeq
+#endif
+#endif
+
+#ifndef fb_memcpy_fromfb
+static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
+{
+	memcpy_fromio(to, from, n);
+}
+#define fb_memcpy_fromfb fb_memcpy_fromfb
+#endif
+
+#ifndef fb_memcpy_tofb
+static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+{
+	memcpy_toio(to, from, n);
+}
+#define fb_memcpy_tofb fb_memcpy_tofb
+#endif
+
+#ifndef fb_memset
+static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+{
+	memset_io(addr, c, n);
+}
+#define fb_memset fb_memset
+#endif
+
 #endif /* __ASM_GENERIC_FB_H_ */
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 4b4d9a5d200a..2cf8efcb9e32 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -17,7 +17,6 @@ 
 #include <linux/slab.h>
 
 #include <asm/fb.h>
-#include <asm/io.h>
 
 struct vm_area_struct;
 struct fb_info;
@@ -513,58 +512,6 @@  struct fb_info {
  */
 #define STUPID_ACCELF_TEXT_SHIT
 
-// This will go away
-#if defined(__sparc__)
-
-/* We map all of our framebuffers such that big-endian accesses
- * are what we want, so the following is sufficient.
- */
-
-// This will go away
-#define fb_readb sbus_readb
-#define fb_readw sbus_readw
-#define fb_readl sbus_readl
-#define fb_readq sbus_readq
-#define fb_writeb sbus_writeb
-#define fb_writew sbus_writew
-#define fb_writel sbus_writel
-#define fb_writeq sbus_writeq
-#define fb_memset sbus_memset_io
-#define fb_memcpy_fromfb sbus_memcpy_fromio
-#define fb_memcpy_tofb sbus_memcpy_toio
-
-#elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) ||	\
-	defined(__hppa__) || defined(__sh__) || defined(__powerpc__) ||	\
-	defined(__arm__) || defined(__aarch64__) || defined(__mips__)
-
-#define fb_readb __raw_readb
-#define fb_readw __raw_readw
-#define fb_readl __raw_readl
-#define fb_readq __raw_readq
-#define fb_writeb __raw_writeb
-#define fb_writew __raw_writew
-#define fb_writel __raw_writel
-#define fb_writeq __raw_writeq
-#define fb_memset memset_io
-#define fb_memcpy_fromfb memcpy_fromio
-#define fb_memcpy_tofb memcpy_toio
-
-#else
-
-#define fb_readb(addr) (*(volatile u8 *) (addr))
-#define fb_readw(addr) (*(volatile u16 *) (addr))
-#define fb_readl(addr) (*(volatile u32 *) (addr))
-#define fb_readq(addr) (*(volatile u64 *) (addr))
-#define fb_writeb(b,addr) (*(volatile u8 *) (addr) = (b))
-#define fb_writew(b,addr) (*(volatile u16 *) (addr) = (b))
-#define fb_writel(b,addr) (*(volatile u32 *) (addr) = (b))
-#define fb_writeq(b,addr) (*(volatile u64 *) (addr) = (b))
-#define fb_memset memset
-#define fb_memcpy_fromfb memcpy
-#define fb_memcpy_tofb memcpy
-
-#endif
-
 #define FB_LEFT_POS(p, bpp)          (fb_be_math(p) ? (32 - (bpp)) : 0)
 #define FB_SHIFT_HIGH(p, val, bits)  (fb_be_math(p) ? (val) >> (bits) : \
 						      (val) << (bits))