Re: [PATCH v2 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 4 potential issue(s) to consider:

New issues:
- [High] thc_rxdma_reset() fails to re-initialize the RxDMA PRD tables, leading to hardware reading shrunken buffer sizes on restart.
- [High] thc_rxdma_reset() unsafely clears PRD base addresses for all DMA channels while TX or SW DMA engines may still be running.
- [Medium] thc_rxdma_reset() ignores the return value of thc_interrupt_quiesce(dev, false) on the success path.

Pre-existing issues:
- [High] Incorrect polling condition and execution order in thc_interrupt_quiesce during unquiesce (int_quiesce = false).
--

commit b0beafd1bc7a02f7a2277c5a6d69c17c58724f01
Author: Even Xu <even.xu@intel.com>

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

Add a helper function thc_rxdma_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..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
[ ... ]
> +int thc_rxdma_reset(struct thc_device *dev)
> +{
[ ... ]
> +	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]
Could this cause memory corruption or IOMMU faults if TX or SW DMA engines
are still running?

thc_rxdma_reset() explicitly pauses RxDMA1 and RxDMA2 but does not pause
TXDMA or SWDMA. It then calls thc_dma_unconfigure(), which loops over all
DMA channels (MAX_THC_DMA_CHANNEL) and blindly overwrites their PRD base
addresses to 0x0.

If TXDMA was left running (for instance, if a prior thc_dma_write() timed
out without clearing the TXDMA start bit), setting its PRD base address to 0
while it is actively fetching could cause the hardware to fetch descriptors
from physical address 0.

> +
> +	ret = thc_dma_configure(dev);

[Severity: High]
Does this leave dirty 'len' descriptors in the PRD memory layout?

thc_dma_configure() restarts the DMA engine pointers to 0, but it does not
re-initialize the PRD memory layout. The hardware updates the 'len' field
of PRD entries to indicate the actual length of received packets.

Restarting the DMA from index 0 without restoring 'len' to the maximum
buffer capacity could cause the hardware to use previously received smaller
packet sizes as the maximum capacity for new transfers, resulting in
truncated packets or DMA errors.

> +	if (ret) {
> +		dev_err(dev->dev, "Re-config DMA failed during RxDMA reset\n");
> +		goto end;
> +	}
> +
> +	thc_interrupt_quiesce(dev, false);

[Severity: Medium]
Is it safe to ignore the return value of thc_interrupt_quiesce() here?

Discarding this return value on the success path could silently hide a
hardware timeout or regmap read failure, potentially leaving interrupts
disabled while the function reports success.

[Severity: High]
This is a pre-existing issue, but is the polling condition and execution
order in thc_interrupt_quiesce() correct when unquiescing?

When thc_interrupt_quiesce() is called with int_quiesce = false, it polls
the HW_STS bit expecting it to be 1 BEFORE it clears the QUIESCE_EN bit:

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c:thc_interrupt_quiesce() {
    ...
    ret = regmap_read_poll_timeout(dev->thc_regmap, THC_M_PRT_CONTROL_OFFSET, ctrl,
                                   ctrl & THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_HW_STS,
                                   THC_REGMAP_POLLING_INTERVAL_US, THC_QUIESCE_EN_TIMEOUT_US);
    ...
    /* Unquiesce device interrupt - Clear the quiesce bit */
    if (!int_quiesce)
        regmap_write_bits(dev->thc_regmap, THC_M_PRT_CONTROL_OFFSET,
                          THC_M_PRT_CONTROL_THC_DEVINT_QUIESCE_EN, 0);
}

Since the device is already quiesced, HW_STS is already 1, causing the poll
to return immediately. It then clears the QUIESCE_EN bit and returns without
ever waiting for HW_STS to become 0 to acknowledge the unquiesce. This could
cause the driver to proceed before interrupts are actually re-enabled.

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