PCI & interrupts modifications related to USB #1071
Conversation
There was a problem hiding this comment.
Looks good mostly, just some small requests.
One comment/question about RwLocks -- they are inefficient and much slower than Mutexes, so they're typically only used when absolutely necessary. It appears that the RwLock around INTX_DEVICES might be necessary, but I can't tell for sure. If the point of using a RwLock there is to allow multiple interrupt handlers to access it simultaneously, then that is kind of defeated by using the interrupt-safe RwLock variant, which prevents other interrupts from firing while it is held.
What about the RwLock around the PciDevice's Waker? Are there any use cases where multiple tasks need to immutably access that waker concurrently?
If those are not needed, then you should switch to Mutex.
You were right; I replaced these with Mutexes. |
| @@ -506,17 +550,20 @@ impl PciLocation { | |||
| } | |||
|
|
|||
| /// Sets the PCI device's command bit 10 to disable legacy interrupts | |||
| pub fn pci_set_interrupt_disable_bit(&self) { | |||
| pub fn pci_set_interrupt_disable_bit(&self, bit: bool) { | |||
There was a problem hiding this comment.
I think we should rename this function to be more clear about what it actually does, and use the standard "enable_xyz" terminology. We can mention what it actually does (i.e., set the disable bit) in the function documentation, but as it stands now it's quite difficult to understand.
I'll accept this now; kindly include that in the next PR about PCI/USB. 👍
|
|
||
| for device in &*devices { | ||
| if device.pci_get_interrupt_status(true) { | ||
| device.pci_enable_interrupts(false); |
There was a problem hiding this comment.
this is also confusing, as upon first glance, the reader of this code would expect that you're just temporarily disabling PCI interrupts here while you handle the interrupt and then re-enabling them later. However, that doesn't happen, so this must mean something else.
We should rename and properly document the pci_enable_interrupts() function to be more clear about how and why it should be invoked (e.g., perhaps we just always want to disable this type of interrupt?)
* Set up a default handler for legacy PCI interrupts (e.g., on PIN A, B, C, D); their handler wakes up a waker (task) that was previously bound to the `PciDevice` that received the interrupt. * Only aarch64 is supported at the moment. * This PR is needed to support USB interrupt handling. Co-authored-by: Kevin Boos <1139460+kevinaboos@users.noreply.github.com> 1e2c7df
These are changes indirectly related to the USB PR. It doesn't build on x86 yet, because the changes were only made on aarch64.
The biggest change is that PCI legacy (INTx) interrupts are supported. Their handler wakes up a task that must have been bound to them, else it panics.
In USB, the woken up task simply checks the state of the USB controller, triggering various mechanisms when appropriate. When that task is done, it goes back to sleep using the blocker returned by this function.