[PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test

[PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test

[Roel Kluin]

In vpif_get_std_info(): std_info doesn't need the NULL test, it was already
dereferenced anyway. If std_info->stdid is 0 we could early return -1.

In vpif_probe(): local variable q was only assigned. If we error out with
either last two goto's then j equals VPIF_DISPLAY_MAX_DEVICES. So after the
probe_out: label, k also reaches VPIF_DISPLAY_MAX_DEVICES after the loop. In
the first iteration in the loop after vpif_int_err: a free_irq() can occur
of an element &vpif_obj.dev[VPIF_DISPLAY_MAX_DEVICES]->channel_id which is
outside vpif_obj.dev[] array boundaries.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Or am I mistaken?

diff --git a/drivers/media/video/davinci/vpif_display.c b/drivers/media/video/davinci/vpif_display.c
index dfddef7..8f6605d 100644
--- a/drivers/media/video/davinci/vpif_display.c
+++ b/drivers/media/video/davinci/vpif_display.c
@@ -383,7 +383,7 @@ static int vpif_get_std_info(struct channel_obj *ch)
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
+	if (!std_info->stdid)
 		return -1;
 
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
@@ -1423,7 +1423,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 {
 	struct vpif_subdev_info *subdevdata;
 	struct vpif_display_config *config;
-	int i, j = 0, k, q, m, err = 0;
+	int i, j = 0, k, m, err = 0;
 	struct i2c_adapter *i2c_adap;
 	struct common_obj *common;
 	struct channel_obj *ch;
@@ -1573,10 +1573,12 @@ probe_out:
 		video_device_release(ch->video_dev);
 		ch->video_dev = NULL;
 	}
+	if (k == VPIF_DISPLAY_MAX_DEVICES)
+		k = VPIF_DISPLAY_MAX_DEVICES - 1;
 vpif_int_err:
 	v4l2_device_unregister(&vpif_obj.v4l2_dev);
 	vpif_err("VPIF IRQ request failed\n");
-	for (q = k; k >= 0; k--) {
+	for (; k >= 0; k--) {
 		for (m = i; m >= res->start; m--)
 			free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
 		res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);

RE: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test

[Karicheri, Muralidharan]

Roel,

Thanks for the patch. 

>In vpif_get_std_info(): std_info doesn't need the NULL test, it was already
>dereferenced anyway. If std_info->stdid is 0 we could early return -1.
>
>In vpif_probe(): local variable q was only assigned. If we error out with
>either last two goto's then j equals VPIF_DISPLAY_MAX_DEVICES. So after the
>probe_out: label, k also reaches VPIF_DISPLAY_MAX_DEVICES after the loop.
>In
>the first iteration in the loop after vpif_int_err: a free_irq() can occur
>of an element &vpif_obj.dev[VPIF_DISPLAY_MAX_DEVICES]->channel_id which is
>outside vpif_obj.dev[] array boundaries.
>
>Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>---
>Or am I mistaken?
>
>diff --git a/drivers/media/video/davinci/vpif_display.c
>b/drivers/media/video/davinci/vpif_display.c
>index dfddef7..8f6605d 100644
>--- a/drivers/media/video/davinci/vpif_display.c
>+++ b/drivers/media/video/davinci/vpif_display.c
>@@ -383,7 +383,7 @@ static int vpif_get_std_info(struct channel_obj *ch)
> 	int index;
>
> 	std_info->stdid = vid_ch->stdid;
>-	if (!std_info)
>+	if (!std_info->stdid)
> 		return -1;
>
This is a NACK. We shouldn't check for stdid since the function is supposed
to update std_info. So just remove

if (!std_info)
	return -1;

I am okay with the below change. So please re-submit the patch with the 
above change and my ACK.

Thanks

Murali

> 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
>@@ -1423,7 +1423,7 @@ static __init int vpif_probe(struct platform_device
>*pdev)
> {
> 	struct vpif_subdev_info *subdevdata;
> 	struct vpif_display_config *config;
>-	int i, j = 0, k, q, m, err = 0;
>+	int i, j = 0, k, m, err = 0;
> 	struct i2c_adapter *i2c_adap;
> 	struct common_obj *common;
> 	struct channel_obj *ch;
>@@ -1573,10 +1573,12 @@ probe_out:
> 		video_device_release(ch->video_dev);
> 		ch->video_dev = NULL;
> 	}
>+	if (k == VPIF_DISPLAY_MAX_DEVICES)
>+		k = VPIF_DISPLAY_MAX_DEVICES - 1;
> vpif_int_err:
> 	v4l2_device_unregister(&vpif_obj.v4l2_dev);
> 	vpif_err("VPIF IRQ request failed\n");
>-	for (q = k; k >= 0; k--) {
>+	for (; k >= 0; k--) {
> 		for (m = i; m >= res->start; m--)
> 			free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
> 		res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test

[roel kluin]

>>-      if (!std_info)
>>+      if (!std_info->stdid)
>>               return -1;
>>
> This is a NACK. We shouldn't check for stdid since the function is supposed
> to update std_info. So just remove
>
> if (!std_info)
>        return -1;

I don't see how std_info could get updated. consider the loop in case
std_info->stdid equals 0:

        for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
                config = &ch_params[index];

(config is a local variable)

                if (config->stdid & std_info->stdid) {

This fails for every index if std_info->stdid equals 0.

                        memcpy(std_info, config, sizeof(*config));
                        break;
                }
        }

So we always reach this with index == ARRAY_SIZE(ch_params)

        if (index == ARRAY_SIZE(ch_params))
                return -1;

So we could have returned -1 earlier.

> I am okay with the below change. So please re-submit the patch with the
> above change and my ACK.
>
> Thanks
>
> Murali
>

>>+      if (k == VPIF_DISPLAY_MAX_DEVICES)
>>+              k = VPIF_DISPLAY_MAX_DEVICES - 1;

actually I think this is still not right. shouldn't it be be

k = VPIF_DISPLAY_MAX_DEVICES - 1;

> are you using this driver in your project?

No, I just found this in the code.

Thanks, Roel

RE: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test

[Karicheri, Muralidharan]

>>>-      if (!std_info)
>>>+      if (!std_info->stdid)
>>>               return -1;
>>>
>> This is a NACK. We shouldn't check for stdid since the function is
>supposed
>> to update std_info. So just remove
>>
>> if (!std_info)
>>        return -1;
>
>I don't see how std_info could get updated. consider the loop in case
>std_info->stdid equals 0:

Ok. You are right! The ch_params[] is a table for keeping the information
about different standards supported. For a given stdid in std_info, the function matches the stdid with that in the table and get the corresponding entry.


>> I am okay with the below change. So please re-submit the patch with the
>> above change and my ACK.
>>
>> Thanks
>>
>> Murali
>>
>
>>>+      if (k == VPIF_DISPLAY_MAX_DEVICES)
>>>+              k = VPIF_DISPLAY_MAX_DEVICES - 1;
>
>actually I think this is still not right. shouldn't it be be
>
>k = VPIF_DISPLAY_MAX_DEVICES - 1;

What you mean here? What you suggest here is same as in your patch, right?

Murali
>
>> are you using this driver in your project?
>
>No, I just found this in the code.
>
>Thanks, Roel

Re: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test

[Roel Kluin]

> Ok. You are right! The ch_params[] is a table for keeping the information
> about different standards supported. For a given stdid in std_info, the function matches the stdid with that in the table and get the corresponding entry.

>>>> +      if (k == VPIF_DISPLAY_MAX_DEVICES)
>>>> +              k = VPIF_DISPLAY_MAX_DEVICES - 1;
>>
>> actually I think this is still not right. shouldn't it be be
>>
>> k = VPIF_DISPLAY_MAX_DEVICES - 1;
> 
> What you mean here? What you suggest here is same as in your patch, right?

I think there were many more issues:

The first loop ends when platform_get_resource() returns NULL. Can it occur
that no platform_get_resource() succeeded? I think we should error return if
that happens. Could k grow larger than VPIF_DISPLAY_MAX_DEVICES there?
Should we err out in that case?

In the loop `for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++)' if
video_device_alloc() fails I think we correctly release the devices,
but we have to do more before we reach label vpif_int_err.

As mentioned, we left the first loop with a res of NULL, which is
dereferenced at label vpif_int_err. So we have to get the resource again,
however, k was incremented at the end of that loop as well. Also i used
as index in the second loop as well should point to res->end before going
to label vpif_int_err, to free all requested irqs. All this needs to be
done for later error labels as well, so a new label should be added.

Variable k can't be reused, it is needed to get the resource in case a
error and cleanup is required.

Also in label probe_out a loop with k as index is used, but k is already
an index that is required to get the resource later.

If we reach label vpif_int_err, res shouldn't be NULL, since we
dereference it. Previously we had:

        for (; k >= 0; k--) {
                for (m = i; m >= res->start; m--)
                        free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
                res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);
                m = res->end;
        }

In the last iteration k equals 0, so we call platform_get_resource() with
-1 as a third argument. Since platform_get_resource() uses an unsigned it
is converted to 0xffffffff. platform_get_resource() fails for every index
and returns NULL. A test is lacking and we dereference NULL.

This all occurs at the new label alloc_vid_fail.

The error "VPIF IRQ request failed" should only be displayed when 
request_irq() failed, not in the case of other errors.

Also I changed some indexes, so a few could be removed.

I must admit I did not test this, except with checkpatch.pl, but I think
the issues are real and should be fixed. Do you have comments?

 drivers/media/video/davinci/vpif_display.c |   61 +++++++++++++++++++---------
 1 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/media/video/davinci/vpif_display.c b/drivers/media/video/davinci/vpif_display.c
index dfddef7..ae8ca94 100644
--- a/drivers/media/video/davinci/vpif_display.c
+++ b/drivers/media/video/davinci/vpif_display.c
@@ -383,7 +383,7 @@ static int vpif_get_std_info(struct channel_obj *ch)
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
+	if (!std_info->stdid)
 		return -1;
 
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
@@ -1423,7 +1423,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 {
 	struct vpif_subdev_info *subdevdata;
 	struct vpif_display_config *config;
-	int i, j = 0, k, q, m, err = 0;
+	int i, j, k, err;
 	struct i2c_adapter *i2c_adap;
 	struct common_obj *common;
 	struct channel_obj *ch;
@@ -1452,12 +1452,18 @@ static __init int vpif_probe(struct platform_device *pdev)
 			if (request_irq(i, vpif_channel_isr, IRQF_DISABLED,
 					"DM646x_Display",
 				(void *)(&vpif_obj.dev[k]->channel_id))) {
+				i--;
 				err = -EBUSY;
+				vpif_err("VPIF IRQ request failed\n");
 				goto vpif_int_err;
 			}
 		}
 		k++;
+		if (k >= VPIF_DISPLAY_MAX_DEVICES)
+			break;
 	}
+	if (k == 0)
+		return -ENODEV;
 
 	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
 
@@ -1472,7 +1478,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 				video_device_release(ch->video_dev);
 			}
 			err = -ENOMEM;
-			goto vpif_int_err;
+			goto alloc_vid_fail;
 		}
 
 		/* Initialize field of video device */
@@ -1489,13 +1495,13 @@ static __init int vpif_probe(struct platform_device *pdev)
 		ch->video_dev = vfd;
 	}
 
-	for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) {
-		ch = vpif_obj.dev[j];
+	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
+		ch = vpif_obj.dev[i];
 		/* Initialize field of the channel objects */
 		atomic_set(&ch->usrs, 0);
-		for (k = 0; k < VPIF_NUMOBJECTS; k++) {
-			ch->common[k].numbuffers = 0;
-			common = &ch->common[k];
+		for (j = 0; j < VPIF_NUMOBJECTS; j++) {
+			ch->common[j].numbuffers = 0;
+			common = &ch->common[j];
 			common->io_usrs = 0;
 			common->started = 0;
 			spin_lock_init(&common->irqlock);
@@ -1506,12 +1512,12 @@ static __init int vpif_probe(struct platform_device *pdev)
 			common->ctop_off = common->cbtm_off = 0;
 			common->cur_frm = common->next_frm = NULL;
 			memset(&common->fmt, 0, sizeof(common->fmt));
-			common->numbuffers = config_params.numbuffers[k];
+			common->numbuffers = config_params.numbuffers[j];
 
 		}
 		ch->initialized = 0;
-		ch->channel_id = j;
-		if (j < 2)
+		ch->channel_id = i;
+		if (i < 2)
 			ch->common[VPIF_VIDEO_INDEX].numbuffers =
 			    config_params.numbuffers[ch->channel_id];
 		else
@@ -1529,7 +1535,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 				(int)ch, (int)&ch->video_dev);
 
 		err = video_register_device(ch->video_dev,
-					  VFL_TYPE_GRABBER, (j ? 3 : 2));
+					  VFL_TYPE_GRABBER, (i ? 3 : 2));
 		if (err < 0)
 			goto probe_out;
 
@@ -1567,20 +1573,35 @@ static __init int vpif_probe(struct platform_device *pdev)
 probe_subdev_out:
 	kfree(vpif_obj.sd);
 probe_out:
-	for (k = 0; k < j; k++) {
-		ch = vpif_obj.dev[k];
+	for (j = 0; j < i; j++) {
+		ch = vpif_obj.dev[j];
 		video_unregister_device(ch->video_dev);
 		video_device_release(ch->video_dev);
 		ch->video_dev = NULL;
 	}
+alloc_vid_fail:
+	while (k--) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		if (res != NULL)
+			break;
+		vpif_err("Couldn't get resource %d, irqs not freed.\n", k);
+	}
+	if (res == NULL) {
+		vpif_err("Couldn't get any resource.\n");
+		return err;
+	}
+
+	i = res->end;
 vpif_int_err:
 	v4l2_device_unregister(&vpif_obj.v4l2_dev);
-	vpif_err("VPIF IRQ request failed\n");
-	for (q = k; k >= 0; k--) {
-		for (m = i; m >= res->start; m--)
-			free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
-		res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);
-		m = res->end;
+
+	for (j = i; j >= res->start; j--)
+		free_irq(j, (void *)(&vpif_obj.dev[k]->channel_id));
+
+	while (k--) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		for (j =  res->end; j >= res->start; j--)
+			free_irq(j, (void *)(&vpif_obj.dev[k]->channel_id));
 	}
 
 	return err;

Re: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test

[Roel Kluin]

The first loop ends when platform_get_resource() returns NULL. Can it occur
that no platform_get_resource() succeeded? I think we should error return if
that happens. Could k grow larger than VPIF_DISPLAY_MAX_DEVICES there?
Should we err out in that case?

In the loop `for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++)' if
video_device_alloc() fails I think we correctly release the devices,
but we have to do more before we reach label vpif_int_err.

As mentioned, we left the first loop with a res of NULL, which is
dereferenced at label vpif_int_err. So we have to get the resource again,
however, k was incremented at the end of that loop as well. Also i used
as index in the second loop as well should point to res->end before going
to label vpif_int_err, to free all requested irqs. All this needs to be
done for later error labels as well, so a new label is added where this
occurs, alloc_vid_fail.

Variable k can't be reused in the third for-loop and at label probe_out.
As mentioned k is needed to get the resource in case a error and clean-up
is required.

If we reach label vpif_int_err, res shouldn't be NULL, since we
dereference it. Previously we had:

        for (; k >= 0; k--) {
                for (m = i; m >= res->start; m--)
                        free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
                res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);
                m = res->end;
        }

In the last iteration k equals 0, so we call platform_get_resource() with
-1 as a third argument. Since platform_get_resource() uses an unsigned it
is converted to 0xffffffff. platform_get_resource() fails for every index
and returns NULL. A test is lacking and we dereference NULL.

The error "VPIF IRQ request failed" should only be displayed when 
request_irq() failed, not in the case of other errors.

Also I changed some indexes, so a few could be removed.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
There were some errors in the changelog and a signoff was missing.

> Ok. You are right! The ch_params[] is a table for keeping the information
> about different standards supported. For a given stdid in std_info, the function matches the stdid with that in the table and get the corresponding entry.

>>>> +      if (k == VPIF_DISPLAY_MAX_DEVICES)
>>>> +              k = VPIF_DISPLAY_MAX_DEVICES - 1;
>>
>> actually I think this is still not right. shouldn't it be be
>>
>> k = VPIF_DISPLAY_MAX_DEVICES - 1;
> 
> What you mean here? What you suggest here is same as in your patch, right?

I must admit I did not test this, except with checkpatch.pl, but I think
the issues are real and should be fixed. Do you have comments?

 drivers/media/video/davinci/vpif_display.c |   61 +++++++++++++++++++---------
 1 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/media/video/davinci/vpif_display.c b/drivers/media/video/davinci/vpif_display.c
index dfddef7..ae8ca94 100644
--- a/drivers/media/video/davinci/vpif_display.c
+++ b/drivers/media/video/davinci/vpif_display.c
@@ -383,7 +383,7 @@ static int vpif_get_std_info(struct channel_obj *ch)
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
+	if (!std_info->stdid)
 		return -1;
 
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
@@ -1423,7 +1423,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 {
 	struct vpif_subdev_info *subdevdata;
 	struct vpif_display_config *config;
-	int i, j = 0, k, q, m, err = 0;
+	int i, j, k, err;
 	struct i2c_adapter *i2c_adap;
 	struct common_obj *common;
 	struct channel_obj *ch;
@@ -1452,12 +1452,18 @@ static __init int vpif_probe(struct platform_device *pdev)
 			if (request_irq(i, vpif_channel_isr, IRQF_DISABLED,
 					"DM646x_Display",
 				(void *)(&vpif_obj.dev[k]->channel_id))) {
+				i--;
 				err = -EBUSY;
+				vpif_err("VPIF IRQ request failed\n");
 				goto vpif_int_err;
 			}
 		}
 		k++;
+		if (k >= VPIF_DISPLAY_MAX_DEVICES)
+			break;
 	}
+	if (k == 0)
+		return -ENODEV;
 
 	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
 
@@ -1472,7 +1478,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 				video_device_release(ch->video_dev);
 			}
 			err = -ENOMEM;
-			goto vpif_int_err;
+			goto alloc_vid_fail;
 		}
 
 		/* Initialize field of video device */
@@ -1489,13 +1495,13 @@ static __init int vpif_probe(struct platform_device *pdev)
 		ch->video_dev = vfd;
 	}
 
-	for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) {
-		ch = vpif_obj.dev[j];
+	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
+		ch = vpif_obj.dev[i];
 		/* Initialize field of the channel objects */
 		atomic_set(&ch->usrs, 0);
-		for (k = 0; k < VPIF_NUMOBJECTS; k++) {
-			ch->common[k].numbuffers = 0;
-			common = &ch->common[k];
+		for (j = 0; j < VPIF_NUMOBJECTS; j++) {
+			ch->common[j].numbuffers = 0;
+			common = &ch->common[j];
 			common->io_usrs = 0;
 			common->started = 0;
 			spin_lock_init(&common->irqlock);
@@ -1506,12 +1512,12 @@ static __init int vpif_probe(struct platform_device *pdev)
 			common->ctop_off = common->cbtm_off = 0;
 			common->cur_frm = common->next_frm = NULL;
 			memset(&common->fmt, 0, sizeof(common->fmt));
-			common->numbuffers = config_params.numbuffers[k];
+			common->numbuffers = config_params.numbuffers[j];
 
 		}
 		ch->initialized = 0;
-		ch->channel_id = j;
-		if (j < 2)
+		ch->channel_id = i;
+		if (i < 2)
 			ch->common[VPIF_VIDEO_INDEX].numbuffers =
 			    config_params.numbuffers[ch->channel_id];
 		else
@@ -1529,7 +1535,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 				(int)ch, (int)&ch->video_dev);
 
 		err = video_register_device(ch->video_dev,
-					  VFL_TYPE_GRABBER, (j ? 3 : 2));
+					  VFL_TYPE_GRABBER, (i ? 3 : 2));
 		if (err < 0)
 			goto probe_out;
 
@@ -1567,20 +1573,35 @@ static __init int vpif_probe(struct platform_device *pdev)
 probe_subdev_out:
 	kfree(vpif_obj.sd);
 probe_out:
-	for (k = 0; k < j; k++) {
-		ch = vpif_obj.dev[k];
+	for (j = 0; j < i; j++) {
+		ch = vpif_obj.dev[j];
 		video_unregister_device(ch->video_dev);
 		video_device_release(ch->video_dev);
 		ch->video_dev = NULL;
 	}
+alloc_vid_fail:
+	while (k--) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		if (res != NULL)
+			break;
+		vpif_err("Couldn't get resource %d, irqs not freed.\n", k);
+	}
+	if (res == NULL) {
+		vpif_err("Couldn't get any resource.\n");
+		return err;
+	}
+
+	i = res->end;
 vpif_int_err:
 	v4l2_device_unregister(&vpif_obj.v4l2_dev);
-	vpif_err("VPIF IRQ request failed\n");
-	for (q = k; k >= 0; k--) {
-		for (m = i; m >= res->start; m--)
-			free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
-		res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);
-		m = res->end;
+
+	for (j = i; j >= res->start; j--)
+		free_irq(j, (void *)(&vpif_obj.dev[k]->channel_id));
+
+	while (k--) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		for (j =  res->end; j >= res->start; j--)
+			free_irq(j, (void *)(&vpif_obj.dev[k]->channel_id));
 	}
 
 	return err;

Re: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test

[Roel Kluin]

The first loop ends when platform_get_resource() returns NULL. Can it occur
that no platform_get_resource() succeeded? I think we should error return if
that happens. Could k grow larger than VPIF_DISPLAY_MAX_DEVICES there? Should
we err out in that case?

In the loop `for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++)' if
video_device_alloc() fails I think we correctly release the devices, but we
have to do more before we reach label vpif_int_err.

As mentioned, we left the first loop with a res of NULL, which is dereferenced
at label vpif_int_err. So we have to get the resource again, however, k was
incremented at the end of that loop as well. Also i used as index in the second
loop as well should point to res->end before going to label vpif_int_err, to
free all requested irqs. All this needs to be done for later error labels as
well, so a new label is added where this occurs, alloc_vid_fail.

Variable k can't be reused in the third for-loop and at label probe_out. As
mentioned k is needed to get the resource in case a error and clean-up is
required.

If we reach label vpif_int_err, res shouldn't be NULL, since we dereference it.
Previously we had:

        for (; k >= 0; k--) {
                for (m = i; m >= res->start; m--)
                        free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
                res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);
                m = res->end;
        }

In the last iteration k equals 0, so we call platform_get_resource() with -1 as
a third argument. Since platform_get_resource() uses an unsigned it is
converted to 0xffffffff. platform_get_resource() fails for every index and
returns NULL. A test is lacking and we dereference NULL.

The error "VPIF IRQ request failed" should only be displayed when request_irq()
failed, not in the case of other errors.

Also I changed some indexes, so a few could be removed.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
> I think there were many more issues:

> I must admit I did not compile test this, except with checkpatch.pl, but I
> think the issues are real and should be fixed. Comments?

I would like to compile test but cannot compile test these because of errors
like drivers/net/davinci_emac.c:69:11: error: unable to open 'mach/dm646x.h'

I found these three other functions in davinci code that have very similar
problems. The changelog is about vpif_probe() in vpif_display.c.

Since the functions are somewhat similar, maybe code should be shared, but
where should one put that?

Roel

 drivers/media/video/davinci/vpif_capture.c |   63 +++++++++++++----------
 drivers/media/video/davinci/vpif_display.c |   76 ++++++++++++++++++----------
 drivers/net/davinci_emac.c                 |   64 ++++++++++++++++--------
 3 files changed, 129 insertions(+), 74 deletions(-)

diff --git a/drivers/media/video/davinci/vpif_capture.c b/drivers/media/video/davinci/vpif_capture.c
index 7813072..096e727 100644
--- a/drivers/media/video/davinci/vpif_capture.c
+++ b/drivers/media/video/davinci/vpif_capture.c
@@ -1915,7 +1915,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 {
 	struct vpif_subdev_info *subdevdata;
 	struct vpif_capture_config *config;
-	int i, j, k, m, q, err;
+	int i, j, k, err;
 	struct i2c_adapter *i2c_adap;
 	struct channel_obj *ch;
 	struct common_obj *common;
@@ -1936,14 +1936,18 @@ static __init int vpif_probe(struct platform_device *pdev)
 		for (i = res->start; i <= res->end; i++) {
 			if (request_irq(i, vpif_channel_isr, IRQF_DISABLED,
 					"DM646x_Capture",
-				(void *)(&vpif_obj.dev[k]->channel_id))) {
-				err = -EBUSY;
+				&vpif_obj.dev[k]->channel_id)) {
 				i--;
+				err = -EBUSY;
 				goto vpif_int_err;
 			}
 		}
 		k++;
+		if (k >= VPIF_DISPLAY_MAX_DEVICES)
+			break;
 	}
+	if (k == 0)
+		return -ENODEV;
 
 	for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
 		/* Get the pointer to the channel object */
@@ -1972,16 +1976,16 @@ static __init int vpif_probe(struct platform_device *pdev)
 		ch->video_dev = vfd;
 	}
 
-	for (j = 0; j < VPIF_CAPTURE_MAX_DEVICES; j++) {
-		ch = vpif_obj.dev[j];
-		ch->channel_id = j;
+	for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
+		ch = vpif_obj.dev[i];
+		ch->channel_id = i;
 		common = &(ch->common[VPIF_VIDEO_INDEX]);
 		spin_lock_init(&common->irqlock);
 		mutex_init(&common->lock);
 		/* Initialize prio member of channel object */
 		v4l2_prio_init(&ch->prio);
 		err = video_register_device(ch->video_dev,
-					    VFL_TYPE_GRABBER, (j ? 1 : 0));
+					    VFL_TYPE_GRABBER, (i ? 1 : 0));
 		if (err)
 			goto probe_out;
 
@@ -2007,24 +2011,24 @@ static __init int vpif_probe(struct platform_device *pdev)
 		goto probe_subdev_out;
 	}
 
-	for (i = 0; i < subdev_count; i++) {
-		subdevdata = &config->subdev_info[i];
-		vpif_obj.sd[i] =
+	for (j = 0; j < subdev_count; j++) {
+		subdevdata = &config->subdev_info[j];
+		vpif_obj.sd[j] =
 			v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
 						  i2c_adap,
 						  subdevdata->name,
 						  &subdevdata->board_info,
 						  NULL);
 
-		if (!vpif_obj.sd[i]) {
+		if (!vpif_obj.sd[j]) {
 			vpif_err("Error registering v4l2 subdevice\n");
 			goto probe_subdev_out;
 		}
 		v4l2_info(&vpif_obj.v4l2_dev, "registered sub device %s\n",
 			  subdevdata->name);
 
-		if (vpif_obj.sd[i])
-			vpif_obj.sd[i]->grp_id = 1 << i;
+		if (vpif_obj.sd[j])
+			vpif_obj.sd[j]->grp_id = 1 << j;
 	}
 	v4l2_info(&vpif_obj.v4l2_dev, "DM646x VPIF Capture driver"
 		  " initialized\n");
@@ -2034,30 +2038,37 @@ static __init int vpif_probe(struct platform_device *pdev)
 probe_subdev_out:
 	/* free sub devices memory */
 	kfree(vpif_obj.sd);
-
-	j = VPIF_CAPTURE_MAX_DEVICES;
 probe_out:
 	v4l2_device_unregister(&vpif_obj.v4l2_dev);
-	for (k = 0; k < j; k++) {
+	for (j = 0; j < i; j++) {
 		/* Get the pointer to the channel object */
-		ch = vpif_obj.dev[k];
+		ch = vpif_obj.dev[j];
 		/* Unregister video device */
 		video_unregister_device(ch->video_dev);
 	}
 
 vpif_dev_alloc_err:
-	k = VPIF_CAPTURE_MAX_DEVICES-1;
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+	while (k--) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		if (res != NULL)
+			break;
+		vpif_err("Couldn't free irqs for resource %d.\n", k);
+	}
+	if (res == NULL)
+		return err;
 	i = res->end;
-
 vpif_int_err:
-	for (q = k; q >= 0; q--) {
-		for (m = i; m >= (int)res->start; m--)
-			free_irq(m, (void *)(&vpif_obj.dev[q]->channel_id));
+	for (j = i; j >= (int)res->start; j--)
+		free_irq(j, &vpif_obj.dev[k]->channel_id);
 
-		res = platform_get_resource(pdev, IORESOURCE_IRQ, q-1);
-		if (res)
-			i = res->end;
+	while (k--) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		if (res == NULL) {
+			vpif_err("Couldn't free irqs for resource %d.\n", k);
+			continue;
+		}
+		for (j = res->end; j >= (int)res->start; j--)
+			free_irq(j, &vpif_obj.dev[k]->channel_id);
 	}
 	return err;
 }
diff --git a/drivers/media/video/davinci/vpif_display.c b/drivers/media/video/davinci/vpif_display.c
index dfddef7..6bcedbf 100644
--- a/drivers/media/video/davinci/vpif_display.c
+++ b/drivers/media/video/davinci/vpif_display.c
@@ -383,7 +383,7 @@ static int vpif_get_std_info(struct channel_obj *ch)
 	int index;
 
 	std_info->stdid = vid_ch->stdid;
-	if (!std_info)
+	if (!std_info->stdid)
 		return -1;
 
 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
@@ -1423,7 +1423,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 {
 	struct vpif_subdev_info *subdevdata;
 	struct vpif_display_config *config;
-	int i, j = 0, k, q, m, err = 0;
+	int i, j, k, err;
 	struct i2c_adapter *i2c_adap;
 	struct common_obj *common;
 	struct channel_obj *ch;
@@ -1452,12 +1452,18 @@ static __init int vpif_probe(struct platform_device *pdev)
 			if (request_irq(i, vpif_channel_isr, IRQF_DISABLED,
 					"DM646x_Display",
 				(void *)(&vpif_obj.dev[k]->channel_id))) {
+				i--;
 				err = -EBUSY;
+				vpif_err("VPIF IRQ request failed\n");
 				goto vpif_int_err;
 			}
 		}
 		k++;
+		if (k >= VPIF_DISPLAY_MAX_DEVICES)
+			break;
 	}
+	if (k == 0)
+		return -ENODEV;
 
 	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
 
@@ -1472,7 +1478,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 				video_device_release(ch->video_dev);
 			}
 			err = -ENOMEM;
-			goto vpif_int_err;
+			goto alloc_vid_fail;
 		}
 
 		/* Initialize field of video device */
@@ -1489,13 +1495,13 @@ static __init int vpif_probe(struct platform_device *pdev)
 		ch->video_dev = vfd;
 	}
 
-	for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) {
-		ch = vpif_obj.dev[j];
+	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
+		ch = vpif_obj.dev[i];
 		/* Initialize field of the channel objects */
 		atomic_set(&ch->usrs, 0);
-		for (k = 0; k < VPIF_NUMOBJECTS; k++) {
-			ch->common[k].numbuffers = 0;
-			common = &ch->common[k];
+		for (j = 0; j < VPIF_NUMOBJECTS; j++) {
+			ch->common[j].numbuffers = 0;
+			common = &ch->common[j];
 			common->io_usrs = 0;
 			common->started = 0;
 			spin_lock_init(&common->irqlock);
@@ -1506,12 +1512,12 @@ static __init int vpif_probe(struct platform_device *pdev)
 			common->ctop_off = common->cbtm_off = 0;
 			common->cur_frm = common->next_frm = NULL;
 			memset(&common->fmt, 0, sizeof(common->fmt));
-			common->numbuffers = config_params.numbuffers[k];
+			common->numbuffers = config_params.numbuffers[j];
 
 		}
 		ch->initialized = 0;
-		ch->channel_id = j;
-		if (j < 2)
+		ch->channel_id = i;
+		if (i < 2)
 			ch->common[VPIF_VIDEO_INDEX].numbuffers =
 			    config_params.numbuffers[ch->channel_id];
 		else
@@ -1529,7 +1535,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 				(int)ch, (int)&ch->video_dev);
 
 		err = video_register_device(ch->video_dev,
-					  VFL_TYPE_GRABBER, (j ? 3 : 2));
+					  VFL_TYPE_GRABBER, (i ? 3 : 2));
 		if (err < 0)
 			goto probe_out;
 
@@ -1548,18 +1554,18 @@ static __init int vpif_probe(struct platform_device *pdev)
 		goto probe_out;
 	}
 
-	for (i = 0; i < subdev_count; i++) {
-		vpif_obj.sd[i] = v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
-						i2c_adap, subdevdata[i].name,
-						&subdevdata[i].board_info,
+	for (j = 0; j < subdev_count; j++) {
+		vpif_obj.sd[j] = v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
+						i2c_adap, subdevdata[j].name,
+						&subdevdata[j].board_info,
 						NULL);
-		if (!vpif_obj.sd[i]) {
+		if (!vpif_obj.sd[j]) {
 			vpif_err("Error registering v4l2 subdevice\n");
 			goto probe_subdev_out;
 		}
 
-		if (vpif_obj.sd[i])
-			vpif_obj.sd[i]->grp_id = 1 << i;
+		if (vpif_obj.sd[j])
+			vpif_obj.sd[j]->grp_id = 1 << j;
 	}
 
 	return 0;
@@ -1567,20 +1573,36 @@ static __init int vpif_probe(struct platform_device *pdev)
 probe_subdev_out:
 	kfree(vpif_obj.sd);
 probe_out:
-	for (k = 0; k < j; k++) {
-		ch = vpif_obj.dev[k];
+	for (j = 0; j < i; j++) {
+		ch = vpif_obj.dev[j];
 		video_unregister_device(ch->video_dev);
 		video_device_release(ch->video_dev);
 		ch->video_dev = NULL;
 	}
+alloc_vid_fail:
+	while (k--) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		if (res != NULL)
+			break;
+		vpif_err("Couldn't free irqs for resource %d.\n", k);
+	}
+	if (res == NULL)
+		return err;
+	i = res->end;
 vpif_int_err:
 	v4l2_device_unregister(&vpif_obj.v4l2_dev);
-	vpif_err("VPIF IRQ request failed\n");
-	for (q = k; k >= 0; k--) {
-		for (m = i; m >= res->start; m--)
-			free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
-		res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);
-		m = res->end;
+
+	for (j = i; j >= res->start; j--)
+		free_irq(j, (void *)(&vpif_obj.dev[k]->channel_id));
+
+	while (k--) {
+		res = platform_get_resource(pdev, IORESOURCE_IRQ, k);
+		if (res == NULL) {
+			vpif_err("Couldn't free irqs for resource %d.\n", k);
+			continue;
+		}
+		for (j =  res->end; j >= res->start; j--)
+			free_irq(j, (void *)(&vpif_obj.dev[k]->channel_id));
 	}
 
 	return err;
diff --git a/drivers/net/davinci_emac.c b/drivers/net/davinci_emac.c
index 33c4fe2..2faa9d3 100644
--- a/drivers/net/davinci_emac.c
+++ b/drivers/net/davinci_emac.c
@@ -2369,12 +2369,12 @@ static int emac_devioctl(struct net_device *ndev, struct ifreq *ifrq, int cmd)
 static int emac_dev_open(struct net_device *ndev)
 {
 	struct device *emac_dev = &ndev->dev;
-	u32 rc, cnt, ch;
+	u32 cnt, ch;
 	int phy_addr;
 	struct resource *res;
-	int q, m;
-	int i = 0;
+	int i, j, k, err;
 	int k = 0;
+	int err;
 	struct emac_priv *priv = netdev_priv(ndev);
 
 	netif_carrier_off(ndev);
@@ -2398,15 +2398,15 @@ static int emac_dev_open(struct net_device *ndev)
 	emac_write(EMAC_MACHASH2, 0);
 
 	/* multi ch not supported - open 1 TX, 1RX ch by default */
-	rc = emac_init_txch(priv, EMAC_DEF_TX_CH);
-	if (0 != rc) {
+	err = emac_init_txch(priv, EMAC_DEF_TX_CH);
+	if (err) {
 		dev_err(emac_dev, "DaVinci EMAC: emac_init_txch() failed");
-		return rc;
+		return err;
 	}
-	rc = emac_init_rxch(priv, EMAC_DEF_RX_CH, priv->mac_addr);
-	if (0 != rc) {
+	err = emac_init_rxch(priv, EMAC_DEF_RX_CH, priv->mac_addr);
+	if (err) {
 		dev_err(emac_dev, "DaVinci EMAC: emac_init_rxch() failed");
-		return rc;
+		return err;
 	}
 
 	/* Request IRQ */
@@ -2414,11 +2414,17 @@ static int emac_dev_open(struct net_device *ndev)
 	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
 		for (i = res->start; i <= res->end; i++) {
 			if (request_irq(i, emac_irq, IRQF_DISABLED,
-					ndev->name, ndev))
+					ndev->name, ndev)) {
+				i--;
+				err = -EBUSY;
+				dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
 				goto rollback;
+			}
 		}
 		k++;
 	}
+	if (k == 0)
+		return -ENODEV;
 
 	/* Start/Enable EMAC hardware */
 	emac_hw_enable(priv);
@@ -2436,7 +2442,8 @@ static int emac_dev_open(struct net_device *ndev)
 
 		if (!priv->phydev) {
 			printk(KERN_ERR "%s: no PHY found\n", ndev->name);
-			return -1;
+			err = -ENODEV;
+			goto phydev_error;
 		}
 
 		priv->phydev = phy_connect(ndev, dev_name(&priv->phydev->dev),
@@ -2445,7 +2452,8 @@ static int emac_dev_open(struct net_device *ndev)
 		if (IS_ERR(priv->phydev)) {
 			printk(KERN_ERR "%s: Could not attach to PHY\n",
 								ndev->name);
-			return PTR_ERR(priv->phydev);
+			err = PTR_ERR(priv->phydev);
+			goto phydev_error;
 		}
 
 		priv->link = 0;
@@ -2456,7 +2464,7 @@ static int emac_dev_open(struct net_device *ndev)
 			"(mii_bus:phy_addr=%s, id=%x)\n", ndev->name,
 			priv->phydev->drv->name, dev_name(&priv->phydev->dev),
 			priv->phydev->phy_id);
-	} else{
+	} else {
 		/* No PHY , fix the link, speed and duplex settings */
 		priv->link = 1;
 		priv->speed = SPEED_100;
@@ -2475,15 +2483,29 @@ static int emac_dev_open(struct net_device *ndev)
 
 	return 0;
 
+phydev_error:
+	while (k--) {
+		res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k);
+		if (res != NULL)
+			break;
+		dev_err(emac_dev, "Couldn't free irqs for resource %d.\n", k);
+	}
+	if (res == NULL)
+		return err;
+	i = res->end;
 rollback:
-
-	dev_err(emac_dev, "DaVinci EMAC: request_irq() failed");
-
-	for (q = k; k >= 0; k--) {
-		for (m = i; m >= res->start; m--)
-			free_irq(m, ndev);
-		res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k-1);
-		m = res->end;
+	for (j = i; j >= res->start; j--)
+		free_irq(j, ndev);
+
+	while (k--) {
+		res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k);
+		if (res == NULL) {
+			dev_err(emac_dev, "Couldn't free irqs for resource %d.\n",
+					 k);
+			continue;
+		}
+		for (j = res->end; j >= res->start; j--)
+			free_irq(j, ndev);
 	}
 	return -EBUSY;
 }