PIO FIFO debug flags and interrupt support#7502
Conversation
9f52906 to
3602529
Compare
|
PR updated with non-blocking support for the blocking operations, and a PIO-based UART driver. |
f952393 to
e7d3cd7
Compare
Fix a few issues discovered after testing the kernel API with a port of the userspace apitest app. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
| int ret; | ||
|
|
||
| if (pio->fw_pio_count >= PIO_SM_GET_DMACTRL) { | ||
| struct rp1_access_hw_args hwargs; |
There was a problem hiding this comment.
The two branches look swapped? The else only runs when fw_pio_count < PIO_SM_GET_DMACTRL, but then rp1_pio_message_resp() rejects the op and returns -EOPNOTSUPP, so the dedicated op is never used.
Swap them and use > for the dedicated op, else READ_HW?
There was a problem hiding this comment.
I'll put the fallback case second, which is what I intended to do.
| struct rp1_pio_device *pio = client->pio; | ||
| uint32_t intmask = 1 << (irqi - pio->irqs); | ||
|
|
||
| spin_lock(&client->lock); |
There was a problem hiding this comment.
Runs in hardirq, but client->lock is also taken plain in process context (irq_wait/irq_map/close). Use spin_lock_irqsave() everywhere (or a same-CPU IRQ deadlocks)?
There was a problem hiding this comment.
Runs in hardirq,
Does it? devm_request_irq is an alias for devm_request_threaded_irq nowadays. I'm happy using spin_lock_irqsave, even if it's only for appearances (and I'm not saying it is).
There was a problem hiding this comment.
AFAIU devm_request_irq() calls devm_request_threaded_irq() with thread_fn = NULL, so the handler stays the primary and runs in hardirq. The alias does not thread it.
There was a problem hiding this comment.
Sorry, just saw that my comment went to the wrong line in the commit, the _irqsave was meant for irq_wait(), irq_remove_handler() and close().
rp1_pio_irq_handler_userspace() already runs with irqs disabled in hardirq.
| enable_irq(irqi->irq); | ||
| irqi->enabled = true; | ||
| } else if (!args->enabled && irqi->enabled) { | ||
| disable_irq(irqi->irq); |
There was a problem hiding this comment.
disable_irq() can sleep, so it can't run under pio->lock. Use disable_irq_nosync()?
There was a problem hiding this comment.
I've not come across that one before.
There was a problem hiding this comment.
Probably will only surface with CONFIG_DEBUG_ATOMIC_SLEEP, disable_irq_nosync() is cheaper anyway, but it's probably academic 😄
|
|
||
| if (!irqi || !irqi->handler) { | ||
| pr_err("disabling unwanted interrupt\n"); | ||
| disable_irq(irq); |
There was a problem hiding this comment.
Handler for irq calling disable_irq() would wait for itself => hang. Use disable_irq_nosync()?
| if (irqi->enabled) | ||
| irq_set_enabled(client, i, false); | ||
|
|
||
| irqi->owner = NULL; |
There was a problem hiding this comment.
Cleared without pio->lock, but claim/add/remove hold it. Needs the lock here too.
| cancel_work_sync(&rfu->tx_work); | ||
| cancel_work_sync(&rfu->rx_rearm_work); | ||
| timer_delete_sync(&rfu->rx_timeout); | ||
| cancel_work_sync(&rfu->rx_timeout_work); | ||
| cancel_work_sync(&rfu->break_work); |
There was a problem hiding this comment.
Consumers cancelled before producers stopped. Break IRQ + RX SM/DMA are shut off later, so a break/RX chunk in between re-queues break_work (runs after rfu->pio = NULL => NULL deref) Stopping IRQ + RX first should do the trick
| /* A chunk just completed, so the FIFO isn't backed up: push the | ||
| * deadline for the CPU fallback back out. | ||
| */ | ||
| mod_timer(&rfu->rx_timeout, jiffies + rp1_pio_uart_rx_timeout_jiffies(&rfu->port)); |
There was a problem hiding this comment.
Re-arms unconditionally, so a chunk completing after timer_delete_sync() revives the timer, nothing cancels it later => use after free on freed rfu. Gating it on shutting_down should work
pio_sm_get_dmactrl is the reverse of pio_sm_set_dmactrl. pio_sm_get_flags allows the FIFO error flags - txoverflow, rxunderflow, txstall and rxstall - to be polled, cleared, and optionally waited for. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
The PIO block exposes 2 assignable interrupts. Make them available to kernel and userspace PIO applications. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
The PIO hardware drives 2 interrupt lines - declare them. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
The piolib API has three blocking operations: * pio_sm_get_blocking * pio_sm_put_blocking * pio_sm_exec_wait_blocking Rather than blocking in the firmware, stalling the whole API, allow the firmware to return EAGAIN to indicate that it has not completed. With a small change on each side it then becomes possible to poll for completion, allowing other operations to continue in the meantime. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
This is a tty driver that implements a UART using RP1's PIO block, ported from pico-examples/pio/uart_dma. Like that example, it is limited to 8N1, with no hardware flow control. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
Enable building the RP1 PIO UART for all kernels that support the Pi 5. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
An overlay to enable the PIO-based UART. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
2 participants
Add support for querying and clearing the FIFO debug/error flags, and for using PIO interrupts, both within the kernel and from the userspace piolib. This needs an RP1 firmware update, and a piolib update for userspace support.