From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753416Ab2GTK0V (ORCPT ); Fri, 20 Jul 2012 06:26:21 -0400 Received: from bishop.asidev.net ([95.141.38.214]:44790 "EHLO bishop.asidev.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751490Ab2GTK0U (ORCPT ); Fri, 20 Jul 2012 06:26:20 -0400 Message-ID: <50093242.8030803@evidence.eu.com> Date: Fri, 20 Jul 2012 12:26:10 +0200 From: Claudio Scordino User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.28) Gecko/20120313 Lightning/1.0b2 Thunderbird/3.1.20 MIME-Version: 1.0 To: Greg KH 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 References: <4FEB2E63.804@evidence.eu.com> <20120706174159.GA5715@kroah.com> <50067975.9010002@evidence.eu.com> <20120719225816.GA6672@kroah.com> In-Reply-To: <20120719225816.GA6672@kroah.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 ? Many thanks, Claudio