Re: [PATCH v13 17/17] media: ti: j721e-csi2rx: Support system suspend using pm_notifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Critical] PM notifiers modify hardware and V4L2 state without holding the context mutex, allowing concurrent userspace ioctls to corrupt state or restart streams during system suspend.
- [Critical] Replacing `.suspend` / `.resume` callbacks entirely with PM notifiers breaks hybrid sleep (suspend-to-both).
- [High] The DMA drain operation in the suspend path is instantly terminated without waiting for completion, rendering it ineffective.
- [High] The PM notifier is unregistered too late during device unbinding, creating a race window for a Use-After-Free.
- [High] Lockless runtime PM status checks allow concurrent clock gating during MMIO access, risking a synchronous external abort.
- [Medium] DMA drains are unconditionally initiated even for already-stopped contexts, submitting descriptors that will never complete.

Pre-existing issues:
- [High] Calling `pm_runtime_set_suspended()` while runtime PM is active leaks DMA channels and violates PM API contracts.
--

commit cb8a02a3113637917c3f826385776d24c0186002
Author: Jai Luthra <jai.luthra@ideasonboard.com>

media: ti: j721e-csi2rx: Support system suspend using pm_notifier

As this device is the "orchestrator" for the rest of the media
pipeline, we need to stop all on-going streams before system suspend and
enable them back when the system wakes up from sleep.

> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index d68b8d6ffeb19..21388284cbaab 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
[ ... ]
> @@ -1544,6 +1545,124 @@ static int ti_csi2rx_runtime_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int ti_csi2rx_suspend(struct device *dev)
> +{
> +	struct ti_csi2rx_dev *csi = dev_get_drvdata(dev);
> +	enum ti_csi2rx_dma_state state;
> +	struct ti_csi2rx_ctx *ctx;
> +	struct ti_csi2rx_dma *dma;
> +	unsigned long flags = 0;
> +	int i, ret = 0;
> +
> +	/* If device was not in use we can simply suspend */
> +	if (pm_runtime_status_suspended(dev))
> +		return 0;

[Severity: Critical]
Since PM notifiers like PM_SUSPEND_PREPARE run before userspace tasks are
frozen, is there anything preventing userspace from calling VIDIOC_STREAMON
concurrently?

Because this function does not hold ctx->mutex or set a state flag, could
userspace restart the streams and DMA engine immediately after this suspend
notifier finishes, leaving active DMA transfers to memory when the system
actually enters sleep?

[Severity: High]
Can a concurrent userspace operation trigger a pm_runtime_put() and drop the
usage counter to zero while this PM notifier is executing?

Because the status is checked locklessly here without acquiring a runtime PM
reference, the device could runtime suspend and gate its clocks while this
notifier is actively writing to memory-mapped registers, which might cause a
synchronous external abort.

> +
> +	/*
> +	 * If device is running, assert the pixel reset to cleanly stop any
> +	 * on-going streams before we suspend.
> +	 */
> +	writel(0, csi->shim + SHIM_CNTL);
> +
> +	for (i = 0; i < csi->num_ctx; i++) {
> +		ctx = &csi->ctx[i];
> +		dma = &ctx->dma;
> +
> +		spin_lock_irqsave(&dma->lock, flags);
> +		state = dma->state;
> +		spin_unlock_irqrestore(&dma->lock, flags);
> +
> +		if (state != TI_CSI2RX_DMA_STOPPED) {
> +			/* Disable source */
> +			ret = v4l2_subdev_disable_streams(&csi->subdev,
> +							  TI_CSI2RX_PAD_FIRST_SOURCE + ctx->idx,
> +							  BIT(0));
> +			if (ret)
> +				dev_err(csi->dev, "Failed to stop subdev stream\n");
> +		}
> +
> +		/* Stop any on-going streams */
> +		writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
> +
> +		/* Drain DMA */
> +		ti_csi2rx_drain_dma(ctx);

[Severity: Medium]
Will this unconditionally submit a DMA drain even for contexts that are already
TI_CSI2RX_DMA_STOPPED?

If the DMA stream was already stopped, could this submit a read descriptor that
waits for hardware data that will never arrive?

> +
> +		/* Terminate DMA */
> +		ret = dmaengine_terminate_sync(ctx->dma.chan);

[Severity: High]
Does this instantly terminate the DMA drain operation submitted just above?

Since there is no wait for the drain to complete, the DMA channel is aborted
immediately, which defeats the purpose of draining the FIFO and leaves stale
data in the hardware.

> +		if (ret)
> +			dev_err(csi->dev, "Failed to stop DMA\n");
> +	}
> +
> +	return ret;
> +}
[ ... ]
> +static int ti_csi2rx_pm_notifier(struct notifier_block *nb,
> +				 unsigned long action, void *data)
> +{
> +	struct ti_csi2rx_dev *csi =
> +		container_of(nb, struct ti_csi2rx_dev, pm_notifier);
> +
> +	switch (action) {
> +	case PM_HIBERNATION_PREPARE:
> +	case PM_SUSPEND_PREPARE:
> +	case PM_RESTORE_PREPARE:
> +		ti_csi2rx_suspend(csi->dev);
> +		break;
> +	case PM_POST_SUSPEND:
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_RESTORE:
> +		ti_csi2rx_resume(csi->dev);

[Severity: Critical]
Does replacing the dev_pm_ops suspend and resume callbacks with PM notifiers
break hybrid sleep (suspend-to-both)?

During hybrid sleep, the system creates a hibernation image, triggers the
PM_POST_HIBERNATION notifier which restarts the streams here, and then directly
transitions to S3 suspend without firing the PM_SUSPEND_PREPARE notifier again.

Because there is no suspend callback to notify the driver of the S3
transition, could the device be actively performing DMA to RAM while the system
goes to sleep?

> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
[ ... ]
> @@ -1644,6 +1777,8 @@ static void ti_csi2rx_remove(struct platform_device *pdev)
>  		ti_csi2rx_cleanup_ctx(&csi->ctx[i]);

[Severity: High]
This is a pre-existing issue, but does calling pm_runtime_set_suspended()
earlier in this function cause ti_csi2rx_cleanup_ctx() to leak DMA channels?

By forcefully setting the status to suspended, ti_csi2rx_cleanup_ctx()
evaluates !pm_runtime_status_suspended(ctx->csi->dev) as false, causing the
driver to skip the dma_release_channel() calls.

>  
>  	ti_csi2rx_cleanup_notifier(csi);
> +	unregister_pm_notifier(&csi->pm_notifier);

[Severity: High]
Is there a race window here that could lead to a use-after-free?

Since unregister_pm_notifier() is called after ti_csi2rx_cleanup_ctx(), which
destroys V4L2 structures and releases DMA channels, if a system suspend is
triggered concurrently during this window, the PM notifier could execute
ti_csi2rx_suspend(). The suspend callback would then dereference ctx->dma.chan
and call dmaengine_terminate_sync() on a freed channel.

> +
>  	ti_csi2rx_cleanup_v4l2(csi);
>  	dma_free_coherent(csi->dev, csi->drain.len, csi->drain.vaddr,
>  			  csi->drain.paddr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520120022.539913-1-r-donadkar@ti.com?part=17