People: |
|
---|
11281 XHCI polled mode support for USB keyboards
Review Request #2028 — Created June 28, 2019 and submitted
Information | |
---|---|
mscheler | |
illumos-gate | |
Reviewers | |
general | |
rm |
These changes add limited polled mode support to the XHCI driver:
- only input is supported
- only USB interrupt transfers are supportedThis is good enough to get a USB keyboard working to be able to use the kernel debugger.
I've tested this on a Celistica Athena. Without my change "mdb -K" on the IPMI remote console will result in a hung system.
With my change the kernel debugger prompt appears and I'm able to request a stack trace with "$C" and exit the debugger with ":c".
Change Summary:
Added missing copyrights that "git pbchk" was complaining about
Diff: |
Revision 2 (+535 -85) |
---|
-
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) I don't think this comment makes sense any more. We should add a note of this file in xhci.c in the big theory statement. I suspect we likely want to discuss polling, so can you please add a section on that there?
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Why is the forward declaration requiered? Is it because we can't do the __KVPRINTFLIKE on the normal function?
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Can this please be formatted the same as everything else, so we're not lining up the arguments, but concatentated like the other functions.
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Would you mind making this file look like all the other files in the driver and not tabbing out between the variable type and declaration. If possible, I'd prefer that the driver looks as similar as possible.
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Please do explicit comparisons, as in != NULL.
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Shouldn't we sanity check that this is non-NULL and return failure if not?
-
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) If we have short data at this point in the transfer, why does that represent an underrun? Could we have a comment here that explains the why?
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Why is it OK to ignore the return value here?
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) What has guaranteed that we are only ever going to have events in the ring that relate just to the console usage here? It's not clear to me that this is a safe assumption and dropping to the console after boot wouldn't lead to us breaking something.
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Why is it OK to just drop the non-polled transfers?
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Why exactly are interrupts relevant to this initialization? I understand why you want to hold the xhci_lock during this whole bit, but it's not clear to me why interrupts matter here.
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Please verify that we have a non-NULL value before just passing it to kmem_free().
-
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Do we need to be disabling interrupts across entry to and from this function? A lot of the comments around here suggest that we're trying to avoid racing with interrupts, so that might be a good thing to do. It also seems like the general endpoint scheduling code should note that we have polled mode on and not or in the bit to generate an interrupt on completion.
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Why do we want to try and enter? Is there a reason we don't want to block for other activity to be set?
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) I feel like we should get this from xep_type.
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Can we add an additional state bit to the xhci_endpoint_state_t enumeration and make sure that we update when we're in polling mode on an endpoint in the driver state tracking?
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) Why are we only doing a tryenter here?
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) So, I feel like the interrupt management here is unclear. Shouldn't none of this be generating interrupts? I don't understand why we need to manipulate this.
-
usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2) We aren't currently ever setting this. What kinds of ignorned events do you see as being critical here?
-
usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h (Diff revision 2) Why can't all of this be a part of xhci.h?
-
usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h (Diff revision 2) Conventionally argument names are left out of forward declarations.
-
usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h (Diff revision 2) Where does 8 come from, please comment this with an explanation like the other constants in xhci.h.
-
usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h (Diff revision 2) I don't see this value ever set to B_TRUE in this change. Is it actually necessary?
Change Summary:
Addressed code review comments
Testing Done: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+607 -118) |
Description: |
|
---|
Testing Done: |
|
---|