dib0700: Fix uninitialized protocol for NEC repeat codes

PATCH In review Geert Uytterhoeven 3 participants 6 messages Oct 13, 2016 – Nov 7, 2016 Media (V4L/DVB)

Share & tools

Geert Uytterhoeven

author

2016-10-13 13:51 UTC #0 lore raw

PATCH 1/1 dib0700: Fix uninitialized protocol for NEC repeat codes

drivers/media/usb/dvb-usb/dib0700_core.c: In function ‘dib0700_rc_urb_completion’:
drivers/media/usb/dvb-usb/dib0700_core.c:679: warning: ‘protocol’ may be used uninitialized in this function

When receiving an NEC repeat code, protocol is indeed not initialized. Set it to RC_TYPE_NECX to fix this.

Fixes 2ceeca0499d74521 ("[media] rc: split nec protocol into its three variants")

Signed-off-by Geert Uytterhoeven <[email protected]>

---
Is RC_TYPE_NECX correct, or should it be RC_TYPE_NEC? I used RC_TYPE_NECX based on the checks for {,not_}data and {,not_}system for the other cases.

---
 drivers/media/usb/dvb-usb/dib0700_core.c | 1 +
 1 file changed, 1 insertion(+)

drivers/media/usb/dvb-usb/dib0700_core.c +1 -0

index f3196658fb700706..5878ae4d20ad27ed 100644
@@ -718,6 +718,7 @@ static void dib0700_rc_urb_completion(struct urb *purb)
 		    poll_reply->nec.data       == 0x00 &&
 		    poll_reply->nec.not_data   == 0xff) {
 			poll_reply->data_state = 2;
+			protocol = RC_TYPE_NECX;
 			break;
 		}

Signature · 1 line

1.9.1

Sean Young

2016-10-13 21:16 UTC #1 lore raw

Quoted 6 lines · Geert Uytterhoeven

drivers/media/usb/dvb-usb/dib0700_core.c: In function ‘dib0700_rc_urb_completion’:
drivers/media/usb/dvb-usb/dib0700_core.c:679: warning: ‘protocol’ may be used uninitialized in this function
When receiving an NEC repeat code, protocol is indeed not initialized. Set it to RC_TYPE_NECX to fix this.

Quoted 6 lines

Fixes 2ceeca0499d74521 ("[media] rc: split nec protocol into its three variants")
Signed-off-by Geert Uytterhoeven <[email protected]>
---
Is RC_TYPE_NECX correct, or should it be RC_TYPE_NEC? I used RC_TYPE_NECX based on the checks for {,not_}data and {,not_}system for the other cases.

It should be the protocol that the last scancode was received with. This code path is very broken; it calls:

	rc_keydown(d->rc_dev, protocol, keycode, toggle);

But keycode in this codepath is never set. Luckily keycode is declared as:

	u32 uninitialized_var(keycode);

I've got another patch for this which I'll send as a reply to this.

Sean

Quoted 23 lines

---
 drivers/media/usb/dvb-usb/dib0700_core.c | 1 +
 1 file changed, 1 insertion(+)

drivers/media/usb/dvb-usb/dib0700_core.c +1 -0

index f3196658fb700706..5878ae4d20ad27ed 100644
@@ -718,6 +718,7 @@ static void dib0700_rc_urb_completion(struct urb *purb)
 		    poll_reply->nec.data       == 0x00 &&
 		    poll_reply->nec.not_data   == 0xff) {
 			poll_reply->data_state = 2;
+			protocol = RC_TYPE_NECX;
 			break;
 		}

Signature · 6 lines

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sean Young

2016-10-13 21:28 UTC #2 lore raw

PATCH 1/1 dib0700: fix nec repeat handling

When receiving a nec repeat, ensure the correct scancode is repeated rather than a random value from the stack. This removes the need for the bogus uninitialized_var() and also fixes the warnings:

Signed-off-by Sean Young <[email protected]>

---
 drivers/media/usb/dvb-usb/dib0700_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/media/usb/dvb-usb/dib0700_core.c +3 -2

index f319665..5bb23ef 100644
@@ -677,7 +677,7 @@ static void dib0700_rc_urb_completion(struct urb *purb)
 	struct dvb_usb_device *d = purb->context;
 	struct dib0700_rc_response *poll_reply;
 	enum rc_type protocol;
-	u32 uninitialized_var(keycode);
+	u32 keycode;
 	u8 toggle;
 
 	deb_info("%s()\n", __func__);

@@ -718,7 +718,8 @@ static void dib0700_rc_urb_completion(struct urb *purb)
 		    poll_reply->nec.data       == 0x00 &&
 		    poll_reply->nec.not_data   == 0xff) {
 			poll_reply->data_state = 2;
-			break;
+			rc_repeat(d->rc_dev);
+			goto resubmit;
 		}
 
 		if ((poll_reply->nec.data ^ poll_reply->nec.not_data) != 0xff) {

Signature · 1 line

2.7.4

Arnd Bergmann

Acked-by on v1 1/1

2016-10-29 21:04 UTC #3 lore raw

Re: [PATCH] [media] dib0700: fix nec repeat handling

Quoted 11 lines · CEST Sean Young

When receiving a nec repeat, ensure the correct scancode is repeated rather than a random value from the stack. This removes the need for the bogus uninitialized_var() and also fixes the warnings:
drivers/media/usb/dvb-usb/dib0700_core.c: In function ‘dib0700_rc_urb_completion’:
drivers/media/usb/dvb-usb/dib0700_core.c:679: warning: ‘protocol’ may be used uninitialized in this function
Signed-off-by Sean Young <[email protected]>
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Acked-by Arnd Bergmann <[email protected]>

Cc [email protected]

Fixes 2ceeca0499d7 ("[media] rc: split nec protocol into its three variants")

Fixes d3c501d1938c ("V4L/DVB: dib0700: Fix RC protocol logic to properly handle NEC/NECx and RC-5")

The warning is gone for me too, so this obsoletes both
https://patchwork.linuxtv.org/patch/37494/ and
https://patchwork.kernel.org/patch/9380747/

Can we get this patch merged into v4.9 soonish? The warning is currently disabled, but I'd like to make sure it gets turned on again by default, and we should fix all the actual bugs in the process.

Arnd

[I replied to Mauro's other address here as [email protected] bounced with "Failed to transport message. Message sending failed since the following recipients were rejected by the server:
[email protected] (The server responded: Requested action not taken: mailbox unavailable invalid DNS MX or A/AAAA resource record)"]

Sean Young

Tested-by

2016-11-02 16:45 UTC #4 lore raw

Re: [PATCH] [media] dib0700: fix nec repeat handling

Quoted 19 lines · Arnd Bergmann

Quoted 11 lines · CEST Sean Young
When receiving a nec repeat, ensure the correct scancode is repeated rather than a random value from the stack. This removes the need for the bogus uninitialized_var() and also fixes the warnings:
drivers/media/usb/dvb-usb/dib0700_core.c: In function ‘dib0700_rc_urb_completion’:
drivers/media/usb/dvb-usb/dib0700_core.c:679: warning: ‘protocol’ may be used uninitialized in this function
Signed-off-by Sean Young <[email protected]>
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Acked-by Arnd Bergmann <[email protected]>
Cc [email protected]
Fixes 2ceeca0499d7 ("[media] rc: split nec protocol into its three variants")
Fixes d3c501d1938c ("V4L/DVB: dib0700: Fix RC protocol logic to properly handle NEC/NECx and RC-5")

Quoted 8 lines

The warning is gone for me too, so this obsoletes both
https://patchwork.linuxtv.org/patch/37494/ and
https://patchwork.kernel.org/patch/9380747/
Can we get this patch merged into v4.9 soonish? The warning is currently disabled, but I'd like to make sure it gets turned on again by default, and we should fix all the actual bugs in the process.

So after writing the patch and submitting it, I've bought the hardware on ebay. Without this patch you get random scancodes on nec repeats, which the patch indeed fixes.

Tested-by Sean Young <[email protected]>

Note that this has been broken forever, so it is not a regression, so does it belong in stable?

Sean

Arnd Bergmann

2016-11-07 13:05 UTC #5 lore raw

Re: [PATCH] [media] dib0700: fix nec repeat handling

Quoted 29 lines · CET Sean Young

Quoted 19 lines · Arnd Bergmann
Quoted 11 lines · CEST Sean Young
When receiving a nec repeat, ensure the correct scancode is repeated rather than a random value from the stack. This removes the need for the bogus uninitialized_var() and also fixes the warnings:
drivers/media/usb/dvb-usb/dib0700_core.c: In function ‘dib0700_rc_urb_completion’:
drivers/media/usb/dvb-usb/dib0700_core.c:679: warning: ‘protocol’ may be used uninitialized in this function
Signed-off-by Sean Young <[email protected]>
---
 drivers/media/usb/dvb-usb/dib0700_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Acked-by Arnd Bergmann <[email protected]>
Cc [email protected]
Fixes 2ceeca0499d7 ("[media] rc: split nec protocol into its three variants")
Fixes d3c501d1938c ("V4L/DVB: dib0700: Fix RC protocol logic to properly handle NEC/NECx and RC-5")
Quoted 8 lines
The warning is gone for me too, so this obsoletes both
https://patchwork.linuxtv.org/patch/37494/ and
https://patchwork.kernel.org/patch/9380747/
Can we get this patch merged into v4.9 soonish? The warning is currently disabled, but I'd like to make sure it gets turned on again by default, and we should fix all the actual bugs in the process.

Quoted 5 lines

So after writing the patch and submitting it, I've bought the hardware on ebay. Without this patch you get random scancodes on nec repeats, which the patch indeed fixes.
Tested-by Sean Young <[email protected]>

Awesome, thanks for testing!

Quoted 2 lines

Note that this has been broken forever, so it is not a regression, so does it belong in stable?

I think it does, it doesn't have to be a regression to qualify for stable kernels, and it is clearly a bugfix.

Arnd