Hi Hans,
On Saturday, June 29, 2019 12:00:24 P.M. CEST Hans Verkuil wrote:
> sd->entity.graph_obj.mdev can be NULL when this function is called, and
> that breaks existing drivers (rcar-vin, but probably others as well).
>
> Check if sd->entity.num_pads is non-zero instead since that doesn't depend
> on mdev.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in
> v4l2_subdev_call()") ---
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> b/drivers/media/v4l2-core/v4l2-subdev.c index 21fb90d66bfc..bc32fc1e0c0b
> 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
> static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> {
> #if defined(CONFIG_MEDIA_CONTROLLER)
> - if (sd->entity.graph_obj.mdev) {
> - if (pad >= sd->entity.num_pads)
> - return -EINVAL;
> - return 0;
> - }
> + if (sd->entity.num_pads)
This reverts a change introduced on Sakari's request in v7 of the series which
is the source of the regression. The intention was to fail if num_pads == 0
on a valid media entity. Maybe we should still keep that restriction and fail
in case mdev is not NULL? In other words:
- if (sd->entity.num_pads)
+ if (sd->entity.num_pads || sd->entity.graph_obj.mdev)
Thanks,
Janusz
> + return pad >= sd->entity.num_pads ? -EINVAL : 0;
This and below look like coding style changes, not related strictly to the
merit. Shouldn't they rather be split into a separate patch?
BTW, the current coding style follows original implementation of check_*
functions present before that series was introduced. Maybe it would be better
to keep them unified, i.e., either as is or all updated with the new style.
Thanks,
Janusz
> #endif
> /* allow pad 0 on subdevices not registered as media entities */
> - if (pad > 0)
> - return -EINVAL;
> - return 0;
> + return pad ? -EINVAL : 0;
> }
>
> static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
