Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Jul 2023 16:11:50 +0200
From:   Jacopo Mondi <jacopo.mondi@ideasonboard.com>
To:     Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Subject: Re: [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field
Message-ID: <rzjtzsvk5q5oobh3khtjopn6ssqyf6akw2z3rswihfec3s3syw@xwjamnaqhplh>
References: <20230619112707.239565-1-tomi.valkeinen@ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230619112707.239565-1-tomi.valkeinen@ideasonboard.com>
Precedence: bulk

Hi Tomi

On Mon, Jun 19, 2023 at 02:27:05PM +0300, Tomi Valkeinen wrote:
> Now that the kernel drivers have been fixed to initialize the stream
> field, and we have the client capability which the userspace uses to say

Not sure I got this. Isn't the capabilities flag intended for drivers
to tell userspace it support streams ? This seems to suggest it is
userspace setting it ?

> it has initialized the stream field, we can drop the implicit zeroing of
> the stream field in the various check functions.
>

I guess this is safe, but I'm not sure why it wasn't before. If a
driver doesn't support streams (!(sd->flags & V4L2_SUBDEV_FL_STREAMS))
then it should have ignored the 'stream' field even if it wasn't
zeroed. So I suspect I am missing the reason for zeroing in first
place...

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 15 ---------------
>  1 file changed, 15 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 2ec179cd1264..c1ac6d7a63d2 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -200,9 +200,6 @@ static inline int check_format(struct v4l2_subdev *sd,
>  	if (!format)
>  		return -EINVAL;
>
> -	if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS))
> -		format->stream = 0;
> -
>  	return check_which(format->which) ? : check_pad(sd, format->pad) ? :
>  	       check_state(sd, state, format->which, format->pad, format->stream);
>  }
> @@ -230,9 +227,6 @@ 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);
> @@ -245,9 +239,6 @@ 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);
> @@ -283,9 +274,6 @@ 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);
> @@ -298,9 +286,6 @@ 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);
>  }
> --
> 2.34.1
>