Re: [PATCH] isp1362-hcd.c: usb message always saved in case of underrun

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Jul 20, 2012 at 12:26:10PM +0200, Claudio Scordino wrote:
> Il 20/07/2012 00:58, Greg KH ha scritto:
> >On Wed, Jul 18, 2012 at 10:53:09AM +0200, Claudio Scordino wrote:
> >>Il 06/07/2012 19:41, Greg KH ha scritto:
> >>>On Wed, Jun 27, 2012 at 06:01:39PM +0200, Claudio Scordino wrote:
> >>>>Hi Olav,
> >>>>
> >>>>	please find below a patch for the isp1362-hcd.c driver to always
> >>>>save the message in case of underrun. More information is provided
> >>>>inside the patch comment. Let us know if you need any further
> >>>>information.
> >>>>
> >>>>Best regards,
> >>>>
> >>>>	Claudio
> >>>>
> >>>>
> >>>>Subject: isp1362-hcd.c: usb message always saved in case of underrun
> >>>>From: Bruno Morelli<bruno@evidence.eu.com>
> >>>>
> >>>>The usb message must be saved also in case the USB endpoint is not a
> >>>>control endpoint (i.e., "endpoint 0"), otherwise in some circumstances
> >>>>we don't have a payload in case of error.
> >>>>
> >>>>The patch has been created by tracing with usbmon the different error
> >>>>messages generated by this driver with respect to the ehci-hcd driver.
> >>>>
> >>>>Signed-off-by: Bruno Morelli<bruno@evidence.eu.com>
> >>>>Signed-off-by: Claudio Scordino<claudio@evidence.eu.com>
> >>>>Tested-by: Bruno Morelli<bruno@evidence.eu.com>
> >>>>---
> >>>>  drivers/usb/host/isp1362-hcd.c |   11 ++++++-----
> >>>>  1 files changed, 6 insertions(+), 5 deletions(-)
> >>>>
> >>>>diff --git a/drivers/usb/host/isp1362-hcd.c b/drivers/usb/host/isp1362-hcd.c
> >>>>index 2ed112d..61bf1b2 100644
> >>>>--- a/drivers/usb/host/isp1362-hcd.c
> >>>>+++ b/drivers/usb/host/isp1362-hcd.c
> >>>>@@ -543,13 +543,14 @@ static void postproc_ep(struct isp1362_hcd *isp1362_hcd, struct isp1362_ep *ep)
> >>>>  			    usb_pipein(urb->pipe) ? "IN" : "OUT", ep->nextpid,
> >>>>  			    short_ok ? "" : "not_",
> >>>>  			    PTD_GET_COUNT(ptd), ep->maxpacket, len);
> >>>>+			/* save the data underrun error code for later and
> >>>>+			 * proceed with the status stage
> >>>>+			 */
> >>>>+			urb->actual_length += PTD_GET_COUNT(ptd);
> >>>>+			BUG_ON(urb->actual_length>
> >>>>+			    urb->transfer_buffer_length);
> >>>
> >>>Please NEVER crash the machine in a driver like this, it's bad and can
> >>>cause problems.  Yes, I know you are just moving it from the lines
> >>>below:
> >>>
> >>>>  			if (usb_pipecontrol(urb->pipe)) {
> >>>>  				ep->nextpid = USB_PID_ACK;
> >>>>-				/* save the data underrun error code for later and
> >>>>-				 * proceed with the status stage
> >>>>-				 */
> >>>>-				urb->actual_length += PTD_GET_COUNT(ptd);
> >>>>-				BUG_ON(urb->actual_length>   urb->transfer_buffer_length);
> >>>
> >>>But really, it should not be in the driver at all.  Please remove it, at
> >>>the most, do a WARN_ON() so that someone can see the problem and at
> >>>least report it.
> >>>
> >>>Actually, what is this checking?  How can someone recover from it?  Who
> >>>is this check for?  The developer of this driver?  Another driver?
> >>>Hardware developer?  End user?  Who would be able to fix the problem if
> >>>this happens?
> >>>
> >>>As it is, I can't take it, sorry.
> >>
> >>
> >>Hi Greg.
> >>
> >>I understand. As you have already said, this driver is full of BUG_ON
> >>statements.
> >>
> >>So, we can shift just the assignment as in the patch below, to have a
> >>correct behavior, leaving the BUG_ON where it already is. Then, we may
> >>propose a different patch to change BUG_ONs to WARN_ONs.
> >
> >Your updated patch is much better, thanks.
> >
> >But it doesn't apply to my tree right now.  Can you please refresh it
> >against the usb-next tree and resend it?
> 
> Actually, I did.
> 
> So, this means that I'm using the wrong tree...
> 
> I'm using the "usb-next" branch available on your tree at
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> 
> Is this the wrong one ?

That is the correct one.  It didn't work for me, so try refreshing your
patch and resending it.

thanks,

greg k-h