Re: [PATCH v3 4/4] HID: Intel-thc-hid: Remove redundant dev_err()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Requesting the IRQ before allocating report buffers and initializing DMA causes an infinite interrupt storm that hangs the CPU.
- [High] Bypassing `quicki2c_dma_deinit()` on the error path causes a memory leak of non-managed DMA buffers and mappings.
--

--- Patch [4]: [PATCH v3 4/4] HID: Intel-thc-hid: Remove redundant dev_err() ---
commit 7eb5c91c4d9eb3ab7af0dabaab924561ec278776
Author: Pan Chuang <panchuang@vivo.com>

HID: Intel-thc-hid: Remove redundant dev_err()

This patch removes redundant driver-specific dev_err() calls after
devm_request_threaded_irq(). The core genirq/devres code automatically
logs detailed error messages on failure now.

> diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> index 46d3e9a019995..59f500345acbf 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -682,11 +682,8 @@ static int quicki2c_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  					quicki2c_irq_thread_handler,
>  					IRQF_ONESHOT, KBUILD_MODNAME,
>  					qcdev);
> -	if (ret) {
> -		dev_err_once(&pdev->dev,
> -			     "Failed to request threaded IRQ, irq = %d.\n", pdev->irq);
> +	if (ret)
>  		goto dev_deinit;
> -	}

[Severity: Critical]
This is a pre-existing issue, but does requesting the IRQ here before allocating
report buffers and initializing DMA cause an interrupt loop?

If a pending or spurious interrupt occurs right after devm_request_threaded_irq()
is called but before quicki2c_alloc_report_buf() and quicki2c_dma_init(), the
RT-priority threaded handler could execute.

The handler would find input_buf is NULL and fail to read the DMA buffer. Any
attempt to recover would fail because dma_init() hasn't run. The handler would
then unmask the interrupt. Because the hardware interrupt condition was never
cleared, it could immediately fire again, preempting the probe thread and
hanging the CPU.

>  
>  	ret = quicki2c_get_device_descriptor(qcdev);
>  	if (ret) {

[Severity: High]
This isn't a bug introduced by this patch, but could the error handling further
down in this function leak unmanaged DMA memory?

After quicki2c_dma_init() successfully allocates DMA buffers, subsequent
failures in thc_interrupt_quiesce(), quicki2c_set_power(), or
quicki2c_reset() jump to the dev_deinit label:

	ret = quicki2c_set_power(qcdev, HIDI2C_ON);
	if (ret) {
		dev_err(&pdev->dev, "Set Power On command failed, ret= %d\n", ret);
		goto dev_deinit;
	}

Jumping to dev_deinit bypasses the dma_deinit label and quicki2c_dma_deinit(),
which leaves the DMA resources allocated.

[ ... ]

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index 4ae2e1718b30c..504ef3c842ab6 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -636,11 +636,8 @@ static int quickspi_probe(struct pci_dev *pdev,
>  					quickspi_irq_thread_handler,
>  					IRQF_ONESHOT, KBUILD_MODNAME,
>  					qsdev);
> -	if (ret) {
> -		dev_err(&pdev->dev,
> -			"Failed to request threaded IRQ, irq = %d.\n", pdev->irq);
> +	if (ret)
>  		goto dev_deinit;
> -	}

[Severity: Critical]
This is a pre-existing issue, but might requesting the IRQ here before allocating
report buffers and initializing DMA lead to a similar interrupt loop?

Just like in quicki2c_probe(), if an interrupt fires right after
devm_request_threaded_irq() is called but before quickspi_alloc_report_buf()
and quickspi_dma_init(), the handler could fail to recover DMA state. It would
then unmask the interrupt, which immediately fires again, hanging the CPU in
an RT-priority interrupt loop.

>  
>  	ret = reset_tic(qsdev);
>  	if (ret) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720084310.210976-1-panchuang@vivo.com?part=4