Hi Hans, Thank you for the patch. On Wed, Feb 15, 2023 at 03:48:17PM +0100, Hans Verkuil wrote: > Both userspace and kernelspace can pass structs with an uninitialized 'stream' > field. Since the check_state() function checks for a non-zero stream field, > suddenly these calls will fails with -EINVAL. > > So check in the wrapper functions in v4l2-subdev.c (which are used by both the > kernel and userspace API) if V4L2_SUBDEV_FL_STREAMS is set, and if not, then zero > the stream field. > > Currently no drivers set V4L2_SUBDEV_FL_STREAMS. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> Looks good to me as a v6.3 fix. I would however like to then revert it in v6.4 once a better fix gets developed: if an application that does support the streams API sets the stream field to an invalid value for a subdev that doesn't support streams, this shouldn't be silently ignored. Could you capture this in the commit message ? > --- > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 1bebcda2bd20..db22da153002 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -197,6 +198,8 @@ static inline int check_format(struct v4l2_subdev *sd, > if (!format) > return -EINVAL; > > + if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > + format->stream = 0; Could you add a blank line here ? Same below. With that, Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > return check_which(format->which) ? : check_pad(sd, format->pad) ? : > check_state(sd, state, format->which, format->pad, format->stream); > } > @@ -224,6 +227,8 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, > if (!code) > return -EINVAL; > > + if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > + code->stream = 0; > return check_which(code->which) ? : check_pad(sd, code->pad) ? : > check_state(sd, state, code->which, code->pad, code->stream) ? : > sd->ops->pad->enum_mbus_code(sd, state, code); > @@ -236,6 +241,8 @@ static int call_enum_frame_size(struct v4l2_subdev *sd, > if (!fse) > return -EINVAL; > > + if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > + fse->stream = 0; > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : > check_state(sd, state, fse->which, fse->pad, fse->stream) ? : > sd->ops->pad->enum_frame_size(sd, state, fse); > @@ -271,6 +278,8 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, > if (!fie) > return -EINVAL; > > + if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > + fie->stream = 0; > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : > check_state(sd, state, fie->which, fie->pad, fie->stream) ? : > sd->ops->pad->enum_frame_interval(sd, state, fie); > @@ -283,6 +292,8 @@ static inline int check_selection(struct v4l2_subdev *sd, > if (!sel) > return -EINVAL; > > + if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > + sel->stream = 0; > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : > check_state(sd, state, sel->which, sel->pad, sel->stream); > } -- Regards, Laurent Pinchart
