| Message ID | 20190404074002.18730-2-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid v4l2_subdev_get_try_ ifdef dance | expand |
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. 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; >
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
Hi Sakari, Jacopo, On 19-04-04 16:39, Jacopo Mondi wrote: > 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 > Are those drivers depending on CONFIG_VIDEO_V4L2_SUBDEV_API used by usb-bridge devices? I Remember that was Mauro's concern on Hans RFC patch. I'm with you the "depending way" is quite better. > > 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 Thats true. One thing to think about your caller. This code won't work if you are using the existing ov* implementations too since the ceu don't depend on the subdev-api too. So including your record increases the complexity. > > 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. Yes, that's also my favourite. @Mauro Can you point out your concerns 'again'? Maybe we can apply Hans approach and will fix the 'maybe' broken usb-bridge devices. Regards, Marco > > 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
Em Fri, 5 Apr 2019 09:26:04 +0200 Marco Felsch <m.felsch@pengutronix.de> escreveu: > Hi Sakari, Jacopo, > > On 19-04-04 16:39, Jacopo Mondi wrote: > > 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. It they depends on VIDEO_V4L2_SUBDEV_API, there's no problem with that. > > > > > > 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. If they require subdev API, but doesn't don't depend on such config, then they're already broken: they won't work, and probably will crash the Kernel somewhere. > > > > > > 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 > > > > Are those drivers depending on CONFIG_VIDEO_V4L2_SUBDEV_API used by > usb-bridge devices? I Remember that was Mauro's concern on Hans RFC patch. > I'm with you the "depending way" is quite better. Neither USB or normal PCI drivers should depend on VIDEO_V4L2_SUBDEV_API. There are, currently, 3 PCI drivers that depends on it: 1) drivers/media/pci/cobalt/Kconfig This is for a development board that not publicly available. So, I don't care about what it depends on. Yet, I suspect that it depends on subdev just because of those drivers, with also depends on it: drivers/media/pci/cobalt/cobalt-driver.c:#include <media/i2c/adv7604.h> drivers/media/pci/cobalt/cobalt-driver.c:#include <media/i2c/adv7842.h> drivers/media/pci/cobalt/cobalt-driver.c:#include <media/i2c/adv7511.h> 2) drivers/media/pci/intel/ipu3/Kconfig This one seems to be the only valid case for the dependency: the IPU3 hardware is actually part of an Intel SoC that depends on subdevs and behave like any other platform driver: it will only work after setting a MC pipeline and command the sub-devices directly via the subdev API. 3) drivers/media/pci/sta2x11/Kconfig This one looks weird for me. It sounds that it uses just the adv7180 i2c driver. I suspect that it depends on VIDEO_V4L2_SUBDEV_API just because the I2C driver assumes that the subdev API is always there. IMO, adv7180 should be fixed to not depend on it anymore, and let sta2x11 to also build without such dependency. Perhaps with the addition of this patch, (1) and (3) can be build and run without depending at the subdev API. > > > > 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 > > Thats true. One thing to think about your caller. This code won't work > if you are using the existing ov* implementations too since the ceu > don't depend on the subdev-api too. > > So including your record increases the complexity. > > > > > 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. > > Yes, that's also my favourite. > > @Mauro > Can you point out your concerns 'again'? Maybe we can apply Hans > approach and will fix the 'maybe' broken usb-bridge devices. > The point is: only platform drivers require the subdev API; for USB/PCI drivers, this is not needed, and the core can be simplified in order to give a smaller footprint. Of course, drivers that depend on subdevs won't be compiled, but sensors can still work. The em28xx cameras, for example, use a few sensor drivers that can optionally export the subdev API, when the bridge driver supports it (with is not the case of the em28xx bridge itself). Looking at this patch, I don't see why it would cause any problems that aren't already there. Thanks, Mauro
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); +} + +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;
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(-)