Hi Laurent,
Thanks for the patch.
On Mon, Nov 13, 2023 at 12:17:18PM +0200, Laurent Pinchart wrote:
> Before v6.3, the v4l2_subdev_link_validate() helper would ignore links
> whose source was a video device and sink a subdev. The helper was (and
> still is) used by drivers in such cases, in particular for subdevs with
> multiple sink pads, some connected to video devices and some to other
> subdevs.
>
> Then, commit a6b995ed03ff ("media: subdev: use streams in
> v4l2_subdev_link_validate()") assumed the entities on the two sides of a
> link are both subdevs, and caused crashes in drivers that use the helper
> with subdev sink pads connected to video devices. Commit 55f1ecb11990
> ("media: v4l: subdev: Make link validation safer"), merged in v6.4,
> fixed the crash by adding an explicit check with a pr_warn_once(),
> mentioning a driver bug.
>
> Links between a subdev and a video device need to be validated, and
> v4l2_subdev_link_validate() can't handle that. Drivers typically handle
> this validation manually at stream start time (either in the .streamon()
> ioctl handler, or in the .start_streaming() vb2 queue operation),
> instead of implementing a custom .link_validate() handler. Forbidding
While some do the validation as part of the streamon callback, it'd be
nicer to move this to the link_validate callback instead: this is what the
callback is for. I'd presume not may drivers depend on
v4l2_subdev_link_validate() fail silently on non-subdevices as the issue
hasn't been reported before while the patch that seems to have broken this
was merged in 6.3.
Not failing silently in link_validate also ensures the validation gets
done: there have been drivers (more than one) that have simply missed the
link validation due to the issue (non-sub-device entity on one end) being
silently ignored by default.
> usage of v4l2_subdev_link_validate() as the .link_validate() handler
> would thus force all subdev drivers that mix source links to subdev and
> video devices to implement a custom .link_validate() handler that
> returns immediately for the video device links and call
> v4l2_subdev_link_validate() for subdev links. This would create lots of
> duplicated code for no real gain. Instead, relax the check in
> v4l2_ctrl_modify_range() to ignore links from a video device to a subdev
> without printing any warning.
>
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 30 ++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 67d43206ce32..b00be1d57e05 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1356,15 +1356,31 @@ int v4l2_subdev_link_validate(struct media_link *link)
> struct v4l2_subdev *source_sd, *sink_sd;
> struct v4l2_subdev_state *source_state, *sink_state;
> bool states_locked;
> + bool is_sink_subdev;
> + bool is_source_subdev;
> int ret;
>
> - if (!is_media_entity_v4l2_subdev(link->sink->entity) ||
> - !is_media_entity_v4l2_subdev(link->source->entity)) {
> - pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> - !is_media_entity_v4l2_subdev(link->sink->entity) ?
> - "sink" : "source",
> - link->source->entity->name, link->source->index,
> - link->sink->entity->name, link->sink->index);
> + is_sink_subdev = is_media_entity_v4l2_subdev(link->sink->entity);
> + is_source_subdev = is_media_entity_v4l2_subdev(link->source->entity);
> +
> + if (!is_sink_subdev || !is_source_subdev) {
> + /*
> + * Do not print the warning if the source is a video device and
> + * the sink a subdev. This is a valid use case, to allow usage
> + * of this helper by subdev drivers that have multiple sink
> + * pads, some connected to video devices and some connected to
> + * other subdevs. The video device to subdev link is typically
> + * validated manually by the driver at stream start time in such
> + * cases.
> + */
> + if (!is_sink_subdev ||
> + !is_media_entity_v4l2_video_device(link->source->entity))
> + pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> + !is_sink_subdev ? "sink" : "source",
> + link->source->entity->name,
> + link->source->index,
> + link->sink->entity->name,
> + link->sink->index);
> return 0;
> }
>
--
Kind regards,
Sakari Ailus
