| Message ID | 20190305095847.21428-7-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various core and virtual driver fixes | expand |
Hi Hans, Thank you for the patch. On Tue, Mar 05, 2019 at 10:58:44AM +0100, hverkuil-cisco@xs4all.nl wrote: > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > If the subdevice created a device node, then the v4l2_subdev cannot > be freed until the last user of the device node closes it. > > This means that we need a release() callback in v4l2_subdev_internal_ops > that is called from the video_device release function so the subdevice > driver can postpone freeing memory until the that callback is called. > > If no video device node was created then the release callback can > be called immediately when the subdev is unregistered. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/v4l2-core/v4l2-device.c | 19 ++++++++++++++----- > include/media/v4l2-subdev.h | 3 +++ > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c > index e0ddb9a52bd1..7cca0de1b730 100644 > --- a/drivers/media/v4l2-core/v4l2-device.c > +++ b/drivers/media/v4l2-core/v4l2-device.c > @@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, > } > EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); > > +static void v4l2_subdev_release(struct v4l2_subdev *sd) > +{ > + struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL; > + > + if (sd->internal_ops && sd->internal_ops->release) > + sd->internal_ops->release(sd); > + module_put(owner); > +} > + > static void v4l2_device_release_subdev_node(struct video_device *vdev) > { > - struct v4l2_subdev *sd = video_get_drvdata(vdev); > - sd->devnode = NULL; > + v4l2_subdev_release(video_get_drvdata(vdev)); > kfree(vdev); > } > > @@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) > media_device_unregister_entity(&sd->entity); > } > #endif > - video_unregister_device(sd->devnode); > - if (!sd->owner_v4l2_dev) > - module_put(sd->owner); > + if (sd->devnode) > + video_unregister_device(sd->devnode); > + else > + v4l2_subdev_release(sd); > } Don't we also need to handle the error path in v4l2_device_register_subdev_nodes() ? > EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 349e1c18cf48..2f2d1c8947e6 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -757,6 +757,8 @@ struct v4l2_subdev_ops { > * > * @close: called when the subdev device node is closed. > * > + * @release: called when the subdev device node is released. > + * I think this should be expanded a bit. First of all, we should mention what happens when the subdev doesn't have a device node, and then we should also explain what drivers are supposed to do in this operation. At what point do you think we should add a WARN_ON(!sd->internal_ops || !sd->internal_ops->release) ? I expect we'll need to refcount the subdev structure, with the video_device only having one of the multiple references to the subdev, but that can be implemented later. Overall this moves us in the right direction, thank you for your work. > * .. note:: > * Never call this from drivers, only the v4l2 framework can call > * these ops. > @@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops { > void (*unregistered)(struct v4l2_subdev *sd); > int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh); > int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh); > + void (*release)(struct v4l2_subdev *sd); > }; > > #define V4L2_SUBDEV_NAME_SIZE 32
On 3/5/19 8:46 PM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Tue, Mar 05, 2019 at 10:58:44AM +0100, hverkuil-cisco@xs4all.nl wrote: >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> If the subdevice created a device node, then the v4l2_subdev cannot >> be freed until the last user of the device node closes it. >> >> This means that we need a release() callback in v4l2_subdev_internal_ops >> that is called from the video_device release function so the subdevice >> driver can postpone freeing memory until the that callback is called. >> >> If no video device node was created then the release callback can >> be called immediately when the subdev is unregistered. >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/media/v4l2-core/v4l2-device.c | 19 ++++++++++++++----- >> include/media/v4l2-subdev.h | 3 +++ >> 2 files changed, 17 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c >> index e0ddb9a52bd1..7cca0de1b730 100644 >> --- a/drivers/media/v4l2-core/v4l2-device.c >> +++ b/drivers/media/v4l2-core/v4l2-device.c >> @@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, >> } >> EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); >> >> +static void v4l2_subdev_release(struct v4l2_subdev *sd) >> +{ >> + struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL; >> + >> + if (sd->internal_ops && sd->internal_ops->release) >> + sd->internal_ops->release(sd); >> + module_put(owner); >> +} >> + >> static void v4l2_device_release_subdev_node(struct video_device *vdev) >> { >> - struct v4l2_subdev *sd = video_get_drvdata(vdev); >> - sd->devnode = NULL; >> + v4l2_subdev_release(video_get_drvdata(vdev)); >> kfree(vdev); >> } >> >> @@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) >> media_device_unregister_entity(&sd->entity); >> } >> #endif >> - video_unregister_device(sd->devnode); >> - if (!sd->owner_v4l2_dev) >> - module_put(sd->owner); >> + if (sd->devnode) >> + video_unregister_device(sd->devnode); >> + else >> + v4l2_subdev_release(sd); >> } > > Don't we also need to handle the error path in > v4l2_device_register_subdev_nodes() ? No, in the error path of v4l2_device_register_subdev_nodes() the registered video devices are simply unregistered, and so they will do the clean up via v4l2_device_release_subdev_node(). Nothing changes there. > >> EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index 349e1c18cf48..2f2d1c8947e6 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -757,6 +757,8 @@ struct v4l2_subdev_ops { >> * >> * @close: called when the subdev device node is closed. >> * >> + * @release: called when the subdev device node is released. >> + * > > I think this should be expanded a bit. First of all, we should mention > what happens when the subdev doesn't have a device node, and then we > should also explain what drivers are supposed to do in this operation. I'll extend this a bit. > At what point do you think we should add a WARN_ON(!sd->internal_ops || > !sd->internal_ops->release) ? I wondered about that myself. I think the first step is to make the regression test for the virtual drivers work flawlessly (i.e. no more compliance errors/warnings/kernel oopses/warnings), then I want to extend the regression test so you can run it for other drivers than the virtual drivers. After that I plan on asking the maintainers of the drivers that use the MC to run the test and see if they hit this issue (they likely will). Once enough of those drivers have been fixed we can add this warning. That was the long answer, the short answer is: hopefully by the end of this year :-) > I expect we'll need to refcount the subdev structure, with the > video_device only having one of the multiple references to the subdev, > but that can be implemented later. Overall this moves us in the right > direction, thank you for your work. Yeah, I need to do more testing for this. One test that isn't in the regression test is unbinding subdevs, but not the main bridge device. If refcounting is really necessary, then such a test should uncover this. It's getting quite difficult to understand all the dependencies. > >> * .. note:: >> * Never call this from drivers, only the v4l2 framework can call >> * these ops. >> @@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops { >> void (*unregistered)(struct v4l2_subdev *sd); >> int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh); >> int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh); >> + void (*release)(struct v4l2_subdev *sd); >> }; >> >> #define V4L2_SUBDEV_NAME_SIZE 32 > Regards, Hans
Hi Hans, On Thu, Mar 07, 2019 at 09:54:53AM +0100, Hans Verkuil wrote: > On 3/5/19 8:46 PM, Laurent Pinchart wrote: > > On Tue, Mar 05, 2019 at 10:58:44AM +0100, hverkuil-cisco@xs4all.nl wrote: > >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> > >> If the subdevice created a device node, then the v4l2_subdev cannot > >> be freed until the last user of the device node closes it. > >> > >> This means that we need a release() callback in v4l2_subdev_internal_ops > >> that is called from the video_device release function so the subdevice > >> driver can postpone freeing memory until the that callback is called. > >> > >> If no video device node was created then the release callback can > >> be called immediately when the subdev is unregistered. > >> > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> --- > >> drivers/media/v4l2-core/v4l2-device.c | 19 ++++++++++++++----- > >> include/media/v4l2-subdev.h | 3 +++ > >> 2 files changed, 17 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c > >> index e0ddb9a52bd1..7cca0de1b730 100644 > >> --- a/drivers/media/v4l2-core/v4l2-device.c > >> +++ b/drivers/media/v4l2-core/v4l2-device.c > >> @@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, > >> } > >> EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); > >> > >> +static void v4l2_subdev_release(struct v4l2_subdev *sd) > >> +{ > >> + struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL; > >> + > >> + if (sd->internal_ops && sd->internal_ops->release) > >> + sd->internal_ops->release(sd); > >> + module_put(owner); > >> +} > >> + > >> static void v4l2_device_release_subdev_node(struct video_device *vdev) > >> { > >> - struct v4l2_subdev *sd = video_get_drvdata(vdev); > >> - sd->devnode = NULL; > >> + v4l2_subdev_release(video_get_drvdata(vdev)); > >> kfree(vdev); > >> } > >> > >> @@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) > >> media_device_unregister_entity(&sd->entity); > >> } > >> #endif > >> - video_unregister_device(sd->devnode); > >> - if (!sd->owner_v4l2_dev) > >> - module_put(sd->owner); > >> + if (sd->devnode) > >> + video_unregister_device(sd->devnode); > >> + else > >> + v4l2_subdev_release(sd); > >> } > > > > Don't we also need to handle the error path in > > v4l2_device_register_subdev_nodes() ? > > No, in the error path of v4l2_device_register_subdev_nodes() the registered > video devices are simply unregistered, and so they will do the clean up > via v4l2_device_release_subdev_node(). Nothing changes there. You're right, my bad. In that case, > >> EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); > >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > >> index 349e1c18cf48..2f2d1c8947e6 100644 > >> --- a/include/media/v4l2-subdev.h > >> +++ b/include/media/v4l2-subdev.h > >> @@ -757,6 +757,8 @@ struct v4l2_subdev_ops { > >> * > >> * @close: called when the subdev device node is closed. > >> * > >> + * @release: called when the subdev device node is released. > >> + * > > > > I think this should be expanded a bit. First of all, we should mention > > what happens when the subdev doesn't have a device node, and then we > > should also explain what drivers are supposed to do in this operation. > > I'll extend this a bit. With the documentation expanded, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > At what point do you think we should add a WARN_ON(!sd->internal_ops || > > !sd->internal_ops->release) ? > > I wondered about that myself. I think the first step is to make the > regression test for the virtual drivers work flawlessly (i.e. no more > compliance errors/warnings/kernel oopses/warnings), then I want to extend > the regression test so you can run it for other drivers than the virtual > drivers. > > After that I plan on asking the maintainers of the drivers that use the MC > to run the test and see if they hit this issue (they likely will). Once > enough of those drivers have been fixed we can add this warning. > > That was the long answer, the short answer is: hopefully by the end of this year :-) :-) The plan sounds good to me. > > I expect we'll need to refcount the subdev structure, with the > > video_device only having one of the multiple references to the subdev, > > but that can be implemented later. Overall this moves us in the right > > direction, thank you for your work. > > Yeah, I need to do more testing for this. One test that isn't in the regression > test is unbinding subdevs, but not the main bridge device. That will be another big can of worms :-S Another issue that needs to be tackled is the race between userspace and unbind. Once the unbind function returns access to the device isn't allowed anymore, so we need to ensure that we block unbind until all userspace calls return, and that we block new ones once unbind has started (I believe we've discussed this face-to-face with Sakari a couple of years ago). Do you plan to work on that too ? I think it's more important than the subdev partial unbind, as it can really lead to crashes when hot-pluggable devices are removed. > If refcounting is really necessary, then such a test should uncover > this. It's getting quite difficult to understand all the dependencies. > > >> * .. note:: > >> * Never call this from drivers, only the v4l2 framework can call > >> * these ops. > >> @@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops { > >> void (*unregistered)(struct v4l2_subdev *sd); > >> int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh); > >> int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh); > >> + void (*release)(struct v4l2_subdev *sd); > >> }; > >> > >> #define V4L2_SUBDEV_NAME_SIZE 32
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c index e0ddb9a52bd1..7cca0de1b730 100644 --- a/drivers/media/v4l2-core/v4l2-device.c +++ b/drivers/media/v4l2-core/v4l2-device.c @@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, } EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); +static void v4l2_subdev_release(struct v4l2_subdev *sd) +{ + struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL; + + if (sd->internal_ops && sd->internal_ops->release) + sd->internal_ops->release(sd); + module_put(owner); +} + static void v4l2_device_release_subdev_node(struct video_device *vdev) { - struct v4l2_subdev *sd = video_get_drvdata(vdev); - sd->devnode = NULL; + v4l2_subdev_release(video_get_drvdata(vdev)); kfree(vdev); } @@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) media_device_unregister_entity(&sd->entity); } #endif - video_unregister_device(sd->devnode); - if (!sd->owner_v4l2_dev) - module_put(sd->owner); + if (sd->devnode) + video_unregister_device(sd->devnode); + else + v4l2_subdev_release(sd); } EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 349e1c18cf48..2f2d1c8947e6 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -757,6 +757,8 @@ struct v4l2_subdev_ops { * * @close: called when the subdev device node is closed. * + * @release: called when the subdev device node is released. + * * .. note:: * Never call this from drivers, only the v4l2 framework can call * these ops. @@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops { void (*unregistered)(struct v4l2_subdev *sd); int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh); int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh); + void (*release)(struct v4l2_subdev *sd); }; #define V4L2_SUBDEV_NAME_SIZE 32