Re: [PATCH v3] HID: intel-thc-hid: intel-thc: restore PRD table capacity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: intel-thc-hid: missing THC_SWDMA PRD restore in
  thc_dma_configure()
- [Medium] HID: intel-thc-hid: un-restored RxDMA2 PRD capacity in
  thc_swdma_read()
--

commit dd12d53039d8c9ed77913f6942ca6c87e513e4db
Author: Emfox Zhou <emfox@debian.org>

HID: intel-thc-hid: intel-thc: restore PRD table capacity

This commit restores the PRD tables for the THC_RXDMA1 and THC_RXDMA2
channels in thc_dma_configure() after rewinding the DMA pointers and
guards thc_copy_sgls_to_prd() against disabled or unallocated channels.

Link: https://lore.kernel.org/r/20261115001100.313270-1-emfox@debian.org

> 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 c80ef8c205742..64fd890e5a583 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

[ ... ]

> @@ -491,6 +494,10 @@ int thc_dma_configure(struct thc_device *dev)
>  		return -EINVAL;
>  	}
>  
> +	/* Pointers rewound, entries still short: restore before restart. */
> +	thc_copy_sgls_to_prd(dev, &dma_ctx->dma_config[THC_RXDMA1]);
> +	thc_copy_sgls_to_prd(dev, &dma_ctx->dma_config[THC_RXDMA2]);
> +
>  	for (chan = 0; chan < MAX_THC_DMA_CHANNEL; chan++) {
>  		dma_set_prd_base_addr(dev,
>  				      dma_ctx->dma_config[chan].prd_tbls_dma_handle,

[Severity: Medium]
Should this also restore dma_ctx->dma_config[THC_SWDMA]?

thc_reset_dma_settings() rewinds the read pointer for THC_SWDMA as well.
If thc_swdma_read() is interrupted or times out in
wait_event_interruptible_timeout() after the controller writes a shorter
frame into THC_SWDMA's PRD table, thc_dma_read() is never reached to
restore the entry capacity via thc_copy_one_sgl_to_prd().  Subsequent
larger SWDMA reads will then fail with a hardware DMA buffer overrun even
after thc_dma_configure() runs.

[Severity: Medium]
Does the thc_reset_dma_settings() path in thc_swdma_read() also leave
THC_RXDMA2 PRD entries shortened?

thc_swdma_read_start() pauses THC_RXDMA1 and THC_RXDMA2 and calls
thc_reset_dma_settings(), which rewinds their TPCRP pointers to 0, and
thc_swdma_read_completion() resets them again before restarting
THC_RXDMA2 with an empty ring (TPCWP set to 0x80).  If the controller
wrote a short frame into a THC_RXDMA2 PRD slot just before
thc_swdma_read_start() paused the engine and before the IRQ thread
consumed and restored it via thc_rxdma_read(), rewinding the pointers
makes the ring appear empty without restoring that PRD table, so the next
larger input report on that slot will trigger a buffer overrun and forced
reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115040756.602092-1-emfox@debian.org?part=1