Messages in this thread | | | Date | Fri, 20 Jul 2012 12:26:10 +0200 | From | Claudio Scordino <> | Subject | Re: [PATCH] isp1362-hcd.c: usb message always saved in case of underrun |
| |
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 ?
Many thanks,
Claudio
| |