Re: [PATCH v3 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA

[sashiko-bot@kernel.org]

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 read DMA ---
commit 64b22c0d882751a67b709351d2c2e3daf211bb52
Author: Even Xu <even.xu@intel.com>

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.

> diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c b/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
[ ... ]
> +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 RxDMA reset\n");
> +		goto end;
> +	}
> +
> +	ret = thc_wait_for_dma_pause(dev, THC_RXDMA1);
> +	if (ret) {
> +		dev_err(dev->dev, "Wait for RxDMA1 pause failed during RxDMA reset\n");
> +		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?

> +	}
> +
> +	ret = thc_wait_for_dma_pause(dev, THC_RXDMA2);
> +	if (ret) {
> +		dev_err(dev->dev, "Wait for RxDMA2 pause failed during RxDMA reset\n");
> +		goto end;
> +	}
> +
> +	thc_dma_unconfigure(dev);

[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->dma_config[chan]);
		dma_clear_prd_control(dev, &dev->dma_ctx->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.

> +
> +	ret = thc_dma_configure(dev);
> +	if (ret) {
> +		dev_err(dev->dev, "Re-config DMA failed during RxDMA reset\n");
> +		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