From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754197Ab2G3N1A (ORCPT ); Mon, 30 Jul 2012 09:27:00 -0400 Received: from bishop.asidev.net ([95.141.38.214]:36957 "EHLO bishop.asidev.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754125Ab2G3N07 (ORCPT ); Mon, 30 Jul 2012 09:26:59 -0400 Message-ID: <50168B9A.5080605@evidence.eu.com> Date: Mon, 30 Jul 2012 15:26:50 +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> <50093242.8030803@evidence.eu.com> <20120720151521.GA3495@kroah.com> In-Reply-To: <20120720151521.GA3495@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 17:15, Greg KH ha scritto: > 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. Hi Greg. I've just refreshed the patch by cloning the repo from scratch and creating the patch against the "usb-next" branch. You can find the patch below, but the content is still the same. Can you please check it again ? Is it possible that you have local changes not yet committed ? Many thanks, 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 | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/usb/host/isp1362-hcd.c b/drivers/usb/host/isp1362-hcd.c index 2ed112d..2563263 100644 --- a/drivers/usb/host/isp1362-hcd.c +++ b/drivers/usb/host/isp1362-hcd.c @@ -543,12 +543,12 @@ 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); 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); if (urb->status == -EINPROGRESS) -- 1.7.1