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

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

[Emfox Zhou]

The controller writes the frame length into the PRD entry it filled and
marks the end of the frame there; only the read path puts the capacity
back. thc_reset_dma_settings() rewinds the read pointers, so the ring
reads as empty while the entries keep those lengths, and the next frame
that does not fit runs past the end of its table: the controller reports
a buffer overrun, which is a transfer error and leads back into the same
reset, with the tables untouched.

Restore the receive tables where the pointers are rewound, which covers
the RxDMA reset, the resume path and the SWDMA round trips in one place.
Every caller has the engines paused: the reset and the SWDMA paths wait
for them, a resume finds them stopped by the suspend, and a probe has
just rebuilt the tables anyway. thc_copy_sgls_to_prd() returns early for
a channel that has no tables.

Fixes: 1d46b8d40655 ("HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA")
Signed-off-by: Emfox Zhou <emfox@debian.org>
---
Changes in v2:
  - Move the restore into thc_reset_dma_settings() so every pointer rewind
    is covered, and add the Fixes tag.

Link: https://lore.kernel.org/r/20261114231733.3916070-1-emfox@debian.org
---
 drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c | 13 +++++++++++++
 1 file changed, 13 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..544bf3a6b 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++)
@@ -285,6 +288,16 @@ static void thc_reset_dma_settings(struct thc_device *dev)
 	regmap_write_bits(dev->thc_regmap, THC_M_PRT_READ_DMA_CNTRL_SW_OFFSET,
 			  THC_M_PRT_READ_DMA_CNTRL_TPCPR,
 			  THC_M_PRT_READ_DMA_CNTRL_TPCPR);
+
+	if (!dev->dma_ctx)
+		return;
+
+	/*
+	 * Pointers back at the start while the entries keep the lengths the
+	 * controller wrote: restore the capacity for the frames that follow.
+	 */
+	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]);
 }
 
 static void release_dma_buffers(struct thc_device *dev,

base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] HID: intel-thc-hid: NULL dereference of prd_tbls in
  thc_copy_sgls_to_prd()
- [Medium] HID: intel-thc-hid: missing THC_SWDMA PRD restore in
  thc_reset_dma_settings()
- [Medium] HID: intel-thc-hid: race on RXDMA2 PRD tables in
  thc_reset_dma_settings()
- [Low] HID: intel-thc-hid: inaccurate Fixes trailer for
  thc_reset_dma_settings() fix
--

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

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

> HID: intel-thc-hid: intel-thc: restore PRD table capacity
>
> The controller writes the frame length into the PRD entry it filled and
> marks the end of the frame there; only the read path puts the capacity
> back. thc_reset_dma_settings() rewinds the read pointers, so the ring
> reads as empty while the entries keep those lengths, and the next frame
> that does not fit runs past the end of its table: the controller reports
> a buffer overrun, which is a transfer error and leads back into the same
> reset, with the tables untouched.
>
> Restore the receive tables where the pointers are rewound, which covers
> the RxDMA reset, the resume path and the SWDMA round trips in one place.
> Every caller has the engines paused: the reset and the SWDMA paths wait
> for them, a resume finds them stopped by the suspend, and a probe has
> just rebuilt the tables anyway. thc_copy_sgls_to_prd() returns early for
> a channel that has no tables.
>
> Fixes: 1d46b8d40655 ("HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA")
> Signed-off-by: Emfox Zhou <emfox@debian.org>

[Severity: Low]
Should the Fixes: tag point to a688404b2e20 ("HID: intel-thc-hid:
intel-thc: Add THC DMA interfaces") instead?

thc_reset_dma_settings() rewound the read pointers without restoring the
PRD tables in that initial commit, which already affected system resume,
error recovery via thc_dma_configure(), and SWDMA round trips via
thc_swdma_read_start() and thc_swdma_read_completion() before 1d46b8d40655
("HID: Intel-thc-hid: Intel-thc: Add API to reset read DMA") added
thc_rxdma_reset().

> 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..544bf3a6ba1af 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));

[Severity: Medium]
Can this cause a NULL pointer dereference on config->prd_tbls when
thc_reset_dma_settings() calls thc_copy_sgls_to_prd()?

dma_set_max_packet_size() sets config->is_enabled = true before
thc_dma_allocate() allocates config->prd_tbls, and release_dma_buffers()
frees config->prd_tbls and sets it to NULL without clearing
config->is_enabled.  During hibernation, quicki2c_poweroff() and
quickspi_poweroff() release the DMA buffers via thc_dma_release(); if
hibernation rolls back or aborts and quicki2c_restore() or
quickspi_restore() calls thc_dma_configure() -> thc_reset_dma_settings(),
thc_copy_sgls_to_prd() passes the config->is_enabled check and passes a
NULL config->prd_tbls to memset().

Checking !config->prd_tbls alongside !config->is_enabled (as
read_dma_buffer() does) would avoid this.

> @@ -285,6 +288,16 @@ static void thc_reset_dma_settings(struct thc_device *dev)
>  	regmap_write_bits(dev->thc_regmap, THC_M_PRT_READ_DMA_CNTRL_SW_OFFSET,
>  			  THC_M_PRT_READ_DMA_CNTRL_TPCPR,
>  			  THC_M_PRT_READ_DMA_CNTRL_TPCPR);
> +
> +	if (!dev->dma_ctx)
> +		return;
> +
> +	/*
> +	 * Pointers back at the start while the entries keep the lengths the
> +	 * controller wrote: restore the capacity for the frames that follow.
> +	 */
> +	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]);

[Severity: Medium]
Can rewriting THC_RXDMA2's PRD tables here race with
quicki2c_irq_thread_handler() reading an input report during an SWDMA
transfer?

thc_swdma_read() holds dev->thc_bus_lock across thc_swdma_read_start() and
thc_swdma_read_completion(), whereas quicki2c_irq_thread_handler() reads
THC_RXDMA2 without dev->thc_bus_lock.  Because thc_interrupt_quiesce() and
thc_wait_for_dma_pause() only poll hardware status bits, they do not wait
for an in-flight threaded IRQ handler:

CPU0                                  CPU1
quicki2c_get_report()                 quicki2c_irq_thread_handler()
  thc_swdma_read()                      handle_input_report()
    thc_swdma_read_start()                thc_rxdma_read(THC_RXDMA2)
      thc_reset_dma_settings()              thc_dma_read()
        thc_copy_sgls_to_prd(THC_RXDMA2)      read_dma_buffer()
          memset(config->prd_tbls, 0, ...)
                                                calc_message_len(prd_tbl)
                                                  <-- reads zeroed or full
                                                      capacity PRD entries
          thc_copy_one_sgl_to_prd()

If calc_message_len() sees the zeroed PRD entries from memset(),
read_dma_buffer() returns 0, thc_dma_read() returns -EIO, and
quicki2c_irq_thread_handler() schedules recovery without re-enabling THC
interrupts, causing the ongoing thc_swdma_read() to time out.  If
calc_message_len() instead sees the restored sg_dma_len(sg) capacity,
read_dma_buffer() copies max_packet_size bytes of stale buffer data into
qcdev->input_buf (which is only sized to max_input_len and can be smaller
than the 4K-aligned max_packet_size).

>  }

[Severity: Medium]
Should thc_reset_dma_settings() also restore
dev->dma_ctx->dma_config[THC_SWDMA]?

thc_reset_dma_settings() rewinds THC_M_PRT_READ_DMA_CNTRL_TPCPR for
THC_SWDMA just above, and thc_swdma_read_start() does not repopulate
dma_config[THC_SWDMA].prd_tbls[0] before starting a transfer.  If
thc_swdma_read() is interrupted or times out in
wait_event_interruptible_timeout() after the controller has written a
short frame into THC_SWDMA's PRD table, or if read_dma_buffer() fails
before reaching thc_copy_one_sgl_to_prd(), dma_config[THC_SWDMA].prd_tbls[0]
keeps the reduced entry length and early end_of_prd marker across the
reset, so a subsequent larger SWDMA read can hit the same buffer overrun.

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