Re: [PATCH v4 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:

New issues:
- [Medium] `thc_rxdma_reset()` aborts the reset sequence if pausing THC_RXDMA1 fails, leaving THC_RXDMA2 running.

Pre-existing issues:
- [High] thc_dma_unconfigure() zeroes PRD base addresses before DMA engines are stopped, risking IOMMU faults.
--

--- Patch [1]: [PATCH v4 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA ---
commit b0b099ba1dc08199ebeeb23013c7c472a6f2c1bb
Author: Even Xu <even.xu@intel.com>

HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA

This commit adds a helper function thc_rxdma_reset() to perform a read DMA
reset. It is intended to be called for recovery when a fatal DMA error
happens.

> 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

[ ... ]

> @@ -561,6 +561,57 @@ static int thc_wait_for_dma_pause(struct thc_device *dev, enum thc_dma_channel c
>  	return 0;
>  }
>  
> +/**
> + * thc_rxdma_reset - Reset all read DMA engines

[ ... ]

> +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: Medium]
If pausing THC_RXDMA1 fails and times out, does jumping to the end label
leave THC_RXDMA2 running? 

Bailing out here without attempting to stop the second RX DMA engine might
leave the hardware in an inconsistent state during a fatal error recovery.

> +	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: High]
This isn't a bug introduced by this patch, but does calling
thc_dma_unconfigure() here without first pausing TXDMA or SWDMA risk IOMMU
faults?

Looking at thc_dma_unconfigure(), it zeroes the PRD base addresses for all
channels before clearing the START bits:

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]);
...

If a previous thc_dma_write() timed out, its TXDMA engine might still be
active since the timeout handler does not pause it.

Zeroing the PRD base address of an active DMA engine could force the
hardware to fetch descriptors at physical address 0, which would cause an
immediate memory corruption or IOMMU panic.

> +	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/20260717035609.3874015-1-even.xu@intel.com?part=1