Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[Andrew Morton]

On Mon, 6 Apr 2009 21:59:01 GMT
Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:

> Gitweb:     http://git.kernel.org/linus/8f912ba4d7cdaf7d31cf39fe5a9b7732308a256d
> Commit:     8f912ba4d7cdaf7d31cf39fe5a9b7732308a256d
> Parent:     20bec8ab1458c24bed0d5492ee15d87807fc415a
> Author:     David Woodhouse <David.Woodhouse@intel.com>
> AuthorDate: Fri Apr 3 15:19:32 2009 +0100
> Committer:  David Woodhouse <David.Woodhouse@intel.com>
> CommitDate: Fri Apr 3 21:45:46 2009 +0100

Three days old?  Never been in linux-next afaict.

Please don't do that.

>     intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros
>     
>     Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>     Acked-by: Ingo Molnar <mingo@elte.hu>
> ---
>  include/linux/dmar.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index 2f34274..0b4aa80 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -44,6 +44,14 @@ extern struct list_head dmar_drhd_units;
>  #define for_each_drhd_unit(drhd) \
>  	list_for_each_entry(drhd, &dmar_drhd_units, list)
>  
> +#define for_each_active_iommu(i, drhd)					\
> +	list_for_each_entry(drhd, &dmar_drhd_units, list)		\
> +		if (i=drhd->iommu, drhd->ignored) {} else
> +
> +#define for_each_iommu(i, drhd)						\
> +	list_for_each_entry(drhd, &dmar_drhd_units, list)		\
> +		if (i=drhd->iommu, 0) {} else 
> +
>  extern int dmar_table_init(void);
>  extern int dmar_dev_scope_init(void);

From: Andrew Morton <akpm@linux-foundation.org>

drivers/pci/intel-iommu.c:2637:35: error: macro "if" passed 2 arguments, but takes just 1
drivers/pci/intel-iommu.c: In function 'init_iommu_hw':
drivers/pci/intel-iommu.c:2637: error: expected '(' before '{' token
drivers/pci/intel-iommu.c:2638: error: '______r' undeclared (first use in this function)
drivers/pci/intel-iommu.c:2638: error: (Each undeclared identifier is reported only once
drivers/pci/intel-iommu.c:2638: error: for each function it appears in.)
drivers/pci/intel-iommu.c:2641:35: error: macro "if" passed 2 arguments, but takes just 1
drivers/pci/intel-iommu.c:2641: error: expected '(' before '{' token
drivers/pci/intel-iommu.c:2662:35: error: macro "if" passed 2 arguments, but takes just 1
drivers/pci/intel-iommu.c: In function 'iommu_flush_all':
drivers/pci/intel-iommu.c:2662: error: expected '(' before '{' token
drivers/pci/intel-iommu.c:2676:35: error: macro "if" passed 2 arguments, but takes just 1
drivers/pci/intel-iommu.c: In function 'iommu_suspend':
drivers/pci/intel-iommu.c:2676: error: expected '(' before '{' token
drivers/pci/intel-iommu.c:2678: error: too few arguments to function 'kzalloc'
drivers/pci/intel-iommu.c:2685:35: error: macro "if" passed 2 arguments, but takes just 1
drivers/pci/intel-iommu.c:2685: error: expected '(' before '{' token
drivers/pci/intel-iommu.c:2688: error: '__dummy' undeclared (first use in this function)
drivers/pci/intel-iommu.c:2704:35: error: macro "if" passed 2 arguments, but takes just 1
drivers/pci/intel-iommu.c:2704: error: expected '(' before '{' token
drivers/pci/intel-iommu.c:2721:35: error: macro "if" passed 2 arguments, but takes just 1
drivers/pci/intel-iommu.c:2737:35: error: macro "if" passed 2 arguments, but takes just 1
drivers/pci/intel-iommu.c: At top level:
drivers/pci/intel-iommu.c:2745: error: 'iommu_resume' undeclared here (not in a function)

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/dmar.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN include/linux/dmar.h~include-linux-dmarh-fix-build-with-config_profile_all_branches include/linux/dmar.h
--- a/include/linux/dmar.h~include-linux-dmarh-fix-build-with-config_profile_all_branches
+++ a/include/linux/dmar.h
@@ -47,11 +47,11 @@ extern struct list_head dmar_drhd_units;
 
 #define for_each_active_iommu(i, drhd)					\
 	list_for_each_entry(drhd, &dmar_drhd_units, list)		\
-		if (i=drhd->iommu, drhd->ignored) {} else
+		if ((i=drhd->iommu, drhd->ignored)) {} else
 
 #define for_each_iommu(i, drhd)						\
 	list_for_each_entry(drhd, &dmar_drhd_units, list)		\
-		if (i=drhd->iommu, 0) {} else 
+		if ((i=drhd->iommu, 0)) {} else
 
 extern int dmar_table_init(void);
 extern int dmar_dev_scope_init(void);
_

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[David Woodhouse]

On Mon, 2009-04-06 at 17:59 -0700, Andrew Morton wrote:
> 
> Three days old?  Never been in linux-next afaict.
> 
> Please don't do that.

Sorry about that. There was some confusion about whether it was going
via Ingo's or my tree, and it ended up in neither. It depended on
certain things in Ingo's tree, so I couldn't easily add it to my tree
any earlier.

I'm unconvinced by the fix -- can we find a way to fix the if() macro so
that normal (if fugly) C code like this doesn't doesn't need this kind
of workaround?

(Yes, it's fugly. But the original was worse IMHO:
https://lists.linux-foundation.org/pipermail/iommu/attachments/20090327/31f84f7d/attachment.txt )

-- 
dwmw2

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[David Woodhouse]

On Mon, 2009-04-06 at 18:09 -0700, David Woodhouse wrote:
> 
> I'm unconvinced by the fix -- can we find a way to fix the if() macro so
> that normal (if fugly) C code like this doesn't doesn't need this kind
> of workaround?

Something like this perhaps? I'd reformat it but dinner calls...

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 6faa7e5..1da2e72 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -114,7 +114,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  * "Define 'is'", Bill Clinton
  * "Define 'if'", Steven Rostedt
  */
-#define if(cond) if (__builtin_constant_p((cond)) ? !!(cond) :		\
+#define if(cond, ...) if (__builtin_constant_p((cond, ## __VA_ARGS__)) ? !!(cond, ## __VA_ARGS__) : \
 	({								\
 		int ______r;						\
 		static struct ftrace_branch_data			\
@@ -125,7 +125,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 				.file = __FILE__,			\
 				.line = __LINE__,			\
 			};						\
-		______r = !!(cond);					\
+		______r = !!(cond, ## __VA_ARGS__);					\
 		______f.miss_hit[______r]++;					\
 		______r;						\
 	}))

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[Andrew Morton]

On Mon, 06 Apr 2009 18:19:06 -0700
David Woodhouse <dwmw2@infradead.org> wrote:

> On Mon, 2009-04-06 at 18:09 -0700, David Woodhouse wrote:
> > 
> > I'm unconvinced by the fix -- can we find a way to fix the if() macro so
> > that normal (if fugly) C code like this doesn't doesn't need this kind
> > of workaround?
> 
> Something like this perhaps? I'd reformat it but dinner calls...
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 6faa7e5..1da2e72 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -114,7 +114,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>   * "Define 'is'", Bill Clinton
>   * "Define 'if'", Steven Rostedt
>   */
> -#define if(cond) if (__builtin_constant_p((cond)) ? !!(cond) :		\
> +#define if(cond, ...) if (__builtin_constant_p((cond, ## __VA_ARGS__)) ? !!(cond, ## __VA_ARGS__) : \
>  	({								\
>  		int ______r;						\
>  		static struct ftrace_branch_data			\
> @@ -125,7 +125,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  				.file = __FILE__,			\
>  				.line = __LINE__,			\
>  			};						\
> -		______r = !!(cond);					\
> +		______r = !!(cond, ## __VA_ARGS__);					\
>  		______f.miss_hit[______r]++;					\
>  		______r;						\
>  	}))

That would be better.  If it works, please send something Linuswards?

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[Steven Rostedt]

On Mon, 6 Apr 2009, David Woodhouse wrote:

> On Mon, 2009-04-06 at 18:09 -0700, David Woodhouse wrote:
> > 
> > I'm unconvinced by the fix -- can we find a way to fix the if() macro so
> > that normal (if fugly) C code like this doesn't doesn't need this kind
> > of workaround?
> 
> Something like this perhaps? I'd reformat it but dinner calls...

Use emacs Ctrl-c / ;-)

> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 6faa7e5..1da2e72 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -114,7 +114,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>   * "Define 'is'", Bill Clinton
>   * "Define 'if'", Steven Rostedt
>   */
> -#define if(cond) if (__builtin_constant_p((cond)) ? !!(cond) :		\
> +#define if(cond, ...) if (__builtin_constant_p((cond, ## __VA_ARGS__)) ? !!(cond, ## __VA_ARGS__) : \
>  	({								\
>  		int ______r;						\
>  		static struct ftrace_branch_data			\
> @@ -125,7 +125,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  				.file = __FILE__,			\
>  				.line = __LINE__,			\
>  			};						\
> -		______r = !!(cond);					\
> +		______r = !!(cond, ## __VA_ARGS__);					\
>  		______f.miss_hit[______r]++;					\
>  		______r;						\
>  	}))
> 

If this works then (after reformating)

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[David Woodhouse]

On Mon, 2009-04-06 at 21:29 -0400, Steven Rostedt wrote:
> If this works then (after reformating)
> 
> Acked-by: Steven Rostedt <rostedt@goodmis.org>

Yeah, it seems to work. This worried me briefly for a while but then I
realised it was already there before my changes...

[dwmw2@macbook linux-2.6]$ grep CONFIG_PROFILE_ALL_BRANCHES .config
# CONFIG_PROFILE_ALL_BRANCHES is not set
[dwmw2@macbook linux-2.6]$ make drivers/net/e100.o
scripts/kconfig/conf -s arch/x86/Kconfig
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CC      kernel/bounds.s
  GEN     include/linux/bounds.h
  CC      arch/x86/kernel/asm-offsets.s
  GEN     include/asm/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      drivers/net/e100.o
[dwmw2@macbook linux-2.6]$ sed -i 's/.*\(CONFIG_PROFILE_ALL_BRANCHES\).*/\1=y/' .config
[dwmw2@macbook linux-2.6]$ make drivers/net/e100.oscripts/kconfig/conf -s arch/x86/Kconfig
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CC      kernel/bounds.s
  GEN     include/linux/bounds.h
  CC      arch/x86/kernel/asm-offsets.s
  GEN     include/asm/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      drivers/net/e100.o
drivers/net/e100.c: In function ‘e100_hw_init’:
drivers/net/e100.c:1344: warning: ‘err’ may be used uninitialized in this function


This happens in a lot of places, not just e100.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[Steven Rostedt]

On Mon, 6 Apr 2009, David Woodhouse wrote:

> On Mon, 2009-04-06 at 21:29 -0400, Steven Rostedt wrote:
> > If this works then (after reformating)
> > 
> > Acked-by: Steven Rostedt <rostedt@goodmis.org>
> 
> Yeah, it seems to work. This worried me briefly for a while but then I
> realised it was already there before my changes...
> 
> [dwmw2@macbook linux-2.6]$ grep CONFIG_PROFILE_ALL_BRANCHES .config
> # CONFIG_PROFILE_ALL_BRANCHES is not set
> [dwmw2@macbook linux-2.6]$ make drivers/net/e100.o
> scripts/kconfig/conf -s arch/x86/Kconfig
>   CHK     include/linux/version.h
>   CHK     include/linux/utsrelease.h
>   SYMLINK include/asm -> include/asm-x86
>   CC      kernel/bounds.s
>   GEN     include/linux/bounds.h
>   CC      arch/x86/kernel/asm-offsets.s
>   GEN     include/asm/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CC      drivers/net/e100.o
> [dwmw2@macbook linux-2.6]$ sed -i 's/.*\(CONFIG_PROFILE_ALL_BRANCHES\).*/\1=y/' .config
> [dwmw2@macbook linux-2.6]$ make drivers/net/e100.oscripts/kconfig/conf -s arch/x86/Kconfig
>   CHK     include/linux/version.h
>   CHK     include/linux/utsrelease.h
>   SYMLINK include/asm -> include/asm-x86
>   CC      kernel/bounds.s
>   GEN     include/linux/bounds.h
>   CC      arch/x86/kernel/asm-offsets.s
>   GEN     include/asm/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CC      drivers/net/e100.o
> drivers/net/e100.c: In function ?e100_hw_init?:
> drivers/net/e100.c:1344: warning: ?err? may be used uninitialized in this function
> 
> 
> This happens in a lot of places, not just e100.

Yeah, that's because the if macro exceeds gcc's threshold of 
calculating initialized variables.

-- Steve

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[David Woodhouse]

The PROFILE_ALL_BRANCHES option causes us to define if() as a macro...
taking precisely one argument. This causes a build failure if the
condition is actually a comma expression -- 'if (a(), b)'.

This patch turns the if() macro into a varargs macro which copes with a
comma expression as its 'arguments'.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>

---
Some might argue that I shouldn't have been using if(a,b) in the first
place, but in my defence it _should_ have worked, and it was less ugly
than the original. I did this:

#define for_each_active_iommu(i, drhd)                                  \
        list_for_each_entry(drhd, &dmar_drhd_units, list)               \
                if (i=drhd->iommu, drhd->ignored) {} else

... instead of what I was originally given, which was (with the first
three macros added to list.h as a generic facility)...

#define list_first_entry_selected(pos, head, member, selector)		  \
	pos = list_entry((head)->next, typeof(*pos), member),		  \
	pos = ({while (!(selector) && (&pos->member != (head)))		  \
		pos = list_entry(pos->member.next, typeof(*pos), member); \
		pos; })
#define list_next_entry_selected(pos, head, member, selector)		  \
	pos = list_entry(pos->member.next, typeof(*pos), member),	  \
	pos = ({while (!(selector) && (&pos->member != (head)))		  \
		pos = list_entry(pos->member.next, typeof(*pos), member); \
		pos; })
#define list_for_each_entry_selected_do(pos, head, member, selector, to_do) \
	for (list_first_entry_selected(pos, head, member, selector),	    \
		({if (&pos->member != (head)) to_do});			    \
		prefetch(pos->member.next), &pos->member != (head);	    \
		list_next_entry_selected(pos, head, member, selector),	    \
		({if (&pos->member != (head)) to_do}))

#define for_each_active_iommu(i, drhd)					\
	list_for_each_entry_selected_do(drhd, &dmar_drhd_units, list,	\
				!drhd->ignored, {i = drhd->iommu; })

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 6faa7e5..c28d876 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -114,7 +114,8 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  * "Define 'is'", Bill Clinton
  * "Define 'if'", Steven Rostedt
  */
-#define if(cond) if (__builtin_constant_p((cond)) ? !!(cond) :		\
+#define if(cond, ...) if (__builtin_constant_p((cond, ## __VA_ARGS__))	\
+			  ? !!(cond, ## __VA_ARGS__) :			\
 	({								\
 		int ______r;						\
 		static struct ftrace_branch_data			\
@@ -125,8 +126,8 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 				.file = __FILE__,			\
 				.line = __LINE__,			\
 			};						\
-		______r = !!(cond);					\
-		______f.miss_hit[______r]++;					\
+		______r = !!(cond, ## __VA_ARGS__);			\
+		______f.miss_hit[______r]++;				\
 		______r;						\
 	}))
 #endif /* CONFIG_PROFILE_ALL_BRANCHES */

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[Ingo Molnar]

* David Woodhouse <dwmw2@infradead.org> wrote:

> On Mon, 2009-04-06 at 17:59 -0700, Andrew Morton wrote:
> > 
> > Three days old?  Never been in linux-next afaict.
> > 
> > Please don't do that.
> 
> Sorry about that. There was some confusion about whether it was 
> going via Ingo's or my tree, and it ended up in neither. It 
> depended on certain things in Ingo's tree, so I couldn't easily 
> add it to my tree any earlier.

Well, they could have ended up in the x86 tree or in the IOMMU tree, 
but the patches were all very late and went through various review 
rounds with me so they had no chance to end up there in a finished 
state before the merge window.

I didnt mind you pushing them to Linus separately and gave an 
Acked-by for the bits and for that merge route, but all the usual 
protocols still have to be followed if you do that: review, testing, 
proper exposure.

	Ingo

Re: intel-iommu: Add for_each_iommu() and for_each_active_iommu() macros

[David Woodhouse]

On Wed, 2009-04-08 at 13:00 +0200, Ingo Molnar wrote:
> * David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > On Mon, 2009-04-06 at 17:59 -0700, Andrew Morton wrote:
> > > 
> > > Three days old?  Never been in linux-next afaict.
> > > 
> > > Please don't do that.

It turns out they had actually been in linux-next, just not for long.

> > Sorry about that. There was some confusion about whether it was 
> > going via Ingo's or my tree, and it ended up in neither. It 
> > depended on certain things in Ingo's tree, so I couldn't easily 
> > add it to my tree any earlier.
> 
> Well, they could have ended up in the x86 tree or in the IOMMU tree, 
> but the patches were all very late and went through various review 
> rounds with me so they had no chance to end up there in a finished 
> state before the merge window.
> 
> I didnt mind you pushing them to Linus separately and gave an 
> Acked-by for the bits and for that merge route, but all the usual 
> protocols still have to be followed if you do that: review, testing, 
> proper exposure.

To avoid extra merges, my plan was to let Linus pull the original
iommu-2.6.git tree, then fast-forward to HEAD and apply the suspend
patches which depended on your new stuff. Then leave them to soak for as
long as possible before asking Linus to pull again.

If Linus had pulled the iommu-2.6.git tree on Tuesday when I first
asked, the suspend set would have been in linux-next for longer. Perhaps
if I'd had the wit to remember to include a diffstat on Tuesday's pull
request... :)

-- 
dwmw2