[GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Haavard Skinnemoen]

Hi Linus,

Sorry about sending you pull requests so rapidly, but Jens broke my
dma-mapping.h yesterday. Or you may say the bug was there to begin with
and Jens merely exposed it, but that makes it sound like my fault ;-)

Anyway, please pull from

  ssh://master.kernel.org/pub/scm/linux/kernel/git/hskinnemoen/avr32-2.6.git for-linus

to receive the following update.

Haavard Skinnemoen (1):
      AVR32: Fix sg_page breakage

 include/asm-avr32/dma-mapping.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/asm-avr32/dma-mapping.h b/include/asm-avr32/dma-mapping.h
index a713163..57dc672 100644
--- a/include/asm-avr32/dma-mapping.h
+++ b/include/asm-avr32/dma-mapping.h
@@ -3,7 +3,7 @@
 
 #include <linux/mm.h>
 #include <linux/device.h>
-#include <asm/scatterlist.h>
+#include <linux/scatterlist.h>
 #include <asm/processor.h>
 #include <asm/cacheflush.h>
 #include <asm/io.h>

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Jens Axboe]

On Wed, Oct 24 2007, Haavard Skinnemoen wrote:
> Hi Linus,
> 
> Sorry about sending you pull requests so rapidly, but Jens broke my
> dma-mapping.h yesterday. Or you may say the bug was there to begin with
> and Jens merely exposed it, but that makes it sound like my fault ;-)
> 
> Anyway, please pull from
> 
>   ssh://master.kernel.org/pub/scm/linux/kernel/git/hskinnemoen/avr32-2.6.git for-linus
> 
> to receive the following update.

You should not take the blame :-)

I'm collecting these patches, so I'll pull your fix and make sure it
gets to Linus today along with the other fixes.

-- 
Jens Axboe

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Sam Ravnborg]

On Wed, Oct 24, 2007 at 01:24:53PM +0200, Jens Axboe wrote:
> On Wed, Oct 24 2007, Haavard Skinnemoen wrote:
> > Hi Linus,
> > 
> > Sorry about sending you pull requests so rapidly, but Jens broke my
> > dma-mapping.h yesterday. Or you may say the bug was there to begin with
> > and Jens merely exposed it, but that makes it sound like my fault ;-)
> > 
> > Anyway, please pull from
> > 
> >   ssh://master.kernel.org/pub/scm/linux/kernel/git/hskinnemoen/avr32-2.6.git for-linus
> > 
> > to receive the following update.
> 
> You should not take the blame :-)
> 
> I'm collecting these patches, so I'll pull your fix and make sure it
> gets to Linus today along with the other fixes.

How about doing a "git grep asm/scatterlist"
and fix all victims to use linux/scatterlist?
Or you maybe did this already.

	Sam

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Jens Axboe]

On Wed, Oct 24 2007, Sam Ravnborg wrote:
> On Wed, Oct 24, 2007 at 01:24:53PM +0200, Jens Axboe wrote:
> > On Wed, Oct 24 2007, Haavard Skinnemoen wrote:
> > > Hi Linus,
> > > 
> > > Sorry about sending you pull requests so rapidly, but Jens broke my
> > > dma-mapping.h yesterday. Or you may say the bug was there to begin with
> > > and Jens merely exposed it, but that makes it sound like my fault ;-)
> > > 
> > > Anyway, please pull from
> > > 
> > >   ssh://master.kernel.org/pub/scm/linux/kernel/git/hskinnemoen/avr32-2.6.git for-linus
> > > 
> > > to receive the following update.
> > 
> > You should not take the blame :-)
> > 
> > I'm collecting these patches, so I'll pull your fix and make sure it
> > gets to Linus today along with the other fixes.
> 
> How about doing a "git grep asm/scatterlist"
> and fix all victims to use linux/scatterlist?
> Or you maybe did this already.

I did earlier today, only xtensa was using sg_* functions and only
including asm/scatterlist.h instead of linux/scatterlist.h

-- 
Jens Axboe

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Robert P. J. Day]

On Wed, 24 Oct 2007, Sam Ravnborg wrote:

> On Wed, Oct 24, 2007 at 01:24:53PM +0200, Jens Axboe wrote:
> > On Wed, Oct 24 2007, Haavard Skinnemoen wrote:
> > > Hi Linus,
> > >
> > > Sorry about sending you pull requests so rapidly, but Jens broke my
> > > dma-mapping.h yesterday. Or you may say the bug was there to begin with
> > > and Jens merely exposed it, but that makes it sound like my fault ;-)
> > >
> > > Anyway, please pull from
> > >
> > >   ssh://master.kernel.org/pub/scm/linux/kernel/git/hskinnemoen/avr32-2.6.git for-linus
> > >
> > > to receive the following update.
> >
> > You should not take the blame :-)
> >
> > I'm collecting these patches, so I'll pull your fix and make sure it
> > gets to Linus today along with the other fixes.
>
> How about doing a "git grep asm/scatterlist"
> and fix all victims to use linux/scatterlist?
> Or you maybe did this already.

i was just about to ask -- is this one of those cases where the asm
versions of scatterlist.h should have a warning/error that they should
not be included directly, and to include linux/scatterlist.h instead?

rday
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
========================================================================

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Jens Axboe]

On Wed, Oct 24 2007, Robert P. J. Day wrote:
> On Wed, 24 Oct 2007, Sam Ravnborg wrote:
> 
> > On Wed, Oct 24, 2007 at 01:24:53PM +0200, Jens Axboe wrote:
> > > On Wed, Oct 24 2007, Haavard Skinnemoen wrote:
> > > > Hi Linus,
> > > >
> > > > Sorry about sending you pull requests so rapidly, but Jens broke my
> > > > dma-mapping.h yesterday. Or you may say the bug was there to begin with
> > > > and Jens merely exposed it, but that makes it sound like my fault ;-)
> > > >
> > > > Anyway, please pull from
> > > >
> > > >   ssh://master.kernel.org/pub/scm/linux/kernel/git/hskinnemoen/avr32-2.6.git for-linus
> > > >
> > > > to receive the following update.
> > >
> > > You should not take the blame :-)
> > >
> > > I'm collecting these patches, so I'll pull your fix and make sure it
> > > gets to Linus today along with the other fixes.
> >
> > How about doing a "git grep asm/scatterlist"
> > and fix all victims to use linux/scatterlist?
> > Or you maybe did this already.
> 
> i was just about to ask -- is this one of those cases where the asm
> versions of scatterlist.h should have a warning/error that they should
> not be included directly, and to include linux/scatterlist.h instead?

No, using asm/scatterlist.h is perfectly fine. The problem is code using
the sg helpers should include linux/scatterlist.h since that is where
those are defined.

If you just need the scatterlist structure definition, then
asm/scatterlist.h is the correct include.

-- 
Jens Axboe

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Adrian Bunk]

On Wed, Oct 24, 2007 at 02:21:20PM +0200, Jens Axboe wrote:
> On Wed, Oct 24 2007, Robert P. J. Day wrote:
> > On Wed, 24 Oct 2007, Sam Ravnborg wrote:
> > 
> > > On Wed, Oct 24, 2007 at 01:24:53PM +0200, Jens Axboe wrote:
> > > > On Wed, Oct 24 2007, Haavard Skinnemoen wrote:
> > > > > Hi Linus,
> > > > >
> > > > > Sorry about sending you pull requests so rapidly, but Jens broke my
> > > > > dma-mapping.h yesterday. Or you may say the bug was there to begin with
> > > > > and Jens merely exposed it, but that makes it sound like my fault ;-)
> > > > >
> > > > > Anyway, please pull from
> > > > >
> > > > >   ssh://master.kernel.org/pub/scm/linux/kernel/git/hskinnemoen/avr32-2.6.git for-linus
> > > > >
> > > > > to receive the following update.
> > > >
> > > > You should not take the blame :-)
> > > >
> > > > I'm collecting these patches, so I'll pull your fix and make sure it
> > > > gets to Linus today along with the other fixes.
> > >
> > > How about doing a "git grep asm/scatterlist"
> > > and fix all victims to use linux/scatterlist?
> > > Or you maybe did this already.
> > 
> > i was just about to ask -- is this one of those cases where the asm
> > versions of scatterlist.h should have a warning/error that they should
> > not be included directly, and to include linux/scatterlist.h instead?
> 
> No, using asm/scatterlist.h is perfectly fine. The problem is code using
> the sg helpers should include linux/scatterlist.h since that is where
> those are defined.
> 
> If you just need the scatterlist structure definition, then
> asm/scatterlist.h is the correct include.

But there's also the general question whether it's good practice for 
not architecture specific code to include asm/ headers.

For APIs available on all architectures linux/ header are the right 
thing to use, and what is in the asm/ header and what in the linux/ 
header becomes an implementation detail that can be changed.

> Jens Axboe

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Robert P. J. Day]

On Wed, 24 Oct 2007, Adrian Bunk wrote:

> On Wed, Oct 24, 2007 at 02:21:20PM +0200, Jens Axboe wrote:
> > On Wed, Oct 24 2007, Robert P. J. Day wrote:

> > > i was just about to ask -- is this one of those cases where the
> > > asm versions of scatterlist.h should have a warning/error that
> > > they should not be included directly, and to include
> > > linux/scatterlist.h instead?

> > No, using asm/scatterlist.h is perfectly fine. The problem is code
> > using the sg helpers should include linux/scatterlist.h since that
> > is where those are defined.

> > If you just need the scatterlist structure definition, then
> > asm/scatterlist.h is the correct include.

> But there's also the general question whether it's good practice for
> not architecture specific code to include asm/ headers.

> For APIs available on all architectures linux/ header are the right
> thing to use, and what is in the asm/ header and what in the linux/
> header becomes an implementation detail that can be changed.

more to the point, what the above shows is that the headers aren't
well defined.  one would think that a header file whose name is
"scatterlist.h" should provide content related solely to that name.

if there's different content, such as the "sg helpers" mentioned
above, then those should require a different header file inclusion.
it makes no sense to suggest that one should include
<asm/scatterlist.h> if you need *only* that content, but to include
<linux/scatterlist.h> if you need stuff *in addition to* the
scatterlist content.  that's just begging for confusion.

rday
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
========================================================================

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Jens Axboe]

On Wed, Oct 24 2007, Adrian Bunk wrote:
> On Wed, Oct 24, 2007 at 02:21:20PM +0200, Jens Axboe wrote:
> > On Wed, Oct 24 2007, Robert P. J. Day wrote:
> > > On Wed, 24 Oct 2007, Sam Ravnborg wrote:
> > > 
> > > > On Wed, Oct 24, 2007 at 01:24:53PM +0200, Jens Axboe wrote:
> > > > > On Wed, Oct 24 2007, Haavard Skinnemoen wrote:
> > > > > > Hi Linus,
> > > > > >
> > > > > > Sorry about sending you pull requests so rapidly, but Jens broke my
> > > > > > dma-mapping.h yesterday. Or you may say the bug was there to begin with
> > > > > > and Jens merely exposed it, but that makes it sound like my fault ;-)
> > > > > >
> > > > > > Anyway, please pull from
> > > > > >
> > > > > >   ssh://master.kernel.org/pub/scm/linux/kernel/git/hskinnemoen/avr32-2.6.git for-linus
> > > > > >
> > > > > > to receive the following update.
> > > > >
> > > > > You should not take the blame :-)
> > > > >
> > > > > I'm collecting these patches, so I'll pull your fix and make sure it
> > > > > gets to Linus today along with the other fixes.
> > > >
> > > > How about doing a "git grep asm/scatterlist"
> > > > and fix all victims to use linux/scatterlist?
> > > > Or you maybe did this already.
> > > 
> > > i was just about to ask -- is this one of those cases where the asm
> > > versions of scatterlist.h should have a warning/error that they should
> > > not be included directly, and to include linux/scatterlist.h instead?
> > 
> > No, using asm/scatterlist.h is perfectly fine. The problem is code using
> > the sg helpers should include linux/scatterlist.h since that is where
> > those are defined.
> > 
> > If you just need the scatterlist structure definition, then
> > asm/scatterlist.h is the correct include.
> 
> But there's also the general question whether it's good practice for 
> not architecture specific code to include asm/ headers.
> 
> For APIs available on all architectures linux/ header are the right 
> thing to use, and what is in the asm/ header and what in the linux/ 
> header becomes an implementation detail that can be changed.

I agree, for non-arch code you almost always want to use
linux/scatterlist.h since you should be using proper accessor methods
anyway.

-- 
Jens Axboe

Re: [GIT PATCH] Fix asm-avr32/dma-mapping.h breakage

[Jens Axboe]

On Wed, Oct 24 2007, Robert P. J. Day wrote:
> On Wed, 24 Oct 2007, Adrian Bunk wrote:
> 
> > On Wed, Oct 24, 2007 at 02:21:20PM +0200, Jens Axboe wrote:
> > > On Wed, Oct 24 2007, Robert P. J. Day wrote:
> 
> > > > i was just about to ask -- is this one of those cases where the
> > > > asm versions of scatterlist.h should have a warning/error that
> > > > they should not be included directly, and to include
> > > > linux/scatterlist.h instead?
> 
> > > No, using asm/scatterlist.h is perfectly fine. The problem is code
> > > using the sg helpers should include linux/scatterlist.h since that
> > > is where those are defined.
> 
> > > If you just need the scatterlist structure definition, then
> > > asm/scatterlist.h is the correct include.
> 
> > But there's also the general question whether it's good practice for
> > not architecture specific code to include asm/ headers.
> 
> > For APIs available on all architectures linux/ header are the right
> > thing to use, and what is in the asm/ header and what in the linux/
> > header becomes an implementation detail that can be changed.
> 
> more to the point, what the above shows is that the headers aren't
> well defined.  one would think that a header file whose name is
> "scatterlist.h" should provide content related solely to that name.
> 
> if there's different content, such as the "sg helpers" mentioned
> above, then those should require a different header file inclusion.
> it makes no sense to suggest that one should include
> <asm/scatterlist.h> if you need *only* that content, but to include
> <linux/scatterlist.h> if you need stuff *in addition to* the
> scatterlist content.  that's just begging for confusion.

Completely disagree, it's well defined. The asm/ header adds the arch
private stuff, like the actual structure definition. That absolutely has
to reside there. The linux/ header adds manipulation headers for that
structure. You'd want to put that in all the asm/ headers? I didn't
think so.

So I'll repeat - arch code may use the asm/ header, if they just need
the structure definition. Drivers should use the linux/ header, since
they should also use the accessor functions. Right now there's still a
lot of open coding of

        for (i = 0; i < sg_nents; i++)
                sg_table[i] ...

usage which should go away eventually and use a

        sg = sg_table;
        do {
                ...
        } while ((sg = sg_next(sg)) != NULL);

construct, in which case there's no way around using the linux/ header.
arch IOMMU code will want to be using the linux/ header as well, since
they will be browsing the list also. That switch should happen when the
driver actually needs it, not needlessly. The problems seen in the last
few days have been code that actually use sg accessors AND don't use the
linux/ header, and that problem only showing up on archs where
linux/scatterlist.h doesn't get magically included from some other file.
That is of course a bug, you should include the header with the
functions that you use. It has nothing to do with asm/ vs linux/ include
messiness.

-- 
Jens Axboe