Re: [RFCv1 PATCH 19/32] v4l2-dev.c: add debug sysfs entry.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Monday 18 June 2012 13:30:18 Hans Verkuil wrote:
> On Mon June 18 2012 11:48:45 Laurent Pinchart wrote:
> > On Sunday 10 June 2012 12:25:41 Hans Verkuil wrote:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > > 
> > >  drivers/media/video/v4l2-dev.c |   24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/drivers/media/video/v4l2-dev.c
> > > b/drivers/media/video/v4l2-dev.c index 1500208..5c0bb18 100644
> > > --- a/drivers/media/video/v4l2-dev.c
> > > +++ b/drivers/media/video/v4l2-dev.c
> > > @@ -46,6 +46,29 @@ static ssize_t show_index(struct device *cd,
> > > 
> > >  	return sprintf(buf, "%i\n", vdev->index);
> > >  
> > >  }
> > > 
> > > +static ssize_t show_debug(struct device *cd,
> > > +			 struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct video_device *vdev = to_video_device(cd);
> > > +
> > > +	return sprintf(buf, "%i\n", vdev->debug);
> > > +}
> > > +
> > > +static ssize_t set_debug(struct device *cd, struct device_attribute
> > > *attr,
> > > +		   const char *buf, size_t len)
> > > +{
> > > +	struct video_device *vdev = to_video_device(cd);
> > > +	int res = 0;
> > > +	u16 value;
> > > +
> > > +	res = kstrtou16(buf, 0, &value);
> > > +	if (res)
> > > +		return res;
> > > +
> > > +	vdev->debug = value;
> > 
> > Can't this race with the various vdev->debug tests we have in the V4L core
> > ?
> Not really. You may have a short race where you set it, but some core test
> is just reading it and gets the old value. But that's no problem in this
> case. The worst that can happen is that you get one more or one less debug
> message in the log.

You test the debug value several times in the __video_do_ioctl() function. I 
haven't checked in details whether changing the value between the two tests 
could for instance lead to a KERN_CONT print without a previous non-KERN_CONT 
message. That won't crash the machine :-) but it should still be avoided.

> It probably deserves a comment, though.

-- 
Regards,

Laurent Pinchart