Hi Janusz,
On Wed, May 15, 2019 at 10:56:36PM +0200, Janusz Krzysztofik wrote:
> Hi Sakari,
>
> On Wednesday, May 15, 2019 9:16:02 AM CEST Sakari Ailus wrote:
> > Hi Janusz,
> >
> > On Wed, May 15, 2019 at 12:48:21AM +0200, Janusz Krzysztofik wrote:
> > > -static int check_crop(struct v4l2_subdev *sd, struct v4l2_subdev_crop
> *crop)
> > > +static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> > > {
> > > - if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
> > > - crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > > + if (sd->entity.num_pads && pad >= sd->entity.num_pads)
> >
> > One more comment.
> >
> > The num_pads doesn't really tell whether a given op is valid for a device.
> > Well, in this case it would have to be a bug in the driver, but those do
> > happen. How about checking for sd->entity.graph_obj.mdev instead? It's
> > non-NULL if the entity is registered with a media device, i.e. when these
> > callback functions are supposed to be called.
>
> Before I do that, let me undestand your point better.
>
> My intentions were:
> 1) to provide a check for validity of a pad ID passed to an operation, not ann
> eligibility of a driver to support the operation,
> 2) to not break drivers which don't set pad_num, especially when building them
> with CONFIG_MEDIA_CONTROLLER turned on for whatever reason.
Indeed.
But these checks still allow calling the pad operations on sub-devices that
have no pads. That should not be allowed. Pads are a Media controller
concept, they do not exist outside it; therefore checking for pads only if
the subdev is a part of the media device would be entirely correct.
It should probably accompany a check that requires the pad number is zero
if the subdev doesn't have a graph object, even if the pad field isn't
supposedly used for any purpose. Would that address your concern?
>
> Since pad IDs are verified against pad_num which may be not set, we should
> obviously check validity of pad_num before comparing against it. Since media
> controller compatible subdevices need at least one pad, I think the check for
> non-zero pad_num is quite reasonable.
>
> Moreover, old drivers are actually using those pad operations you describe as
> not supposed to be called. They are using them because they were converted to
> use them in place of former video ops. Already dealing with pad IDs, they may
> decide to turn on CONFIG_MEDIA_CONTROLLER and use selected functionality, for
> example register pads, without implementing fulll media controller support.
> Why should we refuse to perform pad ID verification for them?
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
