Re: [PATCH] HID: intel-thc-hid: intel-thc: rewrite PRD tables on reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: intel-thc-hid: unrestored PRD tables in
  thc_dma_configure() and SWDMA
- [Low] HID: intel-thc-hid: intel-thc: missing Fixes trailer for
  thc_rxdma_reset()
--

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

> HID: intel-thc-hid: intel-thc: rewrite PRD tables on reset
>
> RxDMA reset takes the DMA configuration down and puts it back, but leaves
> the PRD entries as the controller wrote them. A frame that was queued and
> not read keeps its shortened length, so the next frame runs past the end of
> the table: the controller reports a buffer overrun, which is a transfer
> error and leads back into the same reset, with the tables still untouched.
>
> Write the receive tables again while both engines are paused, before the
> configuration starts them. The configuration is the wrong place for it: on
> resume it runs with a frame that arrived before the suspend still queued.
>
> Signed-off-by: Emfox Zhou <emfox@debian.org>

[Severity: Low]
This fixes the missing PRD table restoration in thc_rxdma_reset().  Should
it include a Fixes: tag?

  Fixes: 1d46b8d40655 ("HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA")

> 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..28a1aa97b69b1 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

[ ... ]

> @@ -598,6 +601,10 @@ int thc_rxdma_reset(struct thc_device *dev)
>  
>  	thc_dma_unconfigure(dev);
>  
> +	/* Still paused: the configuration starts the receive engine again. */
> +	thc_copy_sgls_to_prd(dev, &dev->dma_ctx->dma_config[THC_RXDMA1]);
> +	thc_copy_sgls_to_prd(dev, &dev->dma_ctx->dma_config[THC_RXDMA2]);
> +
>  	ret = thc_dma_configure(dev);
>  	if (ret) {
>  		dev_err(dev->dev, "Re-config DMA failed during RxDMA reset\n");

[Severity: Medium]
Should thc_copy_sgls_to_prd() also run when thc_dma_configure() and
thc_swdma_read_completion() reset the ring pointers?

The commit message notes that thc_dma_configure() runs on resume with a
pre-suspend frame still queued, but thc_dma_configure() calls
thc_reset_dma_settings() and dma_set_start_bit(), which reset TPCRP to 0
and TPCWP to THC_POINTER_WRAPAROUND (0x80).  Because is_dma_buf_empty()
treats that state as an empty ring, any unread frame is discarded from the
ring while its shortened PRD entry length stays in prd_tbls.

If an unread frame or DMA error occurs just before quicki2c_suspend() or
quickspi_suspend() cancels recover_work, resume calls thc_dma_configure()
without thc_rxdma_reset(), leaving shortened PRD entries that will trigger
a buffer overrun on the next larger frame.  thc_swdma_read_completion()
likewise resets TPCRP and TPCWP on THC_RXDMA2 without restoring its PRD
tables if a frame arrived just before thc_swdma_read_start() paused RxDMA.

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