Re: [PATCH v6 03/14] media: mediatek: vcodec: add driver to support vcp encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `.get_venc_capa` callback in `mtk_vcodec_vcp_msg` causes a guaranteed kernel panic on encoder device open.
- [High] Late IPI acknowledgments from the firmware can falsely satisfy subsequent IPI requests after a timeout, leading to data corruption or desynchronization.
- [Medium] Early return on `deregister_feature` failure leaks the `rproc` remote processor reference.
--

--- Patch [3]: [PATCH v6 03/14] media: mediatek: vcodec: add driver to support vcp encoder ---
commit 9d12e27cbb34067860fe1673f4f8bfcc5549ebf1
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: vcodec: add driver to support vcp encoder

This patch adds VCP encoder support to the mediatek vcodec driver. It enables
the encoder to call the VCP interface to communicate with the coprocessor.

> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> index 150d842c7d226..ece4e3e87e62d 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vcp.c
[ ... ]
> @@ -223,8 +225,13 @@ static int mtk_vcodec_vcp_msg_ipi_send(struct mtk_vcodec_fw *fw, int id, void *b
>  	}
>  
>  	mutex_lock(&vcp->ipi_mutex);
> -	feature_id = VDEC_FEATURE_ID;
> -	mailbox_id = IPI_OUT_VDEC_1;
> +	if (fw->fw_use == ENCODER) {
> +		feature_id = VENC_FEATURE_ID;
> +		mailbox_id = IPI_OUT_VENC_0;
> +	} else {
> +		feature_id = VDEC_FEATURE_ID;
> +		mailbox_id = IPI_OUT_VDEC_1;
> +	}
>  
>  	timeout_jiffies = jiffies + msecs_to_jiffies(VCP_SYNC_TIMEOUT_MS);
>  	while (!vcp_device->ops->vcp_is_ready(vcp_device, feature_id)) {

[Severity: High]
Can late IPI acknowledgments from the firmware falsely satisfy subsequent IPI
requests after a timeout?

If wait_event_interruptible_timeout() in mtk_vcodec_vcp_msg_ipi_send() times
out, it aborts and unlocks the IPI mutex. If a new request is submitted reusing
the same ID, and the firmware finally sends a delayed ACK for the original
timed-out request, the background msg_thread could blindly accept it:

mtk_vcodec_vcp_msg_process_thread() {
    ...
    fw->vcp->msg_signaled[obj->id] = true;
    wake_up(&fw->vcp->msg_wq[obj->id]);
    ...
}

Would this cause the new request to immediately return success while reading
stale responses belonging to the timed-out request, leading to protocol
desynchronization?

[ ... ]
> @@ -530,7 +562,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_vcp_init(void *priv, enum mtk_vcodec_fw_use
>  	struct mtk_vcodec_fw *fw;
>  	int i;
>  
> -	if (fw_use == DECODER) {
> +	if (fw_use == ENCODER) {
> +		struct mtk_vcodec_enc_dev *enc_dev = priv;
> +
> +		plat_dev = enc_dev->plat_dev;
> +	} else if (fw_use == DECODER) {
>  		struct mtk_vcodec_dec_dev *dec_dev = priv;
>  
>  		plat_dev = dec_dev->plat_dev;

[Severity: High]
Does this patch miss implementing the get_venc_capa callback in the
mtk_vcodec_vcp_msg operations struct?

When the V4L2 encoder device is opened, fops_vcodec_open() unconditionally
calls mtk_vcodec_fw_get_venc_capa(), which attempts to dereference:

mtk_vcodec_fw_get_venc_capa() {
    return fw->ops->get_venc_capa(fw);
}

If the mtk_vcodec_vcp_msg struct does not have this callback registered,
could this result in a NULL pointer dereference when a userspace application
opens the encoder video device node?

[Severity: Medium]
In mtk_vcodec_vcp_release(), if the coprocessor's deregister_feature operation
fails, the function emits an error and returns early:

mtk_vcodec_vcp_release() {
    ...
    ret = vcp_device->ops->deregister_feature(vcp_device, fw->vcp->feature_id);
    if (ret < 0) {
        dev_err(dev, "deregister feature_id(%d) fail(%d)\n", fw->vcp->feature_id, ret);
        return;
    }
    ...
}

Will this early return skip the put_device label that calls
mtk_vcodec_vcp_put_device(), leading to a leaked rproc reference since
rproc_put() would be bypassed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720012056.1026551-1-kyrie.wu@mediatek.com?part=3