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@xxxxxxxxxxxxxx> > --- > 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. 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. 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. 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@xxxxxxxxxxxxxxx
