Re: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test
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 0x. 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
---
> 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,
Re: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test
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 0x. 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
---
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++) {
-
Re: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test
> 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 0x. 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_DISP
RE: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test
>>>- 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
N�r��yb�X��ǧv�^�){.n�+{���bj)w* jg��� �ݢj/���z�ޖ��2�ޙ&�)ߡ�a���� �G���h��j:+v���w��٥
Re: [PATCH] video_device: don't free_irq() an element past array vpif_obj.dev[] and fix test
>>- 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@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,
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
>---
>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 majord...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
