Re: [PATCH] USB: output an error message when the pipe type doesn't match the endpoint type

[Simon Arlott <simon@fire.lp0.eu>]

On 31/08/10 15:16, Alan Stern wrote:
> On Mon, 30 Aug 2010, Simon Arlott wrote:
>> Commit f661c6f8c67bd55e93348f160d590ff9edf08904 adds a check of the pipe type if
>> CONFIG_USB_DEBUG is enabled, but it doesn't output anything if this scenario
>> occurs.
>> 
>> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
>> index 419e6b3..c14fc08 100644
>> --- a/drivers/usb/core/urb.c
>> +++ b/drivers/usb/core/urb.c
>> @@ -401,8 +401,11 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>>  	/* Check that the pipe's type matches the endpoint's type */
>> -	if (usb_pipetype(urb->pipe) != pipetypes[xfertype])
>> +	if (usb_pipetype(urb->pipe) != pipetypes[xfertype]) {
>> +		dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
>> +			usb_pipetype(urb->pipe), pipetypes[xfertype]);
>>  		return -EPIPE;		/* The most suitable error code :-) */
>> +	}
> 
> This is okay with me.  If you're serious about not changing the
> behavior merely because debugging is enabled, you could move this test
> out of the debug-only region and possibly change the dev_err to
> dev_dbg.  However doing so might break some devices that are currently
> working.

I'd expect that to break potentially many devices, although only cxacru
stopped working for me. The USB API isn't really suitable for adding
this type of check because it allows the drivers to get away with too
much already.

usb_clear_halt() takes a pipe when it really wants the endpoint, the
pipe type is ignored.

usb_bulk_msg() auto-detects the type between interrupt and bulk, as
does usb_interrupt_msg() because the latter just calls the former.

(I think -EINVAL might be a better return code. The pipe isn't broken,
it doesn't exist.)

-- 
Simon Arlott