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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Multiplexing play_work for both playback requests and duration timers causes race conditions where active effect duration timers are overwritten, leading to indefinite playback.
- [High] The driver permanently disables the playback workqueue in `qcom_haptics_close()` but lacks an `open` callback to re-enable it, rendering the device inoperable after the first close.
- [High] Manually disabling the hardware module in `close()` desynchronizes the runtime PM state, causing subsequent playbacks to silently fail if started quickly.
--

--- Patch [3]: [PATCH v5 3/3] input: misc: Add Qualcomm SPMI PMIC haptics driver ---
commit 64b23ca2ca86ee295bdc5ad669c51fb6d603b0fc
Author: Fenglin Wu <[email protected]>

    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. Currently, the driver provides support
    for two play modes using the input force-feedback framework: FF_CONSTANT
    effect for DIRECT_PLAY mode and FF_PERIODIC effect with FF_CUSTOM
    waveform for FIFO mode.

diff --git a/drivers/input/misc/qcom-spmi-haptics.c b/drivers/input/misc/qcom-spmi-haptics.c
--- /dev/null
+++ b/drivers/input/misc/qcom-spmi-haptics.c
[ ... ]
> +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) {
> +		/*
> +		 * Not a new request: the play-duration re-arm timer fired
> +		 * so stop the play
> +		 */
> +		if (h->active_effect_id != -1)
> +			haptics_stop(h, false);
> +	} else if (req->play) {
> +		ret = haptics_start(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 {
> +		if (req->effect_id == h->active_effect_id)
> +			haptics_stop(h, false);
> +	}
> +
> +	kfree(req);
> +
> +	/* Wait for the re-arm to stop the play */
> +	if (rearmed)
> +		return;
> +
> +	/* Handle the remaining requests if the queue is not empty */
> +	guard(spinlock_irqsave)(&h->play_queue_lock);
> +	if (!list_empty(&h->play_queue))
> +		schedule_delayed_work(&h->play_work, 0);
> +}

[Severity: High]
Does this code handle the active effect's stop timer correctly when a STOP
request for a different effect arrives?

If an effect is currently playing with a scheduled stop timer, and userspace
sends a STOP request for a different inactive effect, the worker sees that the
STOP request is for an inactive effect and drops it.

However, it appears it fails to re-arm the duration timer for the active
effect, which could result in the active effect vibrating indefinitely.

[ ... ]
> +static int haptics_playback(struct input_dev *dev, int effect_id, int val)
> +{
> +	struct qcom_haptics *h = input_get_drvdata(dev);
> +	int ret;
> +
> +	ret = haptics_enqueue_play_req(h, effect_id, val > 0);
> +	if (ret)
> +		return ret;
> +
> +	if (val > 0)
> +		/*
> +		 * Queue the play.  If a duration re-arm is already pending this
> +		 * is a no-op, so the new play waits for the current effect to
> +		 * finish before the worker dequeues it.
> +		 */
> +		schedule_delayed_work(&h->play_work, 0);
> +	else
> +		/*
> +		 * Run the worker now, cancelling any pending duration re-arm,
> +		 * so an explicit stop takes effect immediately.
> +		 */
> +		mod_delayed_work(system_percpu_wq, &h->play_work, 0);
> +
> +	return 0;
> +}

[Severity: High]
Can calling mod_delayed_work() here overwrite an active duration timer?

If userspace sends a STOP request for a different inactive effect,
mod_delayed_work() is called unconditionally, cancelling the active effect's
duration timer before the worker gets a chance to process the inactive STOP
request.

[ ... ]
> +static void qcom_haptics_close(struct input_dev *input)
> +{
> +	struct qcom_haptics *h = input_get_drvdata(input);
> +
> +	cancel_work_sync(&h->gain_work);
> +	haptics_enable_play_work(h, false);
> +	scoped_guard(mutex, &h->play_lock) {
> +		haptics_queue_flush(h);
> +		haptics_stop(h, false);
> +	}
> +
> +	haptics_enable_module(h, false);
> +}

[Severity: High]
Does disabling play_work here permanently render the device inoperable?

When the input device is closed, haptics_enable_play_work() uses
disable_delayed_work_sync() which sets the WORK_STRUCT_DISABLE bit on the
play_work item.

Because there is no corresponding open() callback registered to re-enable it,
subsequent playback requests will fail to schedule the work after the device
is opened again.

[Severity: High]
Can manually disabling the hardware module here desynchronize the runtime PM
state?

Calling haptics_enable_module() directly bypasses the PM runtime framework.
Earlier in the close sequence, haptics_stop() calls
pm_runtime_put_autosuspend() which leaves the PM state as RPM_ACTIVE for the
1000ms autosuspend delay.

If userspace reopens the device and requests playback within this window,
haptics_start() will see the device as already active and skip calling
qcom_haptics_runtime_resume(). The hardware module will remain disabled,
causing subsequent register writes to be silently ignored.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3