[PATCH] imon: Input from ffdc device type ignored

[PATCH] imon: Input from ffdc device type ignored

[Corinna Vinschen]

Hi,

I have an iMON device (device ID 15c2:ffdc) in my multimedia pc, which
worked without too many problems with pre-3.x kernels and the lirc_imon
module.  With the new imon module since kernel 3.0 the remote worked as
expected, just everytime the module got reloaded or the machine was
rebooted, the machine got a kernel oops.

With kernel version 3.2, the oops is fixed, but now the input from the
remote is not recognized at all.  There are no input entries in the log.

I have a patch to drivers/media/rc/imon.c for this issue which "works
for me"(tm), but I'm not sure it's the right thing to do.  With this
patch keypresses from the remote are recognized and the kernel oops
doesn't occur either.  It also fixes a minor typo (intf0 instead of
intf1) in imon_init_intf1.

See patch below.  Is that ok to go into mainline?

Please keep me CCed, I'm not subscribed to the list.


Thanks,
Corinna


Signed-off-by: Corinna Vinschen <vinschen@redhat.com>

--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -1658,7 +1658,7 @@ static void usb_rx_callback_intf0(struct
 		return;
 
 	ictx = (struct imon_context *)urb->context;
-	if (!ictx || !ictx->dev_present_intf0)
+	if (!ictx)
 		return;
 
 	switch (urb->status) {
@@ -1669,7 +1669,8 @@ static void usb_rx_callback_intf0(struct
 		break;
 
 	case 0:
-		imon_incoming_packet(ictx, urb, intfnum);
+		if (ictx->dev_present_intf0)
+			imon_incoming_packet(ictx, urb, intfnum);
 		break;
 
 	default:
@@ -2242,7 +2243,7 @@ find_endpoint_failed:
 	mutex_unlock(&ictx->lock);
 	usb_free_urb(rx_urb);
 rx_urb_alloc_failed:
-	dev_err(ictx->dev, "unable to initialize intf0, err %d\n", ret);
+	dev_err(ictx->dev, "unable to initialize intf1, err %d\n", ret);
 
 	return NULL;
 }

[PATCH] imon: don't wedge hardware after early callbacks

[Jarod Wilson]

This patch is just a minor update to one titled "imon: Input from ffdc
device type ignored" from Corinna Vinschen. An earlier patch to prevent
an oops when we got early callbacks also has the nasty side-effect of
wedging imon hardware, as we don't acknowledge the urb. Rework the check
slightly here to bypass processing the packet, as the driver isn't yet
fully initialized, but still acknowlege the urb and submit a new rx_urb.
Do this for both interfaces -- irrelevant for ffdc hardware, but
relevant for newer hardware, though newer hardware doesn't spew the
constant stream of data as soon as the hardware is initialized like the
older ffdc devices, so they'd be less likely to trigger this anyway...

Tested with both an ffdc device and an 0042 device.

CC: Corinna Vinschen <vinschen@redhat.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/rc/imon.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 6ed9646..0292741 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -1658,9 +1658,17 @@ static void usb_rx_callback_intf0(struct urb *urb)
 		return;
 
 	ictx = (struct imon_context *)urb->context;
-	if (!ictx || !ictx->dev_present_intf0)
+	if (!ictx)
 		return;
 
+	/*
+	 * if we get a callback before we're done configuring the hardware, we
+	 * can't yet process the data, as there's nowhere to send it, but we
+	 * still need to acknowledge the URB to avoid wedging the hardware
+	 */
+	if (!ictx->dev_present_intf0)
+		goto out;
+
 	switch (urb->status) {
 	case -ENOENT:		/* usbcore unlink successful! */
 		return;
@@ -1678,6 +1686,7 @@ static void usb_rx_callback_intf0(struct urb *urb)
 		break;
 	}
 
+out:
 	usb_submit_urb(ictx->rx_urb_intf0, GFP_ATOMIC);
 }
 
@@ -1690,9 +1699,17 @@ static void usb_rx_callback_intf1(struct urb *urb)
 		return;
 
 	ictx = (struct imon_context *)urb->context;
-	if (!ictx || !ictx->dev_present_intf1)
+	if (!ictx)
 		return;
 
+	/*
+	 * if we get a callback before we're done configuring the hardware, we
+	 * can't yet process the data, as there's nowhere to send it, but we
+	 * still need to acknowledge the URB to avoid wedging the hardware
+	 */
+	if (!ictx->dev_present_intf1)
+		goto out;
+
 	switch (urb->status) {
 	case -ENOENT:		/* usbcore unlink successful! */
 		return;
@@ -1710,6 +1727,7 @@ static void usb_rx_callback_intf1(struct urb *urb)
 		break;
 	}
 
+out:
 	usb_submit_urb(ictx->rx_urb_intf1, GFP_ATOMIC);
 }
 
@@ -2242,7 +2260,7 @@ find_endpoint_failed:
 	mutex_unlock(&ictx->lock);
 	usb_free_urb(rx_urb);
 rx_urb_alloc_failed:
-	dev_err(ictx->dev, "unable to initialize intf0, err %d\n", ret);
+	dev_err(ictx->dev, "unable to initialize intf1, err %d\n", ret);
 
 	return NULL;
 }
-- 
1.7.7.5

Re: [PATCH] imon: don't wedge hardware after early callbacks

[Corinna Vinschen]

Hi Jarod,

On Jan 25 15:56, Jarod Wilson wrote:
> This patch is just a minor update to one titled "imon: Input from ffdc
> device type ignored" from Corinna Vinschen. An earlier patch to prevent
> an oops when we got early callbacks also has the nasty side-effect of
> wedging imon hardware, as we don't acknowledge the urb. Rework the check
> slightly here to bypass processing the packet, as the driver isn't yet
> fully initialized, but still acknowlege the urb and submit a new rx_urb.
> Do this for both interfaces -- irrelevant for ffdc hardware, but
> relevant for newer hardware, though newer hardware doesn't spew the
> constant stream of data as soon as the hardware is initialized like the
> older ffdc devices, so they'd be less likely to trigger this anyway...

just a question, wouldn't it make sense to bump the version number of the
module to 0.9.4?  Or do you do that for functional changes only?


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] imon: don't wedge hardware after early callbacks

[Jarod Wilson]

Corinna Vinschen wrote:
> Hi Jarod,
>
> On Jan 25 15:56, Jarod Wilson wrote:
>> This patch is just a minor update to one titled "imon: Input from ffdc
>> device type ignored" from Corinna Vinschen. An earlier patch to prevent
>> an oops when we got early callbacks also has the nasty side-effect of
>> wedging imon hardware, as we don't acknowledge the urb. Rework the check
>> slightly here to bypass processing the packet, as the driver isn't yet
>> fully initialized, but still acknowlege the urb and submit a new rx_urb.
>> Do this for both interfaces -- irrelevant for ffdc hardware, but
>> relevant for newer hardware, though newer hardware doesn't spew the
>> constant stream of data as soon as the hardware is initialized like the
>> older ffdc devices, so they'd be less likely to trigger this anyway...
>
> just a question, wouldn't it make sense to bump the version number of the
> module to 0.9.4?  Or do you do that for functional changes only?

I've not been terribly consistent with it, but it does seem the last 
time I bumped the version number *was* to have an easy way to tell if a 
particular fix was included or not. We can bump it here too, doesn't 
really matter to me.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] imon: don't wedge hardware after early callbacks

[Corinna Vinschen]

On Jan 25 17:11, Jarod Wilson wrote:
> Corinna Vinschen wrote:
> >Hi Jarod,
> >
> >On Jan 25 15:56, Jarod Wilson wrote:
> >>This patch is just a minor update to one titled "imon: Input from ffdc
> >>device type ignored" from Corinna Vinschen. An earlier patch to prevent
> >>an oops when we got early callbacks also has the nasty side-effect of
> >>wedging imon hardware, as we don't acknowledge the urb. Rework the check
> >>slightly here to bypass processing the packet, as the driver isn't yet
> >>fully initialized, but still acknowlege the urb and submit a new rx_urb.
> >>Do this for both interfaces -- irrelevant for ffdc hardware, but
> >>relevant for newer hardware, though newer hardware doesn't spew the
> >>constant stream of data as soon as the hardware is initialized like the
> >>older ffdc devices, so they'd be less likely to trigger this anyway...
> >
> >just a question, wouldn't it make sense to bump the version number of the
> >module to 0.9.4?  Or do you do that for functional changes only?
> 
> I've not been terribly consistent with it, but it does seem the last
> time I bumped the version number *was* to have an easy way to tell
> if a particular fix was included or not. We can bump it here too,
> doesn't really matter to me.

Could be helpful here, too, I guess.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[PATCH v2] imon: don't wedge hardware after early callbacks

[Jarod Wilson]

This patch is just a minor update to one titled "imon: Input from ffdc
device type ignored" from Corinna Vinschen. An earlier patch to prevent
an oops when we got early callbacks also has the nasty side-effect of
wedging imon hardware, as we don't acknowledge the urb. Rework the check
slightly here to bypass processing the packet, as the driver isn't yet
fully initialized, but still acknowlege the urb and submit a new rx_urb.
Do this for both interfaces -- irrelevant for ffdc hardware, but
relevant for newer hardware, though newer hardware doesn't spew the
constant stream of data as soon as the hardware is initialized like the
older ffdc devices, so they'd be less likely to trigger this anyway...

Tested with both an ffdc device and an 0042 device.

v2: Per Corinna's suggestion and prior precedent, increment driver
version number so we can more easily tell if a user has this fix.

CC: Corinna Vinschen <vinschen@redhat.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/rc/imon.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 6ed9646..046f529 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -47,7 +47,7 @@
 #define MOD_AUTHOR	"Jarod Wilson <jarod@wilsonet.com>"
 #define MOD_DESC	"Driver for SoundGraph iMON MultiMedia IR/Display"
 #define MOD_NAME	"imon"
-#define MOD_VERSION	"0.9.3"
+#define MOD_VERSION	"0.9.4"
 
 #define DISPLAY_MINOR_BASE	144
 #define DEVICE_NAME	"lcd%d"
@@ -1658,9 +1658,17 @@ static void usb_rx_callback_intf0(struct urb *urb)
 		return;
 
 	ictx = (struct imon_context *)urb->context;
-	if (!ictx || !ictx->dev_present_intf0)
+	if (!ictx)
 		return;
 
+	/*
+	 * if we get a callback before we're done configuring the hardware, we
+	 * can't yet process the data, as there's nowhere to send it, but we
+	 * still need to acknowledge the URB to avoid wedging the hardware
+	 */
+	if (!ictx->dev_present_intf0)
+		goto out;
+
 	switch (urb->status) {
 	case -ENOENT:		/* usbcore unlink successful! */
 		return;
@@ -1678,6 +1686,7 @@ static void usb_rx_callback_intf0(struct urb *urb)
 		break;
 	}
 
+out:
 	usb_submit_urb(ictx->rx_urb_intf0, GFP_ATOMIC);
 }
 
@@ -1690,9 +1699,17 @@ static void usb_rx_callback_intf1(struct urb *urb)
 		return;
 
 	ictx = (struct imon_context *)urb->context;
-	if (!ictx || !ictx->dev_present_intf1)
+	if (!ictx)
 		return;
 
+	/*
+	 * if we get a callback before we're done configuring the hardware, we
+	 * can't yet process the data, as there's nowhere to send it, but we
+	 * still need to acknowledge the URB to avoid wedging the hardware
+	 */
+	if (!ictx->dev_present_intf1)
+		goto out;
+
 	switch (urb->status) {
 	case -ENOENT:		/* usbcore unlink successful! */
 		return;
@@ -1710,6 +1727,7 @@ static void usb_rx_callback_intf1(struct urb *urb)
 		break;
 	}
 
+out:
 	usb_submit_urb(ictx->rx_urb_intf1, GFP_ATOMIC);
 }
 
@@ -2242,7 +2260,7 @@ find_endpoint_failed:
 	mutex_unlock(&ictx->lock);
 	usb_free_urb(rx_urb);
 rx_urb_alloc_failed:
-	dev_err(ictx->dev, "unable to initialize intf0, err %d\n", ret);
+	dev_err(ictx->dev, "unable to initialize intf1, err %d\n", ret);
 
 	return NULL;
 }
-- 
1.7.1

[PATCH] imon: don't wedge hardware after early callbacks

[Jarod Wilson]

This patch is just a minor update to one titled "imon: Input from ffdc
device type ignored" from Corinna Vinschen. An earlier patch to prevent
an oops when we got early callbacks also has the nasty side-effect of
wedging imon hardware, as we don't acknowledge the urb. Rework the check
slightly here to bypass processing the packet, as the driver isn't yet
fully initialized, but still acknowlege the urb and submit a new rx_urb.
Do this for both interfaces -- irrelevant for ffdc hardware, but
relevant for newer hardware, though newer hardware doesn't spew the
constant stream of data as soon as the hardware is initialized like the
older ffdc devices, so they'd be less likely to trigger this anyway...

Tested with both an ffdc device and an 0042 device.

v2: Per Corinna's suggestion and prior precedent, increment driver
version number so we can more easily tell if a user has this fix.

v3: cc stable, as this fixes a functional regression in 3.2

CC: stable@vger.kernel.org
CC: Corinna Vinschen <vinschen@redhat.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/rc/imon.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 6ed9646..3f175eb 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -47,7 +47,7 @@
 #define MOD_AUTHOR	"Jarod Wilson <jarod@wilsonet.com>"
 #define MOD_DESC	"Driver for SoundGraph iMON MultiMedia IR/Display"
 #define MOD_NAME	"imon"
-#define MOD_VERSION	"0.9.3"
+#define MOD_VERSION	"0.9.4"
 
 #define DISPLAY_MINOR_BASE	144
 #define DEVICE_NAME	"lcd%d"
@@ -1658,9 +1658,17 @@ static void usb_rx_callback_intf0(struct urb *urb)
 		return;
 
 	ictx = (struct imon_context *)urb->context;
-	if (!ictx || !ictx->dev_present_intf0)
+	if (!ictx)
 		return;
 
+	/*
+	 * if we get a callback before we're done configuring the hardware, we
+	 * can't yet process the data, as there's nowhere to send it, but we
+	 * still need to submit a new rx URB to avoid wedging the hardware
+	 */
+	if (!ictx->dev_present_intf0)
+		goto out;
+
 	switch (urb->status) {
 	case -ENOENT:		/* usbcore unlink successful! */
 		return;
@@ -1678,6 +1686,7 @@ static void usb_rx_callback_intf0(struct urb *urb)
 		break;
 	}
 
+out:
 	usb_submit_urb(ictx->rx_urb_intf0, GFP_ATOMIC);
 }
 
@@ -1690,9 +1699,17 @@ static void usb_rx_callback_intf1(struct urb *urb)
 		return;
 
 	ictx = (struct imon_context *)urb->context;
-	if (!ictx || !ictx->dev_present_intf1)
+	if (!ictx)
 		return;
 
+	/*
+	 * if we get a callback before we're done configuring the hardware, we
+	 * can't yet process the data, as there's nowhere to send it, but we
+	 * still need to submit a new rx URB to avoid wedging the hardware
+	 */
+	if (!ictx->dev_present_intf1)
+		goto out;
+
 	switch (urb->status) {
 	case -ENOENT:		/* usbcore unlink successful! */
 		return;
@@ -1710,6 +1727,7 @@ static void usb_rx_callback_intf1(struct urb *urb)
 		break;
 	}
 
+out:
 	usb_submit_urb(ictx->rx_urb_intf1, GFP_ATOMIC);
 }
 
@@ -2242,7 +2260,7 @@ find_endpoint_failed:
 	mutex_unlock(&ictx->lock);
 	usb_free_urb(rx_urb);
 rx_urb_alloc_failed:
-	dev_err(ictx->dev, "unable to initialize intf0, err %d\n", ret);
+	dev_err(ictx->dev, "unable to initialize intf1, err %d\n", ret);
 
 	return NULL;
 }
-- 
1.7.1