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

[Greg KH <gregkh@linuxfoundation.org>]

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?

thanks,

greg k-h