Re: [PATCH v2 2/5] HID: intel-thc-hid: intel-ipts: receive reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] HID: intel-thc-hid: intel-ipts: shared IRQ race with
  thc_ipts_recover()
- [High] HID: intel-thc-hid: intel-ipts: IRQF_ONESHOT mismatch on shared
  INTx in thc_ipts_probe()
- [Medium] HID: intel-thc-hid: intel-ipts: unconditional IRQ_HANDLED on
  shared IRQ
- [Medium] HID: intel-thc-hid: intel-ipts: short IRQ cause drain in
  thc_ipts_thread()
--

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

HID: intel-thc-hid: intel-ipts: receive reports

This commit configures the Rx and Tx DMA engines for Intel IPTS ports,
registers a shared threaded interrupt handler to receive frames, and adds a
work item to reset the receive engine after transfer errors.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index f139b8f9443d0..790c3eaae15f2 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> @@ -20,6 +25,12 @@
>  /* Time the controller needs after reset before it answers on the bus. */
>  #define THC_IPTS_BOOT_MS		100
>  
> +/*
> + * The core returns after the first cause it finds, so one interrupt can need
> + * up to four reads: non-DMA, transaction error, overrun, then the data path.
> + */
> +#define THC_IPTS_IRQ_CAUSES		4

[Severity: Medium]
Is THC_IPTS_IRQ_CAUSES off by one for draining thc_interrupt_handler()?

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c:thc_interrupt_handler()
has four separate early returns before it reaches the DMA completion checks:
non-DMA, transaction error, buffer overrun/stall, and fatal error, followed
by the data path on a fifth call.  In addition, thc_ipts_read_frames() runs
inside the loop while the port is masked, so new status bits can arrive
before the fourth pass finishes and remain unread when the handler unmasks
the port, leaving a concurrent thc_dma_write() to time out under
thc_bus_lock.

> @@ -127,6 +138,193 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)

[ ... ]

> +irqreturn_t thc_ipts_hardirq(int irq, void *data)
> +{
> +	struct thc_ipts *idev = data;
> +
> +	/*
> +	 * The pin is shared and this may be another function's interrupt; the
> +	 * cause can only be read in the thread, so claim it. Mask first, or a
> +	 * level triggered line stays asserted on the way out.
> +	 */
> +	if (READ_ONCE(idev->stopping) || !READ_ONCE(idev->dma_ready)) {
> +		thc_interrupt_enable(idev->thc, false);
> +		return IRQ_HANDLED;
> +	}
> +
> +	thc_interrupt_enable(idev->thc, false);
> +
> +	return IRQ_WAKE_THREAD;
> +}

[ ... ]

> +void thc_ipts_recover(struct work_struct *work)
> +{
> +	struct thc_ipts *idev = container_of(work, struct thc_ipts,
> +					     recover_work);
> +
> +	if (thc_rxdma_reset(idev->thc)) {
> +		/* Nothing left: keep the port masked until it restarts. */
> +		dev_err(idev->dev, "cannot reset the receive engine\n");
> +		return;
> +	}
> +
> +	if (!READ_ONCE(idev->stopping))
> +		thc_interrupt_enable(idev->thc, true);
> +}
> +
> +irqreturn_t thc_ipts_thread(int irq, void *data)
> +{
> +	struct thc_ipts *idev = data;
> +	bool recovering = false;
> +	int pass;
> +
> +	/*
> +	 * The port stays masked until the end of this handler, so a cause that
> +	 * arrives in between leaves only its status bit: keep reading until
> +	 * the core reports none.
> +	 */
> +	for (pass = 0; pass < THC_IPTS_IRQ_CAUSES; pass++) {
> +		int mask = thc_interrupt_handler(idev->thc);
> +
> +		/* The core reports this one when no cause is pending. */
> +		if (mask == BIT(THC_UNKNOWN_INT))
> +			break;
> +
> +		if (mask & (BIT(THC_FATAL_ERR_INT) | BIT(THC_TXN_ERR_INT))) {
> +			dev_err(idev->dev,
> +				"transfer error, resetting the engine\n");
> +			recovering = true;
> +			/*
> +			 * Keep reading: the core stops at the first cause, so
> +			 * a Tx completion queued behind the error would wait
> +			 * out its timeout under the bus lock.
> +			 */
> +			continue;
> +		}
> +
> +		if (recovering)
> +			continue;
> +
> +		/*
> +		 * A non-DMA cause needs no action: the core acknowledges it,
> +		 * and this port moves report data through the DMA engines.
> +		 */
> +		if (mask & BIT(THC_RXDMA2_INT) && thc_ipts_read_frames(idev))
> +			recovering = true;
> +	}
> +
> +	/*
> +	 * The work item resets the engine and unmasks the port again; reading
> +	 * frames while it rewrites the ring would corrupt the descriptors.
> +	 */
> +	if (recovering) {
> +		schedule_work(&idev->recover_work);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (!READ_ONCE(idev->stopping))
> +		thc_interrupt_enable(idev->thc, true);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Can a shared interrupt from another PCH function wake thc_ipts_thread() while
thc_ipts_recover() is resetting the DMA engine?

Because recovering is only a local variable in thc_ipts_thread(), idev->stopping
remains false and idev->dma_ready remains true after recover_work is
scheduled.  When another device on the shared INTx line fires an interrupt:

irq/idev->irq                     kworker
thc_ipts_thread()
  recovering = true
  schedule_work(&recover_work)
  return IRQ_HANDLED
                                  thc_ipts_recover()
                                    thc_rxdma_reset(idev->thc)
                                      thc_dma_unconfigure()
<shared INTx from another device>
thc_ipts_hardirq()
  return IRQ_WAKE_THREAD
thc_ipts_thread()
  thc_ipts_read_frames()
    thc_rxdma_read()              <-- races with thc_rxdma_reset()
  thc_interrupt_enable(..., true) <-- unmasks port mid-reset

If no THC cause is pending during that foreign interrupt, thc_ipts_thread()
sees BIT(THC_UNKNOWN_INT) with recovering == false and calls
thc_interrupt_enable(idev->thc, true), unmasking the port while
thc_rxdma_reset() is reconfiguring the PRD tables or after thc_rxdma_reset()
has failed.

[Severity: Medium]
Should thc_ipts_hardirq() and thc_ipts_thread() return IRQ_NONE when no
interrupt came from this controller?

In thc_ipts_hardirq(), returning IRQ_HANDLED while idev->stopping is true or
idev->dma_ready is false (such as during the 100 ms boot delay in
thc_ipts_port_start() after the IRQ is requested) claims every shared
interrupt on the line.  Likewise, when a foreign device triggers the shared
INTx line during normal operation and thc_ipts_thread() breaks out on
pass == 0 with BIT(THC_UNKNOWN_INT), returning IRQ_HANDLED increments
desc->threads_handled and prevents note_interrupt() in kernel/irq/spurious.c
from detecting a screaming unhandled interrupt on the shared line.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> index 48046cd1212a6..5342cf25224fb 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/pci-ipts.c
> @@ -77,10 +78,38 @@ static int thc_ipts_probe(struct pci_dev *pci, const struct pci_device_id *id)
>  	if (IS_ERR(idev->thc))
>  		return PTR_ERR(idev->thc);
>  
> +	/*
> +	 * Interrupt remapping rejects the MSI requests these ports send, so
> +	 * take the legacy pin (shared with other functions on the PCH, hence
> +	 * IRQF_SHARED); MSI is only for a platform that routes no pin.
> +	 */
> +	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_INTX);
> +	if (ret < 0) {
> +		dev_warn(idev->dev, "no interrupt pin, falling back to MSI\n");
> +		ret = pci_alloc_irq_vectors(pci, 1, 1,
> +					    PCI_IRQ_MSI | PCI_IRQ_MSIX);
> +	}
> +	if (ret < 0) {
> +		dev_err(idev->dev, "no interrupt vector, error %d\n", ret);
> +		return ret;
> +	}
> +
> +	idev->irq = pci_irq_vector(pci, 0);
> +
> +	ret = devm_request_threaded_irq(idev->dev, idev->irq, thc_ipts_hardirq,
> +					thc_ipts_thread,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					KBUILD_MODNAME, idev);

[Severity: High]
Does requesting the shared INTx line with IRQF_ONESHOT | IRQF_SHARED fail with
-EBUSY when another PCH driver shares the pin?

__setup_irq() requires all actions on a shared IRQ line to agree on
IRQF_ONESHOT:

kernel/irq/manage.c:__setup_irq() {
    ...
		if ((old->flags & IRQF_ONESHOT) &&
		    (new->flags & IRQF_COND_ONESHOT))
			new->flags |= IRQF_ONESHOT;

		if (!((old->flags & new->flags) & IRQF_SHARED) ||
		    ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK) ||
		    ((old->flags ^ new->flags) & IRQF_ONESHOT))
			goto mismatch;
    ...
}

Other PCH drivers that share legacy INTx pins (such as intel-lpss, i2c-i801,
intel-ish-hid, or mei_me) register without IRQF_ONESHOT, so
devm_request_threaded_irq() returns -EBUSY if any of them probed first (or
causes a later threaded-IRQ driver like mei_me to fail with -EBUSY if
intel-ipts probed first).  Since thc_ipts_hardirq() already masks the port
interrupt in hardware via thc_interrupt_enable(idev->thc, false), IRQF_ONESHOT
can be dropped (or replaced with IRQF_COND_ONESHOT).

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