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@xxxxxxxxxxxxxxx>
> >>>>
> >>>>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@xxxxxxxxxxxxxxx>
> >>>>Signed-off-by: Claudio Scordino<claudio@xxxxxxxxxxxxxxx>
> >>>>Tested-by: Bruno Morelli<bruno@xxxxxxxxxxxxxxx>
> >>>>---
> >>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
