Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/msm/dcp: unsynchronized resp_vq kick in dcp_ipcc_handle_rx() - [Medium] drm/msm/dcp: dcp_ipcc_send_power_notify() misuses mbox_client_txdone() - [Low] drm/msm/dcp: missing MAILBOX dependency in Kconfig causes link failure --
commit 847d1e1e504619e829df644d8cb1af09e3cef49c Author: Jayant Shekhar <[email protected]> drm/msm/dcp: Add IPCC power notifications This commit adds an inter-processor communication controller mailbox client to wake and acknowledge display coprocessor firmware during power transitions. It implements serialized notifications, bounded acknowledgment waits, wake interrupt handling, and callback lifetime management. > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile > index 56c48fe2e2c5f..9c698c4f49495 100644 > --- a/drivers/gpu/drm/msm/Makefile > +++ b/drivers/gpu/drm/msm/Makefile > @@ -73,6 +73,7 @@ msm-display-$(CONFIG_DRM_MSM_MDP5) += \ > > msm-display-$(CONFIG_DRM_MSM_DCP) += \ > dcp/dcp_hfi_core.o \ > + dcp/dcp_ipcc.o \ [Severity: Low] Will this cause a link failure when the mailbox framework is disabled? In drivers/gpu/drm/msm/Makefile, dcp/dcp_ipcc.o is built unconditionally whenever the display coprocessor option is enabled, and dcp_ipcc_init(), dcp_ipcc_free_channel(), and dcp_ipcc_send_power_notify() call mbox_request_channel(), mbox_free_channel(), mbox_send_message(), and mbox_client_txdone(). However, include/linux/mailbox_client.h does not provide inline stubs when mailbox support is disabled, and drivers/gpu/drm/msm/Kconfig only lists: config DRM_MSM_DCP bool "Enable DCP firmware-delegated display support in MSM DRM driver" depends on DRM_MSM depends on VIRTIO Should the Kconfig entry also depend on or select the mailbox subsystem? [ ... ] > diff --git a/drivers/gpu/drm/msm/dcp/dcp_ipcc.c > b/drivers/gpu/drm/msm/dcp/dcp_ipcc.c > new file mode 100644 > index 0000000000000..e161939dddc1e > --- /dev/null > +++ b/drivers/gpu/drm/msm/dcp/dcp_ipcc.c > @@ -0,0 +1,173 @@ [ ... ] > +static void dcp_ipcc_handle_rx(struct dcp_ipcc *ipcc) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&ipcc->state_lock, flags); > + if (ipcc->shutting_down) { > + spin_unlock_irqrestore(&ipcc->state_lock, flags); > + return; > + } > + > + complete(&ipcc->power_notify_done); > + if (ipcc->resp_vq && virtqueue_kick_prepare(ipcc->resp_vq)) > + virtqueue_notify(ipcc->resp_vq); > + > + spin_unlock_irqrestore(&ipcc->state_lock, flags); > +} [Severity: High] Can calling virtqueue_kick_prepare(ipcc->resp_vq) here under only ipcc->state_lock race with concurrent response virtqueue operations? In drivers/gpu/drm/msm/dcp/dcp_hfi_core.c, operations on the same response virtqueue in dcp_hfi_resp_callback(), dcp_hfi_resp_repost_work(), and dcp_hfi_quiesce_queues() are serialized under hfi->resp_drain_lock: drivers/gpu/drm/msm/dcp/dcp_hfi_core.c:dcp_hfi_resp_callback() { ... repost: spin_lock_irqsave(&hfi->resp_drain_lock, drain_flags); dcp_hfi_buf_init_sg(buf, DCP_HFI_BUF_SIZE); if (virtqueue_add_inbuf_premapped(vq, &buf->sg, 1, buf, NULL, GFP_ATOMIC)) { list_add_tail(&buf->node, &hfi->resp_repost_list); schedule_delayed_work(&hfi->resp_repost_work, msecs_to_jiffies(20)); } } if (virtqueue_kick_prepare(vq)) virtqueue_notify(vq); } Because virtqueue_kick_prepare() requires external serialization with other operations on the same virtqueue and updates vq->num_added and vq->in_use, running dcp_ipcc_handle_rx() from dcp_ipcc_rx_callback() or dcp_ipcc_power_notify_irq() without holding hfi->resp_drain_lock can corrupt vq->num_added kick tracking or trigger a virtio ring in_use check failure. In addition, complete(&ipcc->power_notify_done) runs before virtqueue_notify(ipcc->resp_vq), waking waiters in dcp_ipcc_send_power_notify() before the response virtqueue notification has been sent. [ ... ] > +int dcp_ipcc_send_power_notify(struct dcp_ipcc *ipcc) > +{ > + unsigned long timeout; > + int ret; > + > + mutex_lock(&ipcc->power_notify_lock); > + > + reinit_completion(&ipcc->power_notify_done); > + > + ret = mbox_send_message(ipcc->power_chan, NULL); > + mbox_client_txdone(ipcc->power_chan, ret); > + if (ret < 0) { > + dev_err(ipcc->client.dev, > + "IPCC TX: power notify failed: %d\n", ret); > + goto out_unlock; > + } [Severity: Medium] Should mbox_client_txdone() only be called with 0 after verifying that mbox_send_message() succeeded? When mbox_send_message() fails (ret < 0), no message was queued for this request, yet calling mbox_client_txdone(ipcc->power_chan, ret) before the error check still invokes tx_tick(chan, ret) on the mailbox channel (and would dereference a null pointer if ipcc->power_chan were null). Furthermore, when mbox_send_message() succeeds in non-blocking mode (ipcc->client.tx_block = false), it returns the non-negative ring buffer slot index from add_to_rbuf(), whereas mbox_client_txdone() expects 0 on success or a negative errno on failure. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
