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

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

[Simon Arlott]

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.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
Cc: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/usb/core/urb.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

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 :-) */
+	}
 
 	/* enforce simple/standard policy */
 	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
-- 
1.7.0.4
-- 
Simon Arlott

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

[Clemens Ladisch]

Simon Arlott wrote:
> Commit f661c6f8c67bd55e93348f160d590ff9edf08904 adds a check of the pipe type if
> CONFIG_USB_DEBUG is enabled,

I didn't see that commit last year, but wouldn't it break devices like
the ESI MIDI Mate whose descriptors want to have low-speed bulk transfers
and where the driver has to submit interrupt transfers instead to get it
to work at all?


Regards,
Clemens

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

[Simon Arlott]

On Tue, August 31, 2010 07:41, Clemens Ladisch wrote:
> Simon Arlott wrote:
>> Commit f661c6f8c67bd55e93348f160d590ff9edf08904 adds a check of the pipe type if
>> CONFIG_USB_DEBUG is enabled,
>
> I didn't see that commit last year, but wouldn't it break devices like
> the ESI MIDI Mate whose descriptors want to have low-speed bulk transfers
> and where the driver has to submit interrupt transfers instead to get it
> to work at all?

If the transfer type doesn't match the pipe type then yes, but only when
CONFIG_USB_DEBUG is enabled.

Perhaps these need to be moved to a CONFIG_USB_STRICT. Having a "debug" mode
change the behaviour is not a good idea, but some of the code in that ifdef
has been there since the import to git.

Regardless of which configuration option enables it, the pipe type
check needs an error message to explain why the driver received -EPIPE.

-- 
Simon Arlott

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

[Alan Stern]

On Tue, 31 Aug 2010, Clemens Ladisch wrote:

> Simon Arlott wrote:
> > Commit f661c6f8c67bd55e93348f160d590ff9edf08904 adds a check of the pipe type if
> > CONFIG_USB_DEBUG is enabled,
> 
> I didn't see that commit last year, but wouldn't it break devices like
> the ESI MIDI Mate whose descriptors want to have low-speed bulk transfers
> and where the driver has to submit interrupt transfers instead to get it
> to work at all?

When the kernel sees those invalid low-speed bulk endpoint descriptors,
it changes its internal copy of the descriptor to interrupt.  Hence
when the driver submits interrupt URBs, they work correctly.

Alan Stern

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

[Xiaofan Chen]

On Tue, Aug 31, 2010 at 9:52 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 31 Aug 2010, Clemens Ladisch wrote:
>
>> Simon Arlott wrote:
>> > Commit f661c6f8c67bd55e93348f160d590ff9edf08904 adds a check of the pipe type if
>> > CONFIG_USB_DEBUG is enabled,
>>
>> I didn't see that commit last year, but wouldn't it break devices like
>> the ESI MIDI Mate whose descriptors want to have low-speed bulk transfers
>> and where the driver has to submit interrupt transfers instead to get it
>> to work at all?
>
> When the kernel sees those invalid low-speed bulk endpoint descriptors,
> it changes its internal copy of the descriptor to interrupt.  Hence
> when the driver submits interrupt URBs, they work correctly.
>

Interesting.

FYI: even though XP/Win2k allow low-speed bulk endpoints, Vista
and Win7 do not allow that.

There are hacks to get around that.
http://www.recursion.jp/avrcdc/lowbulk.html

I took a look at the libusb-win32 source and indeed the author
is using the same trick of submit interrupt URBs.


-- 
Xiaofan

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

[Alan Stern]

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.
> 
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> ---
>  drivers/usb/core/urb.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> 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 :-) */
> +	}
>  
>  	/* enforce simple/standard policy */
>  	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |

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.

Alan Stern

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

[Simon Arlott]

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

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

[Alan Stern]

On Wed, 1 Sep 2010, Simon Arlott wrote:

> > 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.

Unlike device hardware, drivers can always be changed.  If adding a 
check will help spot errors, it's probably worthwhile.

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

What's wrong with that?  Besides, in the end we shouldn't be using
pipes at all; we should always use pointers to struct
usb_host_endpoint.

> 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.)

Within the kernel, the meanings of the -Exxx error codes are fairly
arbitrary.  But since this code can be passed to userspace you are
right.

Alan Stern

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

[Simon Arlott]

On Wed, September 1, 2010 18:49, Alan Stern wrote:
> On Wed, 1 Sep 2010, Simon Arlott wrote:
>> > 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.
>
> Unlike device hardware, drivers can always be changed.  If adding a
> check will help spot errors, it's probably worthwhile.

Yes, however the information about the device endpoint types hasn't been
required in the past for the driver to work. The only way to check is to
have the hardware available so the error may only show up after a full
release of the kernel and break drivers that used to work. It could be
enabled for all -rc kernels...

>> usb_clear_halt() takes a pipe when it really wants the endpoint, the
>> pipe type is ignored.
>
> What's wrong with that?  Besides, in the end we shouldn't be using
> pipes at all; we should always use pointers to struct
> usb_host_endpoint.

If a driver was trying to conform to the "don't use the wrong pipe type
with an endpoint" rule then it may have to check it was using the
correct pipe when calling usb_clear_halt(), although this is only a
problem with drivers where the devices sometimes have interrupt instead
of bulk endpoints.

Looking at usb_clear_halt(), it doesn't use the direction either... but
drivers can call it in both directions. Several drivers already do this.

-- 
Simon Arlott

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

[Alan Stern]

On Thu, 2 Sep 2010, Simon Arlott wrote:

> On Wed, September 1, 2010 18:49, Alan Stern wrote:
> > On Wed, 1 Sep 2010, Simon Arlott wrote:
> >> > 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.
> >
> > Unlike device hardware, drivers can always be changed.  If adding a
> > check will help spot errors, it's probably worthwhile.
> 
> Yes, however the information about the device endpoint types hasn't been
> required in the past for the driver to work. The only way to check is to
> have the hardware available so the error may only show up after a full
> release of the kernel and break drivers that used to work. It could be
> enabled for all -rc kernels...

Which suggests that the best approach is to print the error message 
always, but allow the submission unless CONFIG_USB_DEBUG is set.

> >> usb_clear_halt() takes a pipe when it really wants the endpoint, the
> >> pipe type is ignored.
> >
> > What's wrong with that?  Besides, in the end we shouldn't be using
> > pipes at all; we should always use pointers to struct
> > usb_host_endpoint.
> 
> If a driver was trying to conform to the "don't use the wrong pipe type
> with an endpoint" rule then it may have to check it was using the
> correct pipe when calling usb_clear_halt(), although this is only a
> problem with drivers where the devices sometimes have interrupt instead
> of bulk endpoints.
> 
> Looking at usb_clear_halt(), it doesn't use the direction either... but
> drivers can call it in both directions. Several drivers already do this.

What do you mean?  Look at the first lines of code in usb_clear_halt():

	if (usb_pipein(pipe))
		endp |= USB_DIR_IN;

Alan Stern

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

[Simon Arlott]

On 02/09/10 15:22, Alan Stern wrote:
> On Thu, 2 Sep 2010, Simon Arlott wrote:
> 
>> On Wed, September 1, 2010 18:49, Alan Stern wrote:
>> > On Wed, 1 Sep 2010, Simon Arlott wrote:
>> >> > 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.
> 
> Which suggests that the best approach is to print the error message 
> always, but allow the submission unless CONFIG_USB_DEBUG is set.

That could result in a lot of error messages, and it doesn't give any
information on which driver caused it, but a single WARN_ON() would
only be triggered once globally. Each USB device (or endpoint) could
have a "warned" flag.

>> Looking at usb_clear_halt(), it doesn't use the direction either... but
>> drivers can call it in both directions. Several drivers already do this.
> 
> What do you mean?  Look at the first lines of code in usb_clear_halt():
> 
> 	if (usb_pipein(pipe))
> 		endp |= USB_DIR_IN;

Sorry, I misread it as always selecting the same direction.

-- 
Simon Arlott

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

[Alan Stern]

On Thu, 2 Sep 2010, Simon Arlott wrote:

> On 02/09/10 15:22, Alan Stern wrote:
> > On Thu, 2 Sep 2010, Simon Arlott wrote:
> > 
> >> On Wed, September 1, 2010 18:49, Alan Stern wrote:
> >> > On Wed, 1 Sep 2010, Simon Arlott wrote:
> >> >> > 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.
> > 
> > Which suggests that the best approach is to print the error message 
> > always, but allow the submission unless CONFIG_USB_DEBUG is set.
> 
> That could result in a lot of error messages, and it doesn't give any
> information on which driver caused it,

It would if you added the endpoint number and direction to the error 
message.

>  but a single WARN_ON() would
> only be triggered once globally. Each USB device (or endpoint) could
> have a "warned" flag.

That seems ridiculous, especially for such a rare bug.

Alan Stern