lkml.org 
[lkml]   [2015]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] staging: panel: change struct bits to a bit array
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



\
 
 \ /
  Last update: 2015-03-15 09:21    [W:0.041 / U:0.168 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site