rc-main: Fix device de-registration logic

PATCH In review Mauro Carvalho Chehab 2 participants 5 messages Jul 29, 2011

Subsystems (2)

Media (V4L/DVB) RC-core / lirc framework

lore

Share & tools

lore

Mauro Carvalho Chehab

author

2011-07-29 05:54 UTC #0 lore raw

PATCH 1/2 rc-main: Fix device de-registration logic

rc unregister logic were deadly broken, preventing some drivers to be removed. Among the broken things, rc_dev_uevent() is being called during device_del(), causing a data filling on an area that it is not ready anymore.

Also, some drivers have a stop callback defined, that needs to be called before data removal, as it stops data polling.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/rc/rc-main.c +20 ~~-11~~

index 51a23f4..666d4bb 100644
@@ -928,10 +928,6 @@ out:
 
 static void rc_dev_release(struct device *device)
 {
-	struct rc_dev *dev = to_rc_dev(device);
-
-	kfree(dev);
-	module_put(THIS_MODULE);
 }
 
 #define ADD_HOTPLUG_VAR(fmt, val...)					\

@@ -945,6 +941,9 @@ static int rc_dev_uevent(struct device *device, struct kobj_uevent_env *env)
 {
 	struct rc_dev *dev = to_rc_dev(device);
 
+	if (!dev || !dev->input_dev)
+		return -ENODEV;
+
 	if (dev->rc_map.name)
 		ADD_HOTPLUG_VAR("NAME=%s", dev->rc_map.name);
 	if (dev->driver_name)

@@ -1013,10 +1012,16 @@ EXPORT_SYMBOL_GPL(rc_allocate_device);
 
 void rc_free_device(struct rc_dev *dev)
 {
-	if (dev) {
+	if (!dev)
+		return;
+
+	if (dev->input_dev)
 		input_free_device(dev->input_dev);
-		put_device(&dev->dev);
-	}
+
+	put_device(&dev->dev);
+
+	kfree(dev);
+	module_put(THIS_MODULE);
 }
 EXPORT_SYMBOL_GPL(rc_free_device);

@@ -1143,14 +1148,18 @@ void rc_unregister_device(struct rc_dev *dev)
 	if (dev->driver_type == RC_DRIVER_IR_RAW)
 		ir_raw_event_unregister(dev);
 
-	input_unregister_device(dev->input_dev);
-	dev->input_dev = NULL;
-
+	/* Freeing the table should also call the stop callback */
 	ir_free_table(&dev->rc_map);
 	IR_dprintk(1, "Freed keycode table\n");
 
-	device_unregister(&dev->dev);
+	input_unregister_device(dev->input_dev);
+	dev->input_dev = NULL;
+
+	device_del(&dev->dev);
+
+	rc_free_device(dev);
 }
+
 EXPORT_SYMBOL_GPL(rc_unregister_device);
 
 /*

Signature · 1 line

1.7.1

Jarod Wilson

2011-07-29 17:31 UTC #1 lore raw

Quoted 15 lines · Mauro Carvalho Chehab

rc unregister logic were deadly broken, preventing some drivers to be removed. Among the broken things, rc_dev_uevent() is being called during device_del(), causing a data filling on an area that it is not ready anymore.
Also, some drivers have a stop callback defined, that needs to be called before data removal, as it stops data polling.
Signed-off-by Mauro Carvalho Chehab <[email protected]>
drivers/media/rc/rc-main.c +0 -0
index 51a23f4..666d4bb 100644

drivers/media/rc/rc-main.c out: · line 931

Quoted 8 lines

drivers/media/rc/rc-main.c +0 -4

@@ -928,10 +928,6 @@ out:
 
 static void rc_dev_release(struct device *device)
 {
-	struct rc_dev *dev = to_rc_dev(device);
-
-	kfree(dev);
-	module_put(THIS_MODULE);
 }

Since this function become a no-op, does it make sense to just remove it and not set a .release function for static struct device_type rc_dev_type?

Other than that, after reading through the patch several times, along with the resulting rc-main.c and some input code, everything seems to make sense to me. Will do some quick sanity-testing with a few of my various devices before I give an ack though, just to be sure. :)

Signature · 2 lines

Jarod Wilson
[email protected]

Jarod Wilson

2011-07-29 20:30 UTC #2 lore raw

Quoted 18 lines · Jarod Wilson

Quoted 16 lines · Mauro Carvalho Chehab
rc unregister logic were deadly broken, preventing some drivers to be removed. Among the broken things, rc_dev_uevent() is being called during device_del(), causing a data filling on an area that it is not ready anymore.
Also, some drivers have a stop callback defined, that needs to be called before data removal, as it stops data polling.
Signed-off-by Mauro Carvalho Chehab <[email protected]>
drivers/media/rc/rc-main.c +0 -0
index 51a23f4..666d4bb 100644
@@ -928,10 +928,6 @@ out:
 

Quoted 10 lines

drivers/media/rc/rc-main.c out: · line 931
Quoted 7 lines
drivers/media/rc/rc-main.c +0 -4
 static void rc_dev_release(struct device *device)
 {
-	struct rc_dev *dev = to_rc_dev(device);
-
-	kfree(dev);
-	module_put(THIS_MODULE);
 }
Since this function become a no-op, does it make sense to just remove it and not set a .release function for static struct device_type rc_dev_type?

Nope, that leads to this:

[  765.095926] ------------[ cut here ]------------
[  765.098076] WARNING: at /home/jarod/src/linux-ir/drivers/base/core.c:143 device_release+0x73/0x7f()
[  765.100215] Hardware name: empty
[  765.102343] Device 'rc0' does not have a release() function, it is broken and must be fixed.

Which may or not be bogus. But I've got a hanging modprobe -r em28xx-dvb with this change in place. Now to test with it rolled back...

Signature · 2 lines

Jarod Wilson
[email protected]

Mauro Carvalho Chehab

author

2011-07-29 21:25 UTC #3 lore raw

In reply to Jarod Wilson

Em 29-07-2011 14:30, Jarod Wilson escreveu:

Quoted 18 lines · Mauro Carvalho Chehab

Quoted 17 lines
rc unregister logic were deadly broken, preventing some drivers to be removed. Among the broken things, rc_dev_uevent() is being called during device_del(), causing a data filling on an area that it is not ready anymore.
Also, some drivers have a stop callback defined, that needs to be called before data removal, as it stops data polling.
Signed-off-by Mauro Carvalho Chehab <[email protected]>
drivers/media/rc/rc-main.c +0 -0
index 51a23f4..666d4bb 100644
@@ -928,10 +928,6 @@ out:
 

Quoted 10 lines

drivers/media/rc/rc-main.c out: · line 931
Quoted 7 lines
drivers/media/rc/rc-main.c +0 -4
 static void rc_dev_release(struct device *device)
 {
-	struct rc_dev *dev = to_rc_dev(device);
-
-	kfree(dev);
-	module_put(THIS_MODULE);
 }
Since this function become a no-op, does it make sense to just remove it and not set a .release function for static struct device_type rc_dev_type?

As you tested, this function needs to exist... well, other drivers sometimes do the same, by defining it as a no-op function.

Quoted 4 lines

Other than that, after reading through the patch several times, along with the resulting rc-main.c and some input code, everything seems to make sense to me. Will do some quick sanity-testing with a few of my various devices before I give an ack though, just to be sure. :)

Thanks! Yeah, a test with other devices is welcome, as we don't want fix for one and break for the others ;)

The logic there looks simple, but it is, in fact, tricky, especially since drivers may have polling tasks running, and they need to be cancelled before freeing the resources.

Cheers,
Mauro

Quoted 1 line

Jarod Wilson

Acked-by

2011-07-29 21:40 UTC #4 lore raw

Quoted 33 lines · Mauro Carvalho Chehab

Em 29-07-2011 14:30, Jarod Wilson escreveu:
Quoted 18 lines · Mauro Carvalho Chehab
Quoted 17 lines
rc unregister logic were deadly broken, preventing some drivers to be removed. Among the broken things, rc_dev_uevent() is being called during device_del(), causing a data filling on an area that it is not ready anymore.
Also, some drivers have a stop callback defined, that needs to be called before data removal, as it stops data polling.
Signed-off-by Mauro Carvalho Chehab <[email protected]>
drivers/media/rc/rc-main.c +0 -0
index 51a23f4..666d4bb 100644
@@ -928,10 +928,6 @@ out:
 
Quoted 10 lines
drivers/media/rc/rc-main.c out: · line 931
Quoted 7 lines
drivers/media/rc/rc-main.c +0 -4
 static void rc_dev_release(struct device *device)
 {
-	struct rc_dev *dev = to_rc_dev(device);
-
-	kfree(dev);
-	module_put(THIS_MODULE);
 }
Since this function become a no-op, does it make sense to just remove it and not set a .release function for static struct device_type rc_dev_type?
As you tested, this function needs to exist... well, other drivers sometimes do the same, by defining it as a no-op function.

Quoted 7 lines

Quoted 4 lines
Other than that, after reading through the patch several times, along with the resulting rc-main.c and some input code, everything seems to make sense to me. Will do some quick sanity-testing with a few of my various devices before I give an ack though, just to be sure. :)
Thanks! Yeah, a test with other devices is welcome, as we don't want fix for one and break for the others ;)

Done. Checked out mceusb, redrat3 and imon, all show no ill effects.

Quoted 3 lines

The logic there looks simple, but it is, in fact, tricky, especially since drivers may have polling tasks running, and they need to be cancelled before freeing the resources.

Indeed. Took a bit to wrap my head around it all, but I think I got it.

Acked-by Jarod Wilson <[email protected]>

Signature · 2 lines

Jarod Wilson
[email protected]