[PATCH v6 0/6] fbdev: Move framebuffer I/O helpers to <asm/fb.h>

[PATCH v6 0/6] fbdev: Move framebuffer I/O helpers to <asm/fb.h>

[Thomas Zimmermann]

Fbdev provides helpers for framebuffer I/O, such as fb_readl(),
fb_writel() or fb_memcpy_to_fb(). The implementation of each helper
depends on the architecture, but they are all equivalent to regular
I/O functions of similar names. So use regular functions instead and
move all helpers into <asm-generic/fb.h>

The first patch a simple whitespace cleanup.

Until now, <linux/fb.h> contained an include of <asm/io.h>. As this
will go away, patches 2 to 4 prepare include statements in the various
drivers. Source files that use regular I/O helpers, such as readl(),
now include <linux/io.h>. Source files that use framebuffer I/O
helpers, such as fb_readl(), now include <linux/fb.h>.

Patch 5 replaces the architecture-based if-else branching in 
<linux/fb.h> by helpers in <asm-generic/fb.h>. All helpers use Linux'
existing I/O functions.

Patch 6 harmonizes naming among fbdev and existing I/O functions.

The patchset has been built for a variety of platforms, such as x86-64,
arm, aarch64, ppc64, parisc, m64k, mips and sparc.

v6:
	* fix build on 64-bit mips (kernel test robot)
	* update fb_io_fops.c
v5:
      	* fix build on s390
v4:
	* keep fb_mem*() as-is on ia64, loongarch, sparc64 (Arnd)
	* don't include <asm/fb.h> (Sam)
v3:
	* add the new helpers in <asm-generic/fb.h>
	* support reordering and native byte order (Geert, Arnd)
v2:
	* use Linux I/O helpers (Sam, Arnd)

Thomas Zimmermann (6):
  fbdev/matrox: Remove trailing whitespaces
  ipu-v3: Include <linux/io.h>
  fbdev: Include <linux/io.h> in various drivers
  fbdev: Include <linux/fb.h> instead of <asm/fb.h>
  fbdev: Move framebuffer I/O helpers into <asm/fb.h>
  fbdev: Rename fb_mem*() helpers

 arch/ia64/include/asm/fb.h                  |  20 ++++
 arch/loongarch/include/asm/fb.h             |  21 ++++
 arch/mips/include/asm/fb.h                  |  22 +++++
 arch/parisc/video/fbdev.c                   |   3 +-
 arch/sparc/include/asm/fb.h                 |  20 ++++
 arch/sparc/video/fbdev.c                    |   1 -
 arch/x86/video/fbdev.c                      |   2 -
 drivers/gpu/ipu-v3/ipu-prv.h                |   1 +
 drivers/staging/sm750fb/sm750.c             |   2 +-
 drivers/video/fbdev/arcfb.c                 |   1 +
 drivers/video/fbdev/aty/atyfb.h             |   2 +
 drivers/video/fbdev/aty/mach64_cursor.c     |   2 +-
 drivers/video/fbdev/chipsfb.c               |   2 +-
 drivers/video/fbdev/core/fb_io_fops.c       |   4 +-
 drivers/video/fbdev/core/fbcon.c            |   1 -
 drivers/video/fbdev/core/fbmem.c            |   2 -
 drivers/video/fbdev/kyro/fbdev.c            |   2 +-
 drivers/video/fbdev/matrox/matroxfb_accel.c |   6 +-
 drivers/video/fbdev/matrox/matroxfb_base.h  |   4 +-
 drivers/video/fbdev/pvr2fb.c                |   2 +-
 drivers/video/fbdev/sstfb.c                 |   2 +-
 drivers/video/fbdev/stifb.c                 |   4 +-
 drivers/video/fbdev/tdfxfb.c                |   2 +-
 drivers/video/fbdev/wmt_ge_rops.c           |   2 +
 include/asm-generic/fb.h                    | 102 ++++++++++++++++++++
 include/linux/fb.h                          |  55 +----------
 26 files changed, 210 insertions(+), 77 deletions(-)

-- 
2.40.1

[PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Thomas Zimmermann]

Fix coding style. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++---
 drivers/video/fbdev/matrox/matroxfb_base.h  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c b/drivers/video/fbdev/matrox/matroxfb_accel.c
index 9cb0685feddd..ce51227798a1 100644
--- a/drivers/video/fbdev/matrox/matroxfb_accel.c
+++ b/drivers/video/fbdev/matrox/matroxfb_accel.c
@@ -88,7 +88,7 @@
 
 static inline void matrox_cfb4_pal(u_int32_t* pal) {
 	unsigned int i;
-	
+
 	for (i = 0; i < 16; i++) {
 		pal[i] = i * 0x11111111U;
 	}
@@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) {
 
 static inline void matrox_cfb8_pal(u_int32_t* pal) {
 	unsigned int i;
-	
+
 	for (i = 0; i < 16; i++) {
 		pal[i] = i * 0x01010101U;
 	}
@@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct matrox_fb_info *minfo, u_int32_t fgx,
 			/* Tell... well, why bother... */
 			while (height--) {
 				size_t i;
-				
+
 				for (i = 0; i < step; i += 4) {
 					/* Hope that there are at least three readable bytes beyond the end of bitmap */
 					fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr);
diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h b/drivers/video/fbdev/matrox/matroxfb_base.h
index 958be6805f87..c93c69bbcd57 100644
--- a/drivers/video/fbdev/matrox/matroxfb_base.h
+++ b/drivers/video/fbdev/matrox/matroxfb_base.h
@@ -301,9 +301,9 @@ struct matrox_altout {
 	int		(*verifymode)(void* altout_dev, u_int32_t mode);
 	int		(*getqueryctrl)(void* altout_dev,
 					struct v4l2_queryctrl* ctrl);
-	int		(*getctrl)(void* altout_dev, 
+	int		(*getctrl)(void *altout_dev,
 				   struct v4l2_control* ctrl);
-	int		(*setctrl)(void* altout_dev, 
+	int		(*setctrl)(void *altout_dev,
 				   struct v4l2_control* ctrl);
 };
 
-- 
2.40.1

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Sui Jingfeng]

Hi, Thomas


I love your patch, yet something to improve:


On 2023/5/10 19:05, Thomas Zimmermann wrote:
> Fix coding style. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>   drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++---
>   drivers/video/fbdev/matrox/matroxfb_base.h  | 4 ++--
>   2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c b/drivers/video/fbdev/matrox/matroxfb_accel.c
> index 9cb0685feddd..ce51227798a1 100644
> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c
> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c
> @@ -88,7 +88,7 @@
>   
>   static inline void matrox_cfb4_pal(u_int32_t* pal) {
>   	unsigned int i;
> -	
> +
>   	for (i = 0; i < 16; i++) {
>   		pal[i] = i * 0x11111111U;
>   	}
> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) {
>   
>   static inline void matrox_cfb8_pal(u_int32_t* pal) {
>   	unsigned int i;
> -	
> +
>   	for (i = 0; i < 16; i++) {
>   		pal[i] = i * 0x01010101U;
>   	}
> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct matrox_fb_info *minfo, u_int32_t fgx,
>   			/* Tell... well, why bother... */
>   			while (height--) {
>   				size_t i;
> -				
> +
>   				for (i = 0; i < step; i += 4) {
>   					/* Hope that there are at least three readable bytes beyond the end of bitmap */
>   					fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr);
> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h b/drivers/video/fbdev/matrox/matroxfb_base.h
> index 958be6805f87..c93c69bbcd57 100644
> --- a/drivers/video/fbdev/matrox/matroxfb_base.h
> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h
> @@ -301,9 +301,9 @@ struct matrox_altout {
>   	int		(*verifymode)(void* altout_dev, u_int32_t mode);
>   	int		(*getqueryctrl)(void* altout_dev,
>   					struct v4l2_queryctrl* ctrl);

Noticed that there are plenty of coding style problems in matroxfb_base.h,

why you only fix a few of them?   Take this two line as an example, 
shouldn't

they be fixed also as following?


  	int		(*verifymode)(void *altout_dev, u_int32_t mode);
  	int		(*getqueryctrl)(void *altout_dev,
  					struct v4l2_queryctrl *ctrl);


> -	int		(*getctrl)(void* altout_dev,
> +	int		(*getctrl)(void *altout_dev,
>   				   struct v4l2_control* ctrl);
> -	int		(*setctrl)(void* altout_dev,
> +	int		(*setctrl)(void *altout_dev,
>   				   struct v4l2_control* ctrl);
>   };
>   

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4433 bytes --]

Hi

Am 10.05.23 um 20:20 schrieb Sui Jingfeng:
> Hi, Thomas
> 
> 
> I love your patch, yet something to improve:
> 
> 
> On 2023/5/10 19:05, Thomas Zimmermann wrote:
>> Fix coding style. No functional changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++---
>>   drivers/video/fbdev/matrox/matroxfb_base.h  | 4 ++--
>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c 
>> b/drivers/video/fbdev/matrox/matroxfb_accel.c
>> index 9cb0685feddd..ce51227798a1 100644
>> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c
>> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c
>> @@ -88,7 +88,7 @@
>>   static inline void matrox_cfb4_pal(u_int32_t* pal) {
>>       unsigned int i;
>> -
>> +
>>       for (i = 0; i < 16; i++) {
>>           pal[i] = i * 0x11111111U;
>>       }
>> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) {
>>   static inline void matrox_cfb8_pal(u_int32_t* pal) {
>>       unsigned int i;
>> -
>> +
>>       for (i = 0; i < 16; i++) {
>>           pal[i] = i * 0x01010101U;
>>       }
>> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct 
>> matrox_fb_info *minfo, u_int32_t fgx,
>>               /* Tell... well, why bother... */
>>               while (height--) {
>>                   size_t i;
>> -
>> +
>>                   for (i = 0; i < step; i += 4) {
>>                       /* Hope that there are at least three readable 
>> bytes beyond the end of bitmap */
>>                       fb_writel(get_unaligned((u_int32_t*)(chardata + 
>> i)),mmio.vaddr);
>> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h 
>> b/drivers/video/fbdev/matrox/matroxfb_base.h
>> index 958be6805f87..c93c69bbcd57 100644
>> --- a/drivers/video/fbdev/matrox/matroxfb_base.h
>> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h
>> @@ -301,9 +301,9 @@ struct matrox_altout {
>>       int        (*verifymode)(void* altout_dev, u_int32_t mode);
>>       int        (*getqueryctrl)(void* altout_dev,
>>                       struct v4l2_queryctrl* ctrl);
> 
> Noticed that there are plenty of coding style problems in matroxfb_base.h,
> 
> why you only fix a few of them?   Take this two line as an example, 
> shouldn't
> 
> they be fixed also as following?

I configured my text editor to remove trailing whitespaces 
automatically. That keeps my own patches free of them.  But the editor 
removes all trailing whitespaces, including those that have been there 
before. If I encounter such a case, I split out the whitespace fix and 
submit it separately.

But the work I do within fbdev is mostly for improving DRM. For the 
other issues in this file, I don't think that matroxfb should even be 
around any longer. Fbdev has been deprecated for a long time. But a 
small number of drivers are still in use and we still need its 
framebuffer console. So someone should either put significant effort 
into maintaining fbdev, or it should be phased out. But neither is 
happening.

Best regards
Thomas

> 
> 
>       int        (*verifymode)(void *altout_dev, u_int32_t mode);
>       int        (*getqueryctrl)(void *altout_dev,
>                       struct v4l2_queryctrl *ctrl);
> 
> 
>> -    int        (*getctrl)(void* altout_dev,
>> +    int        (*getctrl)(void *altout_dev,
>>                      struct v4l2_control* ctrl);
>> -    int        (*setctrl)(void* altout_dev,
>> +    int        (*setctrl)(void *altout_dev,
>>                      struct v4l2_control* ctrl);
>>   };

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Sui Jingfeng]

Hi

On 2023/5/11 15:55, Thomas Zimmermann wrote:
> Hi
>
> Am 10.05.23 um 20:20 schrieb Sui Jingfeng:
>> Hi, Thomas
>>
>>
>> I love your patch, yet something to improve:
>>
>>
>> On 2023/5/10 19:05, Thomas Zimmermann wrote:
>>> Fix coding style. No functional changes.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>> ---
>>>   drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++---
>>>   drivers/video/fbdev/matrox/matroxfb_base.h  | 4 ++--
>>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c 
>>> b/drivers/video/fbdev/matrox/matroxfb_accel.c
>>> index 9cb0685feddd..ce51227798a1 100644
>>> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c
>>> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c
>>> @@ -88,7 +88,7 @@
>>>   static inline void matrox_cfb4_pal(u_int32_t* pal) {
>>>       unsigned int i;
>>> -
>>> +
>>>       for (i = 0; i < 16; i++) {
>>>           pal[i] = i * 0x11111111U;
>>>       }
>>> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) {
>>>   static inline void matrox_cfb8_pal(u_int32_t* pal) {
>>>       unsigned int i;
>>> -
>>> +
>>>       for (i = 0; i < 16; i++) {
>>>           pal[i] = i * 0x01010101U;
>>>       }
>>> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct 
>>> matrox_fb_info *minfo, u_int32_t fgx,
>>>               /* Tell... well, why bother... */
>>>               while (height--) {
>>>                   size_t i;
>>> -
>>> +
>>>                   for (i = 0; i < step; i += 4) {
>>>                       /* Hope that there are at least three readable 
>>> bytes beyond the end of bitmap */
>>> fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr);
>>> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h 
>>> b/drivers/video/fbdev/matrox/matroxfb_base.h
>>> index 958be6805f87..c93c69bbcd57 100644
>>> --- a/drivers/video/fbdev/matrox/matroxfb_base.h
>>> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h
>>> @@ -301,9 +301,9 @@ struct matrox_altout {
>>>       int        (*verifymode)(void* altout_dev, u_int32_t mode);
>>>       int        (*getqueryctrl)(void* altout_dev,
>>>                       struct v4l2_queryctrl* ctrl);
>>
>> Noticed that there are plenty of coding style problems in 
>> matroxfb_base.h,
>>
>> why you only fix a few of them?   Take this two line as an example, 
>> shouldn't
>>
>> they be fixed also as following?
>
> I configured my text editor to remove trailing whitespaces 
> automatically. That keeps my own patches free of them.  But the editor 
> removes all trailing whitespaces, including those that have been there 
> before. If I encounter such a case, I split out the whitespace fix and 
> submit it separately.
>
> But the work I do within fbdev is mostly for improving DRM. For the 
> other issues in this file, I don't think that matroxfb should even be 
> around any longer. Fbdev has been deprecated for a long time. But a 
> small number of drivers are still in use and we still need its 
> framebuffer console. So someone should either put significant effort 
> into maintaining fbdev, or it should be phased out. But neither is 
> happening.
>
Ok, no problem, that sound fine and reasonable then.

The lines being modified has trailing whitespaces.

And I tested your patch again last night on loongarch and mips platform.

It still works in my testing case.

> Best regards
> Thomas
>
>>
>>
>>       int        (*verifymode)(void *altout_dev, u_int32_t mode);
>>       int        (*getqueryctrl)(void *altout_dev,
>>                       struct v4l2_queryctrl *ctrl);
>>
>>
>>> -    int        (*getctrl)(void* altout_dev,
>>> +    int        (*getctrl)(void *altout_dev,
>>>                      struct v4l2_control* ctrl);
>>> -    int        (*setctrl)(void* altout_dev,
>>> +    int        (*setctrl)(void *altout_dev,
>>>                      struct v4l2_control* ctrl);
>>>   };
>

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Helge Deller]

On 5/11/23 09:55, Thomas Zimmermann wrote:
> Hi
>
> Am 10.05.23 um 20:20 schrieb Sui Jingfeng:
>> Hi, Thomas
>>
>>
>> I love your patch, yet something to improve:
>>
>>
>> On 2023/5/10 19:05, Thomas Zimmermann wrote:
>>> Fix coding style. No functional changes.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>> ---
>>>   drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++---
>>>   drivers/video/fbdev/matrox/matroxfb_base.h  | 4 ++--
>>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c b/drivers/video/fbdev/matrox/matroxfb_accel.c
>>> index 9cb0685feddd..ce51227798a1 100644
>>> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c
>>> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c
>>> @@ -88,7 +88,7 @@
>>>   static inline void matrox_cfb4_pal(u_int32_t* pal) {
>>>       unsigned int i;
>>> -
>>> +
>>>       for (i = 0; i < 16; i++) {
>>>           pal[i] = i * 0x11111111U;
>>>       }
>>> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) {
>>>   static inline void matrox_cfb8_pal(u_int32_t* pal) {
>>>       unsigned int i;
>>> -
>>> +
>>>       for (i = 0; i < 16; i++) {
>>>           pal[i] = i * 0x01010101U;
>>>       }
>>> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct matrox_fb_info *minfo, u_int32_t fgx,
>>>               /* Tell... well, why bother... */
>>>               while (height--) {
>>>                   size_t i;
>>> -
>>> +
>>>                   for (i = 0; i < step; i += 4) {
>>>                       /* Hope that there are at least three readable bytes beyond the end of bitmap */
>>>                       fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr);
>>> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h b/drivers/video/fbdev/matrox/matroxfb_base.h
>>> index 958be6805f87..c93c69bbcd57 100644
>>> --- a/drivers/video/fbdev/matrox/matroxfb_base.h
>>> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h
>>> @@ -301,9 +301,9 @@ struct matrox_altout {
>>>       int        (*verifymode)(void* altout_dev, u_int32_t mode);
>>>       int        (*getqueryctrl)(void* altout_dev,
>>>                       struct v4l2_queryctrl* ctrl);
>>
>> Noticed that there are plenty of coding style problems in matroxfb_base.h,
>>
>> why you only fix a few of them?   Take this two line as an example, shouldn't
>>
>> they be fixed also as following?
>
> I configured my text editor to remove trailing whitespaces
> automatically. That keeps my own patches free of them.  But the
> editor removes all trailing whitespaces, including those that have
> been there before. If I encounter such a case, I split out the
> whitespace fix and submit it separately.
>
> But the work I do within fbdev is mostly for improving DRM.

Sure.

> For the
> other issues in this file, I don't think that matroxfb should even be
> around any longer. Fbdev has been deprecated for a long time. But a
> small number of drivers are still in use and we still need its
> framebuffer console. So someone should either put significant effort
> into maintaining fbdev, or it should be phased out. But neither is
> happening.

You're wrong.

You don't mention that for most older machines DRM isn't an acceptable
way to go due to it's limitations, e.g. it's low-speed due to missing
2D-acceleration for older cards and and it's incapability to change screen
resolution at runtime (just to name two of the bigger limitations here).
So, unless we somehow find a good way to move such drivers over to DRM
(with a set of minimal 2D acceleration), they are still important.

Actually, I just did test matroxfb and pm2fb successfully a few days back, and
they worked. For some smaller issues I've prepared patches, which are on hold
due conflicts with your latest file-move-around- and whitespace-changes which are partly
in drm-misc.
And I do have some upcoming additional patches for console support.

Helge

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Geert Uytterhoeven]

Hi Helge,

On Thu, May 11, 2023 at 3:05 PM Helge Deller <deller@gmx.de> wrote:
> On 5/11/23 09:55, Thomas Zimmermann wrote:
> > But the work I do within fbdev is mostly for improving DRM.
>
> Sure.
>
> > For the
> > other issues in this file, I don't think that matroxfb should even be
> > around any longer. Fbdev has been deprecated for a long time. But a
> > small number of drivers are still in use and we still need its
> > framebuffer console. So someone should either put significant effort
> > into maintaining fbdev, or it should be phased out. But neither is
> > happening.
>
> You're wrong.
>
> You don't mention that for most older machines DRM isn't an acceptable
> way to go due to it's limitations, e.g. it's low-speed due to missing
> 2D-acceleration for older cards and and it's incapability to change screen
> resolution at runtime (just to name two of the bigger limitations here).
> So, unless we somehow find a good way to move such drivers over to DRM
> (with a set of minimal 2D acceleration), they are still important.

DRM can change resolution at runtime, just not through the fbdev API.

Or do you mean the resolution of the text console, akin to
"fbset <mode>"? I have to admit I do not know if there is a command
line tool to do that...

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Helge Deller]

On 5/11/23 15:10, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Thu, May 11, 2023 at 3:05 PM Helge Deller <deller@gmx.de> wrote:
>> On 5/11/23 09:55, Thomas Zimmermann wrote:
>>> But the work I do within fbdev is mostly for improving DRM.
>>
>> Sure.
>>
>>> For the
>>> other issues in this file, I don't think that matroxfb should even be
>>> around any longer. Fbdev has been deprecated for a long time. But a
>>> small number of drivers are still in use and we still need its
>>> framebuffer console. So someone should either put significant effort
>>> into maintaining fbdev, or it should be phased out. But neither is
>>> happening.
>>
>> You're wrong.
>>
>> You don't mention that for most older machines DRM isn't an acceptable
>> way to go due to it's limitations, e.g. it's low-speed due to missing
>> 2D-acceleration for older cards and and it's incapability to change screen
>> resolution at runtime (just to name two of the bigger limitations here).
>> So, unless we somehow find a good way to move such drivers over to DRM
>> (with a set of minimal 2D acceleration), they are still important.
>
> DRM can change resolution at runtime,

Right, sure.

> just not through the fbdev API.

... and sadly the simpledrm-based drivers neither.

> Or do you mean the resolution of the text console, akin to
> "fbset <mode>"?

yes.

> I have to admit I do not know if there is a command
> line tool to do that...

Helge

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6494 bytes --]

Hi

Am 11.05.23 um 15:05 schrieb Helge Deller:
> On 5/11/23 09:55, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 10.05.23 um 20:20 schrieb Sui Jingfeng:
>>> Hi, Thomas
>>>
>>>
>>> I love your patch, yet something to improve:
>>>
>>>
>>> On 2023/5/10 19:05, Thomas Zimmermann wrote:
>>>> Fix coding style. No functional changes.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>>> ---
>>>>   drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++---
>>>>   drivers/video/fbdev/matrox/matroxfb_base.h  | 4 ++--
>>>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c 
>>>> b/drivers/video/fbdev/matrox/matroxfb_accel.c
>>>> index 9cb0685feddd..ce51227798a1 100644
>>>> --- a/drivers/video/fbdev/matrox/matroxfb_accel.c
>>>> +++ b/drivers/video/fbdev/matrox/matroxfb_accel.c
>>>> @@ -88,7 +88,7 @@
>>>>   static inline void matrox_cfb4_pal(u_int32_t* pal) {
>>>>       unsigned int i;
>>>> -
>>>> +
>>>>       for (i = 0; i < 16; i++) {
>>>>           pal[i] = i * 0x11111111U;
>>>>       }
>>>> @@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) {
>>>>   static inline void matrox_cfb8_pal(u_int32_t* pal) {
>>>>       unsigned int i;
>>>> -
>>>> +
>>>>       for (i = 0; i < 16; i++) {
>>>>           pal[i] = i * 0x01010101U;
>>>>       }
>>>> @@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct 
>>>> matrox_fb_info *minfo, u_int32_t fgx,
>>>>               /* Tell... well, why bother... */
>>>>               while (height--) {
>>>>                   size_t i;
>>>> -
>>>> +
>>>>                   for (i = 0; i < step; i += 4) {
>>>>                       /* Hope that there are at least three readable 
>>>> bytes beyond the end of bitmap */
>>>>                       fb_writel(get_unaligned((u_int32_t*)(chardata 
>>>> + i)),mmio.vaddr);
>>>> diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h 
>>>> b/drivers/video/fbdev/matrox/matroxfb_base.h
>>>> index 958be6805f87..c93c69bbcd57 100644
>>>> --- a/drivers/video/fbdev/matrox/matroxfb_base.h
>>>> +++ b/drivers/video/fbdev/matrox/matroxfb_base.h
>>>> @@ -301,9 +301,9 @@ struct matrox_altout {
>>>>       int        (*verifymode)(void* altout_dev, u_int32_t mode);
>>>>       int        (*getqueryctrl)(void* altout_dev,
>>>>                       struct v4l2_queryctrl* ctrl);
>>>
>>> Noticed that there are plenty of coding style problems in 
>>> matroxfb_base.h,
>>>
>>> why you only fix a few of them?   Take this two line as an example, 
>>> shouldn't
>>>
>>> they be fixed also as following?
>>
>> I configured my text editor to remove trailing whitespaces
>> automatically. That keeps my own patches free of them.  But the
>> editor removes all trailing whitespaces, including those that have
>> been there before. If I encounter such a case, I split out the
>> whitespace fix and submit it separately.
>>
>> But the work I do within fbdev is mostly for improving DRM.
> 
> Sure.
> 
>> For the
>> other issues in this file, I don't think that matroxfb should even be
>> around any longer. Fbdev has been deprecated for a long time. But a
>> small number of drivers are still in use and we still need its
>> framebuffer console. So someone should either put significant effort
>> into maintaining fbdev, or it should be phased out. But neither is
>> happening.
> 
> You're wrong.

I'm not. I don't claim that these drivers are all broken. But fbdev as a 
whole is bit-rotting and no one attempts to address this. There are 
several recent examples of this:

  * I recently send out a 100-patches series to improve parameter 
parsing and avoid memory leaks. That got shot down. I didn't attempt to 
support parameter parsing for module builds.

  * There's been a 15-yrs old bug in fbdev's read/write where they 
return an incorrect value.

* See the other discussion on this patchset on the state of hitfb.

  * The fbdev code I recently cleaned up had bugs in how it uses some of 
fbdev's basic building blocks (see the screen_base/screen_buffer confusion).

  * <asm-generic/fb.h> has been in the tree since 2009 and no one 
attempted to include it until now.

None of this is a sign of good maintenance.

As I've worked on DRM's fbdev emulation a lot, I try to be a good kernel 
citizen and clean up in fbdev as well when I see a problem. But I'd 
really like to see most of these drivers being moved into staging and 
deleted soon afterwards. Users will complain about those drivers that 
are really still required. Those might be worth to spend effort on.

> 
> You don't mention that for most older machines DRM isn't an acceptable
> way to go due to it's limitations, e.g. it's low-speed due to missing
> 2D-acceleration for older cards and and it's incapability to change screen
> resolution at runtime (just to name two of the bigger limitations here).

You can change resolution at runtime; just not through fbdev ioctls. 
There's no technical limitation here. No one found any use for this, so 
it's not there.

> So, unless we somehow find a good way to move such drivers over to DRM
> (with a set of minimal 2D acceleration), they are still important.

2d acceleration is mostly useful for the framebuffer console. You can do 
that with DRM and drivers have (nouveau). It just didn't make a 
meaningful difference in most cases.

Best regards
Thomas

> 
> Actually, I just did test matroxfb and pm2fb successfully a few days 
> back, and
> they worked. For some smaller issues I've prepared patches, which are on 
> hold
> due conflicts with your latest file-move-around- and whitespace-changes 
> which are partly
> in drm-misc.
> And I do have some upcoming additional patches for console support.
> 
> Helge

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Helge Deller]

On 5/11/23 16:27, Thomas Zimmermann wrote:
>>> But the work I do within fbdev is mostly for improving DRM.
>>
>> Sure.
>>
>>> For the
>>> other issues in this file, I don't think that matroxfb should even be
>>> around any longer. Fbdev has been deprecated for a long time. But a
>>> small number of drivers are still in use and we still need its
>>> framebuffer console. So someone should either put significant effort
>>> into maintaining fbdev, or it should be phased out. But neither is
>>> happening.
>>
>> You're wrong.
>

> I'm not. I don't claim that these drivers are all broken. But fbdev
> as a whole is bit-rotting and no one attempts to address this. There
> are several recent examples of this:
>
> * I recently send out a 100-patches series to improve parameter
> parsing and avoid memory leaks. That got shot down. I didn't attempt
> to support parameter parsing for module builds.

Your work is appreciated and it wasn't shot down, but it wasn't perfect either.

> * There's been a 15-yrs old bug in fbdev's read/write where they
> return an incorrect value.

?

> * See the other discussion on this patchset on the state of hitfb.
>
> * The fbdev code I recently cleaned up had bugs in how it uses some
> of fbdev's basic building blocks (see the screen_base/screen_buffer
> confusion).

As you said... some (little-used/outdated) drivers may have issues
which of course show up if one starts to clean up, as you do.
On a per-driver basis it can make sense to drop a specific driver.

> * <asm-generic/fb.h> has been in the tree since 2009 and no one
> attempted to include it until now.
>
> None of this is a sign of good maintenance.
> As I've worked on DRM's fbdev emulation a lot, I try to be a good
> kernel citizen and clean up in fbdev as well when I see a problem.> But I'd really like to see most of these drivers being moved into
> staging and deleted soon afterwards. Users will complain about those
> drivers that are really still required. Those might be worth to spend
> effort on.

I'd really like to see a way forward and get the required drivers over to
DRM, e.g. based on your simpledrm driver.
If there is a way to get basic on-screen 2D bitblt and fillrect support,
it would drop the need for most of the fbdev drivers.
The current way of bitblt'ing from a buffer on regular basis istoo slow for such older cards. Even on new hardware in emulators there
is a big slowdown visible.

>> You don't mention that for most older machines DRM isn't an
>> acceptable way to go due to it's limitations, e.g. it's low-speed
>> due to missing 2D-acceleration for older cards and and it's
>> incapability to change screen resolution at runtime (just to name
>> two of the bigger limitations here).
>
> You can change resolution at runtime; just not through fbdev ioctls.
> There's no technical limitation here. No one found any use for this,
> so it's not there.

fbdev drivers would need that when ported to DRM.
>> So, unless we somehow find a good way to move such drivers over to
>> DRM (with a set of minimal 2D acceleration), they are still
>> important.
>
> 2d acceleration is mostly useful for the framebuffer console.

and X11

> You can
> do that with DRM and drivers have (nouveau). It just didn't make a
> meaningful difference in most cases.

if nouveau got it, can't it be done for simpledrm in a generic way too?

>> Actually, I just did test matroxfb and pm2fb successfully a few
>> days back, and they worked. For some smaller issues I've prepared
>> patches, which are on hold due conflicts with your latest
>> file-move-around- and whitespace-changes which are partly in
>> drm-misc. And I do have some upcoming additional patches for
>> console support.

Helge

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4861 bytes --]

Hi

Am 11.05.23 um 19:02 schrieb Helge Deller:
> On 5/11/23 16:27, Thomas Zimmermann wrote:
>>>> But the work I do within fbdev is mostly for improving DRM.
>>>
>>> Sure.
>>>
>>>> For the
>>>> other issues in this file, I don't think that matroxfb should even be
>>>> around any longer. Fbdev has been deprecated for a long time. But a
>>>> small number of drivers are still in use and we still need its
>>>> framebuffer console. So someone should either put significant effort
>>>> into maintaining fbdev, or it should be phased out. But neither is
>>>> happening.
>>>
>>> You're wrong.
>>
> 
>> I'm not. I don't claim that these drivers are all broken. But fbdev
>> as a whole is bit-rotting and no one attempts to address this. There
>> are several recent examples of this:
>>
>> * I recently send out a 100-patches series to improve parameter
>> parsing and avoid memory leaks. That got shot down. I didn't attempt
>> to support parameter parsing for module builds.
> 
> Your work is appreciated and it wasn't shot down, but it wasn't perfect 
> either.
> 
>> * There's been a 15-yrs old bug in fbdev's read/write where they
>> return an incorrect value.
> 
> ?

This one:

https://patchwork.freedesktop.org/patch/534668/?series=116931&rev=2

> 
>> * See the other discussion on this patchset on the state of hitfb.
>>
>> * The fbdev code I recently cleaned up had bugs in how it uses some
>> of fbdev's basic building blocks (see the screen_base/screen_buffer
>> confusion).
> 
> As you said... some (little-used/outdated) drivers may have issues
> which of course show up if one starts to clean up, as you do.
> On a per-driver basis it can make sense to drop a specific driver.
> 
>> * <asm-generic/fb.h> has been in the tree since 2009 and no one
>> attempted to include it until now.
>>
>> None of this is a sign of good maintenance.

Let me add that I'm not pointing fingers at anyone. It's just the 
current status AFAICT.

>> As I've worked on DRM's fbdev emulation a lot, I try to be a good
>> kernel citizen and clean up in fbdev as well when I see a problem.> 
>> But I'd really like to see most of these drivers being moved into
>> staging and deleted soon afterwards. Users will complain about those
>> drivers that are really still required. Those might be worth to spend
>> effort on.
> 
> I'd really like to see a way forward and get the required drivers over to
> DRM, e.g. based on your simpledrm driver.
> If there is a way to get basic on-screen 2D bitblt and fillrect support,
> it would drop the need for most of the fbdev drivers.
> The current way of bitblt'ing from a buffer on regular basis istoo slow 
> for such older cards. Even on new hardware in emulators there
> is a big slowdown visible.

I'd be happy to try to drop the unused/obsolete/broken drivers. For the 
rest, I'd designate fbdev as a graphics console for systems without text 
mode. I think that was the original intention.

> 
>>> You don't mention that for most older machines DRM isn't an
>>> acceptable way to go due to it's limitations, e.g. it's low-speed
>>> due to missing 2D-acceleration for older cards and and it's
>>> incapability to change screen resolution at runtime (just to name
>>> two of the bigger limitations here).
>>
>> You can change resolution at runtime; just not through fbdev ioctls.
>> There's no technical limitation here. No one found any use for this,
>> so it's not there.
> 
> fbdev drivers would need that when ported to DRM.

Why? Userspace would then simply use DRM ioctls to set the display mode.

>>> So, unless we somehow find a good way to move such drivers over to
>>> DRM (with a set of minimal 2D acceleration), they are still
>>> important.
>>
>> 2d acceleration is mostly useful for the framebuffer console.
> 
> and X11
> 
>> You can
>> do that with DRM and drivers have (nouveau). It just didn't make a
>> meaningful difference in most cases.
> 
> if nouveau got it, can't it be done for simpledrm in a generic way too?

Probably no, as it depends on the hardware features. The DRM driver 
would have to implement it's own fbdev support. That's not too 
complicated, but still not portable among drivers.

> 
>>> Actually, I just did test matroxfb and pm2fb successfully a few
>>> days back, and they worked. For some smaller issues I've prepared
>>> patches, which are on hold due conflicts with your latest
>>> file-move-around- and whitespace-changes which are partly in
>>> drm-misc. And I do have some upcoming additional patches for
>>> console support.
> 
> Helge

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 1/6] fbdev/matrox: Remove trailing whitespaces

[Finn Thain]

On Thu, 11 May 2023, Thomas Zimmermann wrote:

> But I'd really like to see most of these drivers being moved into 
> staging and deleted soon afterwards. Users will complain about those 
> drivers that are really still required. Those might be worth to spend 
> effort on.
> 

That strategy is not going to find out what functionality is required. 
Instead it will find out which beneficiaries are capable of overcoming all 
of the hurdles to reverting deletion:

 - Find out how to report a regression correctly.
 - Gather all the necessary information.
 - Obtain buy-in from a sympathetic developer.
 - Build a patched kernel, test it and provide the results. (And possibly 
   repeat the same until neglected code becomes accepted.)
 - Wait for the relevant distro to release the relevant kernel update. 

Developers tend to overlook the burden of process because it's ostensibly 
done to raise code quality. But it seems to me that affected users are 
more likely to seek a workaround than undertake the process.

So deletion doesn't discover end-user requirements. What it does is 
advertise a vacancy for an unpaid adoptive maintainer, somehow presumed to 
be found amongst a very well remunerated and very small pool of talent.

The way I look at it, the maintainence of old code is the price of a 
so-called "right to repair". But there ain't no free lunch and if we want 
that right we should seek ways to reduce that price. For example, by 
making a larger talent pool more effective, by re-using more code and by 
improving the tooling and automation.

The code I'd delete first wouldn't be a small amount of old code in need 
of sponsorship. Or even the most buggy code. The first to go would be that 
code which will never find an actual end user because some portion of the 
code required to actually use certain platforms was never mainlined by the 
vendor -- and never will be without some push-back.

[PATCH v6 2/6] ipu-v3: Include <linux/io.h>

[Thomas Zimmermann]

The code uses readl() and writel(). Include the header file to
get the declarations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/ipu-v3/ipu-prv.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/ipu-v3/ipu-prv.h b/drivers/gpu/ipu-v3/ipu-prv.h
index 291ac1bab66d..d4621b1ea7f1 100644
--- a/drivers/gpu/ipu-v3/ipu-prv.h
+++ b/drivers/gpu/ipu-v3/ipu-prv.h
@@ -8,6 +8,7 @@
 
 struct ipu_soc;
 
+#include <linux/io.h>
 #include <linux/types.h>
 #include <linux/device.h>
 #include <linux/clk.h>
-- 
2.40.1

[PATCH v6 3/6] fbdev: Include <linux/io.h> in various drivers

[Thomas Zimmermann]

The code uses writel() and similar I/O-memory helpers. Include
the header file to get the declarations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/video/fbdev/arcfb.c       | 1 +
 drivers/video/fbdev/aty/atyfb.h   | 2 ++
 drivers/video/fbdev/wmt_ge_rops.c | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/drivers/video/fbdev/arcfb.c b/drivers/video/fbdev/arcfb.c
index 7750e020839e..ace104850e50 100644
--- a/drivers/video/fbdev/arcfb.c
+++ b/drivers/video/fbdev/arcfb.c
@@ -41,6 +41,7 @@
 #include <linux/vmalloc.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/fb.h>
 #include <linux/init.h>
 #include <linux/arcfb.h>
diff --git a/drivers/video/fbdev/aty/atyfb.h b/drivers/video/fbdev/aty/atyfb.h
index 465f55beb97f..30da3e82ed3c 100644
--- a/drivers/video/fbdev/aty/atyfb.h
+++ b/drivers/video/fbdev/aty/atyfb.h
@@ -3,8 +3,10 @@
  *  ATI Frame Buffer Device Driver Core Definitions
  */
 
+#include <linux/io.h>
 #include <linux/spinlock.h>
 #include <linux/wait.h>
+
     /*
      *  Elements of the hardware specific atyfb_par structure
      */
diff --git a/drivers/video/fbdev/wmt_ge_rops.c b/drivers/video/fbdev/wmt_ge_rops.c
index 42255d27a1db..99c7b0aea615 100644
--- a/drivers/video/fbdev/wmt_ge_rops.c
+++ b/drivers/video/fbdev/wmt_ge_rops.c
@@ -9,7 +9,9 @@
 
 #include <linux/module.h>
 #include <linux/fb.h>
+#include <linux/io.h>
 #include <linux/platform_device.h>
+
 #include "core/fb_draw.h"
 #include "wmt_ge_rops.h"
 
-- 
2.40.1

[PATCH v6 4/6] fbdev: Include <linux/fb.h> instead of <asm/fb.h>

[Thomas Zimmermann]

Replace include statements for <asm/fb.h> with <linux/fb.h>. Fixes
the coding style: if a header is available in asm/ and linux/, it
is preferable to include the header from linux/. This only affects
a few source files, most of which already include <linux/fb.h>.

Suggested-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 arch/parisc/video/fbdev.c        | 3 +--
 arch/sparc/video/fbdev.c         | 1 -
 arch/x86/video/fbdev.c           | 2 --
 drivers/staging/sm750fb/sm750.c  | 2 +-
 drivers/video/fbdev/core/fbcon.c | 1 -
 drivers/video/fbdev/core/fbmem.c | 2 --
 include/linux/fb.h               | 2 ++
 7 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/parisc/video/fbdev.c b/arch/parisc/video/fbdev.c
index 4a0ae08fc75b..137561d98246 100644
--- a/arch/parisc/video/fbdev.c
+++ b/arch/parisc/video/fbdev.c
@@ -5,10 +5,9 @@
  * Copyright (C) 2001-2002 Thomas Bogendoerfer <tsbogend@alpha.franken.de>
  */
 
+#include <linux/fb.h>
 #include <linux/module.h>
 
-#include <asm/fb.h>
-
 #include <video/sticore.h>
 
 int fb_is_primary_device(struct fb_info *info)
diff --git a/arch/sparc/video/fbdev.c b/arch/sparc/video/fbdev.c
index dadd5799fbb3..25837f128132 100644
--- a/arch/sparc/video/fbdev.c
+++ b/arch/sparc/video/fbdev.c
@@ -4,7 +4,6 @@
 #include <linux/fb.h>
 #include <linux/module.h>
 
-#include <asm/fb.h>
 #include <asm/prom.h>
 
 int fb_is_primary_device(struct fb_info *info)
diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
index 57ee3c158f97..f41a17ebac48 100644
--- a/arch/x86/video/fbdev.c
+++ b/arch/x86/video/fbdev.c
@@ -7,8 +7,6 @@
  *
  */
 
-#include <asm/fb.h>
-
 #include <linux/fb.h>
 #include <linux/module.h>
 #include <linux/pci.h>
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 22ace3168723..55e302a27847 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -16,7 +16,7 @@
 #include <linux/pagemap.h>
 #include <linux/screen_info.h>
 #include <linux/console.h>
-#include <asm/fb.h>
+
 #include "sm750.h"
 #include "sm750_accel.h"
 #include "sm750_cursor.h"
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index eb565a10e5cd..c6c9d040bdec 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -75,7 +75,6 @@
 #include <linux/interrupt.h>
 #include <linux/crc32.h> /* For counting font checksums */
 #include <linux/uaccess.h>
-#include <asm/fb.h>
 #include <asm/irq.h>
 
 #include "fbcon.h"
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 4035a57df116..41b0706be635 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -37,8 +37,6 @@
 #include <linux/mem_encrypt.h>
 #include <linux/pci.h>
 
-#include <asm/fb.h>
-
 #include <video/nomodeset.h>
 #include <video/vga.h>
 
diff --git a/include/linux/fb.h b/include/linux/fb.h
index ec978a4969a9..4b4d9a5d200a 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -15,6 +15,8 @@
 #include <linux/list.h>
 #include <linux/backlight.h>
 #include <linux/slab.h>
+
+#include <asm/fb.h>
 #include <asm/io.h>
 
 struct vm_area_struct;
-- 
2.40.1

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

[Thomas Zimmermann]

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(-)

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))
-- 
2.40.1

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

[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()

> --- 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

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

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

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3412 bytes --]

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
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[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?

Gr{oetje,eeting}s,

                        Geert

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

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

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3847 bytes --]

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
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[kernel test robot]

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  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[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.

       Arnd

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2376 bytes --]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[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.

      Arnd

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2139 bytes --]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Geert Uytterhoeven]

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

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

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

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

[Arnd Bergmann]

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

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

[Artur Rojek]

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

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

[Arnd Bergmann]

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

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2375 bytes --]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH v6 6/6] fbdev: Rename fb_mem*() helpers

[Thomas Zimmermann]

Update the names of the fb_mem*() helpers to be consistent with their
regular counterparts. Hence, fb_memset() now becomes fb_memset_io(),
fb_memcpy_fromfb() now becomes fb_memcpy_fromio() and fb_memcpy_tofb()
becomes fb_memcpy_toio(). No functional changes.

v6:
	* update new file fb_io_fops.c

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 arch/ia64/include/asm/fb.h              | 12 ++++++------
 arch/loongarch/include/asm/fb.h         | 12 ++++++------
 arch/sparc/include/asm/fb.h             | 12 ++++++------
 drivers/video/fbdev/aty/mach64_cursor.c |  2 +-
 drivers/video/fbdev/chipsfb.c           |  2 +-
 drivers/video/fbdev/core/fb_io_fops.c   |  4 ++--
 drivers/video/fbdev/kyro/fbdev.c        |  2 +-
 drivers/video/fbdev/pvr2fb.c            |  2 +-
 drivers/video/fbdev/sstfb.c             |  2 +-
 drivers/video/fbdev/stifb.c             |  4 ++--
 drivers/video/fbdev/tdfxfb.c            |  2 +-
 include/asm-generic/fb.h                | 16 ++++++++--------
 12 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/arch/ia64/include/asm/fb.h b/arch/ia64/include/asm/fb.h
index bcf982043a5c..1717b26fd423 100644
--- a/arch/ia64/include/asm/fb.h
+++ b/arch/ia64/include/asm/fb.h
@@ -20,23 +20,23 @@ 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)
+static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 {
 	memcpy(to, (void __force *)from, n);
 }
-#define fb_memcpy_fromfb fb_memcpy_fromfb
+#define fb_memcpy_fromio fb_memcpy_fromio
 
-static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
 {
 	memcpy((void __force *)to, from, n);
 }
-#define fb_memcpy_tofb fb_memcpy_tofb
+#define fb_memcpy_toio fb_memcpy_toio
 
-static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n)
 {
 	memset((void __force *)addr, c, n);
 }
-#define fb_memset fb_memset
+#define fb_memset fb_memset_io
 
 #include <asm-generic/fb.h>
 
diff --git a/arch/loongarch/include/asm/fb.h b/arch/loongarch/include/asm/fb.h
index c6fc7ef374a4..0b218b10a9ec 100644
--- a/arch/loongarch/include/asm/fb.h
+++ b/arch/loongarch/include/asm/fb.h
@@ -8,23 +8,23 @@
 #include <linux/compiler.h>
 #include <linux/string.h>
 
-static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
+static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 {
 	memcpy(to, (void __force *)from, n);
 }
-#define fb_memcpy_fromfb fb_memcpy_fromfb
+#define fb_memcpy_fromio fb_memcpy_fromio
 
-static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
 {
 	memcpy((void __force *)to, from, n);
 }
-#define fb_memcpy_tofb fb_memcpy_tofb
+#define fb_memcpy_toio fb_memcpy_toio
 
-static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n)
 {
 	memset((void __force *)addr, c, n);
 }
-#define fb_memset fb_memset
+#define fb_memset fb_memset_io
 
 #include <asm-generic/fb.h>
 
diff --git a/arch/sparc/include/asm/fb.h b/arch/sparc/include/asm/fb.h
index 077da91aeba1..572ecd3e1cc4 100644
--- a/arch/sparc/include/asm/fb.h
+++ b/arch/sparc/include/asm/fb.h
@@ -18,23 +18,23 @@ 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)
+static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 {
 	sbus_memcpy_fromio(to, from, n);
 }
-#define fb_memcpy_fromfb fb_memcpy_fromfb
+#define fb_memcpy_fromio fb_memcpy_fromio
 
-static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
 {
 	sbus_memcpy_toio(to, from, n);
 }
-#define fb_memcpy_tofb fb_memcpy_tofb
+#define fb_memcpy_toio fb_memcpy_toio
 
-static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n)
 {
 	sbus_memset_io(addr, c, n);
 }
-#define fb_memset fb_memset
+#define fb_memset fb_memset_io
 
 #include <asm-generic/fb.h>
 
diff --git a/drivers/video/fbdev/aty/mach64_cursor.c b/drivers/video/fbdev/aty/mach64_cursor.c
index 4ad0331a8c57..971355c2cd7e 100644
--- a/drivers/video/fbdev/aty/mach64_cursor.c
+++ b/drivers/video/fbdev/aty/mach64_cursor.c
@@ -153,7 +153,7 @@ static int atyfb_cursor(struct fb_info *info, struct fb_cursor *cursor)
 	    u8 m, b;
 
 	    // Clear cursor image with 1010101010...
-	    fb_memset(dst, 0xaa, 1024);
+	    fb_memset_io(dst, 0xaa, 1024);
 
 	    offset = align - width*2;
 
diff --git a/drivers/video/fbdev/chipsfb.c b/drivers/video/fbdev/chipsfb.c
index 7799d52a651f..2a27ba94f652 100644
--- a/drivers/video/fbdev/chipsfb.c
+++ b/drivers/video/fbdev/chipsfb.c
@@ -332,7 +332,7 @@ static const struct fb_var_screeninfo chipsfb_var = {
 
 static void init_chips(struct fb_info *p, unsigned long addr)
 {
-	fb_memset(p->screen_base, 0, 0x100000);
+	fb_memset_io(p->screen_base, 0, 0x100000);
 
 	p->fix = chipsfb_fix;
 	p->fix.smem_start = addr;
diff --git a/drivers/video/fbdev/core/fb_io_fops.c b/drivers/video/fbdev/core/fb_io_fops.c
index f5299d50f33b..5985e5e1b040 100644
--- a/drivers/video/fbdev/core/fb_io_fops.c
+++ b/drivers/video/fbdev/core/fb_io_fops.c
@@ -42,7 +42,7 @@ ssize_t fb_io_read(struct fb_info *info, char __user *buf, size_t count, loff_t
 	while (count) {
 		c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
 		dst = buffer;
-		fb_memcpy_fromfb(dst, src, c);
+		fb_memcpy_fromio(dst, src, c);
 		dst += c;
 		src += c;
 
@@ -117,7 +117,7 @@ ssize_t fb_io_write(struct fb_info *info, const char __user *buf, size_t count,
 		}
 		c -= trailing;
 
-		fb_memcpy_tofb(dst, src, c);
+		fb_memcpy_toio(dst, src, c);
 		dst += c;
 		src += c;
 		*ppos += c;
diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c
index 0596573ef140..3f277bdb3a32 100644
--- a/drivers/video/fbdev/kyro/fbdev.c
+++ b/drivers/video/fbdev/kyro/fbdev.c
@@ -737,7 +737,7 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			       info->var.bits_per_pixel);
 	size *= info->var.yres_virtual;
 
-	fb_memset(info->screen_base, 0, size);
+	fb_memset_io(info->screen_base, 0, size);
 
 	if (register_framebuffer(info) < 0)
 		goto out_unmap;
diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c
index 550fdb5b4d41..c692cd597ce3 100644
--- a/drivers/video/fbdev/pvr2fb.c
+++ b/drivers/video/fbdev/pvr2fb.c
@@ -801,7 +801,7 @@ static int __maybe_unused pvr2fb_common_init(void)
 		goto out_err;
 	}
 
-	fb_memset(fb_info->screen_base, 0, pvr2_fix.smem_len);
+	fb_memset_io(fb_info->screen_base, 0, pvr2_fix.smem_len);
 
 	pvr2_fix.ypanstep	= nopan  ? 0 : 1;
 	pvr2_fix.ywrapstep	= nowrap ? 0 : 1;
diff --git a/drivers/video/fbdev/sstfb.c b/drivers/video/fbdev/sstfb.c
index da296b2ab54a..582324f5d869 100644
--- a/drivers/video/fbdev/sstfb.c
+++ b/drivers/video/fbdev/sstfb.c
@@ -335,7 +335,7 @@ static int sst_calc_pll(const int freq, int *freq_out, struct pll_timing *t)
 static void sstfb_clear_screen(struct fb_info *info)
 {
 	/* clear screen */
-	fb_memset(info->screen_base, 0, info->fix.smem_len);
+	fb_memset_io(info->screen_base, 0, info->fix.smem_len);
 }
 
 
diff --git a/drivers/video/fbdev/stifb.c b/drivers/video/fbdev/stifb.c
index baca6974e288..01363dccfdaf 100644
--- a/drivers/video/fbdev/stifb.c
+++ b/drivers/video/fbdev/stifb.c
@@ -527,8 +527,8 @@ rattlerSetupPlanes(struct stifb_info *fb)
 	fb->id = saved_id;
 
 	for (y = 0; y < fb->info.var.yres; ++y)
-		fb_memset(fb->info.screen_base + y * fb->info.fix.line_length,
-			0xff, fb->info.var.xres * fb->info.var.bits_per_pixel/8);
+		fb_memset_io(fb->info.screen_base + y * fb->info.fix.line_length,
+			     0xff, fb->info.var.xres * fb->info.var.bits_per_pixel/8);
 
 	CRX24_SET_OVLY_MASK(fb);
 	SETUP_FB(fb);
diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index d17e5e1472aa..cdf8e9fe9948 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -1116,7 +1116,7 @@ static int tdfxfb_cursor(struct fb_info *info, struct fb_cursor *cursor)
 		u8 *mask = (u8 *)cursor->mask;
 		int i;
 
-		fb_memset(cursorbase, 0, 1024);
+		fb_memset_io(cursorbase, 0, 1024);
 
 		for (i = 0; i < cursor->image.height; i++) {
 			int h = 0;
diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h
index 0540eccdbeca..bb7ee9c70e60 100644
--- a/include/asm-generic/fb.h
+++ b/include/asm-generic/fb.h
@@ -108,28 +108,28 @@ static inline void fb_writeq(u64 b, volatile void __iomem *addr)
 #endif
 #endif
 
-#ifndef fb_memcpy_fromfb
-static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
+#ifndef fb_memcpy_fromio
+static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 {
 	memcpy_fromio(to, from, n);
 }
-#define fb_memcpy_fromfb fb_memcpy_fromfb
+#define fb_memcpy_fromio fb_memcpy_fromio
 #endif
 
-#ifndef fb_memcpy_tofb
-static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+#ifndef fb_memcpy_toio
+static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
 {
 	memcpy_toio(to, from, n);
 }
-#define fb_memcpy_tofb fb_memcpy_tofb
+#define fb_memcpy_toio fb_memcpy_toio
 #endif
 
 #ifndef fb_memset
-static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n)
 {
 	memset_io(addr, c, n);
 }
-#define fb_memset fb_memset
+#define fb_memset fb_memset_io
 #endif
 
 #endif /* __ASM_GENERIC_FB_H_ */
-- 
2.40.1