V4L: fix retval in vivi driver for more than one device
PATCH 1/1 V4L: fix retval in vivi driver for more than one device
From: Henrik Kretzschmar <[email protected]>
The variable ret should be set for each device to -ENOMEM, not only the first.
drivers/media/video/vivi.c +0 -0
@@ -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;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.
Quoted 26 lines · Henne
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.drivers/media/video/vivi.c +2
-1@@ -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 ;
Signature · 4 lines
Cheers, Mauro Carvalho Chehab http://linuxtv.org [email protected]
Quoted 27 lines · 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.ccode: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_devis 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.
Quoted 2 lines
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
Quoted 25 lines
Cheers,
Mauro.Quoted 20 lines · Henne
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.(quoted diff) +0
-0
b/drivers/media/video/vivi.cindex 3518af0..d739b59 100644drivers/media/video/vivi.c +1
-1@@ -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;
Quoted 5 lines
Quoted 5 lines
drivers/media/video/vivi.c +1
-0for (i =0 ; i < n_devs; i++) { + ret = -ENOMEM ; dev =kzalloc (sizeof (*dev),GFP_KERNEL );if (NULL == dev)break;
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
Quoted 7 lines
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.