Subject: [PATCH] V4L: fix retval in vivi driver for more than one device

From: Henrik Kretzschmar <[email protected]>
Signed-off-by: Henrik Kretzschmar <[email protected]>

The variable ret should be set for each device to -ENOMEM, not only the first.

diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 3518af0..d739b59 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -1106,11 +1106,12 @@ static struct video_device vivi_template = {

static int __init vivi_init(void)
{
- int ret = -ENOMEM, i;
+ int ret, i;
struct vivi_dev *dev;
struct video_device *vfd;

for (i = 0; i < n_devs; i++) {
+ ret = -ENOMEM;
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (NULL == dev)
break;


2008-08-24 14:06:59

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH] V4L: fix retval in vivi driver for more than one device

This patch is not 100% OK, for some reasons:

1) since ret won't be initialized anymore, it will generate compilation
warnings;

2) with the current code, if you ask to allocate, let's say, 3 virtual
drivers, and the second or the third fails, you'll still have one
allocated. With your change, you'll de-allocate even the ones that
succeeded. IMO, the better is to allow using the ones that succeeded.

That's said, I can see other issues on the current vivi.c code:

1) what happens if someone uses n_dev=0? It will return a wrong error
code.

2) there are still some cases where the driver allocates less devices than
requested, but it will fail to register, and all stuff will be
de-allocated;

3) what if n_dev is a big number? Currently, i think videodev will allow
you to register up to 32 video devices of this type, even if you have
memory for more, due to some limits inside videodev, and due to the
number of minors allocated for V4L. IMO, the driver should allocate up
to the maximum allowed devices by videodev, and let users use they.

Anyway, thanks for your patch. For sure we need to do some fixes here.
I'll try to address all those stuff into a patch.

Cheers,
Mauro.


On Fri, 22 Aug 2008, Henne wrote:

> From: Henrik Kretzschmar <[email protected]>
> Signed-off-by: Henrik Kretzschmar <[email protected]>
>
> The variable ret should be set for each device to -ENOMEM, not only the
> first.
>
> diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> index 3518af0..d739b59 100644
> --- a/drivers/media/video/vivi.c
> +++ b/drivers/media/video/vivi.c
> @@ -1106,11 +1106,12 @@ static struct video_device vivi_template = {
>
> static int __init vivi_init(void)
> {
> - int ret = -ENOMEM, i;
> + int ret, i;
> struct vivi_dev *dev;
> struct video_device *vfd;
>
> for (i = 0; i < n_devs; i++) {
> + ret = -ENOMEM;
> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> if (NULL == dev)
> break;
>
>
>

--
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
[email protected]

2008-08-24 14:35:05

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH] V4L: fix retval in vivi driver for more than one device

On Sunday 24 August 2008 16:00:45 Mauro Carvalho Chehab wrote:
> This patch is not 100% OK, for some reasons:
>
> 1) since ret won't be initialized anymore, it will generate
> compilation warnings;
>
> 2) with the current code, if you ask to allocate, let's say, 3
> virtual drivers, and the second or the third fails, you'll still have
> one allocated. With your change, you'll de-allocate even the ones
> that succeeded. IMO, the better is to allow using the ones that
> succeeded.
>
> That's said, I can see other issues on the current vivi.c code:
>
> 1) what happens if someone uses n_dev=0? It will return a wrong error
> code.
>
> 2) there are still some cases where the driver allocates less devices
> than requested, but it will fail to register, and all stuff will be
> de-allocated;
>
> 3) what if n_dev is a big number? Currently, i think videodev will
> allow you to register up to 32 video devices of this type, even if
> you have memory for more, due to some limits inside videodev, and due
> to the number of minors allocated for V4L. IMO, the driver should
> allocate up to the maximum allowed devices by videodev, and let users
> use they.
>
> Anyway, thanks for your patch. For sure we need to do some fixes
> here. I'll try to address all those stuff into a patch.

Hi Mauro,

Please note that I am working on creating a much improved V4L
infrastructure to take care of such things. It's simply nuts that v4l
drivers need to put in all the plumbing just to be able to have
multiple instances.

In particular, all these limitations on the number of instances should
disappear (unless you run out of minors).

Regards,

Hans

>
> Cheers,
> Mauro.
>
> On Fri, 22 Aug 2008, Henne wrote:
> > From: Henrik Kretzschmar <[email protected]>
> > Signed-off-by: Henrik Kretzschmar <[email protected]>
> >
> > The variable ret should be set for each device to -ENOMEM, not only
> > the first.
> >
> > diff --git a/drivers/media/video/vivi.c
> > b/drivers/media/video/vivi.c index 3518af0..d739b59 100644
> > --- a/drivers/media/video/vivi.c
> > +++ b/drivers/media/video/vivi.c
> > @@ -1106,11 +1106,12 @@ static struct video_device vivi_template =
> > {
> >
> > static int __init vivi_init(void)
> > {
> > - int ret = -ENOMEM, i;
> > + int ret, i;
> > struct vivi_dev *dev;
> > struct video_device *vfd;
> >
> > for (i = 0; i < n_devs; i++) {
> > + ret = -ENOMEM;
> > dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > if (NULL == dev)
> > break;

2008-08-24 15:03:57

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH] V4L: fix retval in vivi driver for more than one device

> Please note that I am working on creating a much improved V4L
> infrastructure to take care of such things. It's simply nuts that v4l
> drivers need to put in all the plumbing just to be able to have
> multiple instances.
>
> In particular, all these limitations on the number of instances should
> disappear (unless you run out of minors).

If you use vivi with a large number of devs (for example, 128), you'll run
out of minors, since we currently have only 64 for video grabber. Due to
the videodev limits, in fact, we can allocate "only" 32 virtual devices
per driver.

Even with the changes on V4L infrastructure, vivi driver still doesn't do
the right thing if you use a large number of devs.

I'm currently fixing the "vivi" driver and adding some notes on it about
the current limits.

Cheers,
Mauro.