2.6.30-rc2-git2: Reported regressions from 2.6.29

Commit Message

Ming Lei April 18, 2009, 4:51 a.m. UTC

On Fri, 17 Apr 2009 19:55:29 -0700 (PDT)
Linus Torvalds <[e-mail]> 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.

From a3b3d72cdd57a0699fb643b41b78eb7beb211ff5 Mon Sep 17 00:00:00 2001
From: Ming Lei <[e-mail]>
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 <[e-mail]>
---
 drivers/media/video/uvc/uvc_video.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Rafael Wysocki April 18, 2009, 12:33 p.m. UTC | #1

On Saturday 18 April 2009, leiming wrote:
> On Fri, 17 Apr 2009 19:55:29 -0700 (PDT)
> Linus Torvalds <[e-mail]> 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 <[e-mail]>
> 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 <[e-mail]>
> ---
>  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 linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart April 20, 2009, 8:08 p.m. UTC | #2

On Saturday 18 April 2009 06:51:11 leiming wrote:
> On Fri, 17 Apr 2009 19:55:29 -0700 (PDT)
>
> Linus Torvalds <[e-mail]> 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 <[e-mail]>
> 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 linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ming Lei April 21, 2009, 1:47 a.m. UTC | #3

2009/4/21 Laurent Pinchart <[e-mail]>:
> On Saturday 18 April 2009 06:51:11 leiming wrote:
>> On Fri, 17 Apr 2009 19:55:29 -0700 (PDT)
>>
>> Linus Torvalds <[e-mail]> 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 <[e-mail]>
>> 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!

Laurent Pinchart April 21, 2009, 11:21 p.m. UTC | #4

Hi,

On Tuesday 21 April 2009 03:47:34 Ming Lei wrote:
> 2009/4/21 Laurent Pinchart <[e-mail]>:
> > On Saturday 18 April 2009 06:51:11 leiming wrote:
> >> On Fri, 17 Apr 2009 19:55:29 -0700 (PDT)
> >>
> >> Linus Torvalds <[e-mail]> 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 <[e-mail]>
> >> 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 linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ming Lei May 9, 2009, 3:28 a.m. UTC | #5

On Wed, 22 Apr 2009 01:21:10 +0200
Laurent Pinchart <[e-mail]> wrote:

> Hi,
> 
> On Tuesday 21 April 2009 03:47:34 Ming Lei wrote:
> > 2009/4/21 Laurent Pinchart <[e-mail]>:
> > > On Saturday 18 April 2009 06:51:11 leiming wrote:

> > >> From a3b3d72cdd57a0699fb643b41b78eb7beb211ff5 Mon Sep 17
> > >> 00:00:00 2001 From: Ming Lei <[e-mail]>
> > >> 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
>

Linus Torvalds May 9, 2009, 4:24 p.m. UTC | #6

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 linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab May 9, 2009, 9:37 p.m. UTC | #7

Em Sat, 9 May 2009 09:24:51 -0700 (PDT)
Linus Torvalds <[e-mail]> 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 linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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.