Messages in this thread Patch in this message | | | Date | Mon, 30 Jul 2012 15:26:50 +0200 | From | Claudio Scordino <> | Subject | Re: [PATCH] isp1362-hcd.c: usb message always saved in case of underrun |
| |
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<bruno@evidence.eu.com> >>>>>> >>>>>> 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@evidence.eu.com> >>>>>> Signed-off-by: Claudio Scordino<claudio@evidence.eu.com> >>>>>> Tested-by: Bruno Morelli<bruno@evidence.eu.com> >>>>>> --- >>>>>> 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 <bruno@evidence.eu.com>
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@evidence.eu.com> Signed-off-by: Claudio Scordino <claudio@evidence.eu.com> Tested-by: Bruno Morelli <bruno@evidence.eu.com> --- 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
| |