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

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Critical] thc_rxdma_reset() destructively unconfigures all DMA channels (including TX and SW) without pausing them first, risking severe memory corruption if they were left active by a previous timeout.
- [High] Error paths in thc_rxdma_reset() bypass the interrupt unquiesce operation, leaving device interrupts permanently disabled.
- [High] The PRD (Physical Region Descriptor) tables are not re-initialized before restarting the RxDMA engines, leading to immediate buffer overruns upon hardware restart.
- [Medium] The return value of thc_interrupt_quiesce(dev, false) is ignored, potentially masking a failure that leaves the device permanently incapable of generating interrupts.

Pre-existing issues:
- [High] thc_dma_write() overwrites its timeout error code, falsely returning success to callers.
- [High] thc_swdma_read() leaks interrupt quiesced state and leaves hardware wedged on error paths.
--

--- Patch [1]: [PATCH v5 1/3] HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA ---
commit c2eb7694465cfedd737d39cb05604d90ef90d739
Author: Even Xu <[email protected]>
Subject: 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, which can be called 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
[ ... ]
> +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 error path bypass the interrupt unquiesce operation?

If wait_for_dma_pause fails, execution jumps directly to the 'end' label.
The 'end' label simply unlocks the mutex and returns, entirely skipping the
thc_interrupt_quiesce(dev, false) cleanup step.

Could this leave the hardware in an inconsistent state where device
interrupts are permanently disabled and the device cannot be recovered?

> +	}
> +
> +	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]
Is it safe to call thc_dma_unconfigure() here while TX and SW DMA engines
might still be active?

If a synchronous operation like thc_dma_write() or thc_swdma_read() times out,
it releases thc_bus_lock while leaving its respective hardware DMA engine
active (START bit = 1). If thc_rxdma_reset() is subsequently called to
recover the device, it invokes thc_dma_unconfigure().

thc_dma_unconfigure() loops over all four DMA channels and sets their
Physical Region Descriptor (PRD) base addresses to 0, but it does not clear
the START bits for TXDMA and 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]);
	}

	regmap_write_bits(dev->thc_regmap, THC_M_PRT_READ_DMA_CNTRL_1_OFFSET,
			  THC_M_PRT_READ_DMA_CNTRL_START, 0);

	regmap_write_bits(dev->thc_regmap, THC_M_PRT_READ_DMA_CNTRL_2_OFFSET,
			  THC_M_PRT_READ_DMA_CNTRL_START, 0);
}

By zeroing out the PRD base address for a still-running DMA engine, could
this direct the hardware to fetch descriptors or write data to physical
address 0, leading to severe memory corruption or IOMMU faults?

> +
> +	ret = thc_dma_configure(dev);

[Severity: High]
Are the PRD tables correctly re-initialized in memory before restarting the
RxDMA engines?

The hardware modifies PRD table entries (e.g., updating length fields) upon
receiving data. Under normal operation, the driver restores these original
sizes in thc_dma_read() using thc_copy_one_sgl_to_prd().

When a fatal error occurs and thc_rxdma_reset() is called, the hardware
read and write pointers are abruptly reset to 0 via thc_dma_configure().
However, if the in-memory PRD tables are not re-initialized, could the
hardware fetch PRD index 0 containing a reduced length and stale flags from
a past partial transaction?

Would this cause the hardware to assume a smaller buffer capacity, triggering
a Buffer Overrun or PRD Entry Error upon the next DMA write?

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

[Severity: Medium]
Should the return value of thc_interrupt_quiesce() be checked and propagated?

This function polls the hardware and can return -ETIMEDOUT if the hardware
state is lost or unresponsive. By discarding this return value and instead
returning the 0 (success) from the prior thc_dma_configure() step, could the
caller falsely assume recovery was successful while the device remains
incapable of generating interrupts?

> +
> +end:
> +	mutex_unlock(&dev->thc_bus_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(thc_rxdma_reset, "INTEL_THC");

[Severity: High]
This isn't a bug introduced by this patch, but does thc_dma_write()
overwrite its timeout error code on failure paths?

Looking at thc_dma_write() in the same file:

	ret = wait_event_interruptible_timeout(dev->write_complete_wait,
					       dev->write_done, 1 * HZ);
	if (ret <= 0 || !dev->write_done) {
		dev_err_once(dev->dev, "timeout for waiting TxDMA completion\n");
		ret = -ETIMEDOUT;
		goto end;
	}
	...
end:
	mutex_unlock(&dev->thc_bus_lock);

	if (restore_interrupts)
		ret = thc_interrupt_quiesce(dev, false);

	return ret;

If wait_event_interruptible_timeout() times out, ret is set to -ETIMEDOUT.
However, since unquiescing typically succeeds and returns 0, the original
-ETIMEDOUT error is overwritten by 0.

Could this falsely return success to callers when the DMA write actually
timed out, leading to data loss and state desynchronization?

[Severity: High]
This isn't a bug introduced by this patch, but do the error paths in
thc_swdma_read() leak the interrupt quiesced state?

Looking at thc_swdma_read() in the same file:

	ret = thc_swdma_read_start(dev, write_buff, write_len, prd_tbl_len);
	if (ret)
		goto end;

	ret = wait_event_interruptible_timeout(dev->swdma_complete_wait,
					       dev->swdma_done, 1 * HZ);
	if (ret <= 0 || !dev->swdma_done) {
		dev_err_once(dev->dev, "timeout for waiting SWDMA completion\n");
		ret = -ETIMEDOUT;
		goto end;
	}

	ret = thc_dma_read(dev, &dev->dma_ctx->dma_config[THC_SWDMA],
			   read_buff, read_len, NULL);
	if (ret)
		goto end;

	ret = thc_swdma_read_completion(dev);

end:
	mutex_unlock(&dev->thc_bus_lock);
	return ret;

If the software DMA read times out or if thc_dma_read() fails, execution
jumps to the 'end' label, bypassing thc_swdma_read_completion().

Since thc_swdma_read_start() previously quiesced interrupts and stopped
RXDMA2, could bypassing the completion routine cause the device to
permanently leak its interrupt quiesced state? Would this leave the SWDMA
engine active and prevent THC_RXDMA2 from being restarted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1