From dfe187df3db9f21b3766808504816746668aafd5 Mon Sep 17 00:00:00 2001 From: Hans Verkuil Date: Thu, 1 Oct 2026 15:02:31 +0200 Subject: [PATCH 1/6] misc: rp1-pio: name the DMA direction with a tx bool rp1_pio_sm_config_xfer_internal() tested dir == RP1_PIO_DIR_TO_SM in half a dozen places, which is both repetitive and awkward to read where the two arms of the resulting conditionals are not in the natural tx/rx order. Derive a tx bool once and use it throughout, putting the tx arm first consistently. Record it in struct dma_info as well, so that code holding only a dma_info does not have to recover the direction from the index it was looked up by, as rp1_pio_close() had to. No functional change. Signed-off-by: Hans Verkuil Assisted-by: Claude-Code:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- drivers/misc/rp1-pio.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/misc/rp1-pio.c b/drivers/misc/rp1-pio.c index 1ade2a3abed0f..0a06144ea0684 100644 --- a/drivers/misc/rp1-pio.c +++ b/drivers/misc/rp1-pio.c @@ -101,6 +101,7 @@ struct dma_info { size_t buf_count; size_t burst_bytes; bool cyclic; + bool tx; unsigned int head_idx; unsigned int tail_idx; struct dma_buf_info bufs[DMA_BOUNCE_BUFFER_COUNT]; @@ -1024,6 +1025,7 @@ static void rp1_pio_sm_dma_free(struct dma_info *dma) dma_release_channel(dma->chan); dma->chan = NULL; dma->cyclic = false; + dma->tx = false; } static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint sm, uint dir, @@ -1037,6 +1039,7 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s struct dma_slave_caps dma_caps; struct dma_info *dma = NULL; bool cyclic = flags & RP1_PIO_SM_CONFIG_XFER_FL_DMA_CYCLE; + bool tx = dir == RP1_PIO_DIR_TO_SM; bool prefer_light_dma = flags & BIT(0); bool force_dma_type = flags & BIT(1); bool reconfigure = false; @@ -1077,7 +1080,7 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s /* Allocate and configure a DMA channel */ /* Careful - each SM FIFO has its own DREQ value */ - chan_name[0] = (dir == RP1_PIO_DIR_TO_SM) ? 't' : 'r'; + chan_name[0] = tx ? 't' : 'r'; chan_name[1] = 'x'; chan_name[2] = '0' + sm; if (prefer_light_dma) @@ -1087,6 +1090,7 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s chan_name[4] = '\0'; dma->cyclic = false; + dma->tx = tx; dma->chan = dma_request_chan(dev, chan_name); if (IS_ERR(dma->chan)) { ret = PTR_ERR(dma->chan); @@ -1143,18 +1147,18 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s fifo_addr = pio->phys_addr; fifo_addr += sm * (RP1_PIO_FIFO_TX1 - RP1_PIO_FIFO_TX0); - fifo_addr += (dir == RP1_PIO_DIR_TO_SM) ? RP1_PIO_FIFO_TX0 : RP1_PIO_FIFO_RX0; + fifo_addr += tx ? RP1_PIO_FIFO_TX0 : RP1_PIO_FIFO_RX0; config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; config.src_addr = fifo_addr; config.dst_addr = fifo_addr; - config.direction = (dir == RP1_PIO_DIR_TO_SM) ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM; + config.direction = tx ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM; dma_caps.max_burst = 4; dma_get_slave_caps(dma->chan, &dma_caps); if (dma_caps.max_burst > RP1_PIO_FIFO_DEPTH) dma_caps.max_burst = RP1_PIO_FIFO_DEPTH; - if (dir == RP1_PIO_DIR_TO_SM) + if (tx) config.dst_maxburst = dma_caps.max_burst; else config.src_maxburst = dma_caps.max_burst; @@ -1165,12 +1169,12 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s goto err_dma_free; set_dmactrl_args.sm = sm; - set_dmactrl_args.is_tx = (dir == RP1_PIO_DIR_TO_SM); - if (dir == RP1_PIO_DIR_FROM_SM) - set_dmactrl_args.ctrl = RP1_PIO_DMACTRL_DEFAULT | config.src_maxburst; - else + set_dmactrl_args.is_tx = tx; + if (tx) set_dmactrl_args.ctrl = RP1_PIO_DMACTRL_DEFAULT | (RP1_PIO_FIFO_DEPTH - config.dst_maxburst); + else + set_dmactrl_args.ctrl = RP1_PIO_DMACTRL_DEFAULT | config.src_maxburst; ret = rp1_pio_sm_set_dmactrl(client, &set_dmactrl_args); if (ret) @@ -1715,7 +1719,7 @@ void rp1_pio_close(struct rp1_pio_client *client) claimed &= ~mask; /* The SMs have been disabled, so this is safe */ - if ((i & 1) == RP1_PIO_DIR_FROM_SM) + if (!dma->tx) rp1_pio_sm_dma_flush_rx(dma); rp1_pio_sm_dma_free(dma); } From 47dce2f3e055ac48f78b425e7bd953238c4ca37a Mon Sep 17 00:00:00 2001 From: Hans Verkuil Date: Mon, 5 Oct 2026 13:44:20 +0200 Subject: [PATCH 2/6] misc: rp1-pio: guard against unreasonable buffer allocations Refuse total buffer sizes of > 512 MB. This also guards against overflows if userspace passes values like ~0. Also allow buf_count values > 4 for cyclic DMA: the limit of 4 is specific to non-cyclic DMA. Signed-off-by: Hans Verkuil --- drivers/misc/rp1-pio.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/misc/rp1-pio.c b/drivers/misc/rp1-pio.c index 0a06144ea0684..dd2498b27acca 100644 --- a/drivers/misc/rp1-pio.c +++ b/drivers/misc/rp1-pio.c @@ -79,6 +79,13 @@ #define DMA_BOUNCE_BUFFER_SIZE 0x1000 #define DMA_BOUNCE_BUFFER_COUNT 4 +/* + * Upper bound on the buffer memory a transfer may ask the driver to + * allocate, i.e. on buf_size * buf_count, for cyclic and non-cyclic + * transfers alike. Larger requests are rejected with -EINVAL. + */ +#define RP1_PIO_MAX_TOTAL_BUF_SIZE (512 * 1024 * 1024) + struct dma_xfer_state { struct dma_info *dma; void (*callback)(void *param); @@ -1052,7 +1059,15 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s return -EINVAL; if ((buf_count || buf_size) && (!buf_size || (buf_size & 3) || - !buf_count || buf_count > DMA_BOUNCE_BUFFER_COUNT)) + !buf_count || (!cyclic && buf_count > DMA_BOUNCE_BUFFER_COUNT))) + return -EINVAL; + + /* + * Sanity check for insane amounts of buffer data. + * This also guards against potential overflow issues if either + * buf_size or buf_count are e.g. ~0. + */ + if (buf_count && buf_size > RP1_PIO_MAX_TOTAL_BUF_SIZE / buf_count) return -EINVAL; /* Cyclic DMA is currently only supported for FROM_SM */ if (cyclic && dir == RP1_PIO_DIR_TO_SM) From 8b758176f589f7004fde283dc7ce6b27a23413a0 Mon Sep 17 00:00:00 2001 From: Hans Verkuil Date: Tue, 29 Sep 2026 10:58:02 +0200 Subject: [PATCH 3/6] misc: rp1-pio: ensure >= 2 buffers are available for cyclic DMA In the case of cyclic DMA it was possible to accept < 2 buffers or a zero buffer size. Check this and return -EINVAL in that case. Signed-off-by: Hans Verkuil Assisted-by: Claude-Code:claude-opus-5 --- drivers/misc/rp1-pio.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/misc/rp1-pio.c b/drivers/misc/rp1-pio.c index dd2498b27acca..6d27259aa3ba0 100644 --- a/drivers/misc/rp1-pio.c +++ b/drivers/misc/rp1-pio.c @@ -1069,9 +1069,14 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s */ if (buf_count && buf_size > RP1_PIO_MAX_TOTAL_BUF_SIZE / buf_count) return -EINVAL; - /* Cyclic DMA is currently only supported for FROM_SM */ - if (cyclic && dir == RP1_PIO_DIR_TO_SM) - return -EINVAL; + if (cyclic) { + /* + * Cyclic DMA is currently only supported for FROM_SM, and + * needs at least two real buffers to cycle through. + */ + if (dir != RP1_PIO_DIR_FROM_SM || (!buf_size || buf_count < 2)) + return -EINVAL; + } dma_mask = 1 << (sm * 2 + dir); From 0cf57a94b4f3e042a7862e3c23f75d407a1cda61 Mon Sep 17 00:00:00 2001 From: Hans Verkuil Date: Wed, 30 Sep 2026 18:29:00 +0200 Subject: [PATCH 4/6] misc: rp1-pio: fix cyclic RX flow control and overflow recovery Several bugs in cyclic (FROM_SM) DMA: - The reader only checks the semaphore count, not the actual head_idx/tail_idx relationship. If the completion tasklet is delayed enough that two periods finish before it runs, only one wakeup is delivered and head_idx permanently lags the hardware by one period, throwing off later flow control and overflow detection. Wait on the live head_idx - tail_idx relationship via a waitqueue instead. buf_sem is now only used for non-cyclic DMA. - Once rp1_pio_sm_rx_user() returns -EOVERFLOW it never advances tail_idx, so every subsequent read returns -EOVERFLOW too and the stream cannot recover without being reconfigured. Resync tail_idx to the oldest still-valid period on overflow, so that later reads succeed again. - The overflow check was one period out: head - tail == buf_count, not just >, already means tail_idx's slot is the one the DMA is writing, and the resync target computed from it, head - buf_count, pointed at that same slot rather than at the oldest intact one. - The DMA can start overwriting the period being read while copy_to_user() is still running, e.g. blocked on a user page fault. Re-check head/tail after the copy, seqlock style, and report -EOVERFLOW if it moved into the danger zone, since the data just copied may be torn. Signed-off-by: Hans Verkuil Assisted-by: Claude-Code:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- drivers/misc/rp1-pio.c | 90 ++++++++++++++++++++++++++++++---- include/uapi/misc/rp1_pio_if.h | 12 +++++ 2 files changed, 93 insertions(+), 9 deletions(-) diff --git a/drivers/misc/rp1-pio.c b/drivers/misc/rp1-pio.c index 6d27259aa3ba0..d69966e0315e7 100644 --- a/drivers/misc/rp1-pio.c +++ b/drivers/misc/rp1-pio.c @@ -34,6 +34,7 @@ #include #include #include +#include #include #include "rp1-fw-pio.h" @@ -103,12 +104,14 @@ struct dma_buf_info { struct dma_info { struct semaphore buf_sem; + wait_queue_head_t cyclic_wait; struct dma_chan *chan; size_t buf_size; size_t buf_count; size_t burst_bytes; bool cyclic; bool tx; + dma_cookie_t cyclic_cookie; unsigned int head_idx; unsigned int tail_idx; struct dma_buf_info bufs[DMA_BOUNCE_BUFFER_COUNT]; @@ -950,12 +953,52 @@ uint rp1_pio_irq_map(struct rp1_pio_client *client, uint irqn) } EXPORT_SYMBOL_GPL(rp1_pio_irq_map); +/* + * Recovers the absolute count of completed cyclic periods from the DMAC's + * reported position, instead of assuming one period per callback. + * vchan_cyclic_callback() (used by dw-axi-dmac) coalesces callbacks, so + * incrementing head_idx once per callback would leave it permanently behind + * the hardware. + * + * This is still an approximation: residue cannot report whole wraps. For RX + * an unchanged slot is treated as one, so that a coalesced full-ring + * completion does not leave a reader asleep indefinitely. + */ +static unsigned int rp1_pio_dma_cyclic_head(struct dma_info *dma) +{ + unsigned int period = dma->buf_size / dma->buf_count; + unsigned int head = dma->head_idx; + struct dma_tx_state state; + unsigned int slot, delta; + size_t completed; + + if (dmaengine_tx_status(dma->chan, dma->cyclic_cookie, &state) == + DMA_ERROR) + return head + 1; + + completed = dma->buf_size - state.residue; + slot = (completed / period) % dma->buf_count; + delta = (slot - head % dma->buf_count + dma->buf_count) % dma->buf_count; + if (!delta && !dma->tx) + delta = dma->buf_count; + + return head + delta; +} + static void rp1_pio_sm_dma_callback(void *param) { struct dma_info *dma = param; - if (dma->cyclic) - WRITE_ONCE(dma->head_idx, dma->head_idx + 1); + if (dma->cyclic) { + /* + * Recompute head_idx from the DMAC's actual position rather + * than assuming this callback covers exactly one period: see + * rp1_pio_dma_cyclic_head(). + */ + WRITE_ONCE(dma->head_idx, rp1_pio_dma_cyclic_head(dma)); + wake_up_interruptible(&dma->cyclic_wait); + return; + } up(&dma->buf_sem); } @@ -1097,6 +1140,7 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s rp1_pio_sm_dma_free(dma); sema_init(&dma->buf_sem, 0); + init_waitqueue_head(&dma->cyclic_wait); /* Allocate and configure a DMA channel */ /* Careful - each SM FIFO has its own DREQ value */ @@ -1218,10 +1262,11 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s desc->callback = rp1_pio_sm_dma_callback; desc->callback_param = dma; - /* Submit the buffer - the callback will kick the semaphore */ + /* Submit the buffer - the callback will wake cyclic_wait */ ret = dmaengine_submit(desc); if (ret < 0) goto err_dma_free; + dma->cyclic_cookie = ret; dma_async_issue_pending(dma->chan); } @@ -1397,15 +1442,29 @@ static int rp1_pio_sm_rx_user(struct rp1_pio_device *pio, struct dma_info *dma, return -EINVAL; /* - * The callback posts the semaphore once per period, so consume - * exactly one 'period' worth of data per read. + * Wait until there is an unread period. Re-checking the live + * head/tail relationship, instead of counting semaphore posts + * from the callback, means a wakeup covering more than one + * completed period cannot leave this counter permanently out + * of sync with the hardware. */ - if (down_interruptible(&dma->buf_sem)) - return -ERESTARTSYS; + ret = wait_event_interruptible(dma->cyclic_wait, + (int)(READ_ONCE(dma->head_idx) - dma->tail_idx) > 0); + if (ret) + return ret; - /* The DMA has wrapped onto the period we were about to read. */ - if (READ_ONCE(dma->head_idx) - dma->tail_idx > dma->buf_count) + /* + * head - tail == buf_count, not just >, already means the DMA + * is writing into the exact ring slot tail_idx maps to, so + * that period has been (or is being) overwritten. Resync tail + * to head - buf_count + 1, the oldest period that cannot be + * the one being written, so that later reads recover instead + * of returning -EOVERFLOW forever, and report the loss here. + */ + if (READ_ONCE(dma->head_idx) - dma->tail_idx >= dma->buf_count) { + dma->tail_idx = READ_ONCE(dma->head_idx) - dma->buf_count + 1; return -EOVERFLOW; + } /* Pair with the DMAC's writes into the period we are about to copy. */ dma_rmb(); @@ -1414,6 +1473,19 @@ static int rp1_pio_sm_rx_user(struct rp1_pio_device *pio, struct dma_info *dma, dbi->buf + (dma->tail_idx % dma->buf_count) * period, bytes)) return -EFAULT; + + /* + * copy_to_user() can block (e.g. on a user page fault), during + * which the DMA may start overwriting this same period. + * Re-check head/tail afterwards, seqlock style: if it moved + * into the danger zone the data just copied may be torn, so + * resync and report the loss instead of returning success. + */ + if (READ_ONCE(dma->head_idx) - dma->tail_idx >= dma->buf_count) { + dma->tail_idx = READ_ONCE(dma->head_idx) - dma->buf_count + 1; + return -EOVERFLOW; + } + dma->tail_idx++; return 0; } diff --git a/include/uapi/misc/rp1_pio_if.h b/include/uapi/misc/rp1_pio_if.h index ee9e7b85b2a03..c22d62be4e335 100644 --- a/include/uapi/misc/rp1_pio_if.h +++ b/include/uapi/misc/rp1_pio_if.h @@ -188,6 +188,18 @@ struct rp1_pio_sm_config_xfer32_args { #define RP1_PIO_SM_CONFIG_XFER_FL_DMA_FORCE_HEAVY 2 #define RP1_PIO_SM_CONFIG_XFER_FL_DMA_FORCE_LIGHT 3 +/* + * Use cyclic DMA: buf_size is the size of one period and buf_count is the + * number of periods. Each PIO_IOC_SM_XFER_DATA* call transfers at most one + * period and blocks until that period is available; a zero-byte transfer is + * rejected with -EINVAL. + * + * For input a read returns -EOVERFLOW if the DMA wrapped onto the period + * that was about to be read; if detected after copying, the destination + * may already have been modified. Detection is best effort, but the + * stream resynchronises to the oldest still-valid period so that + * subsequent reads succeed again without needing to be reconfigured. + */ #define RP1_PIO_SM_CONFIG_XFER_FL_DMA_CYCLE (1 << 2) struct rp1_pio_sm_config_xfer_v2_args { From d54178c20109eb12dc0c1ce1e1cda2e5bd7d08b5 Mon Sep 17 00:00:00 2001 From: Hans Verkuil Date: Mon, 5 Oct 2026 16:05:33 +0200 Subject: [PATCH 5/6] misc: rp1-pio: harden cyclic RX overflow detection and recovery rp1_pio_sm_rx_user() waits for head_idx - tail_idx to become positive as a signed difference. Nothing bounds how long an application may leave a capture running unread, and once the reader falls more than half the counter range behind, that difference turns negative: an overflowed ring becomes indistinguishable from an empty one, and the read sleeps before it ever reaches the overflow check below it. tail_idx can never legitimately pass head_idx, so a negative distance has only one meaning. Fold it into the overflow test and wait for the counters to differ at all, so the reader wakes and resynchronises. Recovery then has to stop trusting head_idx further than it can be trusted. It is recovered from residue, which cannot count laps, so an unchanged sample is resolved by assuming one went by: right in its slot, but possibly a lap or more ahead of the hardware. Resyncing to head_idx - buf_count + 1 therefore hands back periods that may never have been written, or that the caller has already consumed. Resync onto head_idx itself instead and wait for the DMA to complete one more period. That discards whatever the ring was still holding, but the loss is being reported anyway. Signed-off-by: Hans Verkuil Assisted-by: Claude-Code:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- drivers/misc/rp1-pio.c | 43 +++++++++++++++++++++++++++------- include/uapi/misc/rp1_pio_if.h | 5 ++-- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/drivers/misc/rp1-pio.c b/drivers/misc/rp1-pio.c index d69966e0315e7..48cca6722f7af 100644 --- a/drivers/misc/rp1-pio.c +++ b/drivers/misc/rp1-pio.c @@ -1418,6 +1418,25 @@ static int rp1_pio_sm_rx_submit(struct rp1_pio_device *pio, struct dma_info *dma return 0; } +/* + * tail_idx can never legitimately pass head_idx - the reader only consumes + * periods the DMA has completed - so a negative distance is not "nothing to + * read" but a reader that fell more than half the counter range behind, i.e. + * an overflow that wrapped. Nothing bounds how long an application may leave + * a capture running unread, so treat it as the overflow it is. + * + * Deriving this from the counters keeps the decision and the recovery acting + * on it in the reader. A flag raised by the callback would be evaluated + * against a tail_idx the reader can move straight afterwards, so a stale + * decision could rewind it onto periods it had already returned. + */ +static bool rp1_pio_cyclic_rx_overflowed(struct dma_info *dma) +{ + int dist = (int)(READ_ONCE(dma->head_idx) - dma->tail_idx); + + return dist < 0 || dist >= (int)dma->buf_count; +} + /* * A NULL userbuf queues a receive of exactly "bytes" and returns without * waiting: it puts the app in control of its own read-ahead depth and @@ -1449,20 +1468,26 @@ static int rp1_pio_sm_rx_user(struct rp1_pio_device *pio, struct dma_info *dma, * of sync with the hardware. */ ret = wait_event_interruptible(dma->cyclic_wait, - (int)(READ_ONCE(dma->head_idx) - dma->tail_idx) > 0); + READ_ONCE(dma->head_idx) != dma->tail_idx); if (ret) return ret; /* * head - tail == buf_count, not just >, already means the DMA * is writing into the exact ring slot tail_idx maps to, so - * that period has been (or is being) overwritten. Resync tail - * to head - buf_count + 1, the oldest period that cannot be - * the one being written, so that later reads recover instead - * of returning -EOVERFLOW forever, and report the loss here. + * that period has been (or is being) overwritten. Report the + * loss and resync, so that later reads recover instead of + * returning -EOVERFLOW forever. + * + * Resync onto head_idx itself, not onto the oldest period the + * ring holds: head_idx's slot is always right, but its lap + * count is a guess (see rp1_pio_dma_cyclic_head()), so the + * older periods it appears to vouch for may never have been + * written. Waiting for the DMA to complete one more period + * returns data known to be fresh. */ - if (READ_ONCE(dma->head_idx) - dma->tail_idx >= dma->buf_count) { - dma->tail_idx = READ_ONCE(dma->head_idx) - dma->buf_count + 1; + if (rp1_pio_cyclic_rx_overflowed(dma)) { + dma->tail_idx = READ_ONCE(dma->head_idx); return -EOVERFLOW; } @@ -1481,8 +1506,8 @@ static int rp1_pio_sm_rx_user(struct rp1_pio_device *pio, struct dma_info *dma, * into the danger zone the data just copied may be torn, so * resync and report the loss instead of returning success. */ - if (READ_ONCE(dma->head_idx) - dma->tail_idx >= dma->buf_count) { - dma->tail_idx = READ_ONCE(dma->head_idx) - dma->buf_count + 1; + if (rp1_pio_cyclic_rx_overflowed(dma)) { + dma->tail_idx = READ_ONCE(dma->head_idx); return -EOVERFLOW; } diff --git a/include/uapi/misc/rp1_pio_if.h b/include/uapi/misc/rp1_pio_if.h index c22d62be4e335..d053d674af0f9 100644 --- a/include/uapi/misc/rp1_pio_if.h +++ b/include/uapi/misc/rp1_pio_if.h @@ -197,8 +197,9 @@ struct rp1_pio_sm_config_xfer32_args { * For input a read returns -EOVERFLOW if the DMA wrapped onto the period * that was about to be read; if detected after copying, the destination * may already have been modified. Detection is best effort, but the - * stream resynchronises to the oldest still-valid period so that - * subsequent reads succeed again without needing to be reconfigured. + * stream resynchronises onto the next period the DMA completes, dropping + * what the ring was holding, so that subsequent reads succeed again + * without needing to be reconfigured. */ #define RP1_PIO_SM_CONFIG_XFER_FL_DMA_CYCLE (1 << 2) From 331dc928d28762babc4ba054b5c07cdc26f0c9cf Mon Sep 17 00:00:00 2001 From: Hans Verkuil Date: Thu, 1 Oct 2026 15:02:59 +0200 Subject: [PATCH 6/6] misc: rp1-pio: add cyclic DMA support for output This adds the missing support for cyclic DMA for output (i.e. memory to PIO). Defer dma_async_issue_pending() until writes have primed every period of the ring. Issuing it after the first write is not enough: the DMAC races ahead and pulls the still-zeroed following periods into the FIFO, emitting zeros and silently dropping whatever is written there afterwards. Flow control for the writer is a signed lead over head_idx rather than a ring distance. head_idx is only known modulo buf_count, so a whole lap of the ring is indistinguishable from no movement at all - and the DMAC does lap the ring immediately after being issued, as it fills the TX FIFO at full AXI speed. The distance would then report a full ring as an empty one and deadlock the writer. The lead is unambiguous: it is buf_count once every period is spoken for, and shrinks as soon as head_idx moves at all. A genuine underflow is the DMA lapping the writer, reported as -EPIPE with tail_idx resynchronised past the periods it has reached. Measuring the lap against the position recorded at the previous write, as well as against tail_idx, keeps the constant pipeline-fill offset out of the comparison and catches a DMA that stayed ahead of the writer but still replayed the whole ring. Both indices wrap at a multiple of buf_count, so that their mapping onto ring slots survives the counters rolling over. Signed-off-by: Hans Verkuil Assisted-by: Claude-Code:claude-opus-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- drivers/misc/rp1-pio.c | 272 +++++++++++++++++++++++++++++---- include/uapi/misc/rp1_pio_if.h | 20 ++- 2 files changed, 258 insertions(+), 34 deletions(-) diff --git a/drivers/misc/rp1-pio.c b/drivers/misc/rp1-pio.c index 48cca6722f7af..4fecd020961c6 100644 --- a/drivers/misc/rp1-pio.c +++ b/drivers/misc/rp1-pio.c @@ -111,9 +111,14 @@ struct dma_info { size_t burst_bytes; bool cyclic; bool tx; + bool issued; + bool last_head_idx_valid; + unsigned int last_head_idx; + unsigned int cb_cyclic_seq; dma_cookie_t cyclic_cookie; unsigned int head_idx; unsigned int tail_idx; + unsigned int idx_wrap; struct dma_buf_info bufs[DMA_BOUNCE_BUFFER_COUNT]; }; @@ -953,6 +958,34 @@ uint rp1_pio_irq_map(struct rp1_pio_client *client, uint irqn) } EXPORT_SYMBOL_GPL(rp1_pio_irq_map); +/* + * Add n to idx, wrapping if the result > idx_wrap which is a multiple of + * buf_count. + */ +static inline unsigned int rp1_pio_idx_add(struct dma_info *dma, + unsigned int idx, + unsigned int n) +{ + idx += n; + if (idx >= dma->idx_wrap) + idx -= dma->idx_wrap; + return idx; +} + +/* Signed distance from b to a, wrapped against idx_wrap */ +static inline int rp1_pio_idx_diff(struct dma_info *dma, unsigned int a, + unsigned int b) +{ + int diff = (int)(a - b); + + if (diff > (int)(dma->idx_wrap / 2)) + diff -= dma->idx_wrap; + else if (diff < -(int)(dma->idx_wrap / 2)) + diff += dma->idx_wrap; + + return diff; +} + /* * Recovers the absolute count of completed cyclic periods from the DMAC's * reported position, instead of assuming one period per callback. @@ -960,9 +993,21 @@ EXPORT_SYMBOL_GPL(rp1_pio_irq_map); * incrementing head_idx once per callback would leave it permanently behind * the hardware. * - * This is still an approximation: residue cannot report whole wraps. For RX - * an unchanged slot is treated as one, so that a coalesced full-ring - * completion does not leave a reader asleep indefinitely. + * Residue reports the position within the ring, not laps, so a callback + * that finds the DMA on the slot it was last seen on may mean no movement + * or a whole ring. A delivered callback does not settle it: vchan_complete() + * clears the pending cyclic callback before invoking this one, so a period + * finishing meanwhile queues a second callback observing the same slot. + * Both readings are guesses, so guess whichever way is survivable. + * + * For RX, assume a lap: an under-estimate can leave a reader asleep on a + * ring that never produces another callback, while an over-estimate only + * makes the ring look overflowed, which rp1_pio_sm_rx_user() answers by + * discarding what it cannot vouch for. + * + * For TX, assume nothing: an over-estimate makes the writer declare an + * underrun and overwrite periods the DMA has not sent, while an + * under-estimate only delays it until a callback does see movement. */ static unsigned int rp1_pio_dma_cyclic_head(struct dma_info *dma) { @@ -974,7 +1019,7 @@ static unsigned int rp1_pio_dma_cyclic_head(struct dma_info *dma) if (dmaengine_tx_status(dma->chan, dma->cyclic_cookie, &state) == DMA_ERROR) - return head + 1; + return rp1_pio_idx_add(dma, head, 1); completed = dma->buf_size - state.residue; slot = (completed / period) % dma->buf_count; @@ -982,7 +1027,7 @@ static unsigned int rp1_pio_dma_cyclic_head(struct dma_info *dma) if (!delta && !dma->tx) delta = dma->buf_count; - return head + delta; + return rp1_pio_idx_add(dma, head, delta); } static void rp1_pio_sm_dma_callback(void *param) @@ -996,6 +1041,7 @@ static void rp1_pio_sm_dma_callback(void *param) * rp1_pio_dma_cyclic_head(). */ WRITE_ONCE(dma->head_idx, rp1_pio_dma_cyclic_head(dma)); + WRITE_ONCE(dma->cb_cyclic_seq, dma->cb_cyclic_seq + 1); wake_up_interruptible(&dma->cyclic_wait); return; } @@ -1009,7 +1055,7 @@ static void rp1_pio_sm_kernel_dma_callback(void *param) if (dxs->dbi) memcpy(dxs->data, dxs->dbi->buf, dxs->data_bytes); - dxs->dma->tail_idx++; + dxs->dma->tail_idx = rp1_pio_idx_add(dxs->dma, dxs->dma->tail_idx, 1); up(&dxs->dma->buf_sem); dxs->callback(dxs->callback_param); @@ -1076,6 +1122,7 @@ static void rp1_pio_sm_dma_free(struct dma_info *dma) dma->chan = NULL; dma->cyclic = false; dma->tx = false; + dma->issued = false; } static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint sm, uint dir, @@ -1112,14 +1159,16 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s */ if (buf_count && buf_size > RP1_PIO_MAX_TOTAL_BUF_SIZE / buf_count) return -EINVAL; - if (cyclic) { - /* - * Cyclic DMA is currently only supported for FROM_SM, and - * needs at least two real buffers to cycle through. - */ - if (dir != RP1_PIO_DIR_FROM_SM || (!buf_size || buf_count < 2)) - return -EINVAL; - } + + /* + * Cyclic DMA needs at least two (rx) or three (tx) periods to cycle + * through; tx needs the third to detect an application that can't + * keep up with the DMA. buf_size has to exceed FIFO + burst + margin, + * which one minimum covers without depending on HEAVY vs LIGHT DMA. + */ + if (cyclic && (buf_size < RP1_PIO_CYCLIC_MIN_BUF_SIZE || + buf_count < (tx ? 3 : 2))) + return -EINVAL; dma_mask = 1 << (sm * 2 + dir); @@ -1139,6 +1188,7 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s if (reconfigure) rp1_pio_sm_dma_free(dma); + /* Note: the semaphore is only used by non-cyclic DMA */ sema_init(&dma->buf_sem, 0); init_waitqueue_head(&dma->cyclic_wait); @@ -1155,6 +1205,8 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s dma->cyclic = false; dma->tx = tx; + dma->issued = false; + dma->last_head_idx_valid = false; dma->chan = dma_request_chan(dev, chan_name); if (IS_ERR(dma->chan)) { ret = PTR_ERR(dma->chan); @@ -1171,12 +1223,12 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s dma->buf_size = buf_size * buf_count; dma->buf_count = 0; /* Round up the allocations */ - buf_size = ROUND_UP(dma->buf_size, PAGE_SIZE); + uint size = ROUND_UP(dma->buf_size, PAGE_SIZE); /* Alloc and map bounce buffer */ struct dma_buf_info *dbi = &dma->bufs[0]; - dbi->buf = dma_alloc_coherent(dma->chan->device->dev, buf_size, + dbi->buf = dma_alloc_coherent(dma->chan->device->dev, size, &dbi->dma_addr, GFP_KERNEL); if (!dbi->buf) { ret = -ENOMEM; @@ -1189,13 +1241,13 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s } else { dma->buf_size = buf_size; /* Round up the allocations */ - buf_size = ROUND_UP(buf_size, PAGE_SIZE); + uint size = ROUND_UP(buf_size, PAGE_SIZE); /* Alloc and map bounce buffers */ for (dma->buf_count = 0; dma->buf_count < buf_count; dma->buf_count++) { struct dma_buf_info *dbi = &dma->bufs[dma->buf_count]; - dbi->buf = dma_alloc_coherent(dma->chan->device->dev, buf_size, + dbi->buf = dma_alloc_coherent(dma->chan->device->dev, size, &dbi->dma_addr, GFP_KERNEL); if (!dbi->buf) { ret = -ENOMEM; @@ -1208,6 +1260,10 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s dma->head_idx = 0; dma->tail_idx = 0; + dma->idx_wrap = dma->buf_count ? + rounddown(INT_MAX, (unsigned int)dma->buf_count) : 0; + dma->last_head_idx = 0; + dma->last_head_idx_valid = false; fifo_addr = pio->phys_addr; fifo_addr += sm * (RP1_PIO_FIFO_TX1 - RP1_PIO_FIFO_TX0); @@ -1250,9 +1306,9 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s sg_dma_len(&dbi->sgl) = dma->buf_size; desc = dmaengine_prep_dma_cyclic(dma->chan, dbi->dma_addr, - dma->buf_size, dma->buf_size / dma->buf_count, - DMA_DEV_TO_MEM, - DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + dma->buf_size, dma->buf_size / dma->buf_count, + tx ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); if (!desc) { dev_err(dev, "DMA preparation failed\n"); ret = -EIO; @@ -1268,7 +1324,16 @@ static int rp1_pio_sm_config_xfer_internal(struct rp1_pio_client *client, uint s goto err_dma_free; dma->cyclic_cookie = ret; - dma_async_issue_pending(dma->chan); + if (tx) { + /* + * Defer dma_async_issue_pending() until writes have + * primed the whole ring (see rp1_pio_sm_tx_user()). + */ + dma->issued = false; + } else { + dma_async_issue_pending(dma->chan); + dma->issued = true; + } } return 0; @@ -1323,6 +1388,21 @@ static void rp1_pio_sm_xfer_progress(struct rp1_pio_sm_xfer_data32_args *args, args->data_bytes -= bytes; } +/* + * How far ahead of the DMA the writer is, in periods. The writer waits as + * soon as it is buf_count ahead, so a larger lead is the signed difference + * having wrapped after the DMA lapped an idle writer. Report it as the + * deepest possible underrun rather than as a full ring: left alone it would + * park the writer until another half range of output had gone out, and for + * good if the DMA ever stopped. + */ +static int rp1_pio_cyclic_tx_lead(struct dma_info *dma, unsigned int head_idx) +{ + int lead = rp1_pio_idx_diff(dma, dma->tail_idx, head_idx); + + return lead > (int)dma->buf_count ? 0 : lead; +} + static int rp1_pio_sm_tx_user(struct rp1_pio_device *pio, struct dma_info *dma, struct rp1_pio_sm_xfer_data32_args *args) { @@ -1333,15 +1413,136 @@ static int rp1_pio_sm_tx_user(struct rp1_pio_device *pio, struct dma_info *dma, struct device *dev = &pdev->dev; int ret = 0; + if (dma->cyclic) { + size_t period = dma->buf_size / dma->buf_count; + struct dma_buf_info *dbi = &dma->bufs[0]; + unsigned int head_idx, tail_idx; + bool lapped = false; + bool underflow; + void *dst; + int lead; + + if (!bytes || bytes > period) + return -EINVAL; + + /* + * Wait for a period to write into. Periods head_idx .. + * tail_idx - 1 are filled but unsent, so a lead of buf_count + * means the whole ring is spoken for. + * + * Waiting on the ring distance instead would deadlock: the + * DMAC pulls the entire primed ring into the TX FIFO within + * microseconds of being issued, landing exactly a lap on, and + * a lap is invisible in residue - so the coalesced completion + * adds nothing to head_idx and a full ring looks like an empty + * one. The absolute lead has no such ambiguity. Re-examine it + * on every completion, so that a lap credited late still wakes + * the writer. + */ + for (;;) { + unsigned int seq = READ_ONCE(dma->cb_cyclic_seq); + + head_idx = READ_ONCE(dma->head_idx); + lead = rp1_pio_cyclic_tx_lead(dma, head_idx); + + if (!dma->issued || lead < (int)dma->buf_count) + break; + + ret = wait_event_interruptible(dma->cyclic_wait, + READ_ONCE(dma->cb_cyclic_seq) != seq || + rp1_pio_cyclic_tx_lead(dma, + READ_ONCE(dma->head_idx)) < + (int)dma->buf_count); + if (ret) + return ret; + } + + /* + * The ring distance cannot tell a correctly placed writer from + * one the DMA has lapped: with four periods, tail_idx six and + * head_idx seven means period six has already been sent, yet + * the distance is three. Use the signed lead instead. A lead + * of zero or less means the period about to be written has + * gone out holding whatever the previous lap left in it, so + * skip to the first period the DMA cannot have reached yet. + */ + tail_idx = dma->tail_idx; + + if (dma->issued && lead <= 0) { + tail_idx = rp1_pio_idx_add(dma, head_idx, 1); + lead = 1; + lapped = true; + } + + /* + * A genuine underflow is the DMA lapping the writer: either + * the skip above, or a whole ring of progress since the + * previous write. Either way at least one period went out with + * stale contents. Measuring against the position recorded at + * the previous write, as well as against tail_idx, catches a + * DMA that stayed ahead of the writer but still replayed the + * whole ring. + * + * Best effort: head_idx is updated from a tasklet, so a write + * landing between the DMA moving on and the tasklet running + * isn't flagged. + */ + underflow = lapped || + (dma->last_head_idx_valid && + rp1_pio_idx_diff(dma, head_idx, + dma->last_head_idx) >= + (int)dma->buf_count); + + dst = dbi->buf + (tail_idx % dma->buf_count) * period; + if (copy_from_user(dst, userbuf, bytes)) + return -EFAULT; + memset(dst + bytes, 0, period - bytes); + + /* + * Ensure the data just written is visible in memory before + * the DMA engine wraps back around and reads this period + * again. + */ + dma_wmb(); + + /* Detect DMA reaching this slot while the user copy was running. */ + if (dma->issued && + rp1_pio_idx_diff(dma, READ_ONCE(dma->head_idx), + head_idx) >= lead) + underflow = true; + + dma->last_head_idx = head_idx; + dma->last_head_idx_valid = dma->issued; + WRITE_ONCE(dma->tail_idx, rp1_pio_idx_add(dma, tail_idx, 1)); + + if (underflow) + ret = -EPIPE; + + /* + * Only start the DMA once every period holds real data. The TX + * DREQ is asserted as long as the FIFO has room, so issuing it + * after the first write lets the DMAC race ahead and pull the + * still-zeroed periods into the FIFO, emitting zeros and + * dropping whatever is written into those slots afterwards. + */ + if (!dma->issued && dma->tail_idx >= dma->buf_count) { + dma_async_issue_pending(dma->chan); + dma->issued = true; + } + + return ret; + } + while (bytes > 0) { size_t copy_bytes = min(bytes, dma->buf_size); struct dma_buf_info *dbi; /* grab the next free buffer, waiting if they're all full */ - if (dma->head_idx - dma->tail_idx == dma->buf_count) { + if (rp1_pio_idx_diff(dma, dma->head_idx, dma->tail_idx) == + (int)dma->buf_count) { if (down_interruptible(&dma->buf_sem)) return -ERESTARTSYS; - dma->tail_idx++; + dma->tail_idx = rp1_pio_idx_add(dma, dma->tail_idx, 1); } dbi = &dma->bufs[dma->head_idx % dma->buf_count]; @@ -1374,7 +1575,7 @@ static int rp1_pio_sm_tx_user(struct rp1_pio_device *pio, struct dma_info *dma, dma_async_issue_pending(dma->chan); - dma->head_idx++; + dma->head_idx = rp1_pio_idx_add(dma, dma->head_idx, 1); bytes -= copy_bytes; rp1_pio_sm_xfer_progress(args, copy_bytes); } @@ -1412,7 +1613,7 @@ static int rp1_pio_sm_rx_submit(struct rp1_pio_device *pio, struct dma_info *dma if (ret < 0) return ret; - dma->head_idx++; + dma->head_idx = rp1_pio_idx_add(dma, dma->head_idx, 1); dma_async_issue_pending(dma->chan); return 0; @@ -1432,7 +1633,8 @@ static int rp1_pio_sm_rx_submit(struct rp1_pio_device *pio, struct dma_info *dma */ static bool rp1_pio_cyclic_rx_overflowed(struct dma_info *dma) { - int dist = (int)(READ_ONCE(dma->head_idx) - dma->tail_idx); + int dist = rp1_pio_idx_diff(dma, READ_ONCE(dma->head_idx), + dma->tail_idx); return dist < 0 || dist >= (int)dma->buf_count; } @@ -1511,12 +1713,13 @@ static int rp1_pio_sm_rx_user(struct rp1_pio_device *pio, struct dma_info *dma, return -EOVERFLOW; } - dma->tail_idx++; + dma->tail_idx = rp1_pio_idx_add(dma, dma->tail_idx, 1); return 0; } if (!userbuf) { - if (dma->head_idx - dma->tail_idx == dma->buf_count) + if (rp1_pio_idx_diff(dma, dma->head_idx, dma->tail_idx) == + (int)dma->buf_count) return -EBUSY; return rp1_pio_sm_rx_submit(pio, dma, bytes); @@ -1542,7 +1745,7 @@ static int rp1_pio_sm_rx_user(struct rp1_pio_device *pio, struct dma_info *dma, if (down_interruptible(&dma->buf_sem)) return -ERESTARTSYS; - dma->tail_idx++; + dma->tail_idx = rp1_pio_idx_add(dma, dma->tail_idx, 1); if (copy_to_user(userbuf, dbi->buf, len)) return -EFAULT; @@ -1626,6 +1829,10 @@ int rp1_pio_sm_xfer_data(struct rp1_pio_client *client, uint sm, uint dir, dma = &pio->dma_configs[sm][dir]; + /* The cyclic configuration owns the buffer and the descriptor */ + if (dma->cyclic) + return -EINVAL; + if (!dma_addr) { dxs = kmalloc(sizeof(*dxs), GFP_KERNEL); if (!dxs) @@ -1646,7 +1853,8 @@ int rp1_pio_sm_xfer_data(struct rp1_pio_client *client, uint sm, uint dir, * Never block: the caller manages its own outstanding count * and is told about freed slots via its callback. */ - if (dma->head_idx - dma->tail_idx == dma->buf_count) { + if (rp1_pio_idx_diff(dma, dma->head_idx, dma->tail_idx) == + (int)dma->buf_count) { kfree(dxs); return -EBUSY; } @@ -1685,7 +1893,7 @@ int rp1_pio_sm_xfer_data(struct rp1_pio_client *client, uint sm, uint dir, return ret; } - dma->head_idx++; + dma->head_idx = rp1_pio_idx_add(dma, dma->head_idx, 1); dma_async_issue_pending(dma->chan); return 0; diff --git a/include/uapi/misc/rp1_pio_if.h b/include/uapi/misc/rp1_pio_if.h index d053d674af0f9..8f24a5cf3211e 100644 --- a/include/uapi/misc/rp1_pio_if.h +++ b/include/uapi/misc/rp1_pio_if.h @@ -194,13 +194,29 @@ struct rp1_pio_sm_config_xfer32_args { * period and blocks until that period is available; a zero-byte transfer is * rejected with -EINVAL. * + * Input accepts any buf_count of two or more. Output has to stay ahead of a + * DMA engine that reads the ring well in advance of the wire and whose + * position is only observable modulo buf_count, so it additionally requires + * at least three periods. + * + * The minimum value for buf_size is RP1_PIO_CYCLIC_MIN_BUF_SIZE. This ensures + * that the size of each period is more than the PIO FIFO size plus DMA burst + * plus margin. + * * For input a read returns -EOVERFLOW if the DMA wrapped onto the period * that was about to be read; if detected after copying, the destination * may already have been modified. Detection is best effort, but the * stream resynchronises onto the next period the DMA completes, dropping - * what the ring was holding, so that subsequent reads succeed again - * without needing to be reconfigured. + * what the ring was still holding, so that subsequent reads succeed again + * without needing to be reconfigured. For output a write returns -EPIPE if + * the DMA caught up and resent stale data, or reached the period while it + * was being copied. Detection is best effort. The data was copied into the + * ring and may already have been consumed, so blindly retrying on -EPIPE + * can duplicate it. A failed user copy does not advance the writer or start + * DMA, but may modify the period. A write shorter than a period zeroes the + * remainder of that period. */ +#define RP1_PIO_CYCLIC_MIN_BUF_SIZE 128 #define RP1_PIO_SM_CONFIG_XFER_FL_DMA_CYCLE (1 << 2) struct rp1_pio_sm_config_xfer_v2_args {