Hi Marco, Sakari,

On Thu, Apr 04, 2019 at 11:39:34AM +0300, Sakari Ailus wrote:
> Hi Marco,
>
> On Thu, Apr 04, 2019 at 09:39:57AM +0200, Marco Felsch wrote:
> > In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
> > available. So each driver have to add ifdefs around those helpers or
> > add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.
> >
> > Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
> > isn't set to avoid ifdefs. This approach is less error prone too.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > The patch was previously part of series [1]. Since I want to get
> > series [1] merged in 5.2 I split this possible stopper out of the
> > serie and prepared a own series for it. I applied Jacopos comments and
> > switched to Lubomir's approach [2]. During discussion on series [2]
> > Sakari pointed out Hans approach [3] which didn't got into the kernel
> > due to Mauro's concerns. So I think this would be the smalles common
> > dennominator.
> >
> > [1] https://patchwork.kernel.org/cover/10786553/
> > [2] https://patchwork.kernel.org/patch/10703029/
> > [3] https://patchwork.linuxtv.org/patch/53370/
> > ---
> >  include/media/v4l2-subdev.h | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index a7fa5b80915a..eea792757426 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -984,7 +984,34 @@ static inline struct v4l2_rect
> >  		pad = 0;
> >  	return &cfg[pad].try_compose;
> >  }
> > -#endif
> > +
> > +#else /* !defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
> > +
> > +static inline struct v4l2_mbus_framefmt
> > +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > +			    struct v4l2_subdev_pad_config *cfg,
> > +			    unsigned int pad)
> > +{
> > +	return ERR_PTR(-ENOTTY);
> > +}
>
> Almost all users of these functions directly use the struct pointer they
> receive from the said functions, without checks for the pointer value.
>
> These drivers now depend on CONFIG_VIDEO_V4L2_SUBDEV_API, but for new
> drivers it could be possible to miss that, leading to dereferencing
> ERR_PTR(-ENOTTY) as a result if the sub-device API isn't enabled.
>
> I have to say I'm not a huge fan of such a change; it's one more such thing
> to remember that results in a kernel crash if you get it wrong.
>

Unfortunately, I have to agree with Sakari here

> I wonder if v4l2_subdev_call() could become a bit smarter about this;
> checking whether the try format is attempted to be obtained without the
> sub-device API being enabled, and returning an error in that case without
> passing the IOCTL to the driver at all.

I'm not much excited by the idea of specializing v4l2_subdev_call(), I
had a try and I would be ashemed of sharing that code. If someone
could come up with something nice we could consider this too.

For the record, it is not enough to check for V4L2_SUBDEV_API and try
format, as a few (1?) bridge drivers calls the subdev operation providing a
'v4l2_subdev_pad_config' argument, that could be used by the sensor
drivers in place of the one returned from v4l2_subdev_get_try_*, if
V4L2_SUBDEV_API is not available

Caller:
https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/renesas-ceu.c#L858
Sensor:
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9v111.c#L792

As I wrote both, they could be very well be wrong, but if they're not,
one other option would be to refuse NULL v4l2_subdev_pad_config arguments
(changing most of the bridge drivers in mainline) and move the code
linked above here from the mt9v111 driver to the v4l2_subdev_get_try_*
functions.

>
> Just an idea. It doesn't have to be in the same patchset.
>
> I'd still prefer enabling subdev API always to decrease the number of
> possible different kernel configurations.

That would be best imho, but Hans' attempt didn't go very far because
of the issue Marco reported in his cover letter.

Thanks
  j

>
> I wonder what others think.
>
> > +
> > +static inline struct v4l2_rect
> > +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> > +			  struct v4l2_subdev_pad_config *cfg,
> > +			  unsigned int pad)
> > +{
> > +	return ERR_PTR(-ENOTTY);
> > +}
> > +
> > +static inline struct v4l2_rect
> > +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> > +			     struct v4l2_subdev_pad_config *cfg,
> > +			     unsigned int pad)
> > +{
> > +	return ERR_PTR(-ENOTTY);
> > +}
> > +
> > +#endif /* defined(CONFIG_VIDEO_V4L2_SUBDEV_API) */
> >
> >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> >
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com