From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752310Ab2GSW6W (ORCPT ); Thu, 19 Jul 2012 18:58:22 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:39181 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751114Ab2GSW6U (ORCPT ); Thu, 19 Jul 2012 18:58:20 -0400 Date: Thu, 19 Jul 2012 15:58:16 -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: <20120719225816.GA6672@kroah.com> References: <4FEB2E63.804@evidence.eu.com> <20120706174159.GA5715@kroah.com> <50067975.9010002@evidence.eu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50067975.9010002@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 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? thanks, greg k-h