Check for invalid Frame List Sizes and enable Asynchronous Park Capability [gsoc2011]

[gabriel.hartmann]

In src/add-ons/kernel/busses/usb/ehci.cpp beginning of EHCI::Start()

• frameListSize was read but not checked against invalid values (see section 2.3.1 of the EHCI specification). The possible values are:

00b = 1024 elements, 01b = 512 elements, 10b = 256 elements, 11b = Reserved

• if the Asynchronous Park Capability bit in the EHCI_HCCPARAMS register is a one, enable EHCI_USBCMD_ASPME and set the count to 3.

[gabriel.hartmann]

I've added a patch that effects /src/add-ons/kernel/busses/usb/ehci.cpp and ehci.h

It has not been tested.

It should check cause EHCI::Start() to exit if it encounters a frameListSize equal to 11b.

It should also detect if the Asynchronous Park Capability bit is set, and enable EHCI_USBCMD_ASPME and set the count to 3 accordingly.

[gabriel.hartmann]

EHCI driver patch

[gabriel.hartmann]

Added a second patch which removes redundancy in calls to WriteOpReg() in two execution branches. Also added some masks to the input of WriteOpReg().

[mmlr]

Instead of putting everything in an additional if branch, please always try to return early. It makes the code easier to understand and in this case also avoids (ab-)using the "running" variable that is intended for the more narrow scope of the actual "running" check (hence the name).

[gabriel.hartmann]

This new patch returns as early as possible based on an invalid frame list size. It removes the additional if branch mentioned above and no longer relies on the "running" variable. I was under the impression that it was good practice to avoid having more than one return statement in a function, although I recognize adhering to this standard introduced some awkwardness. Would you consider early termination via a return statement to be generally desirable, or just within the scope of this particular situation? For example, should functions which deal with device startup always fail at the first opportunity?

[mmlr]

A few new remarks first:

• Since you don't use the "running" variable anymore, don't move it to the top, keep it where it was, i.e. define it where it's used.
• The output you do in case of encountering the unexpected frame list size doesn't really make sense: "host controller didn't start: invalid frame list size" seems to be a copy of the output that happens further down with "invalid frame list size" added. The "host controller didn't start" part should be dropped, as that's not what happened (that output is correct only after trying to start it but failing, i.e. what happens below). For the "invalid frame list size" part, I don't really like to word things like this way in case of "reserved" stuff. It is not invalid, it is reserved and I'd hate to have Haiku complain like that if it ever is unreserved. More correct IMO in this case would be "unsupported frame list size (%lu)", also outputting the offending value.
• If you look at the patch, it is obvious that you added some stuff and moved some parts around a few times. Ideally you'd clean up such leftovers once the changes are complete. In this case, remove the extra blank lines and the flip of the blank line and the trace as they don't belong with the actual change (and don't really fit the coding style of the rest of the file anyway). At the same time you'd notice the superfluous moving of the "running" variable, as already mentioned. Basically the fewer extra changes a patch includes the easier it is to understand what exactly was done.

Replying to gabriel.hartmann:

... I was under the impression that it was good practice to avoid having more than one return statement in a function, although I recognize adhering to this standard introduced some awkwardness.

Well, since not returning early doesn't scale well, I consider it bad practice. It becomes pretty obvious once you have more than two or three of those cases, where you basically nest so many if branches that you don't see what's really going on anymore. Combined with our 80 character per line limit, the space available for actual code also shrinks quickly once you nest a few branches and therefore it often becomes more messy.

Would you consider early termination via a return statement to be generally desirable, or just within the scope of this particular situation? For example, should functions which deal with device startup always fail at the first opportunity?

I'd say returning early is a good thing most of the time. It depends a bit on how much "cleanup" you have to do in the return case. If you always have to add like 10 lines of cleanup code in each early return then it is obviously more messy than if you just didn't return early. In this case though, where the only thing you do is print out an error and actually return, it makes much more sense. It also has the advantage that you can print out a very specific error message (like in the case above where "host controller didn't start" specifically only is printed for one case of error, hence making finding the offending part of the code easier when debugging).

Most of the time you can also "arrange" your code in a way that it becomes "early return friendly". Like in the constructor above where "fInitOK" is set to false at the very start of the constructor (after checking the init of the base class first though, as otherwise one would overwrite and lose that status). This allows the whole rest of the constructor to just return in case of error without having to set the error condition again. The same can be applied to other situations as well, it is not constructor or device startup specific. Personally I always try to return early and rearrange stuff as needed where it makes sense. It may take a bit more time to think it through and arrange things at first, but in the long run it IMO makes the code more readable and therefore easier to maintain and extended in the future (and you really get used to it and then it becomes natural to do it like that).

[korli]

Gabriel, it seems you made a wrong use of EHCI_USBCMD_ASPMC_MASK and EHCI_USBCMD_ITC_MASK. They should be used shifted and inverted, and before ORing another value, not after.

[bonefish]

Replying to gabriel.hartmann:

Would you consider early termination via a return statement to be generally desirable, or just within the scope of this particular situation?

To add to what mmlr wrote, there are convenient scope-based (i.e. RAII) AutoLocker and AutoDeleter classes which do not only simplify using the early-return style, but also help preventing programming errors like memory leaks and unbalanced locking. Both classes are rather flexible templates and already come with a bunch of handy specializations. For kernel locking needs a whole bunch of specializations is defined in headers/private/kernel/util/AutoLock.h.

[gabriel.hartmann]

Replying to korli:

Gabriel, it seems you made a wrong use of EHCI_USBCMD_ASPMC_MASK and EHCI_USBCMD_ITC_MASK. They should be used shifted and inverted, and before ORing another value, not after.

Thanks. I was actually confused as to their use, since they didn't seem to be doing anything. Now I get it. Clear bits then apply new bits. Very helpful.

[gabriel.hartmann]

• Since you don't use the "running" variable anymore, don't move it to the top, keep it where it was, i.e. define it where it's used.

Fixed.

• The output you do in case of encountering the unexpected frame list size doesn't really make sense: "host controller didn't start: invalid frame list size" seems to be a copy of the output that happens further down with "invalid frame list size" added. The "host controller didn't start" part should be dropped, as that's not what happened (that output is correct only after trying to start it but failing, i.e. what happens below). For the "invalid frame list size" part, I don't really like to word things like this way in case of "reserved" stuff. It is not invalid, it is reserved and I'd hate to have Haiku complain like that if it ever is unreserved. More correct IMO in this case would be "unsupported frame list size (%lu)", also outputting the offending value.

I've changed the error message to the one you recommend. I originally retained the "host controller didn't start:" portion because there was a trace statement at the top that said "starting EHCI host controller," and because of the early return the error statement "host controller didn't start." never got reached.

• If you look at the patch, it is obvious that you added some stuff and moved some parts around a few times. Ideally you'd clean up such leftovers once the changes are complete. In this case, remove the extra blank lines and the flip of the blank line and the trace as they don't belong with the actual change (and don't really fit the coding style of the rest of the file anyway). At the same time you'd notice the superfluous moving of the "running" variable, as already mentioned. Basically the fewer extra changes a patch includes the easier it is to understand what exactly was done.

I've cleaned up the patch so that it's obvious what's been changed and there's no superfluous stuff hanging around.

In regards to early return style, I'm happy to adopt it. It seems more natural to me in any case. I was only avoiding it because I'd been taught multiple return statements are bad.

[mmlr]

A few more remarks:

• You should move the frame list size check to the top, i.e. right after reading it out. If you return early in that case there's no point in reading the capabilities and command registers.
• The trace output is fatal, so it should actually be TRACE_ERROR so it is always logged (the same is true for the "didn't start" error case that is already there).
• There's a space missing after the comma in the trace.
• You use "asyncParkCapable" like a boolean, so you should actually make it one. According to coding style by adding a "(...) != 0" around the statement.
• Since you use "asyncParkCapable" only once you could simply dump the variable and move the expression into the if clause. If you keep it, then move it to where it is used, i.e. directly above the if (coding style, declare variables where they are used).

A general remark, not trying to be negative or anything, but why the whole async park mode setup? I originally left it out because it is defined by the EHCI specs as defaulting to enabled with a count of 3 if supported by the controller. Since we always reset the controller during startup, it will reset to those defaults, making this setup pretty much superfluous to begin with. If it doesn't actually fix any misbehaving implementations out there I'd actually like to keep it out of there as it is just needless code. A comment could instead be added to explain the situation.

[gabriel.hartmann]

Replying to mmlr:

• You should move the frame list size check to the top, i.e. right after reading it out. If you return early in that case there's no point in reading the capabilities and command registers.

Fixed.

• The trace output is fatal, so it should actually be TRACE_ERROR so it is always logged (the same is true for the "didn't start" error case that is already there).

Fixed.

• There's a space missing after the comma in the trace.

Fixed.

• You use "asyncParkCapable" like a boolean, so you should actually make it one. According to coding style by adding a "(...) != 0" around the statement.

Fixed.

• Since you use "asyncParkCapable" only once you could simply dump the variable and move the expression into the if clause. If you keep it, then move it to where it is used, i.e. directly above the if (coding style, declare variables where they are used).

Fixed. I kept the variable because it's clearer what's being checked for in the if statement.

A general remark, not trying to be negative or anything, but why the whole async park mode setup? I originally left it out because it is defined by the EHCI specs as defaulting to enabled with a count of 3 if supported by the controller. Since we always reset the controller during startup, it will reset to those defaults, making this setup pretty much superfluous to begin with. If it doesn't actually fix any misbehaving implementations out there I'd actually like to keep it out of there as it is just needless code. A comment could instead be added to explain the situation.

I was asked to do this by Jérôme Duval as part of the Google Summer of Code vetting process. A comment explaining why it's not necessary to write any code, probably would have been frowned upon, although I don't really know for sure. Feel free not to apply the patch if you think it preferable to avoid executing the code.

[korli]

Replying to gabriel.hartmann:

I was asked to do this by Jérôme Duval as part of the Google Summer of Code vetting process. A comment explaining why it's not necessary to write any code, probably would have been frowned upon, although I don't really know for sure. Feel free not to apply the patch if you think it preferable to avoid executing the code.

Yeah, this part wasn't really to fix anything as I saw two implementations which enforce that and others which don't. Gabriel provided a patch and read a reference documentation, this helps us to evaluate his project application.

[mmlr]

This patch has been obsoleted by recent work to support isochronous streams. The part about the asynchronouse park mode isn't necessary, as described above, since the default is set to enabled after the reset we do. I've made the error case visible though in hrev42965.