11281 XHCI polled mode support for USB keyboards

Reviews
Diff

- Download Diff

Information
Submitter:	Matthias Scheler
Repository:	illumos-gate
Branch:
Bugs:
Depends On:
Reviewers
Groups:	general
People:	rm

Description

These changes add limited polled mode support to the XHCI driver:
- only input is supported
- only USB interrupt transfers are supported

This is good enough to get a USB keyboard working to be able to use the kernel debugger.

Testing Done

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".

Issues

0
19
6
All Issues: 25

Description	From	Last Updated
I don't think this comment makes sense any more. We should add a note of this file in xhci.c in ...	Robert Mustacchi	July 3, 2019, 5:24 a.m.
Why is the forward declaration requiered? Is it because we can't do the __KVPRINTFLIKE on the normal function?	Robert Mustacchi	July 3, 2019, 2:23 a.m.
Can this please be formatted the same as everything else, so we're not lining up the arguments, but concatentated like ...	Robert Mustacchi	July 3, 2019, 2:30 a.m.
Would you mind making this file look like all the other files in the driver and not tabbing out between ...	Robert Mustacchi	July 3, 2019, 4:02 a.m.
Please do explicit comparisons, as in != NULL.	Robert Mustacchi	July 3, 2019, 2:42 a.m.
Shouldn't we sanity check that this is non-NULL and return failure if not?	Robert Mustacchi	July 3, 2019, 2:44 a.m.
Shouldn't this be static?	Robert Mustacchi	July 3, 2019, 2:44 a.m.
If we have short data at this point in the transfer, why does that represent an underrun? Could we have ...	Robert Mustacchi	July 31, 2019, 1:38 a.m.
Why is it OK to ignore the return value here?	Robert Mustacchi	July 3, 2019, 5:28 a.m.
What has guaranteed that we are only ever going to have events in the ring that relate just to the ...	Robert Mustacchi	July 3, 2019, 8:15 a.m.
Why is it OK to just drop the non-polled transfers?	Robert Mustacchi	July 3, 2019, 8:14 a.m.
Why exactly are interrupts relevant to this initialization? I understand why you want to hold the xhci_lock during this whole ...	Robert Mustacchi	July 3, 2019, 3:16 a.m.
Please verify that we have a non-NULL value before just passing it to kmem_free().	Robert Mustacchi	July 3, 2019, 3:18 a.m.
Lint directives are unnecessary.	Robert Mustacchi	July 3, 2019, 4:31 a.m.
Do we need to be disabling interrupts across entry to and from this function? A lot of the comments around ...	Robert Mustacchi	July 3, 2019, 4:29 a.m.
Why do we want to try and enter? Is there a reason we don't want to block for other activity ...	Robert Mustacchi	July 3, 2019, 3:35 a.m.
I feel like we should get this from xep_type.	Robert Mustacchi	July 3, 2019, 5:57 a.m.
Can we add an additional state bit to the xhci_endpoint_state_t enumeration and make sure that we update when we're in ...	Robert Mustacchi	July 3, 2019, 9:15 a.m.
Why are we only doing a tryenter here?	Robert Mustacchi	July 3, 2019, 5:53 a.m.
So, I feel like the interrupt management here is unclear. Shouldn't none of this be generating interrupts? I don't understand ...	Robert Mustacchi	July 3, 2019, 4 a.m.
We aren't currently ever setting this. What kinds of ignorned events do you see as being critical here?	Robert Mustacchi	July 3, 2019, 3:58 a.m.
Why can't all of this be a part of xhci.h?	Robert Mustacchi	July 3, 2019, 3:58 a.m.
Conventionally argument names are left out of forward declarations.	Robert Mustacchi	July 3, 2019, 3:58 a.m.
Where does 8 come from, please comment this with an explanation like the other constants in xhci.h.	Robert Mustacchi	July 3, 2019, 3:56 a.m.
I don't see this value ever set to B_TRUE in this change. Is it actually necessary?	Robert Mustacchi	July 3, 2019, 3:56 a.m.

People:

+	rm

Change Summary:

Added missing copyrights that "git pbchk" was complaining about

Diff:

Revision 2 (+535 -85)

Show changes

	usr/src/uts/common/Makefile.files
	usr/src/uts/common/io/usb/hcd/xhci/xhci_endpoint.c
	usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c
	usr/src/uts/common/io/usb/hcd/xhci/xhci_usba.c
	usr/src/uts/common/sys/usb/hcd/xhci/xhci.h
	usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h

Thanks for working on this. A general question, did you consider ever spinning up a second event ring associated with an interrupt that was never enabled? That might get us out of a lot of the issues where we're trying to share things and process them in a way that they conflict, especially as there's nothing that stops someone from dropping into the system console while other USB devices are being used.

This is a good start.

Matthias Scheler July 29, 2019, 1:45 a.m.

Yes, I've considered spinning up a second ring. But my understanding of the USB protocol and the xHCI is still very limited. And spinning up a second ring felt like a massive change that possibly require changes to the existing code path which would involve considerably more risk and testing.

Robert Mustacchi July 30, 2019, 3:18 p.m.

OK. I understand that perspective. It looks like we've changed things such that we won't actually drop events on the ground any more, which is good. Thanks for doing that.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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?

Matthias Scheler July 29, 2019, 1:45 a.m.

I've added a section and added xhci_polled.c to the list of files.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

Why is the forward declaration requiered? Is it because we can't do the __KVPRINTFLIKE on the normal function?

Matthias Scheler July 29, 2019, 1:45 a.m.

Because you can only put these decorators on function prototypes and not the actual definition of a function.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

Can this please be formatted the same as everything else, so we're not lining up the arguments, but concatentated like the other functions.

Matthias Scheler July 29, 2019, 1:45 a.m.
```
Fixed (Copy & Pasta)
```

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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.

Matthias Scheler July 29, 2019, 1:45 a.m.

The layout was based on "ehci_polled.c". I'm happy to fix it.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

Please do explicit comparisons, as in != NULL.

Matthias Scheler July 29, 2019, 1:45 a.m.
```
Yes, I should (Copy & Pasta again)
```

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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)

Show all issues

Shouldn't this be static?

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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?

Matthias Scheler July 29, 2019, 1:45 a.m.

I'll be honest: I don't know whether this is correct or not.

This routine was pieced together from xhci_endpoint_norm_callback() and I left everything in that seemed necessary.

Robert Mustacchi July 30, 2019, 3:18 p.m.

OK. It seems like what's happens is we've paired down everything and we're assuming that we'll never have USB_ATTRS_SHORT_XFER_OK set. Is that accurate? If that's the case, then it makes sense that we're basically saying that then we'll have ended up with an underrun.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

Why is it OK to ignore the return value here?

Matthias Scheler July 29, 2019, 1:45 a.m.

Because I cannot think of way to handle this. Here we've successfully received a USB scan code so we should return USB_SUCCESS. If we cannot reschedule a transfer we will probably not be able to receive further scan codes. The only way to handle this would be to panic the machine. But that might only re-enter the kernel debugger. And the is always the chance that the USB scan code complets a KMDB command the user gets something out of the machine.
I've added a comment explaining this and a mechanism for persisting this error to make further reads fail.

Robert Mustacchi July 30, 2019, 3:18 p.m.

Thanks for writing the comment and expanding on what we're doing there.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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.

Matthias Scheler July 29, 2019, 1:45 a.m.

Non polled I/O related events are no forwarded to the normal driver code, see below.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

Why is it OK to just drop the non-polled transfers?

Matthias Scheler July 29, 2019, 1:45 a.m.

The problem is how to handle them. I've now added code which passes everything that we cannot handle in polled mode to the normal driver code. And that seems to work fine.

The idea behind the previous behaviour was to minise the risk of doing something that wouldn't work under KMDB.

Robert Mustacchi July 30, 2019, 3:18 p.m.

Thanks for making the change. With this it's probably less important to look at the second ring.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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.

Matthias Scheler July 29, 2019, 1:45 a.m.

The comments are copied & pasted. I've removed them.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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)

Show all issues

Lint directives are unnecessary.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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.

Matthias Scheler July 29, 2019, 1:45 a.m.

As far as I understand the kernel debugger has disabled interrupt handling anyway. So there's nothing to do for us in that regard.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

Why do we want to try and enter? Is there a reason we don't want to block for other activity to be set?

Matthias Scheler July 29, 2019, 1:45 a.m.

My assumption (did I mention that kmdb's internals are underdocumented?) is that we are basically singe threaded at this point. If the lock is taken we would wait forever and the machine would hang. If the call however fails you would hope that kmdb realizes that it cannot initiate console I/O and handles this by e.g. dropping back to the operating system on in case of asynchronous entry reboots the machine.

Robert Mustacchi July 30, 2019, 3:18 p.m.

OK. If the context for this is that we're being called form kmdb context, then that's reasonable. Thanks for the explanation.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

I feel like we should get this from xep_type.

Matthias Scheler July 29, 2019, 1:45 a.m.

I've changed this and added an extra check to xhci_polled_init().

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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)

Show all issues

Why are we only doing a tryenter here?

Matthias Scheler July 29, 2019, 1:45 a.m.

The same reason that we used mutex_tryenter() above: to avoid a permanent hang

It's of course possible to change those two calls. But I think the current solution is better because it at least has a chance of some sensible error handling.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

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.

Matthias Scheler July 29, 2019, 1:45 a.m.

Without this extra code I experienced hangs after entering one character. I also means that the register is in the correct state when we leave the kernel debugger.

As far as I understand interrupt generation is still enabled but they are simply not served.

Robert Mustacchi July 30, 2019, 3:18 p.m.

I guess the theory here is that if an Interrupt was pending, we're going to clear it as we've basically done all the logical work of doing so anyways. I guess that makes sense.

usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c (Diff revision 2)

Show all issues

We aren't currently ever setting this. What kinds of ignorned events do you see as being critical here?

Matthias Scheler July 29, 2019, 1:45 a.m.

This mechanism got defunct when I made some changes based on the testing on the Athena system because it guaranteed a kernel panic when existing the kernel debugger (see above). I've now added one appropriate check back which sets it.

usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h (Diff revision 2)

Show all issues

Why can't all of this be a part of xhci.h?

Matthias Scheler July 29, 2019, 1:45 a.m.

The other USB HCD drivers have separate include files. But I'm happy to move this into "xhci.h" which I didn't want to clutter too much.

usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h (Diff revision 2)

Show all issues

Conventionally argument names are left out of forward declarations.

usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h (Diff revision 2)

Show all issues

Where does 8 come from, please comment this with an explanation like the other constants in xhci.h.

Matthias Scheler July 29, 2019, 1:45 a.m.

This all came for the EHCI polled code. It's amount of space we need to store a USB HID scan code packet.

I got rid off the symbol and expanded the comment above the buffer member in xhci_polled_t.

usr/src/uts/common/sys/usb/hcd/xhci/xhci_polled.h (Diff revision 2)

Show all issues

I don't see this value ever set to B_TRUE in this change. Is it actually necessary?

Matthias Scheler July 29, 2019, 1:45 a.m.

I've replaced this with a persistent error.

Change Summary:

Addressed code review comments

Testing Done:

~		I've tested this on a Celistica Athena (modified Illumos) and SuperMicro X11 (modified IntelliFlash). Without my change "mdb -K" on the IPMI remote console will result in a hung system.
	~	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 ":q".
-
-		Note that there is a risk that the XHCI driver gets unhappy afterwards and panics the system by calling xhci_fm_runtime_reset().

Diff:

Revision 3 (+607 -118)

Show changes

	usr/src/uts/common/Makefile.files
	usr/src/uts/common/io/usb/hcd/xhci/xhci.c
	usr/src/uts/common/io/usb/hcd/xhci/xhci_endpoint.c
	usr/src/uts/common/io/usb/hcd/xhci/xhci_event.c
	usr/src/uts/common/io/usb/hcd/xhci/xhci_polled.c
	usr/src/uts/common/io/usb/hcd/xhci/xhci_usba.c
	usr/src/uts/common/sys/usb/hcd/xhci/xhci.h

Description:

		These changes add limited polled mode support to the XHCI driver:
		- only input is supported
		- only USB interrupt transfers are supported

~		This is good enough to get a USB keyboard working to be able to the kernel debugger.
	~	This is good enough to get a USB keyboard working to be able to use the kernel debugger.

Testing Done:

		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 ":q".
	~	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".

Ship it!

Thanks for the follow ups here, I appreciate it.

Ship it!

Just one note, (c) is not really needed in copyright notice, isnt it so?

Matthias Scheler Aug. 1, 2019, 1:08 a.m.

Possibly not. I'll keep that in mind for future changes.

Ship it!

Ship It!

You have a pending review.

Review Board 2.5.16

Screenshots

Files

Issues

People:

Change Summary:

Diff:

Change Summary:

Testing Done:

Diff:

Description:

Testing Done:

Status: Closed (submitted)