(still) NULL pointer crashes with nuvoton_cir driver

(still) NULL pointer crashes with nuvoton_cir driver

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 2176 bytes --]

(Please keep me CC'd, I'm not on the list)

Hi folks,

I've been suffering from a NULL pointer crash while initializing the
nuvoton_cir driver. It occurs 100% reliable on a 3.5 kernel when I
bombard my receiver with IR signals during bootup.

It seems that 9ef449c: [media] rc: Postpone ISR registration attempts to
fix this very issue, but does not fix it completely.

The error I'm seeing is a NULL pointer dereference at
ir_raw_event_store_with_filter+0xd. I couldn't get gdb to disassemble
this function for some reason, but given the low offset, it's probably
very early in the fuction (code taken from linux-media master):

    int ir_raw_event_store_with_filter(struct rc_dev *dev, struct ir_raw_event *ev)
    {
	    if (!dev->raw)
		return -EINVAL;

	    /* Ignore spaces in idle mode */
	    if (dev->idle && !ev->pulse)
		    return 0;

I suspect that the NULL pointer is because dev is NULL.

The only place where ir_raw_event_store_with_filter is called by
nuvoton_cir is in nvt_process_rx_ir_data:

    ir_raw_event_store_with_filter(nvt->rdev, &rawir);

This would mean nvt->rdev is NULL.

I couldn't get at a backtrace, but the backtrace I got from running 3.2
was:
    ir_raw_event_store_with_filter
    nvt_process_rx_ir_data
    nvt_get_rx_ir_data
    nvt_cir_isr

In other words, the ISR is fired before nvt->rdev is initialized.

Looking at nvt_probe, I see that nvt->rdev is indeed assigned _after_
the request_irq calls are made, which would explain the crash I'm
seeing.

I was going to prepare a patch to move rdev initialization further up
(for all ir drivers involved in the previous "Postpone ISR registration
patch", except ene_cir.c and windbond_cir.c which seem to have that
already), but I'm not sure if this is sufficient.

In particular, I'm not sure if it's ok to start firing IRQ's before
rc_register_device is called. rc_register_device also calls
ir_raw_event_register to init rdev->raw, which is again used by
ir_raw_event_store_with_filter.

I'm currently compiling a 3.5 kernel with just the rdev initialization
moved up to see if this will fix my problem at all, but I'd like your
view on this in the meantime as well.

Gr.

Matthijs

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: (still) NULL pointer crashes with nuvoton_cir driver

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

Hi folks,

> I'm currently compiling a 3.5 kernel with just the rdev initialization
> moved up to see if this will fix my problem at all, but I'd like your
> view on this in the meantime as well.
Ok, this seems to fix my problem:

--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -1066,6 +1066,7 @@
        /* tx bits */
        rdev->tx_resolution = XYZ;
 #endif
+       nvt->rdev = rdev;
 
        ret = -EBUSY;
        /* now claim resources */
@@ -1090,7 +1091,6 @@
                goto failure5;
 
        device_init_wakeup(&pdev->dev, true);
-       nvt->rdev = rdev;
        nvt_pr(KERN_NOTICE, "driver has been successfully loaded\n");
        if (debug) {
                cir_dump_regs(nvt);


I'm still not sure if the rc_register_device shouldn't also be moved up. It
seems this doesn't trigger a problem right now, but if there is a problem, I
suspect its trigger window is a lot smaller than with the rdev initialization
problem...

Gr.

Matthijs

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: (still) NULL pointer crashes with nuvoton_cir driver

[Luis Henriques]

(Adding Mauro to CC has he is the maintainer)

On Thu, Aug 16, 2012 at 10:09:32AM +0200, Matthijs Kooijman wrote:
> Hi folks,
> 
> > I'm currently compiling a 3.5 kernel with just the rdev initialization
> > moved up to see if this will fix my problem at all, but I'd like your
> > view on this in the meantime as well.
> Ok, this seems to fix my problem:
> 
> --- a/drivers/media/rc/nuvoton-cir.c
> +++ b/drivers/media/rc/nuvoton-cir.c
> @@ -1066,6 +1066,7 @@
>         /* tx bits */
>         rdev->tx_resolution = XYZ;
>  #endif
> +       nvt->rdev = rdev;

This makes sense to me.  Note however that there are more drivers with
a similar problem (e.g., fintek-cir.c).

>  
>         ret = -EBUSY; /* now claim resources */ @@ -1090,7 +1091,6
> @@ goto failure5;
>  
>         device_init_wakeup(&pdev->dev, true);
> -       nvt->rdev = rdev;
>         nvt_pr(KERN_NOTICE, "driver has been successfully loaded\n");
>         if (debug) {
>                 cir_dump_regs(nvt);
> 
> 
> I'm still not sure if the rc_register_device shouldn't also be moved up. It
> seems this doesn't trigger a problem right now, but if there is a problem, I
> suspect its trigger window is a lot smaller than with the rdev initialization
> problem...

I'm not sure as well, I'm not very familiar with this code.  However,
it looks like the IRQ request should actually be one of the last
things to do here.

Cheers,
--
Luis

Re: (still) NULL pointer crashes with nuvoton_cir driver

[Mauro Carvalho Chehab]

Em 17-08-2012 12:04, Luis Henriques escreveu:
> (Adding Mauro to CC has he is the maintainer)

I'm actually waiting for Jarod's comments on it, as he wrote this driver
and the similar ones.

Jarod?

Regards,
Mauro

> 
> On Thu, Aug 16, 2012 at 10:09:32AM +0200, Matthijs Kooijman wrote:
>> Hi folks,
>>
>>> I'm currently compiling a 3.5 kernel with just the rdev initialization
>>> moved up to see if this will fix my problem at all, but I'd like your
>>> view on this in the meantime as well.
>> Ok, this seems to fix my problem:
>>
>> --- a/drivers/media/rc/nuvoton-cir.c
>> +++ b/drivers/media/rc/nuvoton-cir.c
>> @@ -1066,6 +1066,7 @@
>>         /* tx bits */
>>         rdev->tx_resolution = XYZ;
>>  #endif
>> +       nvt->rdev = rdev;
> 
> This makes sense to me.  Note however that there are more drivers with
> a similar problem (e.g., fintek-cir.c).
> 
>>  
>>         ret = -EBUSY; /* now claim resources */ @@ -1090,7 +1091,6
>> @@ goto failure5;
>>  
>>         device_init_wakeup(&pdev->dev, true);
>> -       nvt->rdev = rdev;
>>         nvt_pr(KERN_NOTICE, "driver has been successfully loaded\n");
>>         if (debug) {
>>                 cir_dump_regs(nvt);
>>
>>
>> I'm still not sure if the rc_register_device shouldn't also be moved up. It
>> seems this doesn't trigger a problem right now, but if there is a problem, I
>> suspect its trigger window is a lot smaller than with the rdev initialization
>> problem...
> 
> I'm not sure as well, I'm not very familiar with this code.  However,
> it looks like the IRQ request should actually be one of the last
> things to do here.
> 
> Cheers,
> --
> Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: (still) NULL pointer crashes with nuvoton_cir driver

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 2665 bytes --]

Hey Folks,

> I've been suffering from a NULL pointer crash while initializing the
> nuvoton_cir driver. It occurs 100% reliable on a 3.5 kernel when I
> bombard my receiver with IR signals during bootup.
>
> It seems that 9ef449c: [media] rc: Postpone ISR registration attempts to
> fix this very issue, but does not fix it completely.

I've looked a bit more closely at the code and I do not really
understand why this commit actually fixed the crash for some (ite-cir)
users. Looking at the bugreport linked to in that commit [1], users
report their bug is fixed, but I don't really understand why. Perhaps
the window for this race condition is just made smaller, but not
entirely eliminated?

Note that the actual patch tested by the Launchpad users (which is
included in the Ubuntu kernel) is a smaller patch than the one in git,
but the essence is still the same.

[1]: http://bugs.launchpad.net/bugs/972723

> In particular, I'm not sure if it's ok to start firing IRQ's before
> rc_register_device is called. rc_register_device also calls
> ir_raw_event_register to init rdev->raw, which is again used by
> ir_raw_event_store_with_filter.
It seems this particular thing did not cause a (reproducible) problem,
since ir_raw_event_store_with_filter bails out when rdev->raw is NULL
(returning -EINVAL, but that's ignored by nvt_process_rx_ir_data).

Still, since there is still a small time window in which rdev->raw is
not NULL, but not completely initialized either, I believe that the irqs
should not be registered until after rc_register_device was called.


I've also looked at the USB IR drivers and it seems some potentially
have the same problem. In particular, usb_fill_int_urb (for iguanair and
ati-remote) or redrat3_enable_detector (for redrat3) is called before
rc_register_device and other initialization, but I don't know the USB
subsystem well enough to know if this is really a problem or if simply
moving th usb_fill_int_urb / redrat3_enable_detector would help at all.

The mceusb, streamzap and gpio-ir-recv drivers look like they initialize the interrupt
usb at the end and shouldn't suffer from this problem.


I'll send a patch series for this bug in a minute. It also fixes some
problem in cleanup of ene-ir I noticed while reading the code and
renames some of the cleanup labels (which should make the last patch
easier to review). I've tried to separate changes as much as possible,
if there's anything you'd like to see different, just let me know.

Since I only have nuvoton-cir hardware, the changes to the other drivers
are compile-tested only (but the drivers are similar enough that I
expect no problems there).

Gr.

Matthijs

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 1/4] [media] ene-ir: Fix cleanup on probe failure

[Matthijs Kooijman]

This makes the cleanup on probe failure more consistent with other
drivers. This is similar to what commit
f27b853ea24a9b70585f9251384d97929e6551c3 ("[media] rc: Fix invalid
free_region and/or free_irq on probe failure") did for some other
drivers.

In addition to making the cleanup more consistent, this also fixes a
case where (on a ene_hw_detect failure) free_region would be called on a
region that was not requested yet.

This last problem was probably introduced by the moving of code in
commit b31b021988fed9e3741a46918f14ba9b063811db ("[media] ene_ir: Fix
driver initialisation") and commit
9ef449c6b31bb6a8e6dedc24de475a3b8c79be20 ("[media] rc: Postpone ISR
registration").

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 drivers/media/rc/ene_ir.c |   29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c
index 647dd95..62f9076 100644
--- a/drivers/media/rc/ene_ir.c
+++ b/drivers/media/rc/ene_ir.c
@@ -1000,7 +1000,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
 	rdev = rc_allocate_device();
 	if (!dev || !rdev)
-		goto error1;
+		goto failure;
 
 	/* validate resources */
 	error = -ENODEV;
@@ -1011,10 +1011,10 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 
 	if (!pnp_port_valid(pnp_dev, 0) ||
 	    pnp_port_len(pnp_dev, 0) < ENE_IO_SIZE)
-		goto error;
+		goto failure;
 
 	if (!pnp_irq_valid(pnp_dev, 0))
-		goto error;
+		goto failure;
 
 	spin_lock_init(&dev->hw_lock);
 
@@ -1030,7 +1030,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* detect hardware version and features */
 	error = ene_hw_detect(dev);
 	if (error)
-		goto error;
+		goto failure;
 
 	if (!dev->hw_learning_and_tx_capable && txsim) {
 		dev->hw_learning_and_tx_capable = true;
@@ -1075,30 +1075,27 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* claim the resources */
 	error = -EBUSY;
 	if (!request_region(dev->hw_io, ENE_IO_SIZE, ENE_DRIVER_NAME)) {
-		dev->hw_io = -1;
-		dev->irq = -1;
-		goto error;
+		goto failure;
 	}
 
 	dev->irq = pnp_irq(pnp_dev, 0);
 	if (request_irq(dev->irq, ene_isr,
 			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev)) {
-		dev->irq = -1;
-		goto error;
+		goto failure2;
 	}
 
 	error = rc_register_device(rdev);
 	if (error < 0)
-		goto error;
+		goto failure3;
 
 	pr_notice("driver has been successfully loaded\n");
 	return 0;
-error:
-	if (dev && dev->irq >= 0)
-		free_irq(dev->irq, dev);
-	if (dev && dev->hw_io >= 0)
-		release_region(dev->hw_io, ENE_IO_SIZE);
-error1:
+
+failure3:
+	free_irq(dev->irq, dev);
+failure2:
+	release_region(dev->hw_io, ENE_IO_SIZE);
+failure:
 	rc_free_device(rdev);
 	kfree(dev);
 	return error;
-- 
1.7.10

[PATCH 2/4] [media] rc: Make probe cleanup goto labels more verbose

[Matthijs Kooijman]

Before, labels were simply numbered. Now, the labels are named after the
cleanup action they'll perform (first), based on how the winbond-cir
driver does it. This makes the code a bit more clear and makes changes
in the ordering of labels easier to review.

This change is applied only to the rc drivers that do significant
cleanup in their probe functions: ati-remote, ene-ir, fintek-cir,
gpio-ir-recv, ite-cir, nuvoton-cir.

This commit should not change any code, it just renames goto labels.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 drivers/media/rc/ati_remote.c   |   27 ++++++++++++++++-----------
 drivers/media/rc/ene_ir.c       |   20 ++++++++++----------
 drivers/media/rc/fintek-cir.c   |   20 ++++++++++----------
 drivers/media/rc/gpio-ir-recv.c |   19 +++++++++----------
 drivers/media/rc/ite-cir.c      |   18 +++++++++---------
 drivers/media/rc/nuvoton-cir.c  |   30 +++++++++++++++---------------
 6 files changed, 69 insertions(+), 65 deletions(-)

diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index 8fa72e2..58fce6a 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -877,11 +877,11 @@ static int ati_remote_probe(struct usb_interface *interface,
 	ati_remote = kzalloc(sizeof (struct ati_remote), GFP_KERNEL);
 	rc_dev = rc_allocate_device();
 	if (!ati_remote || !rc_dev)
-		goto fail1;
+		goto exit_free_dev_rdev;
 
 	/* Allocate URB buffers, URBs */
 	if (ati_remote_alloc_buffers(udev, ati_remote))
-		goto fail2;
+		goto exit_free_buffers;
 
 	ati_remote->endpoint_in = endpoint_in;
 	ati_remote->endpoint_out = endpoint_out;
@@ -929,12 +929,12 @@ static int ati_remote_probe(struct usb_interface *interface,
 	/* Device Hardware Initialization - fills in ati_remote->idev from udev. */
 	err = ati_remote_initialize(ati_remote);
 	if (err)
-		goto fail3;
+		goto exit_kill_urbs;
 
 	/* Set up and register rc device */
 	err = rc_register_device(ati_remote->rdev);
 	if (err)
-		goto fail3;
+		goto exit_kill_urbs;
 
 	/* use our delay for rc_dev */
 	ati_remote->rdev->input_dev->rep[REP_DELAY] = repeat_delay;
@@ -943,26 +943,31 @@ static int ati_remote_probe(struct usb_interface *interface,
 	if (mouse) {
 		input_dev = input_allocate_device();
 		if (!input_dev)
-			goto fail4;
+			goto exit_unregister_device;
 
 		ati_remote->idev = input_dev;
 		ati_remote_input_init(ati_remote);
 		err = input_register_device(input_dev);
 
 		if (err)
-			goto fail5;
+			goto exit_free_input_device;
 	}
 
 	usb_set_intfdata(interface, ati_remote);
 	return 0;
 
- fail5:	input_free_device(input_dev);
- fail4:	rc_unregister_device(rc_dev);
+ exit_free_input_device:
+	input_free_device(input_dev);
+ exit_unregister_device:
+	rc_unregister_device(rc_dev);
 	rc_dev = NULL;
- fail3:	usb_kill_urb(ati_remote->irq_urb);
+ exit_kill_urbs:
+	usb_kill_urb(ati_remote->irq_urb);
 	usb_kill_urb(ati_remote->out_urb);
- fail2:	ati_remote_free_buffers(ati_remote);
- fail1:	rc_free_device(rc_dev);
+ exit_free_buffers:
+	ati_remote_free_buffers(ati_remote);
+ exit_free_dev_rdev:
+	 rc_free_device(rc_dev);
 	kfree(ati_remote);
 	return err;
 }
diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c
index 62f9076..7337816 100644
--- a/drivers/media/rc/ene_ir.c
+++ b/drivers/media/rc/ene_ir.c
@@ -1000,7 +1000,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
 	rdev = rc_allocate_device();
 	if (!dev || !rdev)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	/* validate resources */
 	error = -ENODEV;
@@ -1011,10 +1011,10 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 
 	if (!pnp_port_valid(pnp_dev, 0) ||
 	    pnp_port_len(pnp_dev, 0) < ENE_IO_SIZE)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (!pnp_irq_valid(pnp_dev, 0))
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	spin_lock_init(&dev->hw_lock);
 
@@ -1030,7 +1030,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* detect hardware version and features */
 	error = ene_hw_detect(dev);
 	if (error)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (!dev->hw_learning_and_tx_capable && txsim) {
 		dev->hw_learning_and_tx_capable = true;
@@ -1075,27 +1075,27 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* claim the resources */
 	error = -EBUSY;
 	if (!request_region(dev->hw_io, ENE_IO_SIZE, ENE_DRIVER_NAME)) {
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	dev->irq = pnp_irq(pnp_dev, 0);
 	if (request_irq(dev->irq, ene_isr,
 			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev)) {
-		goto failure2;
+		goto exit_release_hw_io;
 	}
 
 	error = rc_register_device(rdev);
 	if (error < 0)
-		goto failure3;
+		goto exit_free_irq;
 
 	pr_notice("driver has been successfully loaded\n");
 	return 0;
 
-failure3:
+exit_free_irq:
 	free_irq(dev->irq, dev);
-failure2:
+exit_release_hw_io:
 	release_region(dev->hw_io, ENE_IO_SIZE);
-failure:
+exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(dev);
 	return error;
diff --git a/drivers/media/rc/fintek-cir.c b/drivers/media/rc/fintek-cir.c
index ab30c64..8284d28 100644
--- a/drivers/media/rc/fintek-cir.c
+++ b/drivers/media/rc/fintek-cir.c
@@ -495,18 +495,18 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 	/* input device for IR remote (and tx) */
 	rdev = rc_allocate_device();
 	if (!rdev)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	ret = -ENODEV;
 	/* validate pnp resources */
 	if (!pnp_port_valid(pdev, 0)) {
 		dev_err(&pdev->dev, "IR PNP Port not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	if (!pnp_irq_valid(pdev, 0)) {
 		dev_err(&pdev->dev, "IR PNP IRQ not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	fintek->cir_addr = pnp_port_start(pdev, 0);
@@ -523,7 +523,7 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 
 	ret = fintek_hw_detect(fintek);
 	if (ret)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	/* Initialize CIR & CIR Wake Logical Devices */
 	fintek_config_mode_enable(fintek);
@@ -556,15 +556,15 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 	/* now claim resources */
 	if (!request_region(fintek->cir_addr,
 			    fintek->cir_port_len, FINTEK_DRIVER_NAME))
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (request_irq(fintek->cir_irq, fintek_cir_isr, IRQF_SHARED,
 			FINTEK_DRIVER_NAME, (void *)fintek))
-		goto failure2;
+		goto exit_free_cir_addr;
 
 	ret = rc_register_device(rdev);
 	if (ret)
-		goto failure3;
+		goto exit_free_irq;
 
 	device_init_wakeup(&pdev->dev, true);
 	fintek->rdev = rdev;
@@ -574,11 +574,11 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 
 	return 0;
 
-failure3:
+exit_free_irq:
 	free_irq(fintek->cir_irq, fintek);
-failure2:
+exit_free_cir_addr:
 	release_region(fintek->cir_addr, fintek->cir_port_len);
-failure:
+exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(fintek);
 
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index 04cb272..0c03b7d 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -79,7 +79,7 @@ static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
 	rcdev = rc_allocate_device();
 	if (!rcdev) {
 		rc = -ENOMEM;
-		goto err_allocate_device;
+		goto exit_free_dev;
 	}
 
 	rcdev->priv = gpio_dev;
@@ -104,15 +104,15 @@ static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
 
 	rc = gpio_request(pdata->gpio_nr, "gpio-ir-recv");
 	if (rc < 0)
-		goto err_gpio_request;
+		goto exit_free_rdev;
 	rc  = gpio_direction_input(pdata->gpio_nr);
 	if (rc < 0)
-		goto err_gpio_direction_input;
+		goto exit_free_gpio;
 
 	rc = rc_register_device(rcdev);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "failed to register rc device\n");
-		goto err_register_rc_device;
+		goto exit_free_gpio;
 	}
 
 	platform_set_drvdata(pdev, gpio_dev);
@@ -122,20 +122,19 @@ static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
 			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 					"gpio-ir-recv-irq", gpio_dev);
 	if (rc < 0)
-		goto err_request_irq;
+		goto exit_unregister_device;
 
 	return 0;
 
-err_request_irq:
+exit_unregister_device:
 	platform_set_drvdata(pdev, NULL);
 	rc_unregister_device(rcdev);
-err_register_rc_device:
-err_gpio_direction_input:
+exit_free_gpio:
 	gpio_free(pdata->gpio_nr);
-err_gpio_request:
+exit_free_rdev:
 	rc_free_device(rcdev);
 	rcdev = NULL;
-err_allocate_device:
+exit_free_dev:
 	kfree(gpio_dev);
 	return rc;
 }
diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 36fe5a3..77cb21f 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1472,7 +1472,7 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	/* input device for IR remote (and tx) */
 	rdev = rc_allocate_device();
 	if (!rdev)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	ret = -ENODEV;
 
@@ -1497,12 +1497,12 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	if (!pnp_port_valid(pdev, io_rsrc_no) ||
 	    pnp_port_len(pdev, io_rsrc_no) != dev_desc->io_region_size) {
 		dev_err(&pdev->dev, "IR PNP Port not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	if (!pnp_irq_valid(pdev, 0)) {
 		dev_err(&pdev->dev, "PNP IRQ not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	/* store resource values */
@@ -1594,26 +1594,26 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	/* now claim resources */
 	if (!request_region(itdev->cir_addr,
 				dev_desc->io_region_size, ITE_DRIVER_NAME))
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (request_irq(itdev->cir_irq, ite_cir_isr, IRQF_SHARED,
 			ITE_DRIVER_NAME, (void *)itdev))
-		goto failure2;
+		goto exit_release_cir_addr;
 
 	ret = rc_register_device(rdev);
 	if (ret)
-		goto failure3;
+		goto exit_free_irq;
 
 	itdev->rdev = rdev;
 	ite_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 
 	return 0;
 
-failure3:
+exit_free_irq:
 	free_irq(itdev->cir_irq, itdev);
-failure2:
+exit_release_cir_addr:
 	release_region(itdev->cir_addr, itdev->params.io_region_size);
-failure:
+exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(itdev);
 
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index 699eef3..8ab6843 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -986,25 +986,25 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 	/* input device for IR remote (and tx) */
 	rdev = rc_allocate_device();
 	if (!rdev)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	ret = -ENODEV;
 	/* validate pnp resources */
 	if (!pnp_port_valid(pdev, 0) ||
 	    pnp_port_len(pdev, 0) < CIR_IOREG_LENGTH) {
 		dev_err(&pdev->dev, "IR PNP Port not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	if (!pnp_irq_valid(pdev, 0)) {
 		dev_err(&pdev->dev, "PNP IRQ not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	if (!pnp_port_valid(pdev, 1) ||
 	    pnp_port_len(pdev, 1) < CIR_IOREG_LENGTH) {
 		dev_err(&pdev->dev, "Wake PNP Port not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	nvt->cir_addr = pnp_port_start(pdev, 0);
@@ -1027,7 +1027,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 
 	ret = nvt_hw_detect(nvt);
 	if (ret)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	/* Initialize CIR & CIR Wake Logical Devices */
 	nvt_efm_enable(nvt);
@@ -1070,23 +1070,23 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 	/* now claim resources */
 	if (!request_region(nvt->cir_addr,
 			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (request_irq(nvt->cir_irq, nvt_cir_isr, IRQF_SHARED,
 			NVT_DRIVER_NAME, (void *)nvt))
-		goto failure2;
+		goto exit_release_cir_addr;
 
 	if (!request_region(nvt->cir_wake_addr,
 			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
-		goto failure3;
+		goto exit_free_irq;
 
 	if (request_irq(nvt->cir_wake_irq, nvt_cir_wake_isr, IRQF_SHARED,
 			NVT_DRIVER_NAME, (void *)nvt))
-		goto failure4;
+		goto exit_release_cir_wake_addr;
 
 	ret = rc_register_device(rdev);
 	if (ret)
-		goto failure5;
+		goto exit_free_wake_irq;
 
 	device_init_wakeup(&pdev->dev, true);
 	nvt->rdev = rdev;
@@ -1098,15 +1098,15 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 
 	return 0;
 
-failure5:
+exit_free_wake_irq:
 	free_irq(nvt->cir_wake_irq, nvt);
-failure4:
+exit_release_cir_wake_addr:
 	release_region(nvt->cir_wake_addr, CIR_IOREG_LENGTH);
-failure3:
+exit_free_irq:
 	free_irq(nvt->cir_irq, nvt);
-failure2:
+exit_release_cir_addr:
 	release_region(nvt->cir_addr, CIR_IOREG_LENGTH);
-failure:
+exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(nvt);
 
-- 
1.7.10

[PATCH 3/4] [media] rc: Set rdev before irq setup

[Matthijs Kooijman]

This fixes a problem in fintek-cir, ite-cir and nuvoton-cir where the
irq handler would trigger during module load before the rdev member was
set, causing a NULL pointer crash.

It seems this crash is very reproducible (just bombard the receiver with
IR signals during module load), probably because when request_irq is
called, any pending intterupt is handled immediately, before
request_irq returns and rdev can be set.

This same crash was supposed to be fixed by commit
9ef449c6b31bb6a8e6dedc24de475a3b8c79be20 ("[media] rc: Postpone ISR
registration"), but the crash was still observed on the nuvoton-cir
driver.

This commit was tested on nuvoton-cir only.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 drivers/media/rc/fintek-cir.c  |    4 +++-
 drivers/media/rc/ite-cir.c     |    3 ++-
 drivers/media/rc/nuvoton-cir.c |    3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/fintek-cir.c b/drivers/media/rc/fintek-cir.c
index 8284d28..54809b8 100644
--- a/drivers/media/rc/fintek-cir.c
+++ b/drivers/media/rc/fintek-cir.c
@@ -552,6 +552,8 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 	/* rx resolution is hardwired to 50us atm, 1, 25, 100 also possible */
 	rdev->rx_resolution = US_TO_NS(CIR_SAMPLE_PERIOD);
 
+	fintek->rdev = rdev;
+
 	ret = -EBUSY;
 	/* now claim resources */
 	if (!request_region(fintek->cir_addr,
@@ -567,7 +569,7 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 		goto exit_free_irq;
 
 	device_init_wakeup(&pdev->dev, true);
-	fintek->rdev = rdev;
+
 	fit_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 	if (debug)
 		cir_dump_regs(fintek);
diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 77cb21f..158bd0a 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1590,6 +1590,8 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	rdev->driver_name = ITE_DRIVER_NAME;
 	rdev->map_name = RC_MAP_RC6_MCE;
 
+	itdev->rdev = rdev;
+
 	ret = -EBUSY;
 	/* now claim resources */
 	if (!request_region(itdev->cir_addr,
@@ -1604,7 +1606,6 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	if (ret)
 		goto exit_free_irq;
 
-	itdev->rdev = rdev;
 	ite_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 
 	return 0;
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index 8ab6843..a1b6be6 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -1065,6 +1065,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 	/* tx bits */
 	rdev->tx_resolution = XYZ;
 #endif
+	nvt->rdev = rdev;
 
 	ret = -EBUSY;
 	/* now claim resources */
@@ -1089,7 +1090,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 		goto exit_free_wake_irq;
 
 	device_init_wakeup(&pdev->dev, true);
-	nvt->rdev = rdev;
+
 	nvt_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 	if (debug) {
 		cir_dump_regs(nvt);
-- 
1.7.10

[PATCH 4/4] [media] rc: Call rc_register_device before irq setup

[Matthijs Kooijman]

This should fix a potential race condition, when the irq handler
triggers while rc_register_device is still setting up the rdev->raw
device.

This crash has not been observed in practice, but there should be a very
small window where it could occur. Since ir_raw_event_store_with_filter
checks if rdev->raw is not NULL before using it, this bug is not
triggered if the request_irq triggers a pending irq directly (since
rdev->raw will still be NULL then).

This commit was tested on nuvoton-cir only.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
---
 drivers/media/rc/ene_ir.c      |   14 +++++++-------
 drivers/media/rc/ite-cir.c     |   14 +++++++-------
 drivers/media/rc/nuvoton-cir.c |   14 +++++++-------
 drivers/media/rc/winbond-cir.c |   14 +++++++-------
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c
index 7337816..17b38a9 100644
--- a/drivers/media/rc/ene_ir.c
+++ b/drivers/media/rc/ene_ir.c
@@ -1072,10 +1072,14 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	device_set_wakeup_capable(&pnp_dev->dev, true);
 	device_set_wakeup_enable(&pnp_dev->dev, true);
 
+	error = rc_register_device(rdev);
+	if (error < 0)
+		goto exit_free_dev_rdev;
+
 	/* claim the resources */
 	error = -EBUSY;
 	if (!request_region(dev->hw_io, ENE_IO_SIZE, ENE_DRIVER_NAME)) {
-		goto exit_free_dev_rdev;
+		goto exit_unregister_device;
 	}
 
 	dev->irq = pnp_irq(pnp_dev, 0);
@@ -1084,17 +1088,13 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 		goto exit_release_hw_io;
 	}
 
-	error = rc_register_device(rdev);
-	if (error < 0)
-		goto exit_free_irq;
-
 	pr_notice("driver has been successfully loaded\n");
 	return 0;
 
-exit_free_irq:
-	free_irq(dev->irq, dev);
 exit_release_hw_io:
 	release_region(dev->hw_io, ENE_IO_SIZE);
+exit_unregister_device:
+	rc_unregister_device(rdev);
 exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(dev);
diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 158bd0a..974836a 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1592,28 +1592,28 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 
 	itdev->rdev = rdev;
 
+	ret = rc_register_device(rdev);
+	if (ret)
+		goto exit_free_dev_rdev;
+
 	ret = -EBUSY;
 	/* now claim resources */
 	if (!request_region(itdev->cir_addr,
 				dev_desc->io_region_size, ITE_DRIVER_NAME))
-		goto exit_free_dev_rdev;
+		goto exit_unregister_device;
 
 	if (request_irq(itdev->cir_irq, ite_cir_isr, IRQF_SHARED,
 			ITE_DRIVER_NAME, (void *)itdev))
 		goto exit_release_cir_addr;
 
-	ret = rc_register_device(rdev);
-	if (ret)
-		goto exit_free_irq;
-
 	ite_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 
 	return 0;
 
-exit_free_irq:
-	free_irq(itdev->cir_irq, itdev);
 exit_release_cir_addr:
 	release_region(itdev->cir_addr, itdev->params.io_region_size);
+exit_unregister_device:
+	rc_unregister_device(rdev);
 exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(itdev);
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index a1b6be6..18a50b9 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -1067,11 +1067,15 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 #endif
 	nvt->rdev = rdev;
 
+	ret = rc_register_device(rdev);
+	if (ret)
+		goto exit_free_dev_rdev;
+
 	ret = -EBUSY;
 	/* now claim resources */
 	if (!request_region(nvt->cir_addr,
 			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
-		goto exit_free_dev_rdev;
+		goto exit_unregister_device;
 
 	if (request_irq(nvt->cir_irq, nvt_cir_isr, IRQF_SHARED,
 			NVT_DRIVER_NAME, (void *)nvt))
@@ -1085,10 +1089,6 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 			NVT_DRIVER_NAME, (void *)nvt))
 		goto exit_release_cir_wake_addr;
 
-	ret = rc_register_device(rdev);
-	if (ret)
-		goto exit_free_wake_irq;
-
 	device_init_wakeup(&pdev->dev, true);
 
 	nvt_pr(KERN_NOTICE, "driver has been successfully loaded\n");
@@ -1099,14 +1099,14 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 
 	return 0;
 
-exit_free_wake_irq:
-	free_irq(nvt->cir_wake_irq, nvt);
 exit_release_cir_wake_addr:
 	release_region(nvt->cir_wake_addr, CIR_IOREG_LENGTH);
 exit_free_irq:
 	free_irq(nvt->cir_irq, nvt);
 exit_release_cir_addr:
 	release_region(nvt->cir_addr, CIR_IOREG_LENGTH);
+exit_unregister_device:
+	rc_unregister_device(rdev);
 exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(nvt);
diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 54ee348..1f90e8c 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -1035,11 +1035,15 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
 	data->dev->timeout = MS_TO_NS(100);
 	data->dev->allowed_protos = RC_TYPE_ALL;
 
+	err = rc_register_device(data->dev);
+	if (err)
+		goto exit_free_rc;
+
 	if (!request_region(data->wbase, WAKEUP_IOMEM_LEN, DRVNAME)) {
 		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
 			data->wbase, data->wbase + WAKEUP_IOMEM_LEN - 1);
 		err = -EBUSY;
-		goto exit_free_rc;
+		goto exit_unregister_device;
 	}
 
 	if (!request_region(data->ebase, EHFUNC_IOMEM_LEN, DRVNAME)) {
@@ -1064,24 +1068,20 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
 		goto exit_release_sbase;
 	}
 
-	err = rc_register_device(data->dev);
-	if (err)
-		goto exit_free_irq;
-
 	device_init_wakeup(&device->dev, 1);
 
 	wbcir_init_hw(data);
 
 	return 0;
 
-exit_free_irq:
-	free_irq(data->irq, device);
 exit_release_sbase:
 	release_region(data->sbase, SP_IOMEM_LEN);
 exit_release_ebase:
 	release_region(data->ebase, EHFUNC_IOMEM_LEN);
 exit_release_wbase:
 	release_region(data->wbase, WAKEUP_IOMEM_LEN);
+exit_unregister_device:
+	rc_unregister_device(data->dev);
 exit_free_rc:
 	rc_free_device(data->dev);
 exit_unregister_led:
-- 
1.7.10

Re: (still) NULL pointer crashes with nuvoton_cir driver

[Luis Henriques]

On Mon, Oct 15, 2012 at 01:01:11PM +0200, Matthijs Kooijman wrote:
> Hey Folks,
> 
> > I've been suffering from a NULL pointer crash while initializing the
> > nuvoton_cir driver. It occurs 100% reliable on a 3.5 kernel when I
> > bombard my receiver with IR signals during bootup.
> >
> > It seems that 9ef449c: [media] rc: Postpone ISR registration attempts to
> > fix this very issue, but does not fix it completely.
> 
> I've looked a bit more closely at the code and I do not really
> understand why this commit actually fixed the crash for some (ite-cir)
> users. Looking at the bugreport linked to in that commit [1], users
> report their bug is fixed, but I don't really understand why. Perhaps
> the window for this race condition is just made smaller, but not
> entirely eliminated?

Your diagnosis seem to be correct and I have already tried to address
this:

https://lkml.org/lkml/2012/8/25/84

There's also a bug reported here:

https://bugzilla.kernel.org/attachment.cgi?id=78491

I've submitted a patch to try to fix it but got no comments yet.  My
patch moves the request_irq() invocation further down the
initialisation code, after the rc_register_device() (which I believe
is the correct thing to do).

> 
> Note that the actual patch tested by the Launchpad users (which is
> included in the Ubuntu kernel) is a smaller patch than the one in
> git, but the essence is still the same.

Again, this is correct.  The patch tested was just for a subset of the
drivers and this fix was later ported to the other affected drivers.
But again, as you referred above, this doesn't completely fix the
issue as it just reduced the race window.

Cheers,
--
Luis

> 
> [1]: http://bugs.launchpad.net/bugs/972723
> 
> > In particular, I'm not sure if it's ok to start firing IRQ's before
> > rc_register_device is called. rc_register_device also calls
> > ir_raw_event_register to init rdev->raw, which is again used by
> > ir_raw_event_store_with_filter.
> It seems this particular thing did not cause a (reproducible) problem,
> since ir_raw_event_store_with_filter bails out when rdev->raw is NULL
> (returning -EINVAL, but that's ignored by nvt_process_rx_ir_data).
> 
> Still, since there is still a small time window in which rdev->raw is
> not NULL, but not completely initialized either, I believe that the irqs
> should not be registered until after rc_register_device was called.
> 
> 
> I've also looked at the USB IR drivers and it seems some potentially
> have the same problem. In particular, usb_fill_int_urb (for iguanair and
> ati-remote) or redrat3_enable_detector (for redrat3) is called before
> rc_register_device and other initialization, but I don't know the USB
> subsystem well enough to know if this is really a problem or if simply
> moving th usb_fill_int_urb / redrat3_enable_detector would help at all.
> 
> The mceusb, streamzap and gpio-ir-recv drivers look like they initialize the interrupt
> usb at the end and shouldn't suffer from this problem.
> 
> 
> I'll send a patch series for this bug in a minute. It also fixes some
> problem in cleanup of ene-ir I noticed while reading the code and
> renames some of the cleanup labels (which should make the last patch
> easier to review). I've tried to separate changes as much as possible,
> if there's anything you'd like to see different, just let me know.
> 
> Since I only have nuvoton-cir hardware, the changes to the other drivers
> are compile-tested only (but the drivers are similar enough that I
> expect no problems there).
> 
> Gr.
> 
> Matthijs

Re: (still) NULL pointer crashes with nuvoton_cir driver

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

Hey Luis,

> Your diagnosis seem to be correct and I have already tried to address
> this:
> 
> https://lkml.org/lkml/2012/8/25/84
Oh, w00ps, seems I missed that patch, stupid of me...

> I've submitted a patch to try to fix it but got no comments yet.  My
> patch moves the request_irq() invocation further down the
> initialisation code, after the rc_register_device() (which I believe
> is the correct thing to do).

I applied your patch and ran a diff against my four patches. If I disregard
the label changes, there's still three changes between our patches I can
see:
 - You move the rc_register_device calls only above the request_irq
   call, while I also included the request_region call. I don't think
   this should cause any realy difference.
 - You didn't remove the "dev->hw_io = -1" and "dev->irq = -1" lines in
   ene_ir.c, which I think are no longer needed now there are different
   multiple cleanup labels.
 - You did not move up the "...->rdev = rdev" line in fintek-cir.c,
   nuvoton-cir.c and ite-cir.c, which I think means the bug is not
   completely solved for these drivers with your patch.


As for the last point, I did a test run with your patch and could no
longer reproduce the issue (though I'm pretty confident the original
really was nvt->rdev being NULL). I suspect this means that moving the
rc_register_device() up to before the request_irq, prevents the bug from
triggering because either rc_register_device() did something to trigger
a (pending) interrupt, or it just took long enough for an interrupt to
occur.

In either case, I believe there is still a race condition between the
first interrupt and the initialization of of nvt->rdev, which could be
fixed by also moving the rdev initialization a bit further up. Or am I
missing some point here?

Gr.

Matthijs

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: (still) NULL pointer crashes with nuvoton_cir driver

[Matthijs Kooijman]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

Hey Luis,

seems we're not the only ones working on this, there's a partially
overlapping patch in v3.6.2 for the ite-cir driver only:

http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=commit;h=434f4d3f1209d4e4c8b0d577d1c809f349c4f0da

Gr.

Matthijs

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Updated patches for (potential) NULL pointer crashes in cir drivers

[Matthijs Kooijman]

(Please keep me CC'd, I'm not subscribed to the list)

Hi folks,

I've updated my patches to apply against Mauro's staging/for_v3.8
branch. The patches have only been modified to apply against recent
changes in some of the drivers.

Like before, these patches have been really tested only on nuvoton_cir,
the others received compile-testing only.

Gr.

Matthijs

[PATCH 1/3] [media] rc: Make probe cleanup goto labels more verbose

[Matthijs Kooijman]

Before, labels were simply numbered. Now, the labels are named after the
cleanup action they'll perform (first), based on how the winbond-cir
driver does it. This makes the code a bit more clear and makes changes
in the ordering of labels easier to review.

This change is applied only to the rc drivers that do significant
cleanup in their probe functions: ati-remote, ene-ir, fintek-cir,
gpio-ir-recv, ite-cir, nuvoton-cir.

This commit should not change any code, it just renames goto labels.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
Cc: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/rc/ati_remote.c   |   27 ++++++++++++++++-----------
 drivers/media/rc/ene_ir.c       |   20 ++++++++++----------
 drivers/media/rc/fintek-cir.c   |   20 ++++++++++----------
 drivers/media/rc/gpio-ir-recv.c |   19 +++++++++----------
 drivers/media/rc/ite-cir.c      |   18 +++++++++---------
 drivers/media/rc/nuvoton-cir.c  |   30 +++++++++++++++---------------
 6 files changed, 69 insertions(+), 65 deletions(-)

diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index 2d6fb26..4d6a63f 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -872,11 +872,11 @@ static int ati_remote_probe(struct usb_interface *interface,
 	ati_remote = kzalloc(sizeof (struct ati_remote), GFP_KERNEL);
 	rc_dev = rc_allocate_device();
 	if (!ati_remote || !rc_dev)
-		goto fail1;
+		goto exit_free_dev_rdev;
 
 	/* Allocate URB buffers, URBs */
 	if (ati_remote_alloc_buffers(udev, ati_remote))
-		goto fail2;
+		goto exit_free_buffers;
 
 	ati_remote->endpoint_in = endpoint_in;
 	ati_remote->endpoint_out = endpoint_out;
@@ -924,12 +924,12 @@ static int ati_remote_probe(struct usb_interface *interface,
 	/* Device Hardware Initialization - fills in ati_remote->idev from udev. */
 	err = ati_remote_initialize(ati_remote);
 	if (err)
-		goto fail3;
+		goto exit_kill_urbs;
 
 	/* Set up and register rc device */
 	err = rc_register_device(ati_remote->rdev);
 	if (err)
-		goto fail3;
+		goto exit_kill_urbs;
 
 	/* use our delay for rc_dev */
 	ati_remote->rdev->input_dev->rep[REP_DELAY] = repeat_delay;
@@ -939,7 +939,7 @@ static int ati_remote_probe(struct usb_interface *interface,
 		input_dev = input_allocate_device();
 		if (!input_dev) {
 			err = -ENOMEM;
-			goto fail4;
+			goto exit_unregister_device;
 		}
 
 		ati_remote->idev = input_dev;
@@ -947,19 +947,24 @@ static int ati_remote_probe(struct usb_interface *interface,
 		err = input_register_device(input_dev);
 
 		if (err)
-			goto fail5;
+			goto exit_free_input_device;
 	}
 
 	usb_set_intfdata(interface, ati_remote);
 	return 0;
 
- fail5:	input_free_device(input_dev);
- fail4:	rc_unregister_device(rc_dev);
+ exit_free_input_device:
+	input_free_device(input_dev);
+ exit_unregister_device:
+	rc_unregister_device(rc_dev);
 	rc_dev = NULL;
- fail3:	usb_kill_urb(ati_remote->irq_urb);
+ exit_kill_urbs:
+	usb_kill_urb(ati_remote->irq_urb);
 	usb_kill_urb(ati_remote->out_urb);
- fail2:	ati_remote_free_buffers(ati_remote);
- fail1:	rc_free_device(rc_dev);
+ exit_free_buffers:
+	ati_remote_free_buffers(ati_remote);
+ exit_free_dev_rdev:
+	 rc_free_device(rc_dev);
 	kfree(ati_remote);
 	return err;
 }
diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c
index 22231dd..f7fdfea 100644
--- a/drivers/media/rc/ene_ir.c
+++ b/drivers/media/rc/ene_ir.c
@@ -1003,7 +1003,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	dev = kzalloc(sizeof(struct ene_device), GFP_KERNEL);
 	rdev = rc_allocate_device();
 	if (!dev || !rdev)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	/* validate resources */
 	error = -ENODEV;
@@ -1014,10 +1014,10 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 
 	if (!pnp_port_valid(pnp_dev, 0) ||
 	    pnp_port_len(pnp_dev, 0) < ENE_IO_SIZE)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (!pnp_irq_valid(pnp_dev, 0))
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	spin_lock_init(&dev->hw_lock);
 
@@ -1033,7 +1033,7 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* detect hardware version and features */
 	error = ene_hw_detect(dev);
 	if (error)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (!dev->hw_learning_and_tx_capable && txsim) {
 		dev->hw_learning_and_tx_capable = true;
@@ -1078,27 +1078,27 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	/* claim the resources */
 	error = -EBUSY;
 	if (!request_region(dev->hw_io, ENE_IO_SIZE, ENE_DRIVER_NAME)) {
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	dev->irq = pnp_irq(pnp_dev, 0);
 	if (request_irq(dev->irq, ene_isr,
 			IRQF_SHARED, ENE_DRIVER_NAME, (void *)dev)) {
-		goto failure2;
+		goto exit_release_hw_io;
 	}
 
 	error = rc_register_device(rdev);
 	if (error < 0)
-		goto failure3;
+		goto exit_free_irq;
 
 	pr_notice("driver has been successfully loaded\n");
 	return 0;
 
-failure3:
+exit_free_irq:
 	free_irq(dev->irq, dev);
-failure2:
+exit_release_hw_io:
 	release_region(dev->hw_io, ENE_IO_SIZE);
-failure:
+exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(dev);
 	return error;
diff --git a/drivers/media/rc/fintek-cir.c b/drivers/media/rc/fintek-cir.c
index 936c3f7..3d5e57c 100644
--- a/drivers/media/rc/fintek-cir.c
+++ b/drivers/media/rc/fintek-cir.c
@@ -500,18 +500,18 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 	/* input device for IR remote (and tx) */
 	rdev = rc_allocate_device();
 	if (!rdev)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	ret = -ENODEV;
 	/* validate pnp resources */
 	if (!pnp_port_valid(pdev, 0)) {
 		dev_err(&pdev->dev, "IR PNP Port not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	if (!pnp_irq_valid(pdev, 0)) {
 		dev_err(&pdev->dev, "IR PNP IRQ not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	fintek->cir_addr = pnp_port_start(pdev, 0);
@@ -528,7 +528,7 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 
 	ret = fintek_hw_detect(fintek);
 	if (ret)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	/* Initialize CIR & CIR Wake Logical Devices */
 	fintek_config_mode_enable(fintek);
@@ -561,15 +561,15 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 	/* now claim resources */
 	if (!request_region(fintek->cir_addr,
 			    fintek->cir_port_len, FINTEK_DRIVER_NAME))
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (request_irq(fintek->cir_irq, fintek_cir_isr, IRQF_SHARED,
 			FINTEK_DRIVER_NAME, (void *)fintek))
-		goto failure2;
+		goto exit_free_cir_addr;
 
 	ret = rc_register_device(rdev);
 	if (ret)
-		goto failure3;
+		goto exit_free_irq;
 
 	device_init_wakeup(&pdev->dev, true);
 	fintek->rdev = rdev;
@@ -579,11 +579,11 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 
 	return 0;
 
-failure3:
+exit_free_irq:
 	free_irq(fintek->cir_irq, fintek);
-failure2:
+exit_free_cir_addr:
 	release_region(fintek->cir_addr, fintek->cir_port_len);
-failure:
+exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(fintek);
 
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index ba1a1eb..d240036 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -79,7 +79,7 @@ static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
 	rcdev = rc_allocate_device();
 	if (!rcdev) {
 		rc = -ENOMEM;
-		goto err_allocate_device;
+		goto exit_free_dev;
 	}
 
 	rcdev->priv = gpio_dev;
@@ -104,15 +104,15 @@ static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
 
 	rc = gpio_request(pdata->gpio_nr, "gpio-ir-recv");
 	if (rc < 0)
-		goto err_gpio_request;
+		goto exit_free_rdev;
 	rc  = gpio_direction_input(pdata->gpio_nr);
 	if (rc < 0)
-		goto err_gpio_direction_input;
+		goto exit_free_gpio;
 
 	rc = rc_register_device(rcdev);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "failed to register rc device\n");
-		goto err_register_rc_device;
+		goto exit_free_gpio;
 	}
 
 	platform_set_drvdata(pdev, gpio_dev);
@@ -122,20 +122,19 @@ static int __devinit gpio_ir_recv_probe(struct platform_device *pdev)
 			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
 					"gpio-ir-recv-irq", gpio_dev);
 	if (rc < 0)
-		goto err_request_irq;
+		goto exit_unregister_device;
 
 	return 0;
 
-err_request_irq:
+exit_unregister_device:
 	platform_set_drvdata(pdev, NULL);
 	rc_unregister_device(rcdev);
-err_register_rc_device:
-err_gpio_direction_input:
+exit_free_gpio:
 	gpio_free(pdata->gpio_nr);
-err_gpio_request:
+exit_free_rdev:
 	rc_free_device(rcdev);
 	rcdev = NULL;
-err_allocate_device:
+exit_free_dev:
 	kfree(gpio_dev);
 	return rc;
 }
diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 5e5a7f2..8e0e661 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1472,7 +1472,7 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	/* input device for IR remote (and tx) */
 	rdev = rc_allocate_device();
 	if (!rdev)
-		goto failure;
+		goto exit_free_dev_rdev;
 	itdev->rdev = rdev;
 
 	ret = -ENODEV;
@@ -1498,12 +1498,12 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	if (!pnp_port_valid(pdev, io_rsrc_no) ||
 	    pnp_port_len(pdev, io_rsrc_no) != dev_desc->io_region_size) {
 		dev_err(&pdev->dev, "IR PNP Port not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	if (!pnp_irq_valid(pdev, 0)) {
 		dev_err(&pdev->dev, "PNP IRQ not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	/* store resource values */
@@ -1595,25 +1595,25 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	/* now claim resources */
 	if (!request_region(itdev->cir_addr,
 				dev_desc->io_region_size, ITE_DRIVER_NAME))
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (request_irq(itdev->cir_irq, ite_cir_isr, IRQF_SHARED,
 			ITE_DRIVER_NAME, (void *)itdev))
-		goto failure2;
+		goto exit_release_cir_addr;
 
 	ret = rc_register_device(rdev);
 	if (ret)
-		goto failure3;
+		goto exit_free_irq;
 
 	ite_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 
 	return 0;
 
-failure3:
+exit_free_irq:
 	free_irq(itdev->cir_irq, itdev);
-failure2:
+exit_release_cir_addr:
 	release_region(itdev->cir_addr, itdev->params.io_region_size);
-failure:
+exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(itdev);
 
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index e4ea89a..3477e23 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -986,25 +986,25 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 	/* input device for IR remote (and tx) */
 	rdev = rc_allocate_device();
 	if (!rdev)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	ret = -ENODEV;
 	/* validate pnp resources */
 	if (!pnp_port_valid(pdev, 0) ||
 	    pnp_port_len(pdev, 0) < CIR_IOREG_LENGTH) {
 		dev_err(&pdev->dev, "IR PNP Port not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	if (!pnp_irq_valid(pdev, 0)) {
 		dev_err(&pdev->dev, "PNP IRQ not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	if (!pnp_port_valid(pdev, 1) ||
 	    pnp_port_len(pdev, 1) < CIR_IOREG_LENGTH) {
 		dev_err(&pdev->dev, "Wake PNP Port not valid!\n");
-		goto failure;
+		goto exit_free_dev_rdev;
 	}
 
 	nvt->cir_addr = pnp_port_start(pdev, 0);
@@ -1027,7 +1027,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 
 	ret = nvt_hw_detect(nvt);
 	if (ret)
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	/* Initialize CIR & CIR Wake Logical Devices */
 	nvt_efm_enable(nvt);
@@ -1070,23 +1070,23 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 	/* now claim resources */
 	if (!request_region(nvt->cir_addr,
 			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
-		goto failure;
+		goto exit_free_dev_rdev;
 
 	if (request_irq(nvt->cir_irq, nvt_cir_isr, IRQF_SHARED,
 			NVT_DRIVER_NAME, (void *)nvt))
-		goto failure2;
+		goto exit_release_cir_addr;
 
 	if (!request_region(nvt->cir_wake_addr,
 			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
-		goto failure3;
+		goto exit_free_irq;
 
 	if (request_irq(nvt->cir_wake_irq, nvt_cir_wake_isr, IRQF_SHARED,
 			NVT_DRIVER_NAME, (void *)nvt))
-		goto failure4;
+		goto exit_release_cir_wake_addr;
 
 	ret = rc_register_device(rdev);
 	if (ret)
-		goto failure5;
+		goto exit_free_wake_irq;
 
 	device_init_wakeup(&pdev->dev, true);
 	nvt->rdev = rdev;
@@ -1098,15 +1098,15 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 
 	return 0;
 
-failure5:
+exit_free_wake_irq:
 	free_irq(nvt->cir_wake_irq, nvt);
-failure4:
+exit_release_cir_wake_addr:
 	release_region(nvt->cir_wake_addr, CIR_IOREG_LENGTH);
-failure3:
+exit_free_irq:
 	free_irq(nvt->cir_irq, nvt);
-failure2:
+exit_release_cir_addr:
 	release_region(nvt->cir_addr, CIR_IOREG_LENGTH);
-failure:
+exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(nvt);
 
-- 
1.7.10

[PATCH 2/3] [media] rc: Set rdev before irq setup

[Matthijs Kooijman]

This fixes a problem in fintek-cir and nuvoton-cir where the
irq handler would trigger during module load before the rdev member was
set, causing a NULL pointer crash.

It seems this crash is very reproducible (just bombard the receiver with
IR signals during module load), probably because when request_irq is
called, any pending intterupt is handled immediately, before
request_irq returns and rdev can be set.

This same crash was supposed to be fixed by commit
9ef449c6b31bb6a8e6dedc24de475a3b8c79be20 ("[media] rc: Postpone ISR
registration"), but the crash was still observed on the nuvoton-cir
driver.

This commit was tested on nuvoton-cir only.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
Cc: Jarod Wilson <jarod@redhat.com>
---
 drivers/media/rc/fintek-cir.c  |    4 +++-
 drivers/media/rc/nuvoton-cir.c |    3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/fintek-cir.c b/drivers/media/rc/fintek-cir.c
index 3d5e57c..5eefe65 100644
--- a/drivers/media/rc/fintek-cir.c
+++ b/drivers/media/rc/fintek-cir.c
@@ -557,6 +557,8 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 	/* rx resolution is hardwired to 50us atm, 1, 25, 100 also possible */
 	rdev->rx_resolution = US_TO_NS(CIR_SAMPLE_PERIOD);
 
+	fintek->rdev = rdev;
+
 	ret = -EBUSY;
 	/* now claim resources */
 	if (!request_region(fintek->cir_addr,
@@ -572,7 +574,7 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id
 		goto exit_free_irq;
 
 	device_init_wakeup(&pdev->dev, true);
-	fintek->rdev = rdev;
+
 	fit_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 	if (debug)
 		cir_dump_regs(fintek);
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index 3477e23..c6441e6 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -1065,6 +1065,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 	/* tx bits */
 	rdev->tx_resolution = XYZ;
 #endif
+	nvt->rdev = rdev;
 
 	ret = -EBUSY;
 	/* now claim resources */
@@ -1089,7 +1090,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 		goto exit_free_wake_irq;
 
 	device_init_wakeup(&pdev->dev, true);
-	nvt->rdev = rdev;
+
 	nvt_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 	if (debug) {
 		cir_dump_regs(nvt);
-- 
1.7.10

[PATCH 3/3] [media] rc: Call rc_register_device before irq setup

[Matthijs Kooijman]

This should fix a potential race condition, when the irq handler
triggers while rc_register_device is still setting up the rdev->raw
device.

This crash has not been observed in practice, but there should be a very
small window where it could occur. Since ir_raw_event_store_with_filter
checks if rdev->raw is not NULL before using it, this bug is not
triggered if the request_irq triggers a pending irq directly (since
rdev->raw will still be NULL then).

This commit was tested on nuvoton-cir only.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
Cc: Jarod Wilson <jarod@redhat.com>
Cc: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/ene_ir.c      |   14 +++++++-------
 drivers/media/rc/ite-cir.c     |   14 +++++++-------
 drivers/media/rc/nuvoton-cir.c |   14 +++++++-------
 drivers/media/rc/winbond-cir.c |   14 +++++++-------
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c
index f7fdfea..e601166 100644
--- a/drivers/media/rc/ene_ir.c
+++ b/drivers/media/rc/ene_ir.c
@@ -1075,10 +1075,14 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 	device_set_wakeup_capable(&pnp_dev->dev, true);
 	device_set_wakeup_enable(&pnp_dev->dev, true);
 
+	error = rc_register_device(rdev);
+	if (error < 0)
+		goto exit_free_dev_rdev;
+
 	/* claim the resources */
 	error = -EBUSY;
 	if (!request_region(dev->hw_io, ENE_IO_SIZE, ENE_DRIVER_NAME)) {
-		goto exit_free_dev_rdev;
+		goto exit_unregister_device;
 	}
 
 	dev->irq = pnp_irq(pnp_dev, 0);
@@ -1087,17 +1091,13 @@ static int ene_probe(struct pnp_dev *pnp_dev, const struct pnp_device_id *id)
 		goto exit_release_hw_io;
 	}
 
-	error = rc_register_device(rdev);
-	if (error < 0)
-		goto exit_free_irq;
-
 	pr_notice("driver has been successfully loaded\n");
 	return 0;
 
-exit_free_irq:
-	free_irq(dev->irq, dev);
 exit_release_hw_io:
 	release_region(dev->hw_io, ENE_IO_SIZE);
+exit_unregister_device:
+	rc_unregister_device(rdev);
 exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(dev);
diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
index 8e0e661..e810846 100644
--- a/drivers/media/rc/ite-cir.c
+++ b/drivers/media/rc/ite-cir.c
@@ -1591,28 +1591,28 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id
 	rdev->driver_name = ITE_DRIVER_NAME;
 	rdev->map_name = RC_MAP_RC6_MCE;
 
+	ret = rc_register_device(rdev);
+	if (ret)
+		goto exit_free_dev_rdev;
+
 	ret = -EBUSY;
 	/* now claim resources */
 	if (!request_region(itdev->cir_addr,
 				dev_desc->io_region_size, ITE_DRIVER_NAME))
-		goto exit_free_dev_rdev;
+		goto exit_unregister_device;
 
 	if (request_irq(itdev->cir_irq, ite_cir_isr, IRQF_SHARED,
 			ITE_DRIVER_NAME, (void *)itdev))
 		goto exit_release_cir_addr;
 
-	ret = rc_register_device(rdev);
-	if (ret)
-		goto exit_free_irq;
-
 	ite_pr(KERN_NOTICE, "driver has been successfully loaded\n");
 
 	return 0;
 
-exit_free_irq:
-	free_irq(itdev->cir_irq, itdev);
 exit_release_cir_addr:
 	release_region(itdev->cir_addr, itdev->params.io_region_size);
+exit_unregister_device:
+	rc_unregister_device(rdev);
 exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(itdev);
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index c6441e6..6cf43cc 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -1067,11 +1067,15 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 #endif
 	nvt->rdev = rdev;
 
+	ret = rc_register_device(rdev);
+	if (ret)
+		goto exit_free_dev_rdev;
+
 	ret = -EBUSY;
 	/* now claim resources */
 	if (!request_region(nvt->cir_addr,
 			    CIR_IOREG_LENGTH, NVT_DRIVER_NAME))
-		goto exit_free_dev_rdev;
+		goto exit_unregister_device;
 
 	if (request_irq(nvt->cir_irq, nvt_cir_isr, IRQF_SHARED,
 			NVT_DRIVER_NAME, (void *)nvt))
@@ -1085,10 +1089,6 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 			NVT_DRIVER_NAME, (void *)nvt))
 		goto exit_release_cir_wake_addr;
 
-	ret = rc_register_device(rdev);
-	if (ret)
-		goto exit_free_wake_irq;
-
 	device_init_wakeup(&pdev->dev, true);
 
 	nvt_pr(KERN_NOTICE, "driver has been successfully loaded\n");
@@ -1099,14 +1099,14 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 
 	return 0;
 
-exit_free_wake_irq:
-	free_irq(nvt->cir_wake_irq, nvt);
 exit_release_cir_wake_addr:
 	release_region(nvt->cir_wake_addr, CIR_IOREG_LENGTH);
 exit_free_irq:
 	free_irq(nvt->cir_irq, nvt);
 exit_release_cir_addr:
 	release_region(nvt->cir_addr, CIR_IOREG_LENGTH);
+exit_unregister_device:
+	rc_unregister_device(rdev);
 exit_free_dev_rdev:
 	rc_free_device(rdev);
 	kfree(nvt);
diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 45d2fff..de76474 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -1024,11 +1024,15 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
 	data->dev->timeout = MS_TO_NS(100);
 	data->dev->allowed_protos = RC_BIT_ALL;
 
+	err = rc_register_device(data->dev);
+	if (err)
+		goto exit_free_rc;
+
 	if (!request_region(data->wbase, WAKEUP_IOMEM_LEN, DRVNAME)) {
 		dev_err(dev, "Region 0x%lx-0x%lx already in use!\n",
 			data->wbase, data->wbase + WAKEUP_IOMEM_LEN - 1);
 		err = -EBUSY;
-		goto exit_free_rc;
+		goto exit_unregister_device;
 	}
 
 	if (!request_region(data->ebase, EHFUNC_IOMEM_LEN, DRVNAME)) {
@@ -1053,24 +1057,20 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
 		goto exit_release_sbase;
 	}
 
-	err = rc_register_device(data->dev);
-	if (err)
-		goto exit_free_irq;
-
 	device_init_wakeup(&device->dev, 1);
 
 	wbcir_init_hw(data);
 
 	return 0;
 
-exit_free_irq:
-	free_irq(data->irq, device);
 exit_release_sbase:
 	release_region(data->sbase, SP_IOMEM_LEN);
 exit_release_ebase:
 	release_region(data->ebase, EHFUNC_IOMEM_LEN);
 exit_release_wbase:
 	release_region(data->wbase, WAKEUP_IOMEM_LEN);
+exit_unregister_device:
+	rc_unregister_device(data->dev);
 exit_free_rc:
 	rc_free_device(data->dev);
 exit_unregister_led:
-- 
1.7.10