diff mbox

[1/2,media] rc-main: Fix device de-registration logic

Message ID 20110729025356.28cc99e8@redhat.com (mailing list archive)
State Accepted
Headers show

Commit Message

Mauro Carvalho Chehab July 29, 2011, 5:53 a.m. UTC 
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 <mchehab@redhat.com>

Comments

Jarod Wilson July 29, 2011, 5:30 p.m. UTC | #1 
On Jul 29, 2011, at 1:53 AM, Mauro Carvalho Chehab wrote:

> 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 <mchehab@redhat.com>
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 51a23f4..666d4bb 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -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. :)
Jarod Wilson July 29, 2011, 8:30 p.m. UTC | #2 
On Jul 29, 2011, at 1:30 PM, Jarod Wilson wrote:

> On Jul 29, 2011, at 1:53 AM, Mauro Carvalho Chehab wrote:
> 
>> 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 <mchehab@redhat.com>
>> 
>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index 51a23f4..666d4bb 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -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?

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...
Mauro Carvalho Chehab July 29, 2011, 9:25 p.m. UTC | #3 
Em 29-07-2011 14:30, Jarod Wilson escreveu:
> On Jul 29, 2011, at 1:53 AM, Mauro Carvalho Chehab wrote:
> 
>> 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 <mchehab@redhat.com>
>>
>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>> index 51a23f4..666d4bb 100644
>> --- a/drivers/media/rc/rc-main.c
>> +++ b/drivers/media/rc/rc-main.c
>> @@ -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?

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

> 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

> 

--
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
Jarod Wilson July 29, 2011, 9:39 p.m. UTC | #4 
On Jul 29, 2011, at 5:25 PM, Mauro Carvalho Chehab wrote:

> Em 29-07-2011 14:30, Jarod Wilson escreveu:
>> On Jul 29, 2011, at 1:53 AM, Mauro Carvalho Chehab wrote:
>> 
>>> 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 <mchehab@redhat.com>
>>> 
>>> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
>>> index 51a23f4..666d4bb 100644
>>> --- a/drivers/media/rc/rc-main.c
>>> +++ b/drivers/media/rc/rc-main.c
>>> @@ -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?
> 
> As you tested, this function needs to exist... well, other drivers sometimes
> do the same, by defining it as a no-op function.
> 
>> 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.


> 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 <jarod@redhat.com>
diff mbox

Patch

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 51a23f4..666d4bb 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -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);
 
 /*