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

[Claudio Scordino <claudio@evidence.eu.com>]

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