New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document upper limits on control transfers (INVALID_PARAMETER) (re: MAX_CTRL_BUFFER_LENGTH) #110

Closed
karlp opened this Issue Oct 11, 2015 · 10 comments

Comments

Projects
None yet
4 participants
@karlp

karlp commented Oct 11, 2015

What's the purpose of this limit in submit_control_transfer()?

You must provide a user buffer in control_transfer, and I can't see any internal buffers using this size, it seems to be just a completely irrelevant check for no reason at all.

It's only checked on windows and linux anyway. Can this just be dropped?

@diabolo38

This comment has been minimized.

Show comment
Hide comment
@diabolo38

diabolo38 Oct 11, 2015

it is likely to check in advance known limit of usb and on the os
https://msdn.microsoft.com/en-us/library/windows/hardware/ff538112%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396

this is how the MAX_CTRL_BUFFER_LENGTH seam to be used in the window part.
unless it is to prevent issue on the respective back-end you right the test could be dropped
It would give an error later on submission.

diabolo38 commented Oct 11, 2015

it is likely to check in advance known limit of usb and on the os
https://msdn.microsoft.com/en-us/library/windows/hardware/ff538112%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396

this is how the MAX_CTRL_BUFFER_LENGTH seam to be used in the window part.
unless it is to prevent issue on the respective back-end you right the test could be dropped
It would give an error later on submission.

@karlp

This comment has been minimized.

Show comment
Hide comment
@karlp

karlp Oct 11, 2015

ok, I can see it being useful for windows. However, when you hit this, all you get is in "invalid parameter" return value, and nothing with LIBUSB_DEBUG=99 or anything useful to indicate what is possibly an invalid parameter. I'd like to see it just dropped from linux backend and the feedback or docs improved. Digging the source to try and work out where on earth invalid parameter was coming from wasn't much fun. (I expected a 10k control transfer to fail, but I was expecting it to fail with a stall from my device, not "invalid parameter" inside libusb!)

karlp commented Oct 11, 2015

ok, I can see it being useful for windows. However, when you hit this, all you get is in "invalid parameter" return value, and nothing with LIBUSB_DEBUG=99 or anything useful to indicate what is possibly an invalid parameter. I'd like to see it just dropped from linux backend and the feedback or docs improved. Digging the source to try and work out where on earth invalid parameter was coming from wasn't much fun. (I expected a 10k control transfer to fail, but I was expecting it to fail with a stall from my device, not "invalid parameter" inside libusb!)

@diabolo38

This comment has been minimized.

Show comment
Hide comment
@diabolo38

diabolo38 Oct 11, 2015

Even if libsub is not 100% correct here , using such large control is not 100% correct up to me.
Control pipe are no meant for data exchange usb chapter 5 (5.5) is clear on that.

Control might look handy at first for data exchange in both device and host side but it is not quite the right way to go in most case.

At least if your device is to be used on window you know now you can't rely on large control :)

diabolo38 commented Oct 11, 2015

Even if libsub is not 100% correct here , using such large control is not 100% correct up to me.
Control pipe are no meant for data exchange usb chapter 5 (5.5) is clear on that.

Control might look handy at first for data exchange in both device and host side but it is not quite the right way to go in most case.

At least if your device is to be used on window you know now you can't rely on large control :)

@karlp

This comment has been minimized.

Show comment
Hide comment
@karlp

karlp Oct 11, 2015

Please don't get distracted by questioning what I'm doing :) I'm most certainly not trying to use ctrl transfers for real data exchange. I was attempting to make an "excessive" ctrl transfer to test that the firmware and usb stack behaved correctly in "strange" environments. Regardless, it's legal and valid and there's nothing in section 5.5 that says you can't do it at all. Only that it's best effort. Again, I've no plans on actually doing this, but it's completely legal.

What this is filed for is that I don't think it's libusb's place to put in unnecessary limits (for linux) and if you do have hard internal limits, then they should be better documented than just getting "invalid parameter". Even just as simple as adding INVALID_PARAMETER to http://libusb.sourceforge.net/api-1.0/group__syncio.html#gadb11f7a761bd12fc77a07f4568d56f38 would probably have helped.

karlp commented Oct 11, 2015

Please don't get distracted by questioning what I'm doing :) I'm most certainly not trying to use ctrl transfers for real data exchange. I was attempting to make an "excessive" ctrl transfer to test that the firmware and usb stack behaved correctly in "strange" environments. Regardless, it's legal and valid and there's nothing in section 5.5 that says you can't do it at all. Only that it's best effort. Again, I've no plans on actually doing this, but it's completely legal.

What this is filed for is that I don't think it's libusb's place to put in unnecessary limits (for linux) and if you do have hard internal limits, then they should be better documented than just getting "invalid parameter". Even just as simple as adding INVALID_PARAMETER to http://libusb.sourceforge.net/api-1.0/group__syncio.html#gadb11f7a761bd12fc77a07f4568d56f38 would probably have helped.

@vpelletier

This comment has been minimized.

Show comment
Hide comment
@vpelletier

vpelletier Oct 11, 2015

Just for the pointer: I asked a similar question on the libusb ML earlier this year. Alan mentionned some Linux HCI also do (did ?) not handle control data stages larger than 0x1000.

vpelletier commented Oct 11, 2015

Just for the pointer: I asked a similar question on the libusb ML earlier this year. Alan mentionned some Linux HCI also do (did ?) not handle control data stages larger than 0x1000.

@karlp

This comment has been minimized.

Show comment
Hide comment
@karlp

karlp Oct 12, 2015

All the more reason for this to be documented properly. This was known by people, but is not explained in the API documentation, nor the code itself.

karlp commented Oct 12, 2015

All the more reason for this to be documented properly. This was known by people, but is not explained in the API documentation, nor the code itself.

@karlp karlp changed the title from unnecessary limits: MAX_CTRL_BUFFER_LENGTH to Document upper limits on control transfers (INVALID_PARAMETER) (re: MAX_CTRL_BUFFER_LENGTH) Oct 12, 2015

@diverger

This comment has been minimized.

Show comment
Hide comment
@diverger

diverger Oct 16, 2015

@michel, thanks for your given link. Then does it mean we must limit our data size below some limit when call "libusb_bulk_transfer" ?Because MS state there are some limit on bulk transfer in some xHCI implementation. Or MS have split the data to smaller chunks at lower layer for us?

Date: Mon, 12 Oct 2015 03:15:06 -0700
From: notifications@github.com
To: libusb@noreply.github.com
Subject: Re: [libusb] unnecessary limits: MAX_CTRL_BUFFER_LENGTH (#110)

All the more reason for this to be documented properly. This was known by people, but is not explained in the API documentation, nor the code itself.


Reply to this email directly or view it on GitHub.

diverger commented Oct 16, 2015

@michel, thanks for your given link. Then does it mean we must limit our data size below some limit when call "libusb_bulk_transfer" ?Because MS state there are some limit on bulk transfer in some xHCI implementation. Or MS have split the data to smaller chunks at lower layer for us?

Date: Mon, 12 Oct 2015 03:15:06 -0700
From: notifications@github.com
To: libusb@noreply.github.com
Subject: Re: [libusb] unnecessary limits: MAX_CTRL_BUFFER_LENGTH (#110)

All the more reason for this to be documented properly. This was known by people, but is not explained in the API documentation, nor the code itself.


Reply to this email directly or view it on GitHub.

@diabolo38

This comment has been minimized.

Show comment
Hide comment
@diabolo38

diabolo38 Oct 16, 2015

ask microsoft ;) certainly both. Data Packeting (split) exist at several level (bus, driver, os memory, pages etc..)

i imagine already how the depth of a scatter-gather link-list, use of paged memory can come into picture to limit bulk or iso transfer length.

Even for end user theirs good reason to limit per request size,

I suspect it is hard to provides limits in the api doc due to multiplicity of os/arch and h/w but at least it is worth mentioning their are some.

diabolo38 commented Oct 16, 2015

ask microsoft ;) certainly both. Data Packeting (split) exist at several level (bus, driver, os memory, pages etc..)

i imagine already how the depth of a scatter-gather link-list, use of paged memory can come into picture to limit bulk or iso transfer length.

Even for end user theirs good reason to limit per request size,

I suspect it is hard to provides limits in the api doc due to multiplicity of os/arch and h/w but at least it is worth mentioning their are some.

@karlp

This comment has been minimized.

Show comment
Hide comment
@karlp

karlp Oct 16, 2015

I'd say it's pretty easy to put the limits in the API docs when they're actually enforced by hard numbers in libusb itself...

karlp commented Oct 16, 2015

I'd say it's pretty easy to put the limits in the API docs when they're actually enforced by hard numbers in libusb itself...

@diabolo38

This comment has been minimized.

Show comment
Hide comment
@diabolo38

diabolo38 Oct 17, 2015

sure it's easy it's matter of adding doxygen "@warn blabla" in a couple of function comment ;) what everybody can do and submit patch for.
Getting patch approved all the doc regenerated , and put back to the web site is an over story.

Litlle digging in the doc makeifle show that the api help pages on SF are updated by pbatard

I tried getting the doc regen localy to review changes prior to submit but didn't get it to work (under windows) litle help here will be appreciates.

patch now submited

diabolo38 commented Oct 17, 2015

sure it's easy it's matter of adding doxygen "@warn blabla" in a couple of function comment ;) what everybody can do and submit patch for.
Getting patch approved all the doc regenerated , and put back to the web site is an over story.

Litlle digging in the doc makeifle show that the api help pages on SF are updated by pbatard

I tried getting the doc regen localy to review changes prior to submit but didn't get it to work (under windows) litle help here will be appreciates.

patch now submited

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment