Re: [PATCH v2 19/23] drm/msm/dcp: Add Qualcomm VirtIO transport wrapper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/msm/dcp: use-after-free of vdev due to devm_kzalloc() and empty release
- [High] drm/msm/dcp: broken virtqueue PM sequencing in suspend and resume
- [Medium] drm/msm/dcp: unbalanced IRQ wake in dcp_virtio_transport_resume() on error
- [Low] drm/msm/dcp: build failure in dcp_virtio_transport_suspend() when !PM_SLEEP
--

commit 8e04d90ffae5a36ffd1db1c49707f1bc447c570d
Author: Jayant Shekhar <jayant.shekhar@oss.qualcomm.com>

drm/msm/dcp: Add Qualcomm VirtIO transport wrapper

This commit adds the Qualcomm-specific VirtIO MMIO transport wrapper for DCP
firmware, handling IPCC power notifications, virtqueue setup, interrupt
handling, and system suspend/resume sequencing.

> diff --git a/drivers/gpu/drm/msm/dcp/dcp_virtio_transport.c b/drivers/gpu/drm/msm/dcp/dcp_virtio_transport.c
> new file mode 100644
> index 0000000000000..02c0e2a9fafe9
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dcp/dcp_virtio_transport.c
[ ... ]
> +struct dcp_virtio_transport {
> +	struct virtio_device vdev;
> +	struct platform_device *pdev;
> +
> +	void __iomem *base;
> +	unsigned long version;
> +
> +	bool wake_irq_enabled;
> +	struct dcp_ipcc ipcc;
> +};
[ ... ]
> +static void dcp_virtio_release_dev(struct device *dev)
> +{
> +}
> +
> +struct dcp_ipcc *dcp_virtio_get_ipcc(struct virtio_device *vdev)
> +{
> +	return &to_dcp_virtio_transport(vdev)->ipcc;
> +}
> +
> +static int dcp_virtio_transport_probe(struct platform_device *pdev)
> +{
> +	struct dcp_virtio_transport *transport;
> +	unsigned long magic;
> +	int ret;
> +
> +	transport = devm_kzalloc(&pdev->dev, sizeof(*transport), GFP_KERNEL);
> +	if (!transport)
> +		return -ENOMEM;
> +
> +	transport->pdev = pdev;
> +	transport->vdev.dev.parent = &pdev->dev;
> +	transport->vdev.dev.release = dcp_virtio_release_dev;
> +	transport->vdev.config = &dcp_virtio_config_ops;
> +	platform_set_drvdata(pdev, transport);
[ ... ]
> +	ret = register_virtio_device(&transport->vdev);
> +	if (ret) {
> +		put_device(&transport->vdev.dev);
> +		dcp_ipcc_deinit(&transport->ipcc);
> +		platform_set_drvdata(pdev, NULL);
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +err_node:
> +err_ipcc:
> +	dcp_ipcc_deinit(&transport->ipcc);
> +err_free:
> +	platform_set_drvdata(pdev, NULL);
> +	return ret;
> +}
> +
> +static void dcp_virtio_transport_remove(struct platform_device *pdev)
> +{
> +	struct dcp_virtio_transport *transport = platform_get_drvdata(pdev);
> +
> +	unregister_virtio_device(&transport->vdev);
> +	platform_set_drvdata(pdev, NULL);
> +	dcp_ipcc_deinit(&transport->ipcc);
> +}

[Severity: High]
struct dcp_virtio_transport embeds struct virtio_device vdev, which in turn
embeds a reference-counted struct device dev. In dcp_virtio_transport_probe(),
transport is allocated with devm_kzalloc(&pdev->dev, ...) and
transport->vdev.dev.release is set to the empty callback
dcp_virtio_release_dev().

Once register_virtio_device(&transport->vdev) calls
device_initialize(&dev->dev), the lifetime of transport->vdev.dev is governed
by its reference count rather than the parent platform device's devres
lifetime. When dcp_virtio_transport_remove() returns (or when
register_virtio_device() fails), devres_release_all(&pdev->dev) immediately
frees transport regardless of whether references to transport->vdev.dev are
still held.

Additionally, if transport is instead allocated with kzalloc() and freed in
dcp_virtio_release_dev() (as in drivers/virtio/virtio_mmio.c), calling
dcp_ipcc_deinit(&transport->ipcc) after put_device(&transport->vdev.dev) in
dcp_virtio_transport_probe() or after unregister_virtio_device(&transport->vdev)
in dcp_virtio_transport_remove() would access transport->ipcc after the final
reference drops and frees transport.

Can this cause a use-after-free of transport->vdev.dev and transport->ipcc?

> +
> +static int dcp_virtio_transport_suspend(struct device *dev)
> +{
> +	struct dcp_virtio_transport *transport = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = virtio_device_freeze(&transport->vdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = dcp_ipcc_enable_irq_wake(&transport->ipcc);
> +	if (ret)
> +		goto err_restore;
> +
> +	ret = dcp_ipcc_send_power_notify(&transport->ipcc);
> +	if (ret) {
> +		dcp_ipcc_disable_irq_wake(&transport->ipcc);
> +		goto err_restore;
> +	}
> +
> +	return 0;
> +
> +err_restore:
> +	virtio_device_restore(&transport->vdev);
> +	return ret;
> +}
> +
> +static int dcp_virtio_transport_resume(struct device *dev)
> +{
> +	struct dcp_virtio_transport *transport = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = dcp_ipcc_send_power_notify(&transport->ipcc);
> +	if (ret)
> +		return ret;

[Severity: Medium]
During system suspend, dcp_virtio_transport_suspend() enables IRQ wake on
transport->ipcc.power_notify_irq via dcp_ipcc_enable_irq_wake(&transport->ipcc).

During system resume, dcp_virtio_transport_resume() calls
dcp_ipcc_send_power_notify(&transport->ipcc) before calling
dcp_ipcc_disable_irq_wake(&transport->ipcc).

If dcp_ipcc_send_power_notify() fails or times out, does returning early here
leave wake_depth unbalanced on transport->ipcc.power_notify_irq?

> +
> +	dcp_ipcc_disable_irq_wake(&transport->ipcc);
> +	if (transport->version == 1)
> +		writel(PAGE_SIZE,
> +		       transport->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
> +
> +	return virtio_device_restore(&transport->vdev);
> +}

[Severity: High]
In dcp_virtio_transport_suspend(), virtio_device_freeze(&transport->vdev) runs
before dcp_ipcc_send_power_notify(&transport->ipcc), while
transport->ipcc.resp_vq remains non-NULL because neither this function nor
dcp_virtio_driver clears it during freeze. When the firmware acknowledges the
power notification, dcp_ipcc_handle_rx() executes:

drivers/gpu/drm/msm/dcp/dcp_ipcc.c:dcp_ipcc_handle_rx() {
	...
	complete(&ipcc->power_notify_done);
	if (ipcc->resp_vq && virtqueue_kick_prepare(ipcc->resp_vq))
		virtqueue_notify(ipcc->resp_vq);
	...
}

Because VIRTIO_RING_F_EVENT_IDX is not negotiated, virtqueue_kick_prepare()
returns true whenever VRING_USED_F_NO_NOTIFY is clear, writing to
VIRTIO_MMIO_QUEUE_NOTIFY after the VirtIO device has already been frozen.

Similarly, in dcp_virtio_transport_resume(), dcp_ipcc_send_power_notify() is
called before virtio_device_restore(&transport->vdev), so the RX
acknowledgment kicks ipcc->resp_vq before restore. Immediately after,
virtio_device_restore() calls virtio_reset_device() -> dcp_virtio_reset(),
which writes 0 to VIRTIO_MMIO_STATUS and resets the hardware virtqueue state,
and nothing in the restore path or dcp_virtio_driver reprograms the virtqueue
registers before setting VIRTIO_CONFIG_S_DRIVER_OK.

Will this leave the virtqueues unconfigured in hardware and break the display
transport across suspend and resume?

> +
> +static const struct dev_pm_ops dcp_virtio_transport_pm_ops = {
> +	.suspend = dcp_virtio_transport_suspend,
> +	.resume = dcp_virtio_transport_resume,
> +};
> +
> +static const struct of_device_id dcp_virtio_transport_of_match[] = {
> +	{ .compatible = "qcom,dcp-virtio" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, dcp_virtio_transport_of_match);
> +
> +static struct platform_driver dcp_virtio_transport_driver = {
> +	.probe = dcp_virtio_transport_probe,
> +	.remove = dcp_virtio_transport_remove,
> +	.driver = {
> +		.name = "qcom-dcp-virtio",
> +		.of_match_table = dcp_virtio_transport_of_match,
> +		.pm = &dcp_virtio_transport_pm_ops,
> +	},
> +};

[Severity: Low]
In include/linux/virtio.h, virtio_device_freeze() and virtio_device_restore()
are only declared under #ifdef CONFIG_PM_SLEEP:

#ifdef CONFIG_PM_SLEEP
int virtio_device_freeze(struct virtio_device *dev);
int virtio_device_restore(struct virtio_device *dev);
#endif

Since CONFIG_DRM_MSM_DCP depends on CONFIG_DRM_MSM (which only depends on
CONFIG_PM) and CONFIG_VIRTIO, this file can be built with CONFIG_PM=y and
CONFIG_PM_SLEEP=n.

Won't compiling dcp_virtio_transport_suspend() and
dcp_virtio_transport_resume() unconditionally without #ifdef CONFIG_PM_SLEEP
(and without pm_sleep_ptr() / pm_ptr() on dcp_virtio_transport_pm_ops) cause a
build failure when CONFIG_PM_SLEEP is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=19