Linux-Media Archive on lore.kernel.org
 help / Atom feed
* [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable
@ 2019-02-08  8:49 Hans Verkuil
  2019-02-08  9:06 ` Sakari Ailus
  0 siblings, 1 reply; 4+ messages in thread
From: Hans Verkuil @ 2019-02-08  8:49 UTC (permalink / raw)
  To: Linux Media Mailing List; +Cc: Sakari Ailus, Laurent Pinchart

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;						\
 	})


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable
  2019-02-08  8:49 [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable Hans Verkuil
@ 2019-02-08  9:06 ` Sakari Ailus
  2019-02-08  9:08   ` Hans Verkuil
  0 siblings, 1 reply; 4+ messages in thread
From: Sakari Ailus @ 2019-02-08  9:06 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Linux Media Mailing List, Laurent Pinchart

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable
  2019-02-08  9:06 ` Sakari Ailus
@ 2019-02-08  9:08   ` Hans Verkuil
  2019-02-08 10:37     ` Laurent Pinchart
  0 siblings, 1 reply; 4+ messages in thread
From: Hans Verkuil @ 2019-02-08  9:08 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: Linux Media Mailing List, Laurent Pinchart

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;						\
>>  	})
>>
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable
  2019-02-08  9:08   ` Hans Verkuil
@ 2019-02-08 10:37     ` Laurent Pinchart
  0 siblings, 0 replies; 4+ messages in thread
From: Laurent Pinchart @ 2019-02-08 10:37 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Sakari Ailus, Linux Media Mailing List

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-08  8:49 [PATCH] v4l2-subdev.h: v4l2_subdev_call: use temp __sd variable Hans Verkuil
2019-02-08  9:06 ` Sakari Ailus
2019-02-08  9:08   ` Hans Verkuil
2019-02-08 10:37     ` Laurent Pinchart

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox