Re: [PATCH v2] offb: make the screen properties endian safe

[Cedric Le Goater <clg@fr.ibm.com>]

Hi,

On 11/20/2013 11:50 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-10-31 at 10:36 +0100, Cédric Le Goater wrote:
>> The "screen" properties : depth, width, height, linebytes need
>> to be converted to the host endian order when read from the device
>> tree.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
> 
> Did you actually test that ? IE, using emulated VGA in qemu for
> example ?

Yes, that's how I did. I ran LE and BE guests with "qemu -vga std" and 
different depths.

> I'm asking because there are a few interesting nits here...
> 
>  - fbdev *generally* assume native endian framebuffer, but of course
> under qemu today, the adapter will use a big endian frame buffer
> aperture. You can compile in support for foreign endian but I don't know
> how that actually works.

OK. I will see how I can extend the tests. But, are you suggesting I should
be using the foreign endian framework for the frame buffer ? 
 
>  - The setcolreg fix ... the "value" is used 2 lines above your endian
> swap, is that correct ?

The variables used to calculate "value" are in host endian order. It should
be fine.

Thanks,

C.

 
> Cheers
> Ben.
> 
> 
>> Changes in v2:
>>
>>  - replaced be32_to_cpu() by be32_to_cpup() 
>>  - fixed setcolreg ops 
>>
>>  drivers/video/offb.c |   12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/video/offb.c b/drivers/video/offb.c
>> index 0c4f343..68e8415 100644
>> --- a/drivers/video/offb.c
>> +++ b/drivers/video/offb.c
>> @@ -120,7 +120,7 @@ static int offb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
>>  			mask <<= info->var.transp.offset;
>>  			value |= mask;
>>  		}
>> -		pal[regno] = value;
>> +		pal[regno] = cpu_to_be32(value);
>>  		return 0;
>>  	}
>>  
>> @@ -536,7 +536,7 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
>>  	unsigned int flags, rsize, addr_prop = 0;
>>  	unsigned long max_size = 0;
>>  	u64 rstart, address = OF_BAD_ADDR;
>> -	const u32 *pp, *addrp, *up;
>> +	const __be32 *pp, *addrp, *up;
>>  	u64 asize;
>>  	int foreign_endian = 0;
>>  
>> @@ -552,25 +552,25 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
>>  	if (pp == NULL)
>>  		pp = of_get_property(dp, "depth", &len);
>>  	if (pp && len == sizeof(u32))
>> -		depth = *pp;
>> +		depth = be32_to_cpup(pp);
>>  
>>  	pp = of_get_property(dp, "linux,bootx-width", &len);
>>  	if (pp == NULL)
>>  		pp = of_get_property(dp, "width", &len);
>>  	if (pp && len == sizeof(u32))
>> -		width = *pp;
>> +		width = be32_to_cpup(pp);
>>  
>>  	pp = of_get_property(dp, "linux,bootx-height", &len);
>>  	if (pp == NULL)
>>  		pp = of_get_property(dp, "height", &len);
>>  	if (pp && len == sizeof(u32))
>> -		height = *pp;
>> +		height = be32_to_cpup(pp);
>>  
>>  	pp = of_get_property(dp, "linux,bootx-linebytes", &len);
>>  	if (pp == NULL)
>>  		pp = of_get_property(dp, "linebytes", &len);
>>  	if (pp && len == sizeof(u32) && (*pp != 0xffffffffu))
>> -		pitch = *pp;
>> +		pitch = be32_to_cpup(pp);
>>  	else
>>  		pitch = width * ((depth + 7) / 8);
>>  
> 
>