On Monday, June 13, 2011 11:41:42 Laurent Pinchart wrote: > Hi Hans (and Hans), > > On Saturday 11 June 2011 11:16:10 Laurent Pinchart wrote: > > On Thursday 09 June 2011 13:22:03 Hans de Goede wrote: > > > Hi, > > > > > > When I unplug a uvc camera *while streaming* I get: > > > > > > [15824.809741] BUG: unable to handle kernel NULL pointer dereference at > > > (null) > > > > [snip] > > > > > I've not tested if this also impacts 3.0!! > > > > It probably does. Thanks for the report. I'll fix it. > > It does. Fixing the problem turns to be more complex than expected. > > The crash is caused by media entities life time management issues. > > Entities associated with video device nodes are unregistered in > video_unregister_device(). This removes the entity from its parent's entities > list, and sets the entity's parent to NULL. > > Entities also get/put references to their parent's module through > media_entity_get() and media_entity_put(). Those functions are called in the > open and release handlers of video device nodes and subdev device nodes. The > reason behind this is to avoid a parent module from being removed while a > subdev is opened, as closing a subdev can call to the parent's module through > board code. > > When a UVC device is unplugged while streaming, the uvcvideo driver will call > video_unregister_device() in the disconnect handler. This will in turn call > media_device_unregister_entity() which sets the entity's parent to NULL. When > the user then closes open video device nodes, v4l2_release() calls > media_entity_put() which tries to dereference entity->parent, and oopses. > > I've tried to move the media_device_unregister_entity() call from > video_unregister_device() to v4l2_device_release() (called when the last > reference to the video device is released). media_entity_put() is then called > before the entity is unregistered, but that results in a different oops: as > this happens after the USB disconnect callback is called, entity->parent->dev- > >driver is now NULL, and trying to access entity->parent->dev->driver->owner > to decrement the module use count oopses. > > One possible workaround is to remove media_entity_get()/media_entity_put() > calls from v4l2-dev.c. As the original purpose of those functions was to avoid > a parent module from being removed while still accessible through board code, > and all existing MC-enabled drivers register video device nodes with the owner > equal to the entity's parent's module, we can safely do it. > > I'd rather implement a proper solution though, but that's not straightforward. > We short-circuit the kernel reference management by going through board code. > There's something fundamentally wrong in the way we manage subdevs and > device/module reference counts. I'm not sure where the proper fix should go to > though. Hmm. Tricky. media_device_unregister_entity() is definitely called in the wrong place. It should move to v4l2_device_release(). I wonder why media_entity_get/put are called in v4l2_open and v4l2_release. That should be in __video_register_device and v4l2_device_release as far as I can see. Just closing a device node shouldn't be cause for changing the module's refcount, that device node registration and the release after unregistration. I also wonder whether instead of refcounting the module in media_entity_get/put you should refcount the device (entity->parent->dev). I have to admit that I don't quite understand why the USB disconnect zeroes entity->parent->dev->driver. I hope this gives some ideas... Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
