| Message ID | 20230510110557.14343-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fbdev: Move framebuffer I/O helpers to <asm/fb.h> | expand |
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
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
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
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 >
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
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
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 >
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
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
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
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
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
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
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 --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))