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

PATCH In review Simon Arlott 4 participants 12 messages Aug 30, 2010 – Sep 2, 2010 USB

Share & tools

Simon Arlott

author

2010-08-30 21:56 UTC #0 lore raw

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

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

drivers/usb/core/urb.c +4 -1

index 419e6b3..c14fc08 100644
@@ -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 |

Signature · 3 lines

1.7.0.4
-- 
Simon Arlott

Clemens Ladisch

2010-08-31 06:40 UTC #1 lore raw

Quoted 2 lines · Simon Arlott

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

Simon Arlott

author

2010-08-31 11:31 UTC #2 lore raw

Quoted 8 lines · Clemens Ladisch

Quoted 2 lines · Simon Arlott
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.

Signature · 1 line

Simon Arlott

Alan Stern

2010-08-31 13:52 UTC #3 lore raw

In reply to Clemens Ladisch

Quoted 8 lines · Clemens Ladisch

Quoted 2 lines · Simon Arlott
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

Xiaofan Chen

2010-08-31 14:04 UTC #4 lore raw

Quoted 6 lines · Alan Stern

Quoted 4 lines · Clemens Ladisch
Quoted 2 lines · Simon Arlott
Commit f661c6f8c67bd55e93348f160d590ff9edf08904 adds a check of the pipe type if CONFIG_USB_DEBUG is enabled,

Quoted 9 lines

Quoted 4 lines
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.

Signature · 1 line

Xiaofan

Alan Stern

2010-08-31 14:16 UTC #5 lore raw

In reply to Simon Arlott

Quoted 17 lines · 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 <[email protected]>
Cc Alan Stern <[email protected]>
---
 drivers/usb/core/urb.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/usb/core/urb.c +0 -0
index 419e6b3..c14fc08 100644
@@ -401,8 +401,11 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	};
 

drivers/usb/core/urb.c int usb_submit_urb(struct urb *urb, gfp_t mem_flags) · line 411

Quoted 10 lines

drivers/usb/core/urb.c +4 -1

 	/* 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

Simon Arlott

author

2010-09-01 17:05 UTC #6 lore raw

Quoted 15 lines · Alan Stern

Quoted 14 lines · 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.
drivers/usb/core/urb.c +3 -1
index 419e6b3..c14fc08 100644
@@ -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]);

Quoted 8 lines

drivers/usb/core/urb.c int usb_submit_urb(struct urb *urb, gfp_t mem_flags) · line 408
Quoted 2 lines
drivers/usb/core/urb.c +1 -0
 		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.)

Signature · 1 line

Simon Arlott

Alan Stern

2010-09-01 17:49 UTC #7 lore raw

Quoted 10 lines · Simon Arlott

Quoted 5 lines
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.

Quoted 2 lines

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.

Quoted 5 lines

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

Simon Arlott

author

2010-09-02 11:56 UTC #8 lore raw

Quoted 7 lines · Alan Stern

Quoted 6 lines · Simon Arlott
Quoted 5 lines
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.

Quoted 7 lines

Quoted 4 lines
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...

Quoted 6 lines

Quoted 2 lines
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.

Signature · 1 line

Simon Arlott

Alan Stern

2010-09-02 14:22 UTC #9 lore raw

Quoted 13 lines · Simon Arlott

Quoted 12 lines · Alan Stern
Quoted 10 lines · Simon Arlott
Quoted 5 lines
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.

Quoted 8 lines

Quoted 2 lines
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.

Quoted 7 lines

Quoted 6 lines
Quoted 2 lines
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.

Quoted 8 lines

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

Simon Arlott

author

2010-09-02 17:11 UTC #10 lore raw

Quoted 4 lines · Alan Stern

Quoted 2 lines · Simon Arlott
Quoted 1 line · Alan Stern
On Wed, 1 Sep 2010, Simon Arlott wrote:

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Signature · 1 line

Simon Arlott

Alan Stern

2010-09-02 19:20 UTC #11 lore raw

Quoted 11 lines · Simon Arlott

Quoted 10 lines · Alan Stern
Quoted 7 lines · Simon Arlott
Quoted 6 lines · Alan Stern
Quoted 5 lines · Simon Arlott
Quoted 5 lines
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 3 lines

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