Re: [PATCH 4/9] media: synopsys: hdmirx: add async subdevice support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: missing graph_mutex lock in hdmirx_fwnode_bound() leading to list corruption
--

commit ac73c2eba89ee9517b8e00beeec227f11b4a4eae
Author: Gerald Loacker <gerald.loacker@wolfvision.net>

media: synopsys: hdmirx: add async subdevice support
    
Add V4L2 async notifier support to enable dynamic binding of connected
subdevices in the video pipeline. The driver parses device-tree endpoints
to discover and register video bridge subdevices.

> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> index 11a9b16586a57..0c7cac25bf694 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c

[ ... ]

> @@ -2649,6 +2653,48 @@ static int hdmirx_register_cec(struct snps_hdmirx_dev *hdmirx_dev,
>  	return 0;
>  }
>  
> +static int hdmirx_fwnode_bound(struct v4l2_async_notifier *notifier,
> +			       struct v4l2_subdev *subdev,
> +			       struct v4l2_async_connection *asc)
> +{
> +	struct snps_hdmirx_dev *hdmirx_dev =
> +		container_of(notifier, struct snps_hdmirx_dev, notifier);
> +	int source_pad;
> +	int ret;
> +
> +	source_pad = media_entity_get_fwnode_pad(&subdev->entity,
> +						 asc->match.fwnode,
> +						 MEDIA_PAD_FL_SOURCE);
> +	if (source_pad < 0) {
> +		dev_err(hdmirx_dev->dev, "%s has no source pad for %pfw: %d\n",
> +			subdev->name, asc->match.fwnode, source_pad);
> +		return source_pad;
> +	}
> +
> +	ret = media_create_pad_link(&subdev->entity, source_pad,
> +				    &hdmirx_dev->stream.vdev.entity, 0,
> +				    MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);

[Severity: High]
Is it safe to call media_create_pad_link() here without holding the
mdev->graph_mutex?

This function modifies shared media graph lists (such as mdev->links).

Since the media device has already been registered in hdmirx_probe(),
concurrent access from userspace is possible. A userspace process calling the
MEDIA_IOC_G_TOPOLOGY ioctl could be traversing the topology lists while this
call modifies them, potentially leading to list corruption.

Should this call be wrapped in mutex_lock(&hdmirx_dev->mdev.graph_mutex)?

> +	if (ret) {
> +		dev_err(hdmirx_dev->dev, "Failed to create media link: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> @@ -2772,9 +2820,38 @@ static int hdmirx_probe(struct platform_device *pdev)
>  		goto err_unreg_video_dev;
>  	}
>  
> +	ep_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);

[Severity: High]
Does registering the async notifier after calling media_device_register()
expose a race condition?

The media_device_register() call (which happens just before this block in
hdmirx_probe()) exposes the topology traversal interface (MEDIA_IOC_G_TOPOLOGY)
to userspace prematurely.

This opens a race window where asynchronous bindings handled by the notifier
can conflict with userspace topology traversal.

Would it be better to call media_device_register() after registering the async
notifier?

> +	if (ep_node) {
> +		v4l2_async_nf_init(&hdmirx_dev->notifier, &hdmirx_dev->v4l2_dev);
> +		hdmirx_dev->notifier.ops = &hdmirx_async_ops;
> +
> +		asc = v4l2_async_nf_add_fwnode_remote(&hdmirx_dev->notifier,
> +						      of_fwnode_handle(ep_node),
> +						      struct v4l2_async_connection);
> +		of_node_put(ep_node);
> +
> +		if (IS_ERR(asc)) {
> +			ret = PTR_ERR(asc);
> +			dev_err_probe(dev, ret, "Failed to add remote fwnode\n");
> +			goto err_unreg_media;
> +		}
> +
> +		ret = v4l2_async_nf_register(&hdmirx_dev->notifier);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Failed to register async notifier\n");
> +			goto err_cleanup_notifier;
> +		}
> +	} else {
> +		ret = v4l2_device_register_subdev_nodes(&hdmirx_dev->v4l2_dev);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Failed to register subdev nodes\n");
> +			goto err_unreg_media;
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-hdmirx-media-v1-0-ea3d77d4a5f3@pengutronix.de?part=4