RE: [PATCH v3 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA
From: "Xu, Even" <even.xu@intel.com>
Date: 2026-07-13 03:25:44
Also in:
sashiko-reviews
quoted hunk ↗ jump to hunk
-----Original Message----- From: sashiko-bot@kernel.org <sashiko-bot@kernel.org> Sent: Monday, July 13, 2026 11:18 AM To: Xu, Even <even.xu@intel.com> Cc: linux-input@vger.kernel.org; dmitry.torokhov@gmail.com Subject: Re: [PATCH v3 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] `thc_rxdma_reset` improperly clears descriptor addresses for all DMA channels without pausing them first, potentially causing active TX/SW DMA to access physical address 0. - [High] Error paths in `thc_rxdma_reset` fail to restore the interrupt quiesce state, permanently disabling device interrupts. ----- Patch [1]: [PATCH v3 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset readDMA --- commit 64b22c0d882751a67b709351d2c2e3daf211bb52 Author: Even Xu [off-list ref] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA This commit adds a new helper function thc_rxdma_reset() to perform a read DMA reset. It is intended to be called for RxDMA recovery when fatal DMA errors occur.quoted
diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.cb/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c index 6ee675e0a7384..7ceb8aeeccd37 100644--- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c +++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c[ ... ]quoted
+int thc_rxdma_reset(struct thc_device *dev) { + int ret; + + if (mutex_lock_interruptible(&dev->thc_bus_lock)) + return -EINTR; + + ret = thc_interrupt_quiesce(dev, true); + if (ret) { + dev_err(dev->dev, "Quiesce interrupt failed during RxDMAreset\n");quoted
+ goto end; + } + + ret = thc_wait_for_dma_pause(dev, THC_RXDMA1); + if (ret) { + dev_err(dev->dev, "Wait for RxDMA1 pause failed during RxDMAreset\n");quoted
+ goto end;[Severity: High] Does this skip unquiescing the interrupts? If thc_wait_for_dma_pause() fails, we jump to the end label and return without calling thc_interrupt_quiesce(dev, false). Will this leave device interrupts permanently disabled?quoted
+ } + + ret = thc_wait_for_dma_pause(dev, THC_RXDMA2); + if (ret) { + dev_err(dev->dev, "Wait for RxDMA2 pause failed during RxDMAreset\n");quoted
+ goto end; + } + + thc_dma_unconfigure(dev);
This is expected, thc_rxdma_reset() is designed only be called in fatal DMA error. In this case, if pause RxDMA failed, it means DMA engine cannot be recovered.
[Severity: Critical] Can this cause active TX or SW DMA engines to access physical address 0? thc_rxdma_reset() pauses THC_RXDMA1 and THC_RXDMA2, but thc_dma_unconfigure() unconditionally iterates over MAX_THC_DMA_CHANNEL and sets the PRD base address to 0 for all channels, including THC_TXDMA and THC_SWDMA: void thc_dma_unconfigure(struct thc_device *dev) { int chan; for (chan = 0; chan < MAX_THC_DMA_CHANNEL; chan++) { dma_set_prd_base_addr(dev, 0, &dev->dma_ctx-quoted
dma_config[chan]);dma_clear_prd_control(dev, &dev->dma_ctx-quoted
dma_config[chan]);} ... } If a previous thc_dma_write() or thc_swdma_read() timed out, it unlocks thc_bus_lock but leaves the TX/SW DMA engine running in hardware. Overwriting the PRD base address to 0 while the engine is active could result in memory corruption or an IOMMU fault.
This is not a problem, All DMA operations were protected by THC bus mutex lock. There is no Tx/SW DMA operation when calls into this thc_rxdma_reset().
quoted
+ + ret = thc_dma_configure(dev); + if (ret) { + dev_err(dev->dev, "Re-config DMA failed during RxDMAreset\n");quoted
+ goto end; + } + + thc_interrupt_quiesce(dev, false); + +end: + mutex_unlock(&dev->thc_bus_lock); + return ret; +}-- Sashiko AI review · https://sashiko.dev/#/patchset/20260713030412.2531933-1- even.xu@intel.com?part=1