From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752861Ab2GTPP1 (ORCPT ); Fri, 20 Jul 2012 11:15:27 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:43905 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751912Ab2GTPP0 (ORCPT ); Fri, 20 Jul 2012 11:15:26 -0400 Date: Fri, 20 Jul 2012 08:15:21 -0700 From: Greg KH To: Claudio Scordino Cc: ok@artecdesign.ee, linux-usb@vger.kernel.org, Linux Kernel , bruno Subject: Re: [PATCH] isp1362-hcd.c: usb message always saved in case of underrun Message-ID: <20120720151521.GA3495@kroah.com> References: <4FEB2E63.804@evidence.eu.com> <20120706174159.GA5715@kroah.com> <50067975.9010002@evidence.eu.com> <20120719225816.GA6672@kroah.com> <50093242.8030803@evidence.eu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50093242.8030803@evidence.eu.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 > >>>> > >>>>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 > >>>>Signed-off-by: Claudio Scordino > >>>>Tested-by: Bruno Morelli > >>>>--- > >>>> 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