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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Repeated playback requests for an actively playing effect leak runtime PM usage counters, permanently preventing the device from suspending.
- [High] The threaded IRQ handler sleeps in atomic context by executing sleepable regmap operations while holding a spinlock.
- [High] Incorrect teardown ordering in `remove()` and `suspend()` allows a work item to be re-queued and executed on freed memory (Use-After-Free).
- [High] Erasing or updating a force feedback effect frees the effect's data buffer synchronously, causing a Use-After-Free in the IRQ handler.
- [High] A failed memory allocation during effect upload leaves internal state corrupted, guaranteeing a NULL pointer dereference upon playback.
- [High] A lockless race condition between the input playback callback and the FIFO empty IRQ handler can cause new playback requests to be silently dropped.
- [High] A data race on `h->cur_effect_id` allows userspace to misdirect the stop sequence, leaving hardware IRQs enabled while dropping PM references.
- [Medium] Dynamically allocated custom waveform buffers are permanently leaked when the device is unbound.
- [Medium] Missing `abs()` on `s16` magnitude level causes massive arithmetic overflow for negative effects.
- [Medium] Registering the input device before PM runtime is fully configured creates a race condition for early playback events.
--

commit 84c087855458c41a5a109b1ffb5594db116b5170
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

input: misc: Add Qualcomm SPMI PMIC haptics driver

Add an initial driver for the Qualcomm PMIH010x 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
> new file mode 100644
> index 0000000000000..75b3e338b54eb
> --- /dev/null
> +++ b/drivers/input/misc/qcom-spmi-haptics.c
> @@ -0,0 +1,831 @@
[ ... ]
> +static int haptics_start_direct_play(struct qcom_haptics *h, int effect_id)
> +{
> +	struct ff_effect *ffe = &h->input->ff->effects[effect_id];
> +	u8 amplitude = (u8)((u32)ffe->u.constant.level * 255 / 0x7FFF);

[Severity: Medium]
Does this cast cause an arithmetic overflow for negative effect levels?

The ffe->u.constant.level is an s16 which can be negative to represent
opposite-direction effects. Casting a negative s16 directly to u32
sign-extends it, resulting in a large unsigned value. Multiplying this by
255 overflows the 32-bit boundary, producing a garbage amplitude for the
hardware register.

Consider taking the absolute value of the level before casting and scaling.

[ ... ]
> +static irqreturn_t haptics_fifo_empty_irq(int irq, void *dev_id)
> +{
> +	struct qcom_haptics *h = dev_id;
> +	unsigned long flags;
> +	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;
> +
> +	spin_lock_irqsave(&h->fifo_lock, flags);

[Severity: High]
Is it safe to hold a spinlock while calling sleepable regmap operations?

The haptics_write_fifo_chunk() function maps to regmap_bulk_write(). For an
SPMI PMIC, the regmap operates over a slow bus and takes a mutex to sleep
during transactions. Calling these sleepable functions while holding
fifo_lock results in a scheduling while atomic bug.

[ ... ]
> +	if (h->data_done) {
> +		ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG, 0);
> +		h->fifo_data = NULL;
> +		h->play_request = false;
> +		schedule_work(&h->play_work);

[Severity: High]
Can a lockless race condition here cause new playback requests to be silently
dropped?

When an effect completes, this sets play_request to false. Concurrently, if a
new playback request arrives, haptics_playback() sets it to true. If the IRQ
handler runs immediately after haptics_playback(), it overwrites the new true
value with false.

When haptics_play_work() executes, it would observe play_request as false,
stop the hardware, and drop the newly queued playback request.

> +		spin_unlock_irqrestore(&h->fifo_lock, flags);
> +		return IRQ_HANDLED;
> +	}
> +
> +	/* Refill: write the next chunk, conservatively sized to the threshold */
> +	to_write = min_t(u32, h->data_len - h->data_written,
> +			 h->fifo_len - FIFO_EMPTY_THRESH);
> +	haptics_write_fifo_chunk(h, &h->fifo_data[h->data_written], to_write);

[Severity: High]
Is there a risk of a use-after-free here if an effect is erased while playing?

The input framework allows effect upload or erase to occur concurrently with
playback. In haptics_erase() and haptics_upload_effect(), the driver calls
kfree(priv->fifo_data) synchronously.

However, the stop request only schedules an asynchronous work item to stop
playback. Because the hardware stop is deferred, the hardware may still be
actively playing and asserting the FIFO-empty interrupt. The IRQ handler
will then attempt to read from the freed fifo_data.

[ ... ]
> +static void haptics_play_work(struct work_struct *work)
> +{
> +	struct qcom_haptics *h = container_of(work, struct qcom_haptics, play_work);
> +	int id, ret;
> +
> +	mutex_lock(&h->play_lock);
> +
> +	if (!h->play_request) {
> +		haptics_stop_locked(h);
> +		if (h->pm_ref_held) {
> +			pm_runtime_mark_last_busy(h->dev);
> +			pm_runtime_put_autosuspend(h->dev);
> +			h->pm_ref_held = false;
> +		}
> +		goto unlock;
> +	}
> +
> +	ret = pm_runtime_resume_and_get(h->dev);
> +	if (ret < 0) {
> +		dev_err(h->dev, "failed to resume device: %d\n", ret);
> +		goto unlock;
> +	}
> +	h->pm_ref_held = true;

[Severity: High]
Can this leak runtime PM usage counters?

The pm_ref_held flag is set to true without checking if it is already true.
If userspace issues multiple start events without interleaved stops (for
example, to update parameters), the PM usage counter is incremented multiple
times.

When playback is eventually stopped, the single check drops only one
reference via pm_runtime_put_autosuspend(), permanently leaking all other
references and preventing the device from suspending.

[ ... ]
> +static int haptics_upload_effect(struct input_dev *dev,
> +				 struct ff_effect *effect,
> +				 struct ff_effect *old)
> +{
[ ... ]
> +		priv->play_rate = (u8)buf[CUSTOM_DATA_RATE_IDX];
> +		priv->vmax_mv   = (u32)clamp_val(buf[CUSTOM_DATA_VMAX_IDX], 0, VMAX_MV_MAX);
> +		priv->data_len = effect->u.periodic.custom_len - CUSTOM_DATA_SAMPLE_START;
> +
> +		kfree(priv->fifo_data);
> +		priv->fifo_data = kmalloc(priv->data_len, GFP_KERNEL);
> +		if (!priv->fifo_data) {
> +			kfree(buf);
> +			return -ENOMEM;
> +		}

[Severity: High]
Could a failed memory allocation leave the internal state corrupted?

When updating an existing FF_PERIODIC effect, data_len is updated to a new
size before kmalloc() is called. If kmalloc() fails, the function returns
-ENOMEM, leaving data_len non-zero while fifo_data is NULL.

The input core keeps the existing effect ID valid. If userspace subsequently
requests to play this effect, haptics_start_fifo() computes a non-zero
init_len and passes the NULL pointer to haptics_write_fifo_chunk(), causing
a crash.

[ ... ]
> +static int haptics_playback(struct input_dev *dev, int effect_id, int val)
> +{
> +	struct qcom_haptics *h = input_get_drvdata(dev);
> +
> +	h->cur_effect_id = effect_id;
> +	h->play_request  = (val > 0);

[Severity: High]
Could this lockless update of cur_effect_id misdirect the stop sequence?

If a FIFO effect is actively playing and userspace issues a playback request
for a different DIRECT_PLAY effect, cur_effect_id is updated asynchronously.
When play_work eventually processes the stop/start sequence,
haptics_stop_locked() checks the newly updated mode:

	if (h->effects[h->cur_effect_id].mode == HAPTICS_FIFO) {
		ptn_write(h, HAP_PTN_FIFO_EMPTY_CFG_REG, 0);

Seeing DIRECT_PLAY, it would skip disabling the FIFO IRQ and clearing
fifo_data, while still dropping the PM reference. The device auto-suspends,
but the IRQ remains enabled and fires spuriously.

[ ... ]
> +static int qcom_haptics_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ff = input->ff;
> +	ff->upload   = haptics_upload_effect;
> +	ff->playback = haptics_playback;
> +	ff->erase    = haptics_erase;
> +	ff->set_gain = haptics_set_gain;

[Severity: Medium]
Should the ff->destroy callback be implemented to prevent memory leaks?

Custom waveform buffers are allocated using kmalloc() and stored in
fifo_data. This memory is normally freed in haptics_erase(). However, when
the driver is unbound and the input device is unregistered, the input core
destroys the ff device but does not automatically invoke erase() for
surviving effects.

Without a destroy callback to clean up the effects array, the allocated
buffers appear to be permanently leaked.

> +
> +	ret = input_register_device(input);
> +	if (ret) {
> +		input_ff_destroy(input);
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to register input device\n");
> +	}
> +
> +	platform_set_drvdata(pdev, h);
> +
> +	/*
> +	 * Grab a reference on behalf of probe (usage_count → 1), mark the
> +	 * device active, then enable runtime PM.
> +	 */
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, HAPTICS_AUTOSUSPEND_MS);
> +	devm_pm_runtime_set_active_enabled(&pdev->dev);

[Severity: Medium]
Is there a race condition by registering the input device before PM runtime
is fully configured?

This call makes the force-feedback device immediately visible to userspace.
If an application sends a play request before the probe function finishes,
haptics_play_work() will run and call pm_runtime_resume_and_get(). Since PM
runtime is not yet fully enabled, the PM runtime API will return an error
and fail the playback.

[ ... ]
> +static void qcom_haptics_remove(struct platform_device *pdev)
> +{
> +	struct qcom_haptics *h = platform_get_drvdata(pdev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +
> +	cancel_work_sync(&h->play_work);
> +	mutex_lock(&h->play_lock);
> +	haptics_stop_locked(h);
> +	haptics_enable_module(h, false);
> +	mutex_unlock(&h->play_lock);
> +
> +	input_unregister_device(h->input);

[Severity: High]
Does this teardown ordering allow a work item to be re-queued and executed
on freed memory?

During driver removal, cancel_work_sync() is called before the input device
is unregistered. This creates a race window where the input device is still
visible to userspace, allowing applications to issue playback events that
call schedule_work().

Once remove() completes, the devres framework frees the qcom_haptics memory.
The mistakenly re-queued background work will then execute, dereferencing
the freed driver context.

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