| Message ID | 20090418125111.6646e997@linux-lm |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Saturday 18 April 2009, leiming wrote: > On Fri, 17 Apr 2009 19:55:29 -0700 (PDT) > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > > @@ -742,7 +742,7 @@ static int uvc_alloc_urb_buffers(struct > > > uvc_video_device *video, > > > /* Buffers are already allocated, bail out. */ > > > if (video->urb_size) > > > - return 0; > > > + return DIV_ROUND_UP(video->urb_size, psize); > > > > I don't think this is right. It should round _down_. > > > > It's supposed to return 'npackets', but if you pass it a different > > packet size than it was passed originally, it can now return a > > potentially bigger number than the already allocated buffer, no? > > > > So I think it should round down (ie use a regular divide). No? > > Yes,you are correct, please ignore my last reply, and following is > the fixed patch. > > Thanks. Thanks for the patch, I've updated the bug entry to point to it. Best, Rafael > From a3b3d72cdd57a0699fb643b41b78eb7beb211ff5 Mon Sep 17 00:00:00 2001 > From: Ming Lei <tom.leiming@gmail.com> > Date: Wed, 15 Apr 2009 22:32:51 +0800 > Subject: [PATCH] V4L/DVB:usbvideo:fix uvc resume failed(v2) > > Now urb buffers is not freed before suspend, so uvc_alloc_urb_buffers > should return packet counts allocated originally during uvc resume > , instead of zero. > > This version uses round down to return packet counts on Linus's > suggestions, or else may lead to buffer destructed if packet size > is changed before calling uvc_alloc_urb_buffers() in this kind of > case. > > This patch is against v2.6.30-rc2. > > Signed-off-by: Ming Lei <tom.leiming@gmail.com> > --- > drivers/media/video/uvc/uvc_video.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/uvc/uvc_video.c b/drivers/media/video/uvc/uvc_video.c > index a95e173..6ce974d 100644 > --- a/drivers/media/video/uvc/uvc_video.c > +++ b/drivers/media/video/uvc/uvc_video.c > @@ -742,7 +742,7 @@ static int uvc_alloc_urb_buffers(struct uvc_video_device *video, > > /* Buffers are already allocated, bail out. */ > if (video->urb_size) > - return 0; > + return video->urb_size / psize; > > /* Compute the number of packets. Bulk endpoints might transfer UVC > * payloads accross multiple URBs. > -- > 1.6.0.GIT -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 18 April 2009 06:51:11 leiming wrote: > On Fri, 17 Apr 2009 19:55:29 -0700 (PDT) > > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > @@ -742,7 +742,7 @@ static int uvc_alloc_urb_buffers(struct > > > uvc_video_device *video, > > > /* Buffers are already allocated, bail out. */ > > > if (video->urb_size) > > > - return 0; > > > + return DIV_ROUND_UP(video->urb_size, psize); > > > > I don't think this is right. It should round _down_. > > > > It's supposed to return 'npackets', but if you pass it a different > > packet size than it was passed originally, it can now return a > > potentially bigger number than the already allocated buffer, no? > > > > So I think it should round down (ie use a regular divide). No? > > Yes,you are correct, please ignore my last reply, and following is > the fixed patch. psize and video->urb_size shouldn't have changed before and after resume, otherwise we'll get into trouble anyway. A regular divide and a round-up divide should then return the same result. I'll take the regular divide, as it will be more efficient. > Thanks. > > From a3b3d72cdd57a0699fb643b41b78eb7beb211ff5 Mon Sep 17 00:00:00 2001 > From: Ming Lei <tom.leiming@gmail.com> > Date: Wed, 15 Apr 2009 22:32:51 +0800 > Subject: [PATCH] V4L/DVB:usbvideo:fix uvc resume failed(v2) > > Now urb buffers is not freed before suspend, so uvc_alloc_urb_buffers > should return packet counts allocated originally during uvc resume > , instead of zero. > > This version uses round down to return packet counts on Linus's > suggestions, or else may lead to buffer destructed if packet size > is changed before calling uvc_alloc_urb_buffers() in this kind of > case. The comment is misleading. If the packet size changes we need to reallocate the buffers anyway. Have you checked if the packet size (which depends on the endpoint being selected) can be changed between suspend and resume, either by the uvcvideo driver (I don't think it can) or the USB core ? Best regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/4/21 Laurent Pinchart <laurent.pinchart@skynet.be>: > On Saturday 18 April 2009 06:51:11 leiming wrote: >> On Fri, 17 Apr 2009 19:55:29 -0700 (PDT) >> >> Linus Torvalds <torvalds@linux-foundation.org> wrote: >> > > @@ -742,7 +742,7 @@ static int uvc_alloc_urb_buffers(struct >> > > uvc_video_device *video, >> > > /* Buffers are already allocated, bail out. */ >> > > if (video->urb_size) >> > > - return 0; >> > > + return DIV_ROUND_UP(video->urb_size, psize); >> > >> > I don't think this is right. It should round _down_. >> > >> > It's supposed to return 'npackets', but if you pass it a different >> > packet size than it was passed originally, it can now return a >> > potentially bigger number than the already allocated buffer, no? >> > >> > So I think it should round down (ie use a regular divide). No? >> >> Yes,you are correct, please ignore my last reply, and following is >> the fixed patch. > > psize and video->urb_size shouldn't have changed before and after resume, > otherwise we'll get into trouble anyway. A regular divide and a round-up > divide should then return the same result. I'll take the regular divide, as it > will be more efficient. Yes. > >> Thanks. >> >> From a3b3d72cdd57a0699fb643b41b78eb7beb211ff5 Mon Sep 17 00:00:00 2001 >> From: Ming Lei <tom.leiming@gmail.com> >> Date: Wed, 15 Apr 2009 22:32:51 +0800 >> Subject: [PATCH] V4L/DVB:usbvideo:fix uvc resume failed(v2) >> >> Now urb buffers is not freed before suspend, so uvc_alloc_urb_buffers >> should return packet counts allocated originally during uvc resume >> , instead of zero. >> >> This version uses round down to return packet counts on Linus's >> suggestions, or else may lead to buffer destructed if packet size >> is changed before calling uvc_alloc_urb_buffers() in this kind of >> case. > > The comment is misleading. If the packet size changes we need to reallocate > the buffers anyway. Have you checked if the packet size (which depends on the > endpoint being selected) can be changed between suspend and resume, either by > the uvcvideo driver (I don't think it can) or the USB core ? The packet size does not change between suspend and resume. I mean uvc_alloc_urb_buffers() still can be used in other cases if buffers was not freed and is reuesed in future. It seems there is no such cases in uvcvideo now, but uvc_alloc_urb_buffers() really __can__ work in such case, isn't it? IMHO It is only used to allocate or reserve UVC_URBS usb buffers, which size is video->urb_size, and npackets can be shortened or enlarged if psize is changed, after all. Thanks!
Hi, On Tuesday 21 April 2009 03:47:34 Ming Lei wrote: > 2009/4/21 Laurent Pinchart <laurent.pinchart@skynet.be>: > > On Saturday 18 April 2009 06:51:11 leiming wrote: > >> On Fri, 17 Apr 2009 19:55:29 -0700 (PDT) > >> > >> Linus Torvalds <torvalds@linux-foundation.org> wrote: > >> > > @@ -742,7 +742,7 @@ static int uvc_alloc_urb_buffers(struct > >> > > uvc_video_device *video, > >> > > /* Buffers are already allocated, bail out. */ > >> > > if (video->urb_size) > >> > > - return 0; > >> > > + return DIV_ROUND_UP(video->urb_size, psize); > >> > > >> > I don't think this is right. It should round _down_. > >> > > >> > It's supposed to return 'npackets', but if you pass it a different > >> > packet size than it was passed originally, it can now return a > >> > potentially bigger number than the already allocated buffer, no? > >> > > >> > So I think it should round down (ie use a regular divide). No? > >> > >> Yes,you are correct, please ignore my last reply, and following is > >> the fixed patch. > > > > psize and video->urb_size shouldn't have changed before and after resume, > > otherwise we'll get into trouble anyway. A regular divide and a round-up > > divide should then return the same result. I'll take the regular divide, > > as it will be more efficient. > > Yes. > > >> Thanks. > >> > >> From a3b3d72cdd57a0699fb643b41b78eb7beb211ff5 Mon Sep 17 00:00:00 2001 > >> From: Ming Lei <tom.leiming@gmail.com> > >> Date: Wed, 15 Apr 2009 22:32:51 +0800 > >> Subject: [PATCH] V4L/DVB:usbvideo:fix uvc resume failed(v2) > >> > >> Now urb buffers is not freed before suspend, so uvc_alloc_urb_buffers > >> should return packet counts allocated originally during uvc resume > >> , instead of zero. > >> > >> This version uses round down to return packet counts on Linus's > >> suggestions, or else may lead to buffer destructed if packet size > >> is changed before calling uvc_alloc_urb_buffers() in this kind of > >> case. > > > > The comment is misleading. If the packet size changes we need to > > reallocate the buffers anyway. Have you checked if the packet size (which > > depends on the endpoint being selected) can be changed between suspend > > and resume, either by the uvcvideo driver (I don't think it can) or the > > USB core ? > > The packet size does not change between suspend and resume. I mean > uvc_alloc_urb_buffers() still can be used in other cases if buffers was not > freed and is reuesed in future. It seems there is no such cases in uvcvideo > now, but uvc_alloc_urb_buffers() really __can__ work in such case, isn't it? > > IMHO It is only used to allocate or reserve UVC_URBS usb buffers, which size > is video->urb_size, and npackets can be shortened or enlarged if psize is > changed, after all. You're right. Patch applied, thanks. Best regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 22 Apr 2009 01:21:10 +0200 Laurent Pinchart <laurent.pinchart@skynet.be> wrote: > Hi, > > On Tuesday 21 April 2009 03:47:34 Ming Lei wrote: > > 2009/4/21 Laurent Pinchart <laurent.pinchart@skynet.be>: > > > On Saturday 18 April 2009 06:51:11 leiming wrote: > > >> From a3b3d72cdd57a0699fb643b41b78eb7beb211ff5 Mon Sep 17 > > >> 00:00:00 2001 From: Ming Lei <tom.leiming@gmail.com> > > >> Date: Wed, 15 Apr 2009 22:32:51 +0800 > > >> Subject: [PATCH] V4L/DVB:usbvideo:fix uvc resume failed(v2) > > >> > > >> Now urb buffers is not freed before suspend, so > > >> uvc_alloc_urb_buffers should return packet counts allocated > > >> originally during uvc resume , instead of zero. > > >> > > >> This version uses round down to return packet counts on Linus's > > >> suggestions, or else may lead to buffer destructed if packet size > > >> is changed before calling uvc_alloc_urb_buffers() in this kind of > > >> case. > > > > > > The comment is misleading. If the packet size changes we need to > > > reallocate the buffers anyway. Have you checked if the packet > > > size (which depends on the endpoint being selected) can be > > > changed between suspend and resume, either by the uvcvideo driver > > > (I don't think it can) or the USB core ? > > > > The packet size does not change between suspend and resume. I mean > > uvc_alloc_urb_buffers() still can be used in other cases if buffers > > was not freed and is reuesed in future. It seems there is no such > > cases in uvcvideo now, but uvc_alloc_urb_buffers() really __can__ > > work in such case, isn't it? > > > > IMHO It is only used to allocate or reserve UVC_URBS usb buffers, > > which size is video->urb_size, and npackets can be shortened or > > enlarged if psize is changed, after all. > > You're right. Patch applied, thanks. Rc5 has been released today, why isn't this patch accepted by upstream now? It is really a bug fix. Thanks. > > Best regards, > > Laurent Pinchart >
On Sat, 9 May 2009, Ming Lei wrote: > > Rc5 has been released today, why isn't this patch accepted by upstream > now? It is really a bug fix. I can take it directly, but was hoping to get it through the regular DVB tree. Haven't had a DVB update request yet (or maybe it got lost?) Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 9 May 2009 09:24:51 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> escreveu: > > > On Sat, 9 May 2009, Ming Lei wrote: > > > > Rc5 has been released today, why isn't this patch accepted by upstream > > now? It is really a bug fix. > > I can take it directly, but was hoping to get it through the regular DVB > tree. Haven't had a DVB update request yet (or maybe it got lost?) > > Linus The patch were added on my linux-next tree. I'll move it to the tree I handle bug fixes and I'll ask Linus to pull from it together with a few other fixes I have there, later today or tomorrow. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/uvc/uvc_video.c b/drivers/media/video/uvc/uvc_video.c index a95e173..6ce974d 100644 --- a/drivers/media/video/uvc/uvc_video.c +++ b/drivers/media/video/uvc/uvc_video.c @@ -742,7 +742,7 @@ static int uvc_alloc_urb_buffers(struct uvc_video_device *video, /* Buffers are already allocated, bail out. */ if (video->urb_size) - return 0; + return video->urb_size / psize; /* Compute the number of packets. Bulk endpoints might transfer UVC * payloads accross multiple URBs.