iMon Knob driver issue

PATCH Alexandre LISSY 4 participants 5 messages Dec 29, 2012 – Jan 1, 2013 1 message not in the archive

Subsystems (3)

Media (V4L/DVB) RC-core / lirc framework Imon soundgraph USB IR receiver

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Alexandre LISSY

author

2012-12-29 12:18 UTC #1 lore raw

Hello,

Please find attached a small patch for the iMon Knob driver. I've been trying to reach the latest maintainer (Jarod Wilson) listed in the file since last september, getting no reply (maybe my mail did not made it through, or went to spam).

Basically, the issue is the same as described on this webpage:
http://www.arm.linux.org.uk/docs/faqs/signedchar.php. I've hit it while running multiple distros on my Raspberry Pi and trying to use my remote control.

I've already added it as a patch in OpenElec sources, successfully fixing the issue in my case.

Quoted 1 line

From cca7718a9902a4d5cffbf158b5853980a08ef930 Mon Sep 17 00:00:00 2001

From: Alexandre Lissy <[email protected]>
Date: Sun, 2 Sep 2012 20:35:20 +0200
Subject: [PATCH] fix: iMon Knob event interpretation issues

Events for the iMon Knob pad where not correctly interpreted, resulting in buggy mouse movements (cursor going straight out of the screen), key pad only generating KEY_RIGHT and KEY_DOWN events. A reproducer is:

int main(int argc, char ** argv)

{
        char rel_x = 0x00; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
        rel_x = 0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
        rel_x |= ~0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);

        return 0;
}

(running on x86 or amd64)

$ ./test
rel_x:0 @test.c:6
rel_x:15 @test.c:7
rel_x:-1 @test.c:8

(running on armv6)
rel_x:0 @test.c:6
rel_x:15 @test.c:7
rel_x:255 @test.c:8

Forcing the rel_x and rel_y variables as signed char fixes the issue.

---
 drivers/media/rc/imon.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/media/rc/imon.c +2 -2

index 5dd0386..9d30ca9 100644
@@ -1225,7 +1225,7 @@ static u32 imon_panel_key_lookup(u64 code)
 static bool imon_mouse_event(struct imon_context *ictx,
 			     unsigned char *buf, int len)
 {
-	char rel_x = 0x00, rel_y = 0x00;
+	signed char rel_x = 0x00, rel_y = 0x00;
 	u8 right_shift = 1;
 	bool mouse_input = true;
 	int dir = 0;

@@ -1301,7 +1301,7 @@ static void imon_touch_event(struct imon_context *ictx, unsigned char *buf)
 static void imon_pad_to_keys(struct imon_context *ictx, unsigned char *buf)
 {
 	int dir = 0;
-	char rel_x = 0x00, rel_y = 0x00;
+	signed char rel_x = 0x00, rel_y = 0x00;
 	u16 timeout, threshold;
 	u32 scancode = KEY_RESERVED;
 	unsigned long flags;

Signature · 1 line

1.7.9.5

Alexey Klimov

2012-12-29 12:31 UTC #2 lore raw

Hello Alexandre,

Quoted 3 lines · Alexandre LISSY

Hello,
Please find attached a small patch for the iMon Knob driver. I've been

Could you please also add your Signed-off-by to the patch? It looks like it's missed.

Signature · 1 line

Best regards, Klimov Alexey

Alexandre Lissy

2012-12-29 13:11 UTC #3 lore raw

Quoted 10 lines · Alexey Klimov

Hello Alexandre,
Quoted 3 lines · Alexandre LISSY
Hello,
Please find attached a small patch for the iMon Knob driver. I've been
Could you please also add your Signed-off-by to the patch? It looks like it's missed.

Sure, sorry for forgetting this, here it is !

Quoted 1 line

From cca7718a9902a4d5cffbf158b5853980a08ef930 Mon Sep 17 00:00:00 2001

From: Alexandre Lissy <[email protected]>
Date: Sun, 2 Sep 2012 20:35:20 +0200
Subject: [PATCH] fix: iMon Knob event interpretation issues

int main(int argc, char ** argv)

{
        char rel_x = 0x00; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
        rel_x = 0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
        rel_x |= ~0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);

        return 0;
}

(running on x86 or amd64)

$ ./test
rel_x:0 @test.c:6
rel_x:15 @test.c:7
rel_x:-1 @test.c:8

(running on armv6)
rel_x:0 @test.c:6
rel_x:15 @test.c:7
rel_x:255 @test.c:8

Forcing the rel_x and rel_y variables as signed char fixes the issue.

Signed-off-by Alexandre Lissy <[email protected]>

---
 drivers/media/rc/imon.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/media/rc/imon.c +2 -2

index 5dd0386..9d30ca9 100644
@@ -1225,7 +1225,7 @@ static u32 imon_panel_key_lookup(u64 code)
 static bool imon_mouse_event(struct imon_context *ictx,
 			     unsigned char *buf, int len)
 {
-	char rel_x = 0x00, rel_y = 0x00;
+	signed char rel_x = 0x00, rel_y = 0x00;
 	u8 right_shift = 1;
 	bool mouse_input = true;
 	int dir = 0;

@@ -1301,7 +1301,7 @@ static void imon_touch_event(struct imon_context *ictx, unsigned char *buf)
 static void imon_pad_to_keys(struct imon_context *ictx, unsigned char *buf)
 {
 	int dir = 0;
-	char rel_x = 0x00, rel_y = 0x00;
+	signed char rel_x = 0x00, rel_y = 0x00;
 	u16 timeout, threshold;
 	u32 scancode = KEY_RESERVED;
 	unsigned long flags;

Signature · 1 line

1.7.10.4

Mauro Carvalho Chehab

2013-01-01 13:13 UTC #4 lore raw

Em Sat, 29 Dec 2012 14:01:57 +0100
Alexandre Lissy <[email protected]> escreveu:

Quoted 41 lines

From cca7718a9902a4d5cffbf158b5853980a08ef930 Mon Sep 17 00:00:00 2001
From: Alexandre Lissy <[email protected]>
Date: Sun, 2 Sep 2012 20:35:20 +0200
Subject: [PATCH] fix: iMon Knob event interpretation issues
Events for the iMon Knob pad where not correctly interpreted, resulting in buggy mouse movements (cursor going straight out of the screen), key pad only generating KEY_RIGHT and KEY_DOWN events. A reproducer is:
int main(int argc, char ** argv)
{
        char rel_x = 0x00; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
        rel_x = 0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
        rel_x |= ~0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);

        return 0;
}
(running on x86 or amd64)
$ ./test
rel_x:0 @test.c:6
rel_x:15 @test.c:7
rel_x:-1 @test.c:8
(running on armv6)
rel_x:0 @test.c:6
rel_x:15 @test.c:7
rel_x:255 @test.c:8
Forcing the rel_x and rel_y variables as signed char fixes the issue.
Signed-off-by Alexandre Lissy <[email protected]>
---
 drivers/media/rc/imon.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/rc/imon.c +0 -0
index 5dd0386..9d30ca9 100644

drivers/media/rc/imon.c static u32 imon_panel_key_lookup(u64 code) · line 1228

Quoted 5 lines

drivers/media/rc/imon.c +1 -1

@@ -1225,7 +1225,7 @@ static u32 imon_panel_key_lookup(u64 code)
 static bool imon_mouse_event(struct imon_context *ictx,
 			     unsigned char *buf, int len)
 {
-	char rel_x = 0x00, rel_y = 0x00;
+	signed char rel_x = 0x00, rel_y = 0x00;

(c/c Jarod, as he is the maintainer of this driver)

That looks weird, as, AFAIKT "char" is signed. Are you sure that this fix is correct?

If so, maybe this could be a gcc-version-specific bug. What gcc version are you using?

Btw, we generally use "s8" type for signed 8bit integers inside the Kernel.

Quoted 12 lines

drivers/media/rc/imon.c +1 -1

 	u8 right_shift = 1;
 	bool mouse_input = true;
 	int dir = 0;

@@ -1301,7 +1301,7 @@ static void imon_touch_event(struct imon_context *ictx, unsigned char *buf)
 static void imon_pad_to_keys(struct imon_context *ictx, unsigned char *buf)
 {
 	int dir = 0;
-	char rel_x = 0x00, rel_y = 0x00;
+	signed char rel_x = 0x00, rel_y = 0x00;
 	u16 timeout, threshold;
 	u32 scancode = KEY_RESERVED;
 	unsigned long flags;

Signature · 2 lines

Cheers,
Mauro

Alexandre LISSY

author

2013-01-01 14:11 UTC #5 lore raw

Hello,

I had the same opinion than you at first, yet I found this webpage while searching for a fix :
http://www.arm.linux.org.uk/docs/faqs/signedchar.php. One line
especially: "The above code is actually buggy in that it assumes that the type "char" is equivalent to "signed char". The C standards do say that "char" may either be a "signed char" or "unsigned char" and it is up to the compilers implementation or the platform which is followed."

This made me think that the fix was the correct one. Maybe the "signed char" should be changed to s8 as you suggested, however.

Quoted 73 lines · Mauro Carvalho Chehab

Em Sat, 29 Dec 2012 14:01:57 +0100
Alexandre Lissy <[email protected]> escreveu:
Quoted 41 lines
From cca7718a9902a4d5cffbf158b5853980a08ef930 Mon Sep 17 00:00:00 2001
From: Alexandre Lissy <[email protected]>
Date: Sun, 2 Sep 2012 20:35:20 +0200
Subject: [PATCH] fix: iMon Knob event interpretation issues
Events for the iMon Knob pad where not correctly interpreted, resulting in buggy mouse movements (cursor going straight out of the screen), key pad only generating KEY_RIGHT and KEY_DOWN events. A reproducer is:
int main(int argc, char ** argv)
{
        char rel_x = 0x00; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
        rel_x = 0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);
        rel_x |= ~0x0f; printf("rel_x:%d @%s:%d\n", rel_x, __FILE__, __LINE__);

        return 0;
}
(running on x86 or amd64)
$ ./test
rel_x:0 @test.c:6
rel_x:15 @test.c:7
rel_x:-1 @test.c:8
(running on armv6)
rel_x:0 @test.c:6
rel_x:15 @test.c:7
rel_x:255 @test.c:8
Forcing the rel_x and rel_y variables as signed char fixes the issue.
Signed-off-by Alexandre Lissy <[email protected]>
---
 drivers/media/rc/imon.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/rc/imon.c +0 -0
index 5dd0386..9d30ca9 100644
drivers/media/rc/imon.c static u32 imon_panel_key_lookup(u64 code) · line 1228
Quoted 5 lines
drivers/media/rc/imon.c +1 -1
@@ -1225,7 +1225,7 @@ static u32 imon_panel_key_lookup(u64 code)
 static bool imon_mouse_event(struct imon_context *ictx,
 			     unsigned char *buf, int len)
 {
-	char rel_x = 0x00, rel_y = 0x00;
+	signed char rel_x = 0x00, rel_y = 0x00;
(c/c Jarod, as he is the maintainer of this driver)
That looks weird, as, AFAIKT "char" is signed. Are you sure that this fix is correct?
If so, maybe this could be a gcc-version-specific bug. What gcc version are you using?
Btw, we generally use "s8" type for signed 8bit integers inside the Kernel.
Quoted 12 lines
drivers/media/rc/imon.c +1 -1
 	u8 right_shift = 1;
 	bool mouse_input = true;
 	int dir = 0;
@@ -1301,7 +1301,7 @@ static void imon_touch_event(struct imon_context *ictx, unsigned char *buf)
 static void imon_pad_to_keys(struct imon_context *ictx, unsigned char *buf)
 {
 	int dir = 0;
-	char rel_x = 0x00, rel_y = 0x00;
+	signed char rel_x = 0x00, rel_y = 0x00;
 	u16 timeout, threshold;
 	u32 scancode = KEY_RESERVED;
 	unsigned long flags;