Re: [PATCH v4 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] Missing stop logic in the haptics worker causes indefinite playback and PM reference leak when effect duration expires.
- [Medium] Explicit input_unregister_device() on a devm-managed device violates input subsystem rules.
- [Medium] Data race on `irq_enabled` causes permanently unbalanced IRQ disable depth on bus errors.
- [Low] Use of undefined macro `kmalloc_obj` causes compilation failure.
--

--- Patch [3]: [PATCH v4 3/3] input: misc: Add Qualcomm SPMI PMIC haptics driver ---
commit e63f14da39f2a372d48e418f9fac8b20cd9d0698
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

input: misc: Add Qualcomm SPMI PMIC haptics driver

Adds an initial driver for the Qualcomm PMIH0108 PMIC haptics module.
Supports DIRECT_PLAY and FIFO modes via the input force-feedback framework.

> diff --git a/drivers/input/misc/qcom-spmi-haptics.c b/drivers/input/misc/qcom-spmi-haptics.c
> new file mode 100644
> index 0000000000000..235f77e0229c5
> --- /dev/null
> +++ b/drivers/input/misc/qcom-spmi-haptics.c

[ ... ]

> +static int haptics_enqueue_play_req(struct qcom_haptics *h, int effect_id, bool play)
> +{
> +	struct haptics_play_req *req;
> +
> +	guard(spinlock_irqsave)(&h->play_queue_lock);
> +
> +	/*
> +	 * Coalesce with an already queued request for
> +	 * the same effect instead of appending.
> +	 */
> +	list_for_each_entry(req, &h->play_queue, node) {
> +		if (req->effect_id == effect_id) {
> +			req->play = play;
> +			return 0;
> +		}
> +	}
> +
> +	req = kmalloc_obj(*req, GFP_ATOMIC);
> +	if (!req)
> +		return -ENOMEM;

[Severity: Low]
Does this code compile successfully? I couldn't find kmalloc_obj() defined in
the standard kernel headers. Should this use kmalloc() or kzalloc() instead?

> +
> +	req->effect_id = effect_id;
> +	req->play = play;

[ ... ]

> +static irqreturn_t haptics_fifo_empty_irq(int irq, void *dev_id)
> +{

[ ... ]

> +	/* 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);
> +		/*
> +		 * If data refilling is failed,stop the HW play and disable the
> +		 * IRQ to prevent the FIFO empty IRQ being fired continuously.
> +		 */
> +		cfg_write(h, HAP_CFG_SPMI_PLAY_REG, 0);
> +		disable_irq_nosync(h->fifo_empty_irq);
> +		h->irq_enabled = false;

[Severity: Medium]
Can a data race occur on h->irq_enabled here? 

If userspace stops playback concurrently with a bus write failure in
haptics_fifo_empty_irq(), both paths could check and modify h->irq_enabled
without mutual exclusion.

For example, this path:

haptics_fifo_empty_irq()
    disable_irq_nosync(h->fifo_empty_irq);
    h->irq_enabled = false;

Could race with:

haptics_stop_locked()
    haptics_fifo_irq_enable(h, false);

This might result in disable_irq() being called twice while enable_irq() is
only called once, leaving the IRQ permanently disabled and breaking future
FIFO playback.

> +		return IRQ_HANDLED;
> +	}
> +
> +	h->data_written += to_write;

[ ... ]

> +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);
> +
> +	/* No valid request available */
> +	if (req == NULL)
> +		return;

[Severity: High]
When a finite-length effect expires, schedule_delayed_work() wakes up this
worker, but haptics_dequeue_play_req() returns NULL since the queue is
empty. 

Since we return early here when req is NULL, how does the driver stop the
active effect and release the runtime PM reference? This appears to leave
the device vibrating indefinitely.

> +
> +	if (req->play) {
> +		ret = haptics_start_locked(h, req->effect_id);

[ ... ]

> +static void qcom_haptics_remove(struct platform_device *pdev)
> +{
> +	struct qcom_haptics *h = platform_get_drvdata(pdev);
> +
> +	/*
> +	 * Unregister the input device explicitly at the beginning
> +	 * to avoid the input device being used after the resources
> +	 * are freed.
> +	 */
> +	input_unregister_device(h->input);

[Severity: Medium]
Is it safe to explicitly unregister this input device here?

The input device is allocated with devm_input_allocate_device() during
probe, which means the input core will automatically handle unregistration
when the provider device is unbound.

Manually calling input_unregister_device() on a devm-managed device can
lead to double-unregistration.

> +	disable_delayed_work_sync(&h->play_work);
> +	scoped_guard(mutex, &h->play_lock) {
> +		haptics_queue_flush(h);
> +		haptics_stop_locked(h, true);
> +	}
> +
> +	haptics_enable_module(h, false);

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