Re: [PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro

[Sakari Ailus]

On Tue, Dec 19, 2017 at 09:03:55AM -0200, Mauro Carvalho Chehab wrote:
> [PATCH] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro
> 
> The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
> 3 functions that have the same arguments. The code of those
> functions is simple enough to just declare them, de-obfuscating
> the code.
> 
> While here, replace BUG_ON() by WARN_ON() as there's no reason
> why to panic the Kernel if this fails.
> 
> Signed-off-by: Mauro Carvalho Chehab 

Thanks!

Acked-by: Sakari Ailus 

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro

[Mauro Carvalho Chehab]

Em Tue, 19 Dec 2017 10:24:51 +0200
Sakari Ailus  escreveu:

> On Mon, Dec 18, 2017 at 05:27:04PM -0200, Mauro Carvalho Chehab wrote:
> > Em Mon, 9 Oct 2017 23:23:56 +0300
> > Sakari Ailus  escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > On Mon, Oct 09, 2017 at 07:19:21AM -0300, Mauro Carvalho Chehab wrote:  
> > > > The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
> > > > 3 functions that have the same arguments. The code of those
> > > > functions is simple enough to just declare them, de-obfuscating
> > > > the code.
> > > > 
> > > > While here, replace BUG_ON() by WARN_ON() as there's no reason
> > > > why to panic the Kernel if this fails.  
> > > 
> > > BUG_ON() might actually be a better idea as this will lead to memory
> > > corruption. I presume it's not been hit often.  
> > 
> > Well, let's then change the code to:
> > 
> > if (WARN_ON(pad >= sd->entity.num_pads)) 
> > return -EINVAL;
> > 
> > This way, it won't try to use an invalid value. As those are default
> > handlers for ioctls, userspace should be able to handle it.  
> 
> Another approach would be to return the entry for a valid pad. Few if any
> drivers perform error handling on the value returned for the simple reason
> that they know how many pads their own entities have.

Well, they should check for errors on returned values :-)

Anyway, I guess that using pad=0 as a way to avoid Kernel crashes
is not a bad idea.

Patch enclosed. It replaces patch 6/8.

Thanks,
Mauro

[PATCH] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro

The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
3 functions that have the same arguments. The code of those
functions is simple enough to just declare them, de-obfuscating
the code.

While here, replace BUG_ON() by WARN_ON() as there's no reason
why to panic the Kernel if this fails.

Signed-off-by: Mauro Carvalho Chehab 

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 71b8ff4b2e0e..443e5e019006 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -896,19 +896,35 @@ struct v4l2_subdev_fh {
container_of(fh, struct v4l2_subdev_fh, vfh)
 
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)  \
-   static inline struct rtype *\
-   fun_name(struct v4l2_subdev *sd,\
-struct v4l2_subdev_pad_config *cfg,\
-unsigned int pad)  \
-   {   \
-   BUG_ON(pad >= sd->entity.num_pads); \
-   return [pad].field_name;\
-   }
+static inline struct v4l2_mbus_framefmt
+*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
+   struct v4l2_subdev_pad_config *cfg,
+   unsigned int pad)
+{
+   if (WARN_ON(pad >= sd->entity.num_pads))
+   pad = 0;
+   return [pad].try_fmt;
+}
+
+static inline struct v4l2_rect
+*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ unsigned int pad)
+{
+   if (WARN_ON(pad >= sd->entity.num_pads))
+   pad = 0;
+   return [pad].try_crop;
+}
 
-__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, v4l2_subdev_get_try_format, 
try_fmt)
-__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_crop, try_crop)
-__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_compose, try_compose)
+static inline struct v4l2_rect
+*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+unsigned int pad)
+{
+   if (WARN_ON(pad >= sd->entity.num_pads))
+   pad = 0;
+   return [pad].try_compose;
+}
 #endif
 
 extern const struct v4l2_file_operations v4l2_subdev_fops;

Re: [PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro

[Sakari Ailus]

On Mon, Dec 18, 2017 at 05:27:04PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 9 Oct 2017 23:23:56 +0300
> Sakari Ailus  escreveu:
> 
> > Hi Mauro,
> > 
> > On Mon, Oct 09, 2017 at 07:19:21AM -0300, Mauro Carvalho Chehab wrote:
> > > The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
> > > 3 functions that have the same arguments. The code of those
> > > functions is simple enough to just declare them, de-obfuscating
> > > the code.
> > > 
> > > While here, replace BUG_ON() by WARN_ON() as there's no reason
> > > why to panic the Kernel if this fails.
> > 
> > BUG_ON() might actually be a better idea as this will lead to memory
> > corruption. I presume it's not been hit often.
> 
> Well, let's then change the code to:
> 
> if (WARN_ON(pad >= sd->entity.num_pads)) 
> return -EINVAL;
> 
> This way, it won't try to use an invalid value. As those are default
> handlers for ioctls, userspace should be able to handle it.

Another approach would be to return the entry for a valid pad. Few if any
drivers perform error handling on the value returned for the simple reason
that they know how many pads their own entities have.

> 
> > 
> > That said, I, too, favour WARN_ON() in this case. In case pad exceeds the
> > number of pads, then zero could be used, for instance. The only real
> > problem comes if there were no pads to begin with. The callers of these
> > functions also don't expect to receive NULL. Another option would be to
> > define a static, dummy variable for the purpose that would be at least safe
> > to access. Or we could just use the dummy entry whenever the pad isn't
> > valid.
> > 
> > This will make the functions more complex and I might just keep the
> > original macro. Even grep works on it nowadays.
> > 
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab 
> > > ---
> > >  include/media/v4l2-subdev.h | 37 +
> > >  1 file changed, 25 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 1f34045f07ce..35c4476c56ee 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -897,19 +897,32 @@ struct v4l2_subdev_fh {
> > >   container_of(fh, struct v4l2_subdev_fh, vfh)
> > >  
> > >  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > > -#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)
> > > \
> > > - static inline struct rtype *\
> > > - fun_name(struct v4l2_subdev *sd,\
> > > -  struct v4l2_subdev_pad_config *cfg,\
> > > -  unsigned int pad)  \
> > > - {   \
> > > - BUG_ON(pad >= sd->entity.num_pads); \
> > > - return [pad].field_name;\
> > > - }
> > > +static inline struct v4l2_mbus_framefmt
> > > +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + unsigned int pad)
> > > +{
> > > + WARN_ON(pad >= sd->entity.num_pads);
> > > + return [pad].try_fmt;
> > > +}
> > >  
> > > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, v4l2_subdev_get_try_format, 
> > > try_fmt)
> > > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_crop, try_crop)
> > > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_compose, 
> > > try_compose)
> > > +static inline struct v4l2_rect
> > > +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> > > +   struct v4l2_subdev_pad_config *cfg,
> > > +   unsigned int pad)
> > > +{
> > > + WARN_ON(pad >= sd->entity.num_pads);
> > > + return [pad].try_crop;
> > > +}
> > > +
> > > +static inline struct v4l2_rect
> > > +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> > > +  struct v4l2_subdev_pad_config *cfg,
> > > +  unsigned int pad)
> > > +{
> > > + WARN_ON(pad >= sd->entity.num_pads);
> > > + return [pad].try_compose;
> > > +}
> > >  #endif
> > >  
> > >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> > 
> 
> 
> 
> Thanks,
> Mauro

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi

Re: [PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro

[Mauro Carvalho Chehab]

Em Mon, 9 Oct 2017 23:23:56 +0300
Sakari Ailus  escreveu:

> Hi Mauro,
> 
> On Mon, Oct 09, 2017 at 07:19:21AM -0300, Mauro Carvalho Chehab wrote:
> > The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
> > 3 functions that have the same arguments. The code of those
> > functions is simple enough to just declare them, de-obfuscating
> > the code.
> > 
> > While here, replace BUG_ON() by WARN_ON() as there's no reason
> > why to panic the Kernel if this fails.
> 
> BUG_ON() might actually be a better idea as this will lead to memory
> corruption. I presume it's not been hit often.

Well, let's then change the code to:

if (WARN_ON(pad >= sd->entity.num_pads)) 
return -EINVAL;

This way, it won't try to use an invalid value. As those are default
handlers for ioctls, userspace should be able to handle it.

> 
> That said, I, too, favour WARN_ON() in this case. In case pad exceeds the
> number of pads, then zero could be used, for instance. The only real
> problem comes if there were no pads to begin with. The callers of these
> functions also don't expect to receive NULL. Another option would be to
> define a static, dummy variable for the purpose that would be at least safe
> to access. Or we could just use the dummy entry whenever the pad isn't
> valid.
> 
> This will make the functions more complex and I might just keep the
> original macro. Even grep works on it nowadays.
> 
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >  include/media/v4l2-subdev.h | 37 +
> >  1 file changed, 25 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 1f34045f07ce..35c4476c56ee 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -897,19 +897,32 @@ struct v4l2_subdev_fh {
> > container_of(fh, struct v4l2_subdev_fh, vfh)
> >  
> >  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > -#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)  
> > \
> > -   static inline struct rtype *\
> > -   fun_name(struct v4l2_subdev *sd,\
> > -struct v4l2_subdev_pad_config *cfg,\
> > -unsigned int pad)  \
> > -   {   \
> > -   BUG_ON(pad >= sd->entity.num_pads); \
> > -   return [pad].field_name;\
> > -   }
> > +static inline struct v4l2_mbus_framefmt
> > +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > +   struct v4l2_subdev_pad_config *cfg,
> > +   unsigned int pad)
> > +{
> > +   WARN_ON(pad >= sd->entity.num_pads);
> > +   return [pad].try_fmt;
> > +}
> >  
> > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, v4l2_subdev_get_try_format, 
> > try_fmt)
> > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_crop, try_crop)
> > -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_compose, 
> > try_compose)
> > +static inline struct v4l2_rect
> > +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + unsigned int pad)
> > +{
> > +   WARN_ON(pad >= sd->entity.num_pads);
> > +   return [pad].try_crop;
> > +}
> > +
> > +static inline struct v4l2_rect
> > +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> > +struct v4l2_subdev_pad_config *cfg,
> > +unsigned int pad)
> > +{
> > +   WARN_ON(pad >= sd->entity.num_pads);
> > +   return [pad].try_compose;
> > +}
> >  #endif
> >  
> >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> 



Thanks,
Mauro

Re: [PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro

[Sakari Ailus]

Hi Mauro,

On Mon, Oct 09, 2017 at 07:19:21AM -0300, Mauro Carvalho Chehab wrote:
> The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
> 3 functions that have the same arguments. The code of those
> functions is simple enough to just declare them, de-obfuscating
> the code.
> 
> While here, replace BUG_ON() by WARN_ON() as there's no reason
> why to panic the Kernel if this fails.

BUG_ON() might actually be a better idea as this will lead to memory
corruption. I presume it's not been hit often.

That said, I, too, favour WARN_ON() in this case. In case pad exceeds the
number of pads, then zero could be used, for instance. The only real
problem comes if there were no pads to begin with. The callers of these
functions also don't expect to receive NULL. Another option would be to
define a static, dummy variable for the purpose that would be at least safe
to access. Or we could just use the dummy entry whenever the pad isn't
valid.

This will make the functions more complex and I might just keep the
original macro. Even grep works on it nowadays.

> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  include/media/v4l2-subdev.h | 37 +
>  1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1f34045f07ce..35c4476c56ee 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -897,19 +897,32 @@ struct v4l2_subdev_fh {
>   container_of(fh, struct v4l2_subdev_fh, vfh)
>  
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> -#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)
> \
> - static inline struct rtype *\
> - fun_name(struct v4l2_subdev *sd,\
> -  struct v4l2_subdev_pad_config *cfg,\
> -  unsigned int pad)  \
> - {   \
> - BUG_ON(pad >= sd->entity.num_pads); \
> - return [pad].field_name;\
> - }
> +static inline struct v4l2_mbus_framefmt
> +*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return [pad].try_fmt;
> +}
>  
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, v4l2_subdev_get_try_format, 
> try_fmt)
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_crop, try_crop)
> -__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_compose, try_compose)
> +static inline struct v4l2_rect
> +*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> +   struct v4l2_subdev_pad_config *cfg,
> +   unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return [pad].try_crop;
> +}
> +
> +static inline struct v4l2_rect
> +*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> +  struct v4l2_subdev_pad_config *cfg,
> +  unsigned int pad)
> +{
> + WARN_ON(pad >= sd->entity.num_pads);
> + return [pad].try_compose;
> +}
>  #endif
>  
>  extern const struct v4l2_file_operations v4l2_subdev_fops;

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi

[PATCH 15/24] media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro

[Mauro Carvalho Chehab]

The __V4L2_SUBDEV_MK_GET_TRY() macro is used to define
3 functions that have the same arguments. The code of those
functions is simple enough to just declare them, de-obfuscating
the code.

While here, replace BUG_ON() by WARN_ON() as there's no reason
why to panic the Kernel if this fails.

Signed-off-by: Mauro Carvalho Chehab 
---
 include/media/v4l2-subdev.h | 37 +
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1f34045f07ce..35c4476c56ee 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -897,19 +897,32 @@ struct v4l2_subdev_fh {
container_of(fh, struct v4l2_subdev_fh, vfh)
 
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-#define __V4L2_SUBDEV_MK_GET_TRY(rtype, fun_name, field_name)  \
-   static inline struct rtype *\
-   fun_name(struct v4l2_subdev *sd,\
-struct v4l2_subdev_pad_config *cfg,\
-unsigned int pad)  \
-   {   \
-   BUG_ON(pad >= sd->entity.num_pads); \
-   return [pad].field_name;\
-   }
+static inline struct v4l2_mbus_framefmt
+*v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
+   struct v4l2_subdev_pad_config *cfg,
+   unsigned int pad)
+{
+   WARN_ON(pad >= sd->entity.num_pads);
+   return [pad].try_fmt;
+}
 
-__V4L2_SUBDEV_MK_GET_TRY(v4l2_mbus_framefmt, v4l2_subdev_get_try_format, 
try_fmt)
-__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_crop, try_crop)
-__V4L2_SUBDEV_MK_GET_TRY(v4l2_rect, v4l2_subdev_get_try_compose, try_compose)
+static inline struct v4l2_rect
+*v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
+ struct v4l2_subdev_pad_config *cfg,
+ unsigned int pad)
+{
+   WARN_ON(pad >= sd->entity.num_pads);
+   return [pad].try_crop;
+}
+
+static inline struct v4l2_rect
+*v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
+struct v4l2_subdev_pad_config *cfg,
+unsigned int pad)
+{
+   WARN_ON(pad >= sd->entity.num_pads);
+   return [pad].try_compose;
+}
 #endif
 
 extern const struct v4l2_file_operations v4l2_subdev_fops;
-- 
2.13.6