[PATCH] media: tw686x: hold dev->lock during streamoff DMA disable

[PATCH] media: tw686x: hold dev->lock during streamoff DMA disable

[Shuhao Fu]

tw686x_dma_delay() consumes and clears dev->pending_dma_en /
dev->pending_dma_cmd under dev->lock before replaying that deferred DMA
image to the hardware registers. tw686x_disable_channel() updates the
same fields. Most call sites serialize tw686x_disable_channel() with
dev->lock, but tw686x_stop_streaming() is the exception: it checks
dev->pci_dev under dev->lock, drops the lock, and then calls
tw686x_disable_channel().

If dma_delay_timer fires in that window, the timer path can race the
streamoff disable path and consume stale or partially updated pending
DMA state. This can leave deferred DMA programming out of sync with the
requested stream stop, so STREAMOFF may not immediately reflect the
intended channel-disable state.

Fix with holding dev->lock across the stop-side pci_dev check and
tw686x_disable_channel() call, so VIDIOC_STREAMOFF follows the same
locking contract as the other channel enable/disable paths.

Fixes: 704a84ccdbf1 ("[media] media: Support Intersil/Techwell TW686x-based video capture cards")
Signed-off-by: Shuhao Fu <sfual@cse.ust.hk>
---
 drivers/media/pci/tw686x/tw686x-video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
index 785dd797d921b5..19fb6196cf9c05 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -552,9 +552,9 @@ static void tw686x_stop_streaming(struct vb2_queue *vq)
 	/* Check device presence */
 	spin_lock_irqsave(&dev->lock, flags);
 	pci_dev = dev->pci_dev;
-	spin_unlock_irqrestore(&dev->lock, flags);
 	if (pci_dev)
 		tw686x_disable_channel(dev, vc->ch);
+	spin_unlock_irqrestore(&dev->lock, flags);
 
 	spin_lock_irqsave(&vc->qlock, flags);
 	tw686x_clear_queue(vc, VB2_BUF_STATE_ERROR);
-- 
2.43.0

Re: [PATCH] media: tw686x: hold dev->lock during streamoff DMA disable

[Shuhao Fu]

Hi,

Below is the local KUnit/KCSAN setup I used to reproduce the TW686x
warning.

I do not have a TW686x board or a QEMU device model for this chip, so
this is not a hardware-backed userspace repro. This setup is only my
best-effort local reference for the race.

To stay as close as possible to the real userspace trigger, I moved the
writer side up to the same internal callback reached by
VIDIOC_STREAMOFF: tw686x_stop_streaming(). The reader side stayed the
real tw686x_dma_delay() timer callback, which consumes and clears
pending_dma_en / pending_dma_cmd under dev->lock.

Locally, I used the existing kernel/kcsan/tw686x_dma_race.kunitconfig
setup, but rebuilt it with stricter KCSAN settings:

- CONFIG_KCSAN_STRICT=y
- CONFIG_KCSAN_INTERRUPT_WATCHER=y
- CONFIG_KCSAN_REPORT_ONCE_IN_MS=0
- CONFIG_KCSAN_SKIP_WATCH=100
- CONFIG_KCSAN_SKIP_WATCH_RANDOMIZE=n
- CONFIG_KCSAN_UDELAY_TASK=200
- CONFIG_KCSAN_UDELAY_INTERRUPT=100
- CONFIG_KCSAN_DELAY_RANDOMIZE=n

The build command was:

  ./tools/testing/kunit/kunit.py build \
    --arch=x86_64 \
    --kunitconfig=kernel/kcsan/tw686x_dma_race.kunitconfig \
    --build_dir=../out-tw686x-kcsan-kunit-strict \
    --make_options CC=clang-20 \
    --make_options LD=ld.bfd \
    --make_options AR=llvm-ar-20 \
    --make_options NM=llvm-nm-20 \
    --make_options OBJCOPY=llvm-objcopy-20 \
    --make_options READELF=llvm-readelf-20 \
    --make_options LLVM_IAS=1 \
    --jobs 8

Then I booted only the focused TW686x disable-vs-delay test under QEMU:

  timeout 120 qemu-system-x86_64 \
    -smp 8 \
    -m 2048 \
    -kernel out-tw686x-kcsan-kunit-strict/arch/x86/boot/bzImage \
    -append 'kunit.filter_glob=kcsan.test_tw686x_pending_dma_disable_vs_delay kunit.enable=1 console=ttyS0 kunit_shutdown=reboot' \
    -nographic \
    -no-reboot

The KUnit harness uses a fake tw686x_dev plus a minimal fake
tw686x_video_channel and vb2_queue, so the writer side can call the
real tw686x_stop_streaming() path instead of jumping straight into
tw686x_disable_channel(). The internal call chain is:

- writer side:
  test_kernel_tw686x_stop_streaming()
  -> test_tw686x_kunit_stop_streaming()
  -> tw686x_stop_streaming()
  -> tw686x_disable_channel()

- reader side:
  test_kernel_tw686x_dma_delay()
  -> test_tw686x_kunit_dma_delay()
  -> tw686x_dma_delay()

With that setup I got repeated KCSAN reports of:

  BUG: KCSAN: data-race in test_tw686x_kunit_dma_delay / tw686x_disable_channel

The first clean hit in my local log was:

  read to 0xffff8f55021d7910 of 4 bytes by interrupt on cpu 5:
   test_tw686x_kunit_dma_delay+0x3c/0x110
   test_kernel_tw686x_dma_delay+0x29/0x40
   ...
   tw686x_disable_channel+0x1e8/0x200
   tw686x_stop_streaming+0x81/0x1e0
   test_tw686x_kunit_stop_streaming+0x22/0x30
   test_kernel_tw686x_stop_streaming+0x29/0x40

  write to 0xffff8f55021d7910 of 4 bytes by task 83 on cpu 5:
   tw686x_disable_channel+0x1e8/0x200
   tw686x_stop_streaming+0x81/0x1e0
   test_tw686x_kunit_stop_streaming+0x22/0x30
   test_kernel_tw686x_stop_streaming+0x29/0x40

I then saw the same pair again later in the same run, still with
tw686x_stop_streaming() in the writer stack and
test_tw686x_kunit_dma_delay() on the reader side.

Again, this KUnit result is a local reference and best-effort proof of
the overlap, not a claim of a full hardware-backed userspace repro.

Thanks,
Shuhao

On Thu, Apr 23, 2026 at 09:05:42PM +0800, Shuhao Fu wrote:
> tw686x_dma_delay() consumes and clears dev->pending_dma_en /
> dev->pending_dma_cmd under dev->lock before replaying that deferred DMA
> image to the hardware registers. tw686x_disable_channel() updates the
> same fields. Most call sites serialize tw686x_disable_channel() with
> dev->lock, but tw686x_stop_streaming() is the exception: it checks
> dev->pci_dev under dev->lock, drops the lock, and then calls
> tw686x_disable_channel().
> 
> If dma_delay_timer fires in that window, the timer path can race the
> streamoff disable path and consume stale or partially updated pending
> DMA state. This can leave deferred DMA programming out of sync with the
> requested stream stop, so STREAMOFF may not immediately reflect the
> intended channel-disable state.
> 
> Fix with holding dev->lock across the stop-side pci_dev check and
> tw686x_disable_channel() call, so VIDIOC_STREAMOFF follows the same
> locking contract as the other channel enable/disable paths.