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 <[email protected]> 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/[email protected]?part=19
