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<br...@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<br...@evidence.eu.com> > >>Signed-off-by: Claudio Scordino<clau...@evidence.eu.com> > >>Tested-by: Bruno Morelli<br...@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? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html