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

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

[Emfox Zhou]

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>
---
 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c | 7 +++++++
 1 file changed, 7 insertions(+)

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 c80ef8c20..28a1aa97b 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
@@ -211,6 +211,9 @@ static void thc_copy_sgls_to_prd(struct thc_device *dev,
 {
 	unsigned int i;
 
+	if (!config->is_enabled)
+		return;
+
 	memset(config->prd_tbls, 0, array_size(PRD_TABLE_SIZE, config->prd_tbl_num));
 
 	for (i = 0; i < config->prd_tbl_num; i++)
@@ -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");

base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb

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

[sashiko-bot]

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