Re: [PATCH 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] Using dev_err_once() hides subsequent failures during fatal error recovery.

Pre-existing issues:
- [High] Zeroing the Physical Region Descriptor (PRD) base addresses before pausing the DMA engines can cause IOMMU faults or memory corruption.
--

commit fb285a619300634b9f208053996047e32855d49b
Author: Even Xu <even.xu@intel.com>
Date:   Wed Jul 1 02:04:30 2026 +0000

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

Add a helper function thc_rxdam_reset() to do read DMA reset, it can be
called when 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..d771c85dff28b 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

[ ... ]

> @@ -532,6 +532,43 @@ void thc_dma_unconfigure(struct thc_device *dev)
>  }
>  EXPORT_SYMBOL_NS_GPL(thc_dma_unconfigure, "INTEL_THC");
>  
> +/**
> + * thc_rxdma_reset - Reset all read DMA engines
> + *
> + * @dev: The pointer of THC private device context
> + *
> + * This is a helper function to reset RxDMA configure. It's typically used
> + * for RxDMA recovery when fatal error happens.
> + */
> +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_once(dev->dev, "Quiesce interrupt failed during RxDMA reset\n");

[Severity: Medium]
Since thc_rxdma_reset() is called as part of a recurring fatal error
recovery flow, will dev_err_once() hide subsequent failures?

If a device successfully recovers the first time but encounters an
unrecoverable failure during a subsequent fatal error later in its uptime,
the failure will be silently ignored because dev_err_once() only prints
once per boot.

Should dev_err_ratelimited() or dev_err() be used instead?

> +		goto end;
> +	}
> +
> +	thc_dma_unconfigure(dev);

[Severity: High]
This is a pre-existing issue, but does calling thc_dma_unconfigure()
here risk IOMMU faults or memory corruption?

When thc_rxdma_reset() invokes thc_dma_unconfigure() during fatal error
recovery, the hardware DMA engine might still be actively fetching or in
an unpredictable state. 

Looking at thc_dma_unconfigure(), it zeroes out the PRD base addresses
before clearing the DMA START bits:

thc_dma_unconfigure() {
    for (chan = 0; chan < MAX_THC_DMA_CHANNEL; chan++) {
        dma_set_prd_base_addr(dev, 0, &dev->dma_ctx->dma_config[chan]);
        ...
    }
    regmap_write_bits(dev->thc_regmap, THC_M_PRT_READ_DMA_CNTRL_1_OFFSET,
                      THC_M_PRT_READ_DMA_CNTRL_START, 0);
}

If the hardware DMA engine is active and continues fetching descriptors
before the START bit is cleared, will it attempt to fetch from physical
address 0x0?

Should it wait for the ACTIVE status to clear (e.g., using
thc_wait_for_dma_pause()) before clearing the base addresses?

> +
> +	ret = thc_dma_configure(dev);
> +	if (ret) {
> +		dev_err_once(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;
> +}
> +EXPORT_SYMBOL_NS_GPL(thc_rxdma_reset, "INTEL_THC");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701020432.3774514-1-even.xu@intel.com?part=1