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

[Greg KH <gregkh@linuxfoundation.org>]

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.

greg k-h