Re: [PATCH] zr36067: Debugging cleanups (updated again)

On Thu, 24 May 2007, Jean Delvare wrote:
> >  /*
> >     Number and size of grab buffers for Video 4 Linux
> > @@ -130,21 +132,21 @@ MODULE_PARM_DESC(video_nr, "video device
> >
> >  int v4l_nbufs = 2;
> >  int v4l_bufsize = 128;		/* Everybody should be able to work with this setting */
> > -module_param(v4l_nbufs, int, 0);
> > +module_param(v4l_nbufs, int, 0644);
> >  MODULE_PARM_DESC(v4l_nbufs, "Maximum number of V4L buffers to use");
>
> This one I don't think is safe in its current form. First reason,
> v4l_nbufs is used in VIDIOC_REQBUFS to control user input. If its value
> has changed since the device was opened, I expect problems. To be on

Why?  The buffers aren't allocated until the call to REQBUFS.

> the safe side, I believe that you need to change the code in
> VIDIOC_REQBUFS to use fh->v4l_buffers.num_buffers instead of v4l_nbufs.

That's wrong, fh->v4l_buffers.num_buffers is the number of buffers actually
allocated, not the limit.  One could call REQBUFS to get 2 buffers, then
unmap them and call REQBUFS with a different amount.

> Second reason, there are limit checks done at init time on the value of
> v4l_nbufs (in init_dc10_cards). If the user can change the values
> through sysfs, these checks are bypassed. Thus I would suggest that you
> move the limit checks to zoran_open_init_session, where the value of
> v4l_nbufs is copied to the device-specific structure.

They're also changed from kB to bytes.  I figure someone changing the
values at runtime should know what they're doing.

> Wouldn't it make more sense to print a warning in the logs and default
> to 0 in this case? Also, default_input is an int so checking for
> negative values would be welcome too.

Maybe it would be better to just change the parameter to unsigned.  I could
add a warning, but I don't want to bloat the driver with a lot of warnings no
one is every going to care about.

> Imagine that lock_norm is originally set to 2, and the user switches it
> to 0 between the 1st and 2nd line. The test "lock_norm > 1"  will fail,
> and the "else" statement will be executed, while it corresponds to
> lock_norm = 1 which was never set.
>
> I agree it's not very important in practice, but OTOH this can be easily
> fixed that way:

That won't fix any race conditions.  You have no spinlock or memory barrier
or even made lock_norm volatile.  The compiler can and will re-order the
instructions in a way that might re-introduce a race condition.

Modifying driver parameters while a driver is running is an advanced
feature and people using it should know what they are doing.  It's full of
possible race conditions in every driver.  Bloating and complicating
everything in the kernel to deal with it isn't worth it.

> (As a side note, I'm curious as to why this module parameter exists in
> the first place, it doesn't seem particularly useful, and it gives
> users an easy way to fill the kernel log buffer when used. I couldn't
> find any other media/video driver offering such a parameter.)

My guess is it's a hack for badly written software.

> Very nice. I can confirm this fixes an error I was receiving from
> tvtime. This still isn't enough for tvtime to work with my DC10+, but
> at least this is a move in the right direction. (The remaining error
> is: "videoinput: Can't get tuner info: Invalid argument".)

That's not a problem, the zoran cards don't have tuners.  After my patch to
fix poll, tvtime will work.

Note that tvtime only supports a vertical resolution of 480 for NTSC.  You
need to tell tvtime to use a horizontal resolution that fits in the v4l
buffer, 136 is the max for 128k buffers.

Changing inputs in tvtime still doesn't work.  I think it's a driver bug
from stopping and re-starting capture.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/