[PATCH] missing includes from infiniband merge

[PATCH] missing includes from infiniband merge

[Al Viro]

indirect chains of includes are arch-specific and can't
be relied upon...  (hell, even attempt to build it for
itanic would trigger vmalloc.h ones; err.h triggers
on e.g. alpha).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/infiniband/core/mad_priv.h           |    1 +
 drivers/infiniband/hw/amso1100/c2_provider.c |    1 +
 drivers/infiniband/hw/amso1100/c2_rnic.c     |    1 +
 drivers/infiniband/hw/ipath/ipath_diag.c     |    1 +
 4 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/mad_priv.h b/drivers/infiniband/core/mad_priv.h
index 1da9adb..d06b590 100644
--- a/drivers/infiniband/core/mad_priv.h
+++ b/drivers/infiniband/core/mad_priv.h
@@ -38,6 +38,7 @@ #ifndef __IB_MAD_PRIV_H__
 #define __IB_MAD_PRIV_H__
 
 #include <linux/completion.h>
+#include <linux/err.h>
 #include <linux/pci.h>
 #include <linux/workqueue.h>
 #include <rdma/ib_mad.h>
diff --git a/drivers/infiniband/hw/amso1100/c2_provider.c b/drivers/infiniband/hw/amso1100/c2_provider.c
index 8fddc8c..dd6af55 100644
--- a/drivers/infiniband/hw/amso1100/c2_provider.c
+++ b/drivers/infiniband/hw/amso1100/c2_provider.c
@@ -49,6 +49,7 @@ #include <linux/tcp.h>
 #include <linux/init.h>
 #include <linux/dma-mapping.h>
 #include <linux/if_arp.h>
+#include <linux/vmalloc.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
diff --git a/drivers/infiniband/hw/amso1100/c2_rnic.c b/drivers/infiniband/hw/amso1100/c2_rnic.c
index 1c3c9d6..f49a32b 100644
--- a/drivers/infiniband/hw/amso1100/c2_rnic.c
+++ b/drivers/infiniband/hw/amso1100/c2_rnic.c
@@ -50,6 +50,7 @@ #include <linux/init.h>
 #include <linux/dma-mapping.h>
 #include <linux/mm.h>
 #include <linux/inet.h>
+#include <linux/vmalloc.h>
 
 #include <linux/route.h>
 
diff --git a/drivers/infiniband/hw/ipath/ipath_diag.c b/drivers/infiniband/hw/ipath/ipath_diag.c
index 28b6b46..29958b6 100644
--- a/drivers/infiniband/hw/ipath/ipath_diag.c
+++ b/drivers/infiniband/hw/ipath/ipath_diag.c
@@ -43,6 +43,7 @@
 
 #include <linux/io.h>
 #include <linux/pci.h>
+#include <linux/vmalloc.h>
 #include <asm/uaccess.h>
 
 #include "ipath_kernel.h"
-- 
1.4.2.GIT

Re: [PATCH] missing includes from infiniband merge

[Roland Dreier]

Thanks, applied to my tree.

Sorry about that -- my cross-compilation testbed broke at an
inopportune moment.

 - R.

Re: [PATCH] missing includes from infiniband merge

[Sam Ravnborg]

On Sat, Sep 23, 2006 at 04:44:16PM +0100, Al Viro wrote:
> indirect chains of includes are arch-specific and can't
> be relied upon...  (hell, even attempt to build it for
> itanic would trigger vmalloc.h ones; err.h triggers
> on e.g. alpha).
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/infiniband/core/mad_priv.h           |    1 +
>  drivers/infiniband/hw/amso1100/c2_provider.c |    1 +
>  drivers/infiniband/hw/amso1100/c2_rnic.c     |    1 +
>  drivers/infiniband/hw/ipath/ipath_diag.c     |    1 +
>  4 files changed, 4 insertions(+), 0 deletions(-)
A better fix would be to avoid the arch dependency in the non-arch .h
files so that in most cases it just works??

That will result in a few files being included twice or more but
does that matter with current gcc - I doubt.

	Sam

Re: [PATCH] missing includes from infiniband merge

[Al Viro]

On Sat, Sep 23, 2006 at 10:29:12PM +0200, Sam Ravnborg wrote:
> On Sat, Sep 23, 2006 at 04:44:16PM +0100, Al Viro wrote:
> > indirect chains of includes are arch-specific and can't
> > be relied upon...  (hell, even attempt to build it for
> > itanic would trigger vmalloc.h ones; err.h triggers
> > on e.g. alpha).
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> >  drivers/infiniband/core/mad_priv.h           |    1 +
> >  drivers/infiniband/hw/amso1100/c2_provider.c |    1 +
> >  drivers/infiniband/hw/amso1100/c2_rnic.c     |    1 +
> >  drivers/infiniband/hw/ipath/ipath_diag.c     |    1 +
> >  4 files changed, 4 insertions(+), 0 deletions(-)
> A better fix would be to avoid the arch dependency in the non-arch .h
> files so that in most cases it just works??

What "it"?  Use of vmalloc() without including vmalloc.h since on i386
it just happens to be pulled via the
linux/pci.h -> linux/dmapool.h -> asm-i386/io.h -> linux/vmalloc.h
chain?

Should we replicate it on every platform?  Along with all kinds of fun that's
going to cause wrt ordering, BTW...

Re: [PATCH] missing includes from infiniband merge

[Al Viro]

On Sat, Sep 23, 2006 at 09:36:05PM +0100, Al Viro wrote:
> On Sat, Sep 23, 2006 at 10:29:12PM +0200, Sam Ravnborg wrote:
> > On Sat, Sep 23, 2006 at 04:44:16PM +0100, Al Viro wrote:
> > > indirect chains of includes are arch-specific and can't
> > > be relied upon...  (hell, even attempt to build it for
> > > itanic would trigger vmalloc.h ones; err.h triggers
> > > on e.g. alpha).
> > > 
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > ---
> > >  drivers/infiniband/core/mad_priv.h           |    1 +
> > >  drivers/infiniband/hw/amso1100/c2_provider.c |    1 +
> > >  drivers/infiniband/hw/amso1100/c2_rnic.c     |    1 +
> > >  drivers/infiniband/hw/ipath/ipath_diag.c     |    1 +
> > >  4 files changed, 4 insertions(+), 0 deletions(-)
> > A better fix would be to avoid the arch dependency in the non-arch .h
> > files so that in most cases it just works??
> 
> What "it"?  Use of vmalloc() without including vmalloc.h since on i386
> it just happens to be pulled via the
> linux/pci.h -> linux/dmapool.h -> asm-i386/io.h -> linux/vmalloc.h
> chain?

BTW, to do what you suggest we would mean the following:

let H(arch,foo) = {bar | asm-arch/foo.h pulls asm-arch/bar.h via chain that
doesn't include linux/*}
let M(arch,foo) = {baz | asm-arch/bar.h includes linux/baz.h for some bar in
H(arch,foo)}
let L(foo) = union of M(arch,foo) by all arch.
make sure that for each arch and for each bar in L(foo) we have asm-arch/foo.h
pulling linux/bar.h

Have fun doing that and maintaining the results.  Note that addition of
include between asm-weird-crap/foo.h and asm-weird-crap/bar.h will require
changes all over the place in asm-*/* - not only for asm-*/foo.h.

It won't be pretty, it will be hell to even try to clean dependencies
_and_ it will break all the time due to ordering requirements.

Re: [PATCH] missing includes from infiniband merge

[Sam Ravnborg]

On Sat, Sep 23, 2006 at 09:36:05PM +0100, Al Viro wrote:
> > A better fix would be to avoid the arch dependency in the non-arch .h
> > files so that in most cases it just works??
> 
> What "it"?  Use of vmalloc() without including vmalloc.h since on i386
> it just happens to be pulled via the
> linux/pci.h -> linux/dmapool.h -> asm-i386/io.h -> linux/vmalloc.h
> chain?
The other way around. Try to get rid of the evil includes in arch-$(ASM)
that is just sitting there for no other purpose than to let a developer skip
a single include.
In this case the right fix IMO would have been to kill the include of
linux/vmalloc.h from asm-i386/io.h and let all users that previously failed
to include vmalloc.h now do so themself.

Looking through asm-i386/io.h at fist look there is zero use of
linux/vmalloc.h so the include has no business there.

With this your patch would obviously be needed and on top of this we would
have to fix other places that 'forget' to include vmalloc.h but the good thing
would be that this is now a bit more consistent across architectures.

	Sam

Re: [PATCH] missing includes from infiniband merge

[Al Viro]

On Sun, Sep 24, 2006 at 08:44:47AM +0200, Sam Ravnborg wrote:
> On Sat, Sep 23, 2006 at 09:36:05PM +0100, Al Viro wrote:
> > > A better fix would be to avoid the arch dependency in the non-arch .h
> > > files so that in most cases it just works??
> > 
> > What "it"?  Use of vmalloc() without including vmalloc.h since on i386
> > it just happens to be pulled via the
> > linux/pci.h -> linux/dmapool.h -> asm-i386/io.h -> linux/vmalloc.h
> > chain?
> The other way around. Try to get rid of the evil includes in arch-$(ASM)
> that is just sitting there for no other purpose than to let a developer skip
> a single include.
> In this case the right fix IMO would have been to kill the include of
> linux/vmalloc.h from asm-i386/io.h and let all users that previously failed
> to include vmalloc.h now do so themself.

> Looking through asm-i386/io.h at fist look there is zero use of
> linux/vmalloc.h so the include has no business there.

There are obvious asm/page.h uses, so just ripping it out won't be enough.
Even for that particular case.  And we have shitloads of places were
asm-foo/bar.h genuinely needs linux/baz.h for e.g. implementation of
an inlined helper.  With other targets not needing it at all.  Would you
mandate including it from every user of asm/foo.h?  And maintain such
rules afterwards ("asm/foo.h needs linux/baz.h included before it since
on $WEIRD_TARGET we include asm/unique_turd.h that won't compile unless
linux/baz.h will be aready there").

Re: [PATCH] missing includes from infiniband merge

[Sam Ravnborg]

On Sun, Sep 24, 2006 at 08:19:17PM +0100, Al Viro wrote:
> 
> > Looking through asm-i386/io.h at fist look there is zero use of
> > linux/vmalloc.h so the include has no business there.
> 
> There are obvious asm/page.h uses, so just ripping it out won't be enough.
> Even for that particular case.  And we have shitloads of places were
> asm-foo/bar.h genuinely needs linux/baz.h for e.g. implementation of
> an inlined helper.  With other targets not needing it at all.  Would you
> mandate including it from every user of asm/foo.h?  And maintain such
> rules afterwards ("asm/foo.h needs linux/baz.h included before it since
> on $WEIRD_TARGET we include asm/unique_turd.h that won't compile unless
> linux/baz.h will be aready there").

The only thing I like to see is minimal suprise. And minimal suprise in
this case is to be considered as "works on almost all archs if not all".
In practical terms it could be that users of asm/* had to include
baz.h before bar.h. Or we could stick to current mess where one has
to have a shitload of crosscompiles and CPU power to check even trivial
changes to a few include files.

Partly this could be fixed by making header files in asm-$(ARCH)
second class citizen - that always got included via their linux/
counterpart.

Take a look at uaccess.h for example.
A grep shows that most architectures include almost the same header files
with a few that require string.h.
So it would be so simple to move the include of string.h to the
linux/uaccess.h counter part and no arch specific dependencies.

Grepping the tree I only find one user of linux/uaccess.h : filemap.*
But even though my point holds that there are easy way to deal with this
without the maintenace nightmare you try to picture up.

That said this would maybe work in two third of the cases and is no
bullet proof solution. But each time we can decrease the arch differences
we are one step in the right direction.

	Sam

Re: [PATCH] missing includes from infiniband merge

[Al Viro]

On Sun, Sep 24, 2006 at 10:52:44PM +0200, Sam Ravnborg wrote:
> The only thing I like to see is minimal suprise. And minimal suprise in
> this case is to be considered as "works on almost all archs if not all".
> In practical terms it could be that users of asm/* had to include
> baz.h before bar.h.

Even though dependency on baz.h is due to implementation of one of the
helpers in bar.h on a few targets and makes no sense whatsoever for
everything else?

> Or we could stick to current mess where one has
> to have a shitload of crosscompiles and CPU power to check even trivial
> changes to a few include files.

We _are_ stuck with it.

> Partly this could be fixed by making header files in asm-$(ARCH)
> second class citizen - that always got included via their linux/
> counterpart.

Which only makes dependency graph fatter...  What's the difference
between including asm/uaccess.h and linux/uaccess.h?

Basically, you pull tons of includes into linux/blah.h because it
happens to include asm/foo.h and _that_ depends on having linux/barf.h
for $WEIRD_TARGET.  And guess what?  You are back to the same cross-compiles,
since attempt to remove blah.h -> barf.h will break $WEIRD_TARGET, but
you won't notice that unless you cross-compile for it.

So all you get is a bunch of harder to explain includes between linux/*.h,
_and_ extra dependencies that make no sense whatsoever.

Re: [PATCH] missing includes from infiniband merge

[Sam Ravnborg]

> > Or we could stick to current mess where one has
> > to have a shitload of crosscompiles and CPU power to check even trivial
> > changes to a few include files.
> 
> We _are_ stuck with it.
For old stuff - to a high degree yes.

> > Partly this could be fixed by making header files in asm-$(ARCH)
> > second class citizen - that always got included via their linux/
> > counterpart.
> 
> Which only makes dependency graph fatter...  What's the difference
> between including asm/uaccess.h and linux/uaccess.h?
It makes it fatter on the horizontal level - yes.
But then files do not rely on files being included by asm-* files
anymore - so less difference between architecutes.

> Basically, you pull tons of includes into linux/blah.h because it
> happens to include asm/foo.h and _that_ depends on having linux/barf.h
> for $WEIRD_TARGET.
That would be wrong. Any file in asm-* that needs a file from linux/
should pull that include to the linux/ counterpart and in this way
we are on track again.

It boils down to something simple as:
Do we want to keep arch independence lower on the price of higher number
includes in asm- counter part files in linux/?


> And guess what?  You are back to the same cross-compiles,
> since attempt to remove blah.h -> barf.h will break $WEIRD_TARGET, but
> you won't notice that unless you cross-compile for it.
No.
There is a huge difference modifying architecture independent code like
infiniband or module.c and to try to clean up headers.
Cleaning up headers really really require that one cross compile
a lot no matter how files are organized.

> So all you get is a bunch of harder to explain includes between linux/*.h,
> _and_ extra dependencies that make no sense whatsoever.
They are easy to explain: They are there to keep arch independence lower -
and the cost is a number (tons?) of extra includes.

OK - this was a fun thread talking about arch specific includes.
But since I do not plan to follow-up with patches I will
drop of for now. bugzilla and others tell me I should concentrate
on other things.

	Sam