Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans,

On Sat, Jun 29, 2019 at 03:08:23PM +0200, Hans Verkuil wrote:
> On 6/29/19 2:57 PM, Hans Verkuil wrote:
> > On 6/29/19 2:06 PM, Janusz Krzysztofik wrote:
> >> 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@xs4all.nl>
> >>> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> 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)
> > 
> > If an entity has no pads, then it doesn't have pad ops either and this function
> > would never be called.
> 
> Sakari, do you think it is ever possible that an entity may have pad ops,
> but num_pads goes at some point to 0?
> 
> If so, then we can check for sd->entity.function != 0. All MC subdevs must
> set that to a non-0 value, otherwise the core will issue a WARN_ON. I think
> it is the only reliable indicator that can be used.

I don't think checking the num_pads field is is a great way to test whether
an entity supports the pad ops; I don't have a better proposal either as it
seems that some drivers call these ops before registering the subdev.

There are sub-device drivers that expose their own device node and thus
work with MC-enabled master drivers but have no pad ops: this is what makes
the fundamental difference in API support, it's not the number of pads. We
just happen to see this in pad ops only (I guess).

Currently the drivers may set the HAS_DEVNODE flag (that really tells about
MC compatibility) just before registering the sub-device. This might be a
better test for allowing pad ops, but the name of the flag is somewhat
misleading. I wouldn't want to start testing this now however, it'd risk
exposing these issues to a much wider audience.

I guess in practice testing for num_pads still works; it however does leave
some gray area between MC-enabled sub-device drivers and the rest. Perhaps
something to improve in the future, but for a later kernel release.

So,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com