Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] AB-BA deadlock in the error path of haptics_start_fifo() between `h->fifo_lock` and `disable_irq()`.
- [High] Failure to write to the FIFO in the IRQ handler leaves the interrupt condition asserted, leading to an interrupt storm.
- [Medium] Stop requests for inactive effects incorrectly terminate the currently playing effect.
- [Low] Mixing `goto`-based error handling with `guard()` scope-based cleanup in `haptics_start_fifo()`.
--

--- Patch [3]: [PATCH v3 3/3] input: misc: Add Qualcomm SPMI PMIC haptics driver ---

commit 516c3c8b367d0b2e322ea97c78aee0907ed541f0
Author: Fenglin Wu [off-list ref]

    input: misc: Add Qualcomm SPMI PMIC haptics driver
    
    Add an initial driver for the Qualcomm PMIH0108 PMIC haptics module,
    named as HAP530_HV. This module supports several play modes, including
    DIRECT_PLAY, FIFO, PAT_MEM, and SWR, each with distinct data sourcing
    and hardware data handling logic.

[ ... ]

+static int haptics_start_fifo(struct qcom_haptics *h, int effect_id)
+{
+	struct qcom_haptics_effect *eff = &h->effects[effect_id];
+	u32 vmax = eff->vmax_mv ? eff->vmax_mv : h->vmax_mv;
+	u32 init_len;
+	bool data_done;
+	int ret;
+
+	if (!eff->fifo_data || !eff->data_len)
+		return -EINVAL;
+
+	ret = haptics_clear_faults(h);
+	if (ret)
+		return ret;
+
+	/* Disable auto-resonance for FIFO mode */
+	ret = cfg_update_bits(h, HAP_CFG_AUTORES_CFG_REG, AUTORES_EN_BIT, 0);
+	if (ret)
+		return ret;
+
+	ret = haptics_set_vmax(h, haptics_gain_scaled_vmax(h, vmax));
+	if (ret)
+		return ret;
+
+	ret = ptn_update_bits(h, HAP_PTN_FIFO_PLAY_RATE_REG,
+			      FIFO_PLAY_RATE_MASK,
+			      FIELD_PREP(FIFO_PLAY_RATE_MASK, eff->play_rate));
+	if (ret)
+		return ret;
+
+	/* Flush FIFO before loading new data */
+	ret = ptn_write(h, HAP_PTN_MEM_OP_ACCESS_REG, MEM_FLUSH_RELOAD_BIT);
+	if (ret)
+		return ret;
+	ret = ptn_write(h, HAP_PTN_MEM_OP_ACCESS_REG, 0);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&h->fifo_lock);
+
+	/* Write the initial chunk and initialise streaming state */
+	init_len = min_t(u32, eff->data_len, FIFO_INIT_FILL);
+	ret = haptics_write_fifo_chunk(h, eff->fifo_data, init_len);

[Severity: Low]
Is it intended to mix goto-based error handling with scope-based guard()
cleanup here? The subsystem cleanup guidelines specify that functions should
use either goto-based cleanups or scope-based cleanups, but never both in
the same function.

+	if (ret)
+		return ret;
+
+	h->fifo_data    = eff->fifo_data;
+	h->data_len     = eff->data_len;
+	h->data_written = init_len;
+
+	/*
+	 * Set empty threshold.  When threshold > 0 the hardware fires the
+	 * FIFO-empty interrupt when occupancy drops below the threshold,
+	 * allowing the driver to refill.  A threshold of 0 disables the IRQ.
+	 */
+	data_done = (h->data_written >= h->data_len);
+	if (!data_done) {
+		ret = ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG,
+				FIFO_EMPTY_THRESH / FIFO_THRESH_LSB);
+		if (ret)
+			goto free_fifo;
+
+		haptics_fifo_irq_enable(h, true);
+	}
+
+	ret = cfg_write(h, HAP_CFG_SPMI_PLAY_REG,
+			 PLAY_EN_BIT | FIELD_PREP(PAT_SRC_MASK, PAT_SRC_FIFO));
+	if (ret) {
+		dev_err(h->dev, "trigger FIFO play failed, ret=%d\n", ret);
+		goto fail;
+	}
+
+	return 0;
+
+fail:
+	ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG, 0);
+	haptics_fifo_irq_enable(h, false);
+free_fifo:
+	h->fifo_data = NULL;
+	return ret;
+}

[Severity: High]
Can this sequence cause a deadlock? In haptics_start_fifo(), h->fifo_lock is
still held via the guard() scope when jumping to the fail label.

The code then calls haptics_fifo_irq_enable() which disables the IRQ
synchronously using disable_irq().

If the threaded IRQ handler haptics_fifo_empty_irq() is concurrently running,
it will block waiting for h->fifo_lock, while disable_irq() blocks waiting
for the handler to complete.

[ ... ]

+static irqreturn_t haptics_fifo_empty_irq(int irq, void *dev_id)
+{
+	struct qcom_haptics *h = dev_id;
+	u32 sts, to_write;
+	int ret;
+
+	ret = regmap_read(h->regmap,
+			  h->cfg_base + HAP_CFG_INT_RT_STS_REG, &sts);
+	if (ret || !(sts & FIFO_EMPTY_BIT))
+		return IRQ_HANDLED;
+
+	guard(mutex)(&h->fifo_lock);
+
+	if (!h->fifo_data)
+		return IRQ_HANDLED;
+
+	/* Refill: write the next chunk */
+	to_write = min_t(u32, h->data_len - h->data_written,
+			 h->fifo_len - FIFO_EMPTY_THRESH);
+	ret = haptics_write_fifo_chunk(h, &h->fifo_data[h->data_written], to_write);
+	if (ret) {
+		dev_err(h->dev, "refill FIFO samples failed, ret=%d\n", ret);
+		return IRQ_HANDLED;
+	}

[Severity: High]
If haptics_write_fifo_chunk() fails here, could this cause an interrupt
storm?

By returning IRQ_HANDLED without writing samples to the FIFO or disabling
the empty threshold via ptn_write(), the hardware FIFO occupancy remains
below the threshold.

Once the IRQ is unmasked upon return, the hardware will immediately trigger
the interrupt again.

[ ... ]

+static void haptics_play_work(struct work_struct *work)
+{
+	struct qcom_haptics *h = container_of(to_delayed_work(work),
+					      struct qcom_haptics, play_work);
+	struct haptics_play_req *req = haptics_dequeue_play_req(h);
+	bool rearmed = false;
+	u32 length_us;
+	int ret;
+
+	guard(mutex)(&h->play_lock);
+
+	if (req && req->play) {
+		ret = haptics_start_locked(h, req->effect_id);
+		if (ret) {
+			dev_err(h->dev, "play haptics failed, ret=%d\n", ret);
+		} else {
+			/*
+			 * Re-arm the work to stop the play or advance
+			 * to next play after the play duration
+			 */
+			length_us = h->effects[req->effect_id].length_us;
+			if (length_us) {
+				schedule_delayed_work(&h->play_work,
+						      usecs_to_jiffies(length_us));
+				rearmed = true;
+			}
+		}
+	} else {
+		haptics_stop_locked(h, false);
+	}

[Severity: Medium]
Will this unconditionally stop the active effect even if the stop request
was for a different effect?

Since the ff-core tracks multiple effects, an application might send a stop
command for an inactive effect. The else block here does not verify if
req->effect_id matches h->active_effect_id before terminating the hardware
playback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-spmi-haptics-v3-0-c931bb7cb94f@oss.qualcomm.com?part=3