Messages in this thread Patch in this message | | | Date | Wed, 18 Jul 2012 10:53:09 +0200 | From | Claudio Scordino <> | Subject | Re: [PATCH] isp1362-hcd.c: usb message always saved in case of underrun |
| |
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.
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 | 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
| |