Hi Hans, On Wednesday 27 March 2013 09:41:33 Hans Verkuil wrote: > On Wed March 27 2013 02:11:23 Laurent Pinchart wrote: > > On Monday 18 March 2013 17:38:16 Hans Verkuil wrote: > > > From: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > > > > > Simplify the debugging ioctls by creating the VIDIOC_DBG_G_CHIP_NAME > > > ioctl. This will eventually replace VIDIOC_DBG_G_CHIP_IDENT. Chip > > > matching is done by the name or index of subdevices or an index to a > > > bridge chip. Most of this can all be done automatically, so most drivers > > > just need to provide get/set register ops. > > > > > > In particular, it is now possible to get/set subdev registers without > > > requiring assistance of the bridge driver. > > > > My biggest question is why don't we use the media controller API to get > > the information provided by this new ioctl ? > > Because the media controller is implemented by only a handful of drivers, > and this debug API is used by many more drivers. Shouldn't we then fix that instead of adding a new ioctl ? > So I don't really see how this would be feasible today. > > > > Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > > --- > > > > > > drivers/media/v4l2-core/v4l2-common.c | 5 +- > > > drivers/media/v4l2-core/v4l2-dev.c | 5 +- > > > drivers/media/v4l2-core/v4l2-ioctl.c | 115 ++++++++++++++++++++++++-- > > > include/media/v4l2-ioctl.h | 3 + > > > include/uapi/linux/videodev2.h | 34 +++++++--- > > > 5 files changed, 146 insertions(+), 16 deletions(-) > > > > [snip] > > > > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c > > > b/drivers/media/v4l2-core/v4l2-dev.c index de1e9ab..c0c651d 100644 > > > --- a/drivers/media/v4l2-core/v4l2-dev.c > > > +++ b/drivers/media/v4l2-core/v4l2-dev.c > > > @@ -591,9 +591,10 @@ static void determine_valid_ioctls(struct > > > video_device > > > > [snip] > > > > > +static int v4l_dbg_g_chip_name(const struct v4l2_ioctl_ops *ops, > > > + struct file *file, void *fh, void *arg) > > > > As this is a debug ioctl that should never be used in application, I would > > like to guard the whole implementation with #ifdef CONFIG_VIDEO_ADV_DEBUG. > > This will make sure that no applications will abuse it, as it won't be > > available in distro kernels. > > Agreed. I'll make that change. Actually, it's not so much userspace abuse I > am worried about, but kernel space abuse. I've found several drivers where > the bridge driver calls g_chip_ident to get information about subdevice > drivers. That was never intended and it complicates my work of removing > g_chip_ident. By putting chip_name under ADV_DEBUG we can avoid similar > abuse in the future. -- 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
