Hi,
On 15/03/2015 08:57, Willy Tarreau wrote:
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@xxxxxxxxxxxxxxxx>
---
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
The thing is that I don't have this real hardware, that's why I choose
to start fixing this error that nothing don't direct affect the
hardware. Also to say that it's my first patch sent to the linux kernel
and I don't have many excuses. I'm sorry for having sent many patches
because I didn't pay attention to my code, but I think that now it's
done (unless someone finds a bug). I'm going to resend the patch, and I
hope it's the last time I have to do it. Thanks.
Isaac
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
