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

[sashiko-bot@kernel.org]

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 <fenglin.wu@oss.qualcomm.com>

    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.

> diff --git a/drivers/input/misc/qcom-spmi-haptics.c b/drivers/input/misc/qcom-spmi-haptics.c
> new file mode 100644
> index 0000000000000..ffbef7453459a
> --- /dev/null
> +++ b/drivers/input/misc/qcom-spmi-haptics.c
> @@ -0,0 +1,1160 @@

[ ... ]

> +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