Hi Mauro,
Thanks for the review.
On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
> > Create a device node named subdevX for every registered subdev.
> > As the device node is registered before the subdev core::s_config
> > function is called, return -EGAIN on open until initialization
> > completes.
[snip]
> > diff --git a/drivers/media/video/v4l2-subdev.c
> > b/drivers/media/video/v4l2-subdev.c new file mode 100644
> > index 0000000..a048161
> > --- /dev/null
> > +++ b/drivers/media/video/v4l2-subdev.c
> > @@ -0,0 +1,65 @@
[snip]
> > +static int subdev_open(struct file *file)
> > +{
> > + struct video_device *vdev = video_devdata(file);
> > + struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> > +
> > + if (!sd->initialized)
> > + return -EAGAIN;
>
> Those internal interfaces should not be used on normal
> devices/applications, as none of the existing drivers are tested or
> supposed to properly work if an external program is touching on its
> internal interfaces. So, please add:
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
As Hans pointed out, subdev device nodes should only be created if the subdev
request it explicitly. I'll fix the patch accordingly. Existing subdevs will
not have a device node by default anymore, so the CAP_SYS_ADMIN capability
won't be required (new subdevs that explicitly ask for a device node are
supposed to handle the calls properly, otherwise it's a bit pointless :-)).
> > +
> > + return 0;
> > +}
[snip]
> > +static long subdev_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return video_usercopy(file, cmd, arg, subdev_do_ioctl);
>
> This is a legacy call. Please, don't use it.
What should I use instead then ? I need the functionality of video_usercopy. I
could copy it to v4l2-subdev.c verbatim. As video_ioctl2 shares lots of code
with video_usercopy I think video_usercopy should stay, and video_ioctl2
should use it.
> Also, while the API doc says that only certain ioctls are supported on
> subdev, there's no code here preventing the usage of invalid ioctls. So,
> it is possible to do bad things, like changing the video standard format
> individually on each subdev, creating all sorts of problems.
Invalid (or rather unsupported) ioctls will be routed to the subdev
core::ioctl operation. Formats will not be changed automagically just because
a userspace application issues a VIDIOC_S_FMT ioctl.
As the device node creation will need to be requested explicitly this won't be
an issue anyway.
--
Regards,
Laurent Pinchart
--
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
