[PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable

[PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable

[Hans Verkuil]

The sd argument of this macro can be a more complex expression. Since it
is used 5 times in the macro it can be evaluated that many times as well.

So assign it to a temp variable in the beginning and use that instead.

This also avoids any potential side-effects of evaluating sd.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 47af609dc8f1..34da094a3f40 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1093,13 +1093,14 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
  */
 #define v4l2_subdev_call(sd, o, f, args...)				\
 	({								\
+		struct v4l2_subdev *__sd = (sd);			\
 		int __result;						\
-		if (!(sd))						\
+		if (!__sd)						\
 			__result = -ENODEV;				\
-		else if (!((sd)->ops->o && (sd)->ops->o->f))		\
+		else if (!(__sd->ops->o && __sd->ops->o->f))		\
 			__result = -ENOIOCTLCMD;			\
 		else							\
-			__result = (sd)->ops->o->f((sd), ##args);	\
+			__result = __sd->ops->o->f(__sd, ##args);	\
 		__result;						\
 	})

Re: [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable

[Sakari Ailus]

On Fri, Feb 08, 2019 at 09:49:23AM +0100, Hans Verkuil wrote:
> The sd argument of this macro can be a more complex expression. Since it
> is used 5 times in the macro it can be evaluated that many times as well.
> 
> So assign it to a temp variable in the beginning and use that instead.
> 
> This also avoids any potential side-effects of evaluating sd.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Nice one!

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

I wonder if this addresses some of the sparse issues related to using a
macro to come up with sd?

> ---
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 47af609dc8f1..34da094a3f40 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1093,13 +1093,14 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
>   */
>  #define v4l2_subdev_call(sd, o, f, args...)				\
>  	({								\
> +		struct v4l2_subdev *__sd = (sd);			\
>  		int __result;						\
> -		if (!(sd))						\
> +		if (!__sd)						\
>  			__result = -ENODEV;				\
> -		else if (!((sd)->ops->o && (sd)->ops->o->f))		\
> +		else if (!(__sd->ops->o && __sd->ops->o->f))		\
>  			__result = -ENOIOCTLCMD;			\
>  		else							\
> -			__result = (sd)->ops->o->f((sd), ##args);	\
> +			__result = __sd->ops->o->f(__sd, ##args);	\
>  		__result;						\
>  	})
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable

[Hans Verkuil]

On 2/8/19 10:06 AM, Sakari Ailus wrote:
> On Fri, Feb 08, 2019 at 09:49:23AM +0100, Hans Verkuil wrote:
>> The sd argument of this macro can be a more complex expression. Since it
>> is used 5 times in the macro it can be evaluated that many times as well.
>>
>> So assign it to a temp variable in the beginning and use that instead.
>>
>> This also avoids any potential side-effects of evaluating sd.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Nice one!
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> I wonder if this addresses some of the sparse issues related to using a
> macro to come up with sd?

It does solve those as well, in fact :-)

I rejected the omap3/4 patches in favor of this one, which is a much, much
cleaner solution.

Regards,

	Hans

> 
>> ---
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 47af609dc8f1..34da094a3f40 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1093,13 +1093,14 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
>>   */
>>  #define v4l2_subdev_call(sd, o, f, args...)				\
>>  	({								\
>> +		struct v4l2_subdev *__sd = (sd);			\
>>  		int __result;						\
>> -		if (!(sd))						\
>> +		if (!__sd)						\
>>  			__result = -ENODEV;				\
>> -		else if (!((sd)->ops->o && (sd)->ops->o->f))		\
>> +		else if (!(__sd->ops->o && __sd->ops->o->f))		\
>>  			__result = -ENOIOCTLCMD;			\
>>  		else							\
>> -			__result = (sd)->ops->o->f((sd), ##args);	\
>> +			__result = __sd->ops->o->f(__sd, ##args);	\
>>  		__result;						\
>>  	})
>>
> 

Re: [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable

[Laurent Pinchart]

Hi Hans,

On Fri, Feb 08, 2019 at 10:08:22AM +0100, Hans Verkuil wrote:
> On 2/8/19 10:06 AM, Sakari Ailus wrote:
> > On Fri, Feb 08, 2019 at 09:49:23AM +0100, Hans Verkuil wrote:
> >> The sd argument of this macro can be a more complex expression. Since it
> >> is used 5 times in the macro it can be evaluated that many times as well.
> >>
> >> So assign it to a temp variable in the beginning and use that instead.
> >>
> >> This also avoids any potential side-effects of evaluating sd.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > 
> > Nice one!
> > 
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > I wonder if this addresses some of the sparse issues related to using a
> > macro to come up with sd?
> 
> It does solve those as well, in fact :-)
> 
> I rejected the omap3/4 patches in favor of this one, which is a much, much
> cleaner solution.

Thank you for looking into this. Great work :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >> ---
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 47af609dc8f1..34da094a3f40 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -1093,13 +1093,14 @@ void v4l2_subdev_init(struct v4l2_subdev *sd,
> >>   */
> >>  #define v4l2_subdev_call(sd, o, f, args...)				\
> >>  	({								\
> >> +		struct v4l2_subdev *__sd = (sd);			\
> >>  		int __result;						\
> >> -		if (!(sd))						\
> >> +		if (!__sd)						\
> >>  			__result = -ENODEV;				\
> >> -		else if (!((sd)->ops->o && (sd)->ops->o->f))		\
> >> +		else if (!(__sd->ops->o && __sd->ops->o->f))		\
> >>  			__result = -ENOIOCTLCMD;			\
> >>  		else							\
> >> -			__result = (sd)->ops->o->f((sd), ##args);	\
> >> +			__result = __sd->ops->o->f(__sd, ##args);	\
> >>  		__result;						\
> >>  	})
> >>

-- 
Regards,

Laurent Pinchart