Re: [PATCH v2 08/23] drm/msm/dcp: Add IPCC power notifications

[sashiko-bot@kernel.org]

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 <jayant.shekhar@oss.qualcomm.com>

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/20261026-jshekhar-dcp-hlos-v2-0-7cf014efdee1@oss.qualcomm.com?part=8