Message-ID: <20080515235102.756407d3@gaivota>
Date:	Thu, 15 May 2008 23:51:02 -0300
From:	Mauro Carvalho Chehab <mchehab@...radead.org>
To:	Greg KH <greg@...ah.com>
Cc:	v4l-dvb-maintainer@...uxtv.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, video4linux-list@...hat.com,
	Dean Anderson <dean@...soray.com>
Subject: Re: [PATCH] USB: add Sensoray 2255 v4l driver

Hi Greg and Dean,

In general, the driver seems sane and almost ready for their kernel inclusion.
However, there were several videobuf changes affecting mostly the users of
videobuf-vmalloc during the last kernel development cycle, to fix lock issues.
The code should be reviewed at the light of the latest version, and tested. The
good news is that the videobuf handler will be simpler, but a lock is now required.

I have other comments to help improving the driver quality.

Btw, I noticed the lack of Dean's SOB. Is this intentional?

Cheers,
Mauro.

On Wed, 14 May 2008 13:59:27 -0700
Greg KH <greg@...ah.com> wrote:

> From: Dean Anderson <dean@...soray.com>
> 
> This driver adds support for the Sensoray 2255 devices.
> 
> It was primarily developed by Dean Anderson with only a little bit of
> guidance and cleanup by Greg.

> +#define DIR_IN			0
> +#define DIR_OUT			1

It seems a little dangerous to use just DIR_IN/DIR_OUT, since a future change
on a kernel header could cause namespace conflict. IMO, the better is to rename
those macros to something more specific, like S2255_DIR_IN.

The same comment is also true to struct and other namespace definitions, like
"framei", "bufferi", etc.

> +
> +/* buffer for one video frame */
> +struct s2255_buffer {
> +	/* common v4l buffer stuff -- must be first */
> +	struct videobuf_buffer vb;
> +	const struct s2255_fmt *fmt;
> +	/* future use */
> +	int reserved[32];

Why to reserve 32 integers here? While this may have some sense at userspace
API, I can't see any reason to use this internally.

> +#define S2255_NORMS		(V4L2_STD_PAL_B | V4L2_STD_NTSC_M)

Just those two norms? Are you sure it doesn't support the other european PAL
variants (PAL/G, PAL/D, PAL/K)?

> +static DEFINE_MUTEX(usb_s2255_open_mutex);

Hmmm... what happens if the user plugs two or more of such devices?

> +/* supported controls */
> +static struct v4l2_queryctrl s2255_qctrl[] = {
> +	{
> +	.id = V4L2_CID_BRIGHTNESS,
> +	.type = V4L2_CTRL_TYPE_INTEGER,
> +	.name = "Brightness",
> +	.minimum = -127,
> +	.maximum = 128,
> +	.step = 1,
> +	.default_value = 0,
> +	.flags = 0,
> +	},
> +	{

This seems to be violating CodingStyle. It should be something like:
	{
		.id = foo,
<snip/>
	}, {

The same applies also to other structs. The better is to check the patch with
checkpatch.pl, since other violations might be present.

> +
> +/*
> + * convert from YUV(YCrCb) to RGB
> + * 65536 R = 76533(Y-16) + 104936 * (Cr-128)
> + * 65536 G = 76533(Y-16) - 53451(Cr-128) - 25703(Cb -128)
> + * 65536 B = 76533(Y-16) + 132677(Cb-128)
> + */

As already discussed, the driver should support only the video standards
supported by the hardware. colorspace conversion can happen at userspace apps.

> +/* this loads the firmware asynchronously.
> +   Originally this was done synchroously in probe.
> +   But it is better to load it asynchronously here than block
> +   inside the probe function. Blocking inside probe affects boot time.
> +   FW loading is triggered by the timer in the probe function
> +*/

I saw another discussion about this.

I like the idea of loading firmware asynchronously, provided that you block at
open() or at ioctl(), if the user requested to open before having the firmware
loaded.

> +static int restart_video_queue(struct s2255_dmaqueue *dma_q)

This is probably obsolete. We've cleaned up those code. The better is to take a
look at videobuf implementation at em28xx-video, where some lock bugs were fixed and the
code were simplified. I don't have your hardware to test, but I suspect that
the driver suffers some bugs at videobuf implementation that we've removed recently.

> +/* ------------------------------------------------------------------
> +   IOCTL vidioc handling
> +   ------------------------------------------------------------------*/
> +static int vidioc_querycap(struct file *file, void *priv,
> +			   struct v4l2_capability *cap)
> +{
> +	strcpy(cap->driver, "s2255");
> +	strcpy(cap->card, "s2255");
> +	cap->version = S2255_VERSION;
> +	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	return 0;
> +}

Better to use "strlcpy" instead of just "strcpy", to avoid the risk of buffer
overflow. Ok, currently, there's no risk, but, if tomorrow someone changes
something here, the bug will emerge.

Also, please fill bus info, with something like:

	strlcpy(cap->bus_info, dev->udev->dev.bus_id, sizeof(cap->bus_info));

This is required for some udev implementations to properly work.

> +/* FIXME: This seems to be generic enough to be at videodev2 */
> +static int vidioc_s_fmt_cap(struct file *file, void *priv,
> +			    struct v4l2_format *f)

The implementation you did is not generic enough for videodev2. I agree that we
may improve the implementation of try_fmt/s_fmt at videobuf2. Feel free to
propose such improvements.
> +#ifdef CONFIG_VIDEO_V4L1_COMPAT
> +static int vidiocgmbuf(struct file *file, void *priv, struct video_mbuf *mbuf)

There are now a videobuf method to implement vidiocgmbuf. The better is to use
it, instead of duplicating the code.

> +#define EP_NUM_CONFIG 2

This define here sounds strange. I would move it to the beginning at the
driver. Better to write a short comment about this magic number.

> +	s2255_stop_acquire(dev, fh->channel);
> +	/* give time for last frame to be received/flush in the usb
> +	   receive pipe*/
> +	msleep(50);

The sleep above seems a hack to my eyes. The better is to protect with a
spinlock. The current code, at vivi and em28xx already prevents such issues (at
least, it went fine at the tests we did).

> +	res = videobuf_streamoff(&fh->vb_vidq);
> +	res_free(dev, fh);
> +	/* expire the queue timer now in case still active */
> +	mod_timer(&dmaq->timeout, jiffies + HZ);

Also, the timeout is obsolete

> +	return res;
> +}
> +	if (*i == V4L2_STD_NTSC_M) {
> +		dprintk(4, "vidioc_s_std NTSC\n");
> +		mode->format = FORMAT_NTSC;

No. NTSC/M is just one variant of NTSC, used on US/Canada. There are other
variants at Asia, in Korea and Japan. The difference is at the audio carrier.

> +	} else if (*i == V4L2_STD_PAL_B) {
> +		dprintk(4, "vidioc_s_std PAL\n");

Also, PAL/B is just one variant. 

A proper code would be to do this:
	if (*i & V4L2_STD_PAL)

(notice that the equal operator were replaced by the logical operation _AND_.
This covers all european PAL. However, this mask doesn't cover PAL/N, PAL/Nc
and PAL/M.

If all you need is to check if the video mode is 50Hz/60Hz, the proper code would be, instead:

	if (*i & V4L2_STD_525_60)
		/* some logic for NTSC/M, PAL/M and PAL/60 */
	else
		/* some logic for the other PAL variations and SECAM */

> +/* Sensoray 2255 is a multiple channel capture device.
> +   It does not have a "crossbar" of inputs.
> +   We use one V4L device per channel. The user must
> +   be aware that certain combinations are not allowed.
> +   For instance, you cannot do full FPS on more than 2 channels(2 videodevs)
> +   at once in color(you can do full fps on 4 channels with greyscale.
> +*/

The invalid combinations should be tested, to avoid OOPSes and other bad behaviours.

> +			/* update the mode to the corresponding value */
> +			if (ctrl->id == V4L2_CID_BRIGHTNESS)
> +				mode->bright = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_CONTRAST)
> +				mode->contrast = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_HUE)
> +				mode->hue = ctrl->value;
> +			else if (ctrl->id == V4L2_CID_SATURATION)
> +				mode->saturation = ctrl->value;

You have just a few controls. However, the better would be to use a switch(),
since the compiler can reorder the operations to improve the code speed and
size.

> +	if (dev->fw_data->fw_state == FWSTATE_FAILED) {
> +		err("2255 firmware wasn't loaded\n");
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -ENODEV;
> +	}

It seems too hard to return ENODEV. Couldn't the driver try again to load the
firmware here?

> +	if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
> +		/* give 1 second for firmware to load in case
> +		   driver loaded and then device immediately opened */
> +		msleep(1000);

Argh. I would use a wait_event_timeout() instead, and use a higher timeout value. 

Also, it seems that fw_state needs to be atomic.

> +		if (dev->fw_data->fw_state == FWSTATE_NOTLOADED) {
> +			err("2255 firmware loading stalled\n");
> +			mutex_unlock(&usb_s2255_open_mutex);
> +			return -EAGAIN;

Hmm... I'm in doubt if we should return, instead, -EBUSY.

> +	/* if first open after firmware loaded, release the firmware */
> +	if (dev->fw_data->fw) {
> +		release_firmware(dev->fw_data->fw);
> +		dev->fw_data->fw = NULL;
> +	}

This seems weird. Why to release the firmware?

> +
> +	dev->users[cur_channel]++;
> +
> +	if (dev->users[cur_channel] > 1) {
> +		dev->users[cur_channel]--;
> +		dev_err(&dev->udev->dev, "one user at a time\n");
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -EAGAIN;

In this case, it should return -EBUSY.

> +	}
> +
> +	dprintk(1, "open minor=%d type=%s users=%d\n",
> +		minor, v4l2_type_names[type], dev->users[cur_channel]);
> +
> +	/* allocate + initialize per filehandle data */
> +	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
> +	if (NULL == fh) {
> +		dev->users[cur_channel]--;
> +		mutex_unlock(&usb_s2255_open_mutex);
> +		return -ENOMEM;
> +	}


Instead of incrementing the usage, printing the open message just to discover
that you don't have enough memory, I would move the users increment and the
dprintk to happen after the allocation of fh.

> +	videobuf_queue_vmalloc_init(&fh->vb_vidq, &s2255_video_qops,
> +				    NULL, NULL,
> +				    fh->type,
> +				    V4L2_FIELD_INTERLACED,
> +				    sizeof(struct s2255_buffer), fh);

You'll need to define a spinlock for videoqueue, otherwise the kernel will now
complain. Please test your code against the latest development version (or the
latest code at mainstream).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists