[RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[Archit Taneja]

This removes the sparse warnings on arm platforms:

warning: cast removes address space of expression

Signed-off-by: Archit Taneja <archit@ti.com>
---
 include/linux/fb.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fb.h b/include/linux/fb.h
index c7a9571..7fce1e1 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -548,7 +548,7 @@ static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
 #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(__avr32__) || defined(__bfin__)
+#elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) || defined(__hppa__) || defined(__sh__) || defined(__powerpc__) || defined(__avr32__) || defined(__bfin__) || defined(__arm__)
 
 #define fb_readb __raw_readb
 #define fb_readw __raw_readw
-- 
1.7.9.5

Re: [RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On 2012-11-16 11:16, Archit Taneja wrote:
> This removes the sparse warnings on arm platforms:
> 
> warning: cast removes address space of expression
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  include/linux/fb.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index c7a9571..7fce1e1 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -548,7 +548,7 @@ static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
>  #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(__avr32__) || defined(__bfin__)
> +#elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) || defined(__hppa__) || defined(__sh__) || defined(__powerpc__) || defined(__avr32__) || defined(__bfin__) || defined(__arm__)
>  
>  #define fb_readb __raw_readb
>  #define fb_readw __raw_readw

Looks good and works for me:

Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 Tomi



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

RE: [RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[H Hartley Sweeten]

On Friday, November 16, 2012 2:16 AM, Archit Taneja wrote:
>
> This removes the sparse warnings on arm platforms:
>
> warning: cast removes address space of expression
>
> Signed-off-by: Archit Taneja <archit@ti.com>

I submitted the same patch around early March 2012. So FWIW:

Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Regards,
Hartley

Re: [RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[Archit Taneja]

On Friday 16 November 2012 10:14 PM, H Hartley Sweeten wrote:
> On Friday, November 16, 2012 2:16 AM, Archit Taneja wrote:
>>
>> This removes the sparse warnings on arm platforms:
>>
>> warning: cast removes address space of expression
>>
>> Signed-off-by: Archit Taneja <archit@ti.com>
>
> I submitted the same patch around early March 2012. So FWIW:
>
> Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Thanks.

Florian,

Could you queue this for 3.8 merge window?

Regards,
Archit

Re: [RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[Russell King - ARM Linux]

On Mon, Nov 19, 2012 at 10:51:08AM +0530, Archit Taneja wrote:
> On Friday 16 November 2012 10:14 PM, H Hartley Sweeten wrote:
>> On Friday, November 16, 2012 2:16 AM, Archit Taneja wrote:
>>>
>>> This removes the sparse warnings on arm platforms:
>>>
>>> warning: cast removes address space of expression
>>>
>>> Signed-off-by: Archit Taneja <archit@ti.com>
>>
>> I submitted the same patch around early March 2012. So FWIW:
>>
>> Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>
> Thanks.
>
> Florian,
>
> Could you queue this for 3.8 merge window?

Actually no.  Has anyone checked whether this has any impact for the BE
ARM platforms?

Re: [RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On 2012-11-19 11:15, Russell King - ARM Linux wrote:
> On Mon, Nov 19, 2012 at 10:51:08AM +0530, Archit Taneja wrote:
>> On Friday 16 November 2012 10:14 PM, H Hartley Sweeten wrote:
>>> On Friday, November 16, 2012 2:16 AM, Archit Taneja wrote:
>>>>
>>>> This removes the sparse warnings on arm platforms:
>>>>
>>>> warning: cast removes address space of expression
>>>>
>>>> Signed-off-by: Archit Taneja <archit@ti.com>
>>>
>>> I submitted the same patch around early March 2012. So FWIW:
>>>
>>> Reviewed-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>>
>> Thanks.
>>
>> Florian,
>>
>> Could you queue this for 3.8 merge window?
> 
> Actually no.  Has anyone checked whether this has any impact for the BE
> ARM platforms?

Probably not. I can't say anything to that matter, but I wonder if this
patch is just going around the problem that we get sparse warnings when
falling into the else ifdef block in fb.h.

The macros in the else block are defined as:

#define fb_readb(addr) (*(volatile u8 *) (addr))                                 

And fb code passes a pointer to __iomem. So shouldn't the cast be to
(volatile u8 __iomem *)?

 Tomi



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

Re: [RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[Russell King - ARM Linux]

On Mon, Nov 19, 2012 at 11:36:24AM +0200, Tomi Valkeinen wrote:
> Probably not. I can't say anything to that matter, but I wonder if this
> patch is just going around the problem that we get sparse warnings when
> falling into the else ifdef block in fb.h.
> 
> The macros in the else block are defined as:
> 
> #define fb_readb(addr) (*(volatile u8 *) (addr))                                 
> 
> And fb code passes a pointer to __iomem. So shouldn't the cast be to
> (volatile u8 __iomem *)?

No, because sparse won't let you directly dereference an __iomem pointer.

Directly dereferencing an __iomem pointer is wrong, always has been.  It's
marked with __iomem _because_ it's a separate cookied address space from
the virtual address space.

This is one of the situations which has been left because the warning is
correct - and this is one of those situations where silencing them via
casts et.al. is the wrong thing to do.  The right thing to do is to leave
the warning in place.

This is one of the hardest things that I seem to get people to understand:
if the code is wrong then we _should_ get a warning for it and we should
definitely not paper over the warning.

Re: [RFC] fbdev: arm has __raw I/O accessors, use them in fb.h

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]

On 2012-11-19 11:46, Russell King - ARM Linux wrote:
> On Mon, Nov 19, 2012 at 11:36:24AM +0200, Tomi Valkeinen wrote:
>> Probably not. I can't say anything to that matter, but I wonder if this
>> patch is just going around the problem that we get sparse warnings when
>> falling into the else ifdef block in fb.h.
>>
>> The macros in the else block are defined as:
>>
>> #define fb_readb(addr) (*(volatile u8 *) (addr))                                 
>>
>> And fb code passes a pointer to __iomem. So shouldn't the cast be to
>> (volatile u8 __iomem *)?
> 
> No, because sparse won't let you directly dereference an __iomem pointer.
> 
> Directly dereferencing an __iomem pointer is wrong, always has been.  It's
> marked with __iomem _because_ it's a separate cookied address space from
> the virtual address space.
> 
> This is one of the situations which has been left because the warning is
> correct - and this is one of those situations where silencing them via
> casts et.al. is the wrong thing to do.  The right thing to do is to leave
> the warning in place.
> 
> This is one of the hardest things that I seem to get people to understand:
> if the code is wrong then we _should_ get a warning for it and we should
> definitely not paper over the warning.

Yes, you're right. Well, I'm not very experienced with handling
different endiannesses, but my analysis:

fb.h uses either __raw_read/write functions or (*(volatile u32 *)
(addr)) and (*(volatile u32 *) (addr) = (b)) for read/write.

__raw_read/write are defined in arch/arm/include/asm/io.h and are the
same for BE and LE. I made a test c-file with two functions, one using
raw_read style assembly, other using pointer dereference. I compiled it
with -mlittle-endian and -mbig-endian, and the resulting assembly was
identical for both, and the assembly for both functions were the same.

So if I didn't make any mistakes above, using __raw_read/write instead
of the direct pointer dereference results in identical operation for
both big and little endian arms. So if big-endian fb reads/writes is
correct now, it should be correct with raw_read/write also.

 Tomi



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