Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 03/12/14 12:17, Hans Verkuil wrote:
> Hi Sylwester,
> 
> On 12/03/14 12:14, Sylwester Nawrocki wrote:
>> > Hi Hans,
>> > 
>> > On 02/12/14 13:21, Hans Verkuil wrote:
>>> >> -static int s5k6aa_set_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>>> >> -			   struct v4l2_subdev_crop *crop)
>>> >> +static int s5k6aa_set_selection(struct v4l2_subdev *sd,
>>> >> +				struct v4l2_subdev_fh *fh,
>>> >> +				struct v4l2_subdev_selection *sel)
>>> >>  {
>>> >>  	struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>>> >>  	struct v4l2_mbus_framefmt *mf;
>>> >>  	unsigned int max_x, max_y;
>>> >>  	struct v4l2_rect *crop_r;
>>> >>  
>>> >> +	if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
>>> >> +		return -EINVAL;
>>> >> +
>> > 
>> > Isn't checking sel->pad redundant here ? There is already the pad index
>> > validation in check_selection() in v4l2-subdev.c and this driver has only
>> > one pad.
>
> If it is called from a bridge driver, then it hasn't gone through
> check_selection().
> 
> That said, if it is called from a bridge driver, then one might expect
> correct usage of pad.

Indeed, there is still a possibility to have wrong pad index passed
to those functions.  I won't object to this patch being merged as is,
even though functional changes could be minimized by not adding a
check which wasn't originally there. :)

Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

-- 
Regards,
Sylwester