On 13/11/2023 12:17, 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
Didn't it also ignore links where the sink is a video device?
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
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
v4l2_ctrl_modify_range? copy-paste error?
Should the check also be relaxed wrt. video device as a sink?
without printing any warning.
As Sakari said, it would make sense to use .link_validate() to validate
all links.
But if this warning is getting printed at the moment, then I think this
is a valid fix (maybe with the sink side handled too, if needed).
Tomi
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;
}
