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 <[email protected]>
Cc: Alan Stern <[email protected]>
---
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
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
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
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
On Tue, Aug 31, 2010 at 9:52 PM, Alan Stern <[email protected]> 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
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 <[email protected]>
> Cc: Alan Stern <[email protected]>
> ---
> 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