Re: [PATCH] isp1362-hcd.c: usb message always saved in case of underrun

[Claudio Scordino]

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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Claudio Scordino<claudio@xxxxxxxxxxxxxxx>
> > Tested-by: Bruno Morelli<bruno@xxxxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxx>
Signed-off-by: Claudio Scordino <claudio@xxxxxxxxxxxxxxx>
Tested-by: Bruno Morelli <bruno@xxxxxxxxxxxxxxx>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/