media: tw686x: hold dev->lock during streamoff DMA disable
Subsystems (2)
PATCH 1/1 media: tw686x: hold dev->lock during streamoff DMA disable
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.
--- drivers/media/pci/tw686x/tw686x-video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/pci/tw686x/tw686x-video.c +1 -1
@@ -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 );
Signature · 1 line
2.43.0
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 byVIDIOC_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 8Then 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-rebootThe 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
Quoted 17 lines · Shuhao Fu
tw686x_dma_delay()consumes and clearsdev->pending_dma_en/dev->pending_dma_cmdunderdev->lockbefore replaying that deferred DMA image to the hardware registers.tw686x_disable_channel()updates the same fields. Most call sites serializetw686x_disable_channel()withdev->lock, buttw686x_stop_streaming()is the exception: it checksdev->pci_devunderdev->lock, drops the lock, and then callstw686x_disable_channel().If
dma_delay_timerfires 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->lockacross the stop-sidepci_devcheck andtw686x_disable_channel()call, soVIDIOC_STREAMOFFfollows the same locking contract as the other channel enable/disable paths.