Hi,
I've pushed the first patch, thanks!
For the second see my inline comments.
Is the m5602 branch working for you?
My current plan has been to develop the "real" driver in the m5602 branch
with all others being experimental and only used to stabilize each sensor to
the point that they can be merged.
2008/7/13 David Fries <[EMAIL PROTECTED]>:
> I was testing under qemu, which doesn't support isochroneous
> transfers. It crashed and pointed out a bug and here is the fix.
>
> 1st patch
> When urb submit fails, m5602_start_transfer failed to set
> cam->isoc_in_buffer to NULL and caused a double free in
> m5602_stop_transfer.
>
> 2nd patch
> m5602_release_buffers, redid the error out logic.
>
> When a usb_submit_urb fails it will now kill any succsfully submitted
> URBs, previously it wouldn't kill any. I'm not sure if this is
> required, but I assume it is.
>
> The previous logic is more efficient by jumping to and only freeing
> the required buffers, but it is an error condition and wasn't ever
> being hit (or at least not without crashing in the second open, see
> first patch) so it might as well have one simpler loop and iterator
> over all buffers.
>
> m5602.c, set cam->control_buffer to NULL when freed
> m5602_v4l2.c, set cam->canvas, cam->isoc_in_buffer[i] to NULL when freed
>
> Signed-off-by: David Fries <[EMAIL PROTECTED]>
>
> ---
> branches/m5602-s5k83a/m5602_v4l2.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/branches/m5602-s5k83a/m5602_v4l2.c
> b/branches/m5602-s5k83a/m5602_v4l2.c
> index cc90392..dec6f8e 100644
> --- a/branches/m5602-s5k83a/m5602_v4l2.c
> +++ b/branches/m5602-s5k83a/m5602_v4l2.c
> @@ -509,7 +509,7 @@ free_urbs:
> free_buffers:
> for (i = 0; (i < M5602_URBS) && cam->isoc_in_buffer[i]; i++) {
> kfree(cam->isoc_in_buffer[i]);
> - cam->urb[i] = NULL;
> + cam->isoc_in_buffer[i] = NULL;
> }
>
> PDEBUG(DBG_V4L2, "failed to start transfer");
> --
> 1.4.4.4
>
>
> ---
> branches/m5602-s5k83a/m5602.c | 1 +
> branches/m5602-s5k83a/m5602_v4l2.c | 18 ++++++++++--------
> 2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/branches/m5602-s5k83a/m5602.c b/branches/m5602-s5k83a/m5602.c
> index 275c820..a617050 100644
> --- a/branches/m5602-s5k83a/m5602.c
> +++ b/branches/m5602-s5k83a/m5602.c
> @@ -339,6 +339,7 @@ static void m5602_release_resources(struct m5602_camera
> *cam)
> /* up(&m5602_sysfs_lock); */
>
> kfree(cam->control_buffer);
> + cam->control_buffer = NULL;
Why should we set this to NULL, AFAICT we never test for this to be NULL.
>
> }
>
> struct file_operations v4l_m5602_fops = {
> diff --git a/branches/m5602-s5k83a/m5602_v4l2.c
> b/branches/m5602-s5k83a/m5602_v4l2.c
> index dec6f8e..cd884bf 100644
> --- a/branches/m5602-s5k83a/m5602_v4l2.c
> +++ b/branches/m5602-s5k83a/m5602_v4l2.c
> @@ -166,6 +166,7 @@ static void m5602_release_buffers(struct m5602_camera
> *cam)
>
> /* Free the canvas */
> kfree(cam->canvas);
> + cam->canvas = NULL;
Same for this
>
>
> mutex_unlock(&cam->canvas_mutex);
>
> @@ -449,7 +450,7 @@ static int m5602_start_transfer(struct m5602_camera
> *cam)
> if (!urb) {
> PDEBUG(DBG_DATA, "No enough memory for URB");
> retval = -ENOMEM;
> - goto free_urbs;
> + goto free_buffers;
> }
>
> cam->urb[i] = urb;
> @@ -474,7 +475,7 @@ static int m5602_start_transfer(struct m5602_camera
> *cam)
>
> retval = usb_set_interface(cam->udev, 0, 2);
> if (retval)
> - goto free_urbs;
> + goto free_buffers;
>
> for (i = 0; i < M5602_URBS; i++) {
> if (cam->urb[i]) {
> @@ -482,7 +483,10 @@ static int m5602_start_transfer(struct m5602_camera
> *cam)
> if (retval) {
> PDEBUG(DBG_V4L2, "usb_submit_urb failed!
> %d",
> retval);
> - goto free_urbs;
> + /* Start killing the previous URB. */
> + for (i--; i>=0; i--)
> + usb_kill_urb(cam->urb[i]);
> + goto free_buffers;
I see your point here but isn't it more easily read code to do all cleanup
at the end of the function?
Thanks,
Erik
>
> }
> }
> }
> @@ -500,14 +504,11 @@ static int m5602_start_transfer(struct m5602_camera
> *cam)
>
> return retval;
>
> -free_urbs:
> - for (i = 0; (i < M5602_URBS) && cam->urb[i]; i++) {
> +free_buffers:
> + for (i = 0; i < M5602_URBS; i++) {
> usb_free_urb(cam->urb[i]);
> cam->urb[i] = NULL;
> - }
>
> -free_buffers:
> - for (i = 0; (i < M5602_URBS) && cam->isoc_in_buffer[i]; i++) {
> kfree(cam->isoc_in_buffer[i]);
> cam->isoc_in_buffer[i] = NULL;
> }
> @@ -534,6 +535,7 @@ static int m5602_stop_transfer(struct m5602_camera
> *cam)
> }
>
> kfree(cam->isoc_in_buffer[i]);
> + cam->isoc_in_buffer[i] = NULL;
> }
>
> PDEBUG(DBG_V4L2, "isochronous buffers and urbs freed");
> --
> 1.4.4.4
>
>
> --
> David Fries <[EMAIL PROTECTED]>
> http://fries.net/~david/ <http://fries.net/%7Edavid/> (PGP encryption key
> available)
>
> -------------------------------------------------------------------------
> Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
> Studies have shown that voting for your favorite open source project,
> along with a healthy diet, reduces your potential for chronic lameness
> and boredom. Vote Now at http://www.sourceforge.net/community/cca08
> _______________________________________________
> M560x-driver-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/m560x-driver-devel
>
-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08
_______________________________________________
M560x-driver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/m560x-driver-devel
