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

[Janusz Krzysztofik <jmkrzyszt@gmail.com>]

Hi Sakari,

On Monday, July 1, 2019 9:50:55 A.M. CEST Sakari Ailus wrote:
> 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. 

That's why I proposed to cover that area by still checking for a non-NULL mdev 
member alternatively.

Are you sure HAS_DEVNODE flag is applicable only for media entities? AFAICS 
subdev drivers may expose some IOCTLs regardless of CONFIG_MEDIA_CONTROLLER 
and CONFIG_V4L2_SUBDEV_API settings.

Thanks,
Janusz


> Perhaps
> something to improve in the future, but for a later kernel release.
> 
> So,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>