Re: [PATCH v3] staging: panel: change struct bits to a bit array

[Willy Tarreau <w@1wt.eu>]

Hello,

On Sat, Mar 14, 2015 at 11:14:43AM +0100, Isaac Lleida wrote:
> This path implements a bit array representing the LCD signal states instead of the old "struct bits", which used char to represent a single bit. This will reduce the memory usage.
> 
> Signed-off-by: Isaac Lleida <illeida@openaliasbox.org>
> ---
> v3: some more stupid errors I introduced in last patch fixed.

Let me guess, you have never tested this patch series, right ?
I have not checked the code with the patch applied, but I'm seeing
this :

>  /*
> + * LCD signal states
> + */
> +#define LCD_BIT_E_MASK		0x1	/* E (data latch on falling edge) */
> +#define LCD_BIT_RS_MASK		0x2	/* RS  (0 = cmd, 1 = data) */
> +#define LCD_BIT_RW_MASK		0x4	/* R/W (0 = W, 1 = R) */
> +#define LCD_BIT_BL_MASK		0x8	/* backlight (0 = off, 1 = on) */
> +#define LCD_BIT_CL_MASK		0x10	/* clock (latch on rising edge) */
> +#define LCD_BIT_DA_MASK		0x20	/* data */
> +
> +/*
> + * Bit array operations
> + */
> +#define BIT_ON(b, m)	(b |= m)
> +#define BIT_OFF(b, m)	(b &= ~m)
> +#define BIT_CHK(b, m)	(b & m)

Then this :

> -	val |= lcd_bits[LCD_PORT_D][LCD_BIT_E][bits.e]
> -	    | lcd_bits[LCD_PORT_D][LCD_BIT_RS][bits.rs]
> -	    | lcd_bits[LCD_PORT_D][LCD_BIT_RW][bits.rw]
> -	    | lcd_bits[LCD_PORT_D][LCD_BIT_BL][bits.bl]
> -	    | lcd_bits[LCD_PORT_D][LCD_BIT_CL][bits.cl]
> -	    | lcd_bits[LCD_PORT_D][LCD_BIT_DA][bits.da];
> +	val |= lcd_bits[LCD_PORT_D][LCD_BIT_E][BIT_CHK(bits, LCD_BIT_E_MASK)]
> +	    | lcd_bits[LCD_PORT_D][LCD_BIT_RS][BIT_CHK(bits, LCD_BIT_RS_MASK)]
> +	    | lcd_bits[LCD_PORT_D][LCD_BIT_RW][BIT_CHK(bits, LCD_BIT_RW_MASK)]
> +	    | lcd_bits[LCD_PORT_D][LCD_BIT_BL][BIT_CHK(bits, LCD_BIT_BL_MASK)]
> +	    | lcd_bits[LCD_PORT_D][LCD_BIT_CL][BIT_CHK(bits, LCD_BIT_CL_MASK)]
> +	    | lcd_bits[LCD_PORT_D][LCD_BIT_DA][BIT_CHK(bits, LCD_BIT_DA_MASK)];
 
So as you can see, previously lcd_bits[x][y][z] was used
with values 0 or 1 for bits, and now it's being used with
values of z matching the bit position in your array. So
it seems to me that it's a significant overflow. Thus I
think you should modify your macro this way :

#define BIT_CHK(b, m)	(!!(b & m))

That said, given the amount of sensitive changes, and the
risk of breakage (as was apparently done here), you *must*
absolutely test you changes on real hardware before risking
to break the driver for every user. Also, the fact that you
needed 3 iterations of this patch after discovering breakage
is clearly a sign of the fact that you need to test it.

Thanks,
Willy